[PATCH 0/3] fixes for ata port runtime pm support

[PATCH 0/3] fixes for ata port runtime pm support

[Lin Ming]

Hi Jeff,

Here are some fixes for ata port runtime pm support.
Applied on top of libata tree.

Lin Ming (3):
      [SCSI] sd: fix runtime status check in sd_shutdown
      [SCSI]: runtime resume parent for child's system-resume
      ata: update ata port's runtime status during system resume

 drivers/ata/libata-core.c |   18 ++++++++++++++++--
 drivers/scsi/scsi_pm.c    |   11 ++++++++++-
 drivers/scsi/sd.c         |    7 ++++++-
 3 files changed, 32 insertions(+), 4 deletions(-)

Thanks,
Lin Ming

[PATCH 1/3] [SCSI] sd: fix runtime status check in sd_shutdown

[Lin Ming]

Commit af8db15 disabled device's runtime PM during shutdown.
So sd's runtime status can't be checked with pm_runtime_suspended(dev)
any more.

Fix it by checking runtime status with pm_runtime_status_suspended(dev).

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/scsi/sd.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 7b3f807..284b087 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2742,7 +2742,12 @@ static void sd_shutdown(struct device *dev)
 	if (!sdkp)
 		return;         /* this can happen */
 
-	if (pm_runtime_suspended(dev))
+	/*
+	 * Check runtime status with pm_runtime_status_suspended(dev)
+	 * instead of pm_runtime_suspended(dev),
+	 * because device_shutdown() has disabled the device's runtime PM.
+	 */
+	if (pm_runtime_status_suspended(dev))
 		goto exit;
 
 	if (sdkp->WCE) {
-- 
1.7.2.5

Re: [PATCH 1/3] [SCSI] sd: fix runtime status check in sd_shutdown

[Sergei Shtylyov]

Hello.

On 14-12-2011 7:17, Lin Ming wrote:

> Commit af8db15 disabled device's runtime PM during shutdown.

    Please also specify that commit's summary in parens.

> So sd's runtime status can't be checked with pm_runtime_suspended(dev)
> any more.

> Fix it by checking runtime status with pm_runtime_status_suspended(dev).

> Signed-off-by: Lin Ming<ming.m.lin@intel.com>

WBR, Sergei

Re: [PATCH 1/3] [SCSI] sd: fix runtime status check in sd_shutdown

[Lin Ming]

On Wed, 2011-12-14 at 18:58 +0800, Sergei Shtylyov wrote:
> Hello.
> 
> On 14-12-2011 7:17, Lin Ming wrote:
> 
> > Commit af8db15 disabled device's runtime PM during shutdown.
> 
>     Please also specify that commit's summary in parens.

OK. Here it is.

>From 7e3dcebe235d005126c03a3976bca859691110bc Mon Sep 17 00:00:00 2001
From: Lin Ming <ming.m.lin@intel.com>
Date: Wed, 14 Dec 2011 10:14:21 +0800
Subject: [PATCH 1/3] [SCSI] sd: fix runtime status check in sd_shutdown

Commit af8db15(PM / driver core: disable device's runtime PM during
shutdown) makes pm_runtime_suspended(dev) always return false.
So sd's runtime status can't be checked with pm_runtime_suspended(dev)
any more.

Fix it by checking runtime status with pm_runtime_status_suspended(dev).

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/scsi/sd.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 7b3f807..284b087 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2742,7 +2742,12 @@ static void sd_shutdown(struct device *dev)
 	if (!sdkp)
 		return;         /* this can happen */
 
-	if (pm_runtime_suspended(dev))
+	/*
+	 * Check runtime status with pm_runtime_status_suspended(dev)
+	 * instead of pm_runtime_suspended(dev),
+	 * because device_shutdown() has disabled the device's runtime PM.
+	 */
+	if (pm_runtime_status_suspended(dev))
 		goto exit;
 
 	if (sdkp->WCE) {
-- 
1.7.2.5

Re: [PATCH 1/3] [SCSI] sd: fix runtime status check in sd_shutdown

[Alan Stern]

On Wed, 14 Dec 2011, Lin Ming wrote:

> Commit af8db15 disabled device's runtime PM during shutdown.
> So sd's runtime status can't be checked with pm_runtime_suspended(dev)
> any more.
> 
> Fix it by checking runtime status with pm_runtime_status_suspended(dev).
> 
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> ---
>  drivers/scsi/sd.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 7b3f807..284b087 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2742,7 +2742,12 @@ static void sd_shutdown(struct device *dev)
>  	if (!sdkp)
>  		return;         /* this can happen */
>  
> -	if (pm_runtime_suspended(dev))
> +	/*
> +	 * Check runtime status with pm_runtime_status_suspended(dev)
> +	 * instead of pm_runtime_suspended(dev),
> +	 * because device_shutdown() has disabled the device's runtime PM.
> +	 */
> +	if (pm_runtime_status_suspended(dev))
>  		goto exit;
>  
>  	if (sdkp->WCE) {

This is no longer needed.  See commit
fe6b91f47080eb17d21cbf2a39311877d57f6938 (PM / Driver core: leave
runtime PM enabled during system shutdown).

Alan Stern

Re: [PATCH 1/3] [SCSI] sd: fix runtime status check in sd_shutdown

[Lin Ming]

On Thu, 2011-12-15 at 00:49 +0800, Alan Stern wrote:
> On Wed, 14 Dec 2011, Lin Ming wrote:
> 
> > Commit af8db15 disabled device's runtime PM during shutdown.
> > So sd's runtime status can't be checked with pm_runtime_suspended(dev)
> > any more.
> > 
> > Fix it by checking runtime status with pm_runtime_status_suspended(dev).
> > 
> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > ---
> >  drivers/scsi/sd.c |    7 ++++++-
> >  1 files changed, 6 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index 7b3f807..284b087 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -2742,7 +2742,12 @@ static void sd_shutdown(struct device *dev)
> >  	if (!sdkp)
> >  		return;         /* this can happen */
> >  
> > -	if (pm_runtime_suspended(dev))
> > +	/*
> > +	 * Check runtime status with pm_runtime_status_suspended(dev)
> > +	 * instead of pm_runtime_suspended(dev),
> > +	 * because device_shutdown() has disabled the device's runtime PM.
> > +	 */
> > +	if (pm_runtime_status_suspended(dev))
> >  		goto exit;
> >  
> >  	if (sdkp->WCE) {
> 
> This is no longer needed.  See commit
> fe6b91f47080eb17d21cbf2a39311877d57f6938 (PM / Driver core: leave
> runtime PM enabled during system shutdown).

I'll remove this patch.

Thanks.

> 
> Alan Stern
> 

[PATCH 2/3] [SCSI]: runtime resume parent for child's system-resume

[Lin Ming]

[Patch description from Alan Stern]

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.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/scsi/scsi_pm.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index a633076..bf8bf79 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -72,8 +72,17 @@ static int scsi_bus_resume_common(struct device *dev)
 {
 	int err = 0;
 
-	if (scsi_is_sdev_device(dev))
+	if (scsi_is_sdev_device(dev)) {
+		/*
+		 * Parent device may have runtime suspended as soon as
+		 * it is woken up during the system resume.
+		 *
+		 * Resume it on behalf of child.
+		 */
+		pm_runtime_get_sync(dev->parent);
 		err = scsi_dev_type_resume(dev);
+		pm_runtime_put_sync(dev->parent);
+	}
 
 	if (err == 0) {
 		pm_runtime_disable(dev);
-- 
1.7.2.5

Re: [PATCH 2/3] [SCSI]: runtime resume parent for child's system-resume

[Lin Ming]

On Wed, Dec 14, 2011 at 11:18 AM, Lin Ming <ming.m.lin@intel.com> wrote:
> [Patch description from Alan Stern]
>
> 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.
>
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>

Hi Alan,

May I add your SOB?

Thanks,
Lin Ming

> ---
>  drivers/scsi/scsi_pm.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index a633076..bf8bf79 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -72,8 +72,17 @@ static int scsi_bus_resume_common(struct device *dev)
>  {
>        int err = 0;
>
> -       if (scsi_is_sdev_device(dev))
> +       if (scsi_is_sdev_device(dev)) {
> +               /*
> +                * Parent device may have runtime suspended as soon as
> +                * it is woken up during the system resume.
> +                *
> +                * Resume it on behalf of child.
> +                */
> +               pm_runtime_get_sync(dev->parent);
>                err = scsi_dev_type_resume(dev);
> +               pm_runtime_put_sync(dev->parent);
> +       }
>
>        if (err == 0) {
>                pm_runtime_disable(dev);
> --
> 1.7.2.5
>
> --
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] [SCSI]: runtime resume parent for child's system-resume

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1214 bytes --]

On Wed, 14 Dec 2011, Lin Ming wrote:

> On Wed, Dec 14, 2011 at 11:18 AM, Lin Ming <ming.m.lin@intel.com> wrote:
> > [Patch description from Alan Stern]
> >
> > 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.
> >
> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> 
> Hi Alan,
> 
> May I add your SOB?

You can add: Acked-by: Alan Stern <stern@rowland.harvard.edu>

Did you test this?  Does it do what you want?

Alan Stern

Re: [PATCH 2/3] [SCSI]: runtime resume parent for child's system-resume

[Lin Ming]

On Thu, 2011-12-15 at 00:51 +0800, Alan Stern wrote:
> On Wed, 14 Dec 2011, Lin Ming wrote:
> 
> > On Wed, Dec 14, 2011 at 11:18 AM, Lin Ming <ming.m.lin@intel.com> wrote:
> > > [Patch description from Alan Stern]
> > >
> > > 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.
> > >
> > > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > 
> > Hi Alan,
> > 
> > May I add your SOB?
> 
> You can add: Acked-by: Alan Stern <stern@rowland.harvard.edu>
> 
> Did you test this?  Does it do what you want?

Yes, I have tested this and it fixes the problem.

Thanks.

> 
> Alan Stern
> 

[PATCH 3/3] ata: update ata port's runtime status during system resume

[Lin Ming]

The ata port is brought back to full power state during system resume.
So its runtime PM status will have to be updated to reflect
the actual post-system sleep status.

This also fixes below warning during system suspend/resume.

WARNING: at /work/linux/drivers/ata/libata-eh.c:4034
ata_scsi_port_error_handler+0x89/0x557()

4034         WARN_ON(!(ap->pflags & ATA_PFLAG_SUSPENDED));

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/ata/libata-core.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

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,
 };
 
-- 
1.7.2.5

Re: [PATCH 3/3] ata: update ata port's runtime status during system resume

[Lin Ming]

On Wed, Dec 14, 2011 at 11:18 AM, Lin Ming <ming.m.lin@intel.com> wrote:
> The ata port is brought back to full power state during system resume.
> So its runtime PM status will have to be updated to reflect
> the actual post-system sleep status.
>
> This also fixes below warning during system suspend/resume.
>
> WARNING: at /work/linux/drivers/ata/libata-eh.c:4034
> ata_scsi_port_error_handler+0x89/0x557()
>
> 4034         WARN_ON(!(ap->pflags & ATA_PFLAG_SUSPENDED));
>
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>

Hi Alan,

Would you like to Ack this one also?

Thanks,
Lin Ming

> ---
>  drivers/ata/libata-core.c |   18 ++++++++++++++++--
>  1 files changed, 16 insertions(+), 2 deletions(-)
>
> 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,
>  };
>
> --
> 1.7.2.5
>
> --
> 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