Patchwork [v5,5/6] ata: add ata port system PM callbacks

login
register
mail settings
Submitter Lin Ming
Date Dec. 13, 2011, 2:20 p.m.
Message ID <1323786040.3769.24.camel@hp6530s>
Download mbox | patch
Permalink /patch/131109/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Lin Ming - Dec. 13, 2011, 2:20 p.m.
On Tue, 2011-12-13 at 21:56 +0800, Lin Ming wrote:
> +
> +static int ata_port_resume(struct device *dev)
> +{
> +       struct ata_port *ap = to_ata_port(dev);
> +       int rc;
> +
> +       rc = ata_port_request_pm(ap, PMSG_ON, ATA_EH_RESET,
> +               ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET, 1);
> +       return rc;
> +}

I just realized that the ata port runtime PM status need to be updated
after system resume.

I tried below patch.
Unfortunately, it causes a problem that sd can't resume correctly.

During system resume, ata port is resumed first and then sd resumed.

When ata port resumed, device_resume(...) calls pm_runtime_put_sync(..),
which causes ata port to be runtime suspended immediately.

So sd resume fails because it requires ata port to be active to handle
start device command.

This seems a PM core's bug.

device_resume(...) should not runtime suspend the parent device, because
its children have not resumed yet.

Alan, 

What do you think?

---
 drivers/ata/libata-core.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)







--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Stern - Dec. 13, 2011, 3:47 p.m.
On Tue, 13 Dec 2011, Lin Ming wrote:

> I just realized that the ata port runtime PM status need to be updated
> after system resume.
> 
> I tried below patch.
> Unfortunately, it causes a problem that sd can't resume correctly.
> 
> During system resume, ata port is resumed first and then sd resumed.
> 
> When ata port resumed, device_resume(...) calls pm_runtime_put_sync(..),
> which causes ata port to be runtime suspended immediately.
> 
> So sd resume fails because it requires ata port to be active to handle
> start device command.
> 
> This seems a PM core's bug.
> 
> device_resume(...) should not runtime suspend the parent device, because
> its children have not resumed yet.
> 
> Alan, 
> 
> What do you think?

This appears to be the first time this problem has come up.  But it is 
a real problem.

If a child device was runtime-suspended when a system suspend began, 
then there will be nothing to prevent its parent from 
runtime-suspending as soon as it is woken up during the system resume.  
Then when the time comes to resume the child, the resume will fail 
because the parent is already back at low power.

On the other hand, there are some devices which should remain at low 
power across an entire suspend-resume cycle.  The details depend on the 
device and the platform.

This suggests that the PM core is not the right place to solve the 
problem.  One possible solution is for the subsystem or device driver 
to call pm_runtime_get_sync(dev->parent) at the start of the 
system-resume procedure and pm_runtime_put_sync(dev->parent) at the 
end.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 15a3d4d..8996758 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5298,7 +5298,7 @@  static int ata_port_suspend(struct device *dev)
 	return ata_port_suspend_common(dev);
 }
 
-static int ata_port_resume(struct device *dev)
+static int ata_port_resume_common(struct device *dev)
 {
 	struct ata_port *ap = to_ata_port(dev);
 	int rc;
@@ -5308,6 +5308,20 @@  static int ata_port_resume(struct device *dev)
 	return rc;
 }
 
+static int ata_port_resume(struct device *dev)
+{
+	int rc;
+
+	rc = ata_port_resume_common(dev);
+	if (!rc) {
+		pm_runtime_disable(dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+	}
+
+	return rc;
+}
+
 static int ata_port_runtime_idle(struct device *dev)
 {
 	return pm_runtime_suspend(dev);
@@ -5318,7 +5332,7 @@  static const struct dev_pm_ops ata_port_pm_ops = {
 	.resume = ata_port_resume,
 
 	.runtime_suspend = ata_port_suspend_common,
-	.runtime_resume = ata_port_resume,
+	.runtime_resume = ata_port_resume_common,
 	.runtime_idle = ata_port_runtime_idle,
 };