Hi,
Thanks for your replies. I am ccing the ide list now (and re-pasting
the ide patch).
Let me try and elaborate on what I am trying to achieve: I would like
to add a way to move the marvell sata controller to D3hot to save some
power. For my short term use case, I am just interested in turning off
the controllers which don't have any disks attached to them. For this,
I guess I don't really need to do much as the pci pm subsystem takes
care of suspending the controller. However, my goal is to keep the
patch generic, i.e. be able to turn off any marvell controller. So, if
there are disks attached to it, then I think that it should spin down
all the disks and power off the controller.
I would think that some userspace algorithm would decide if the
sata_mv controller should be moved into auto mode (i.e runtime power
managed). This might be done when there are no disks attached or the
system doesn't expect to use the disks behind the controller. If there
is intermittent disk usage, then the user space would not move it into
auto and keep the state as ON.
I am using the ata_pci_device_suspend for the runtime _suspend method
also because AFAICT it does exactly what I am trying to achieve. This
will try and do error handling (scsi_error_handler) which tries to
resume a device if its in the suspending state causing both threads to
be stuck.
Rafael, you mentioned in one of your emails that the patch doesn't
seem to be doing anything useful. How else do I suspend the sata_mv
controller in a proper way? Do you think that there isn't a need to
suspend the disks attached to the controller?
Thanks a lot,
Priyanka
From: Priyanka Gupta <ankaguptaca@xxxxxxxxx>
Date: Tue, 28 Dec 2010 20:49:45 -0600
Subject: [PATCH] Marvell controller Runtime Power Management
---
arch/x86/configs/x86_64_defconfig | 1 +
drivers/ata/sata_mv.c | 46 +++++++++++++++++++++++++++++++++++-
2 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/arch/x86/configs/x86_64_defconfig
b/arch/x86/configs/x86_64_defconfig
index ee01a9d..b48d6ae 100644
--- a/arch/x86/configs/x86_64_defconfig
+++ b/arch/x86/configs/x86_64_defconfig
@@ -55,6 +55,7 @@ CONFIG_KEXEC=y
CONFIG_CRASH_DUMP=y
# CONFIG_COMPAT_VDSO is not set
CONFIG_PM=y
+CONFIG_PM_RUNTIME=y
CONFIG_PM_DEBUG=y
CONFIG_PM_TRACE_RTC=y
CONFIG_HIBERNATION=y
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index bf74a36..34a2eed 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -65,6 +65,7 @@
#include <linux/mbus.h>
#include <linux/bitops.h>
#include <linux/gfp.h>
+#include <linux/pm_runtime.h>
#include <scsi/scsi_host.h>
#include <scsi/scsi_cmnd.h>
#include <scsi/scsi_device.h>
@@ -4191,21 +4192,35 @@ static struct platform_driver mv_platform_driver = {
#ifdef CONFIG_PCI
static int mv_pci_init_one(struct pci_dev *pdev,
const struct pci_device_id *ent);
+
+void mv_pci_remove_one(struct pci_dev *pdev);
+
#ifdef CONFIG_PM
static int mv_pci_device_resume(struct pci_dev *pdev);
#endif
+#ifdef CONFIG_PM_OPS
+static int mv_pci_runtime_device_suspend(struct device *dev);
+static int mv_pci_runtime_device_resume(struct device *dev);
+
+static const struct dev_pm_ops mv_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(ata_pci_device_suspend, mv_pci_device_resume)
+ SET_RUNTIME_PM_OPS(mv_pci_runtime_device_suspend,
mv_pci_runtime_device_resume, NULL)
+};
+#endif
static struct pci_driver mv_pci_driver = {
.name = DRV_NAME,
.id_table = mv_pci_tbl,
.probe = mv_pci_init_one,
- .remove = ata_pci_remove_one,
+ .remove = mv_pci_remove_one,
#ifdef CONFIG_PM
.suspend = ata_pci_device_suspend,
.resume = mv_pci_device_resume,
#endif
-
+#ifdef CONFIG_PM_OPS
+ .driver.pm = &mv_pm_ops,
+#endif
};
/* move to PCI layer or libata core? */
@@ -4284,6 +4299,18 @@ static void mv_print_info(struct ata_host *host)
}
/**
+ * mv_pci_remove_one - handle a removal of the marvell controller
+ * @pdev: PCI device to be removed
+ *
+ * LOCKING:
+ * Inherited from caller.
+ */
+void mv_pci_remove_one(struct pci_dev *pdev) {
+ ata_pci_remove_one(pdev);
+ pm_runtime_get_noresume(&pdev->dev);
+}
+
+/**
* mv_pci_init_one - handle a positive probe of a PCI Marvell host
* @pdev: PCI device found
* @ent: PCI device ID entry for the matched host
@@ -4359,10 +4386,25 @@ static int mv_pci_init_one(struct pci_dev *pdev,
pci_set_master(pdev);
pci_try_set_mwi(pdev);
+ pm_runtime_put_noidle(&pdev->dev);
return ata_host_activate(host, pdev->irq, mv_interrupt, IRQF_SHARED,
IS_GEN_I(hpriv) ? &mv5_sht : &mv6_sht);
}
+#ifdef CONFIG_PM_OPS
+static int mv_pci_runtime_device_suspend(struct device *dev)
+{
+ struct pci_dev *pdev = to_pci_dev(dev);
+ return ata_pci_device_suspend(pdev, PMSG_SUSPEND);
+}
+
+static int mv_pci_runtime_device_resume(struct device *dev)
+{
+ struct pci_dev *pdev = to_pci_dev(dev);
+ return mv_pci_device_resume(pdev);
+}
+#endif
+
#ifdef CONFIG_PM
static int mv_pci_device_resume(struct pci_dev *pdev)
{
--
1.7.3.1
On Fri, Dec 31, 2010 at 6:01 AM, Tejun Heo <tj@xxxxxxxxxx> wrote:
>
> Hello,
>
> On Fri, Dec 31, 2010 at 12:09:11AM +0100, Rafael J. Wysocki wrote:
> > Adding Tejun to the CC list, because he's our SATA expert.
> >
> > Also appending the patch so that he can actually see it.
>
> And you forgot. :-)
>
> > [Priyanka, it usually is better do discuss such things on mailing
> > lists, so that people can look into the archives and see the patches
> > etc.]
>
> Yes, when you reply, please at least cc linux-ide@xxxxxxxxxxxxxxxx
>
> > > > already been moved to SUSPENDING state by the __pm_runtime_suspend).
> > > > This in turn invokes the scsi_error_handler to run, which ends up
> > > > calling scsi_autopm_get_host which then tries to get the device and
> > > > hence tries to resume it. And they end up stuck, one thread trying to
> > > > suspend which won't succeed, and the other thread trying to resume.
> > > > See below the stack trace for both.
> > > >
> > > > If I skip the error handling the suspend works fine as you would
> > > > expect. However, that isn't a solution. I was wondering if you can
> > > > suggest what might be the best possible solution to this problem?
> > > >
> > > > I am attaching a patch as an attachment. Hope you can help.
> > >
> > > The problem is that you call ata_pci_device_suspend(). That routine
> > > apparently was not meant for use by runtime PM.
> >
> > Agreed.
>
> Hmmm... I'm not familiar with runtime PM. What's it supposed to
> achieve? Should it spin down the drive and power off the controller?
> If so, the only proper way would be going through EH, but how often is
> this gonna be activated? Even auto-spindown feature implemented in
> hardware often doesn't help much (or even consumes more power) unless
> the idle period is quite long.
>
> Thanks.
>
> --
> tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
