[PATCH v2 2/2] Adds new device resume mode in PM core, async plus non-blocking

[PATCH v2 2/2] Adds new device resume mode in PM core, async plus non-blocking

[Brandt, Todd E]

Updates the drivers/base/power subsystem to allow any devices which
have registred as asynchronous and who have not registered "complete"
callbacks to be non-blocking. i.e system resume can finish and return
control to the user while these devices continue resuming.

Changelog:
v2:
        - Updated patch submission. Incorporates comments from Tejun Heo.   
          Fixed comment format, removed camelcase. No functional changes.

Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>
---
 drivers/base/power/main.c |   20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 2b7f77d..1b16379 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -713,7 +713,6 @@ void dpm_resume(pm_message_t state)
 		put_device(dev);
 	}
 	mutex_unlock(&dpm_list_mtx);
-	async_synchronize_full();
 	dpm_show_time(starttime, state, NULL);
 }
 
@@ -726,11 +725,14 @@ static void device_complete(struct device *dev, pm_message_t state)
 {
 	void (*callback)(struct device *) = NULL;
 	char *info = NULL;
+	bool hascb = false;
 
 	if (dev->power.syscore)
 		return;
 
-	device_lock(dev);
+ docomplete:
+	if (hascb)
+		device_lock(dev);
 
 	if (dev->pm_domain) {
 		info = "completing power domain ";
@@ -751,13 +753,21 @@ static void device_complete(struct device *dev, pm_message_t state)
 		callback = dev->driver->pm->complete;
 	}
 
+	/*
+	 * if a callback exists, lock the device and call it
+	 * otherwise don't even lock/unlock the device
+	 */
 	if (callback) {
+		if (!hascb) {
+			hascb = true;
+			goto docomplete;
+		}
+
 		pm_dev_dbg(dev, state, info);
 		callback(dev);
+		device_unlock(dev);
 	}
 
-	device_unlock(dev);
-
 	pm_runtime_put_sync(dev);
 }
 
@@ -1180,6 +1190,8 @@ int dpm_suspend(pm_message_t state)
 	might_sleep();
 
 	mutex_lock(&dpm_list_mtx);
+	/* wait for any processes still resuming */
+	async_synchronize_full();
 	pm_transition = state;
 	async_error = 0;
 	while (!list_empty(&dpm_prepared_list)) {

Re: [PATCH v2 2/2] Adds new device resume mode in PM core, async plus non-blocking

[Greg Kroah-Hartman]

On Fri, May 17, 2013 at 07:05:33PM +0000, Brandt, Todd E wrote:
> Updates the drivers/base/power subsystem to allow any devices which
> have registred as asynchronous and who have not registered "complete"
> callbacks to be non-blocking. i.e system resume can finish and return
> control to the user while these devices continue resuming.
> 
> Changelog:
> v2:
>         - Updated patch submission. Incorporates comments from Tejun Heo.   
>           Fixed comment format, removed camelcase. No functional changes.
> 
> Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>
> ---
>  drivers/base/power/main.c |   20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 2b7f77d..1b16379 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -713,7 +713,6 @@ void dpm_resume(pm_message_t state)
>  		put_device(dev);
>  	}
>  	mutex_unlock(&dpm_list_mtx);
> -	async_synchronize_full();
>  	dpm_show_time(starttime, state, NULL);
>  }
>  
> @@ -726,11 +725,14 @@ static void device_complete(struct device *dev, pm_message_t state)
>  {
>  	void (*callback)(struct device *) = NULL;
>  	char *info = NULL;
> +	bool hascb = false;

"hascb"?  Please spell out what this is.

>  
>  	if (dev->power.syscore)
>  		return;
>  
> -	device_lock(dev);
> + docomplete:
> +	if (hascb)
> +		device_lock(dev);
>  
>  	if (dev->pm_domain) {
>  		info = "completing power domain ";
> @@ -751,13 +753,21 @@ static void device_complete(struct device *dev, pm_message_t state)
>  		callback = dev->driver->pm->complete;
>  	}
>  
> +	/*
> +	 * if a callback exists, lock the device and call it
> +	 * otherwise don't even lock/unlock the device
> +	 */
>  	if (callback) {
> +		if (!hascb) {
> +			hascb = true;
> +			goto docomplete;

You want to jump backwards?  This isn't the scheduler, please, you can
do better here.

thanks,

greg k-h

RE: [PATCH v2 2/2] Adds new device resume mode in PM core, async plus non-blocking

[Brandt, Todd E]

Hi, will do, thanks for the feedback. I used the goto to make the code addition smaller, but I obviously sacrificed code clarity.

________________________________________
From: Greg Kroah-Hartman [gregkh@linuxfoundation.org]
Sent: Friday, May 17, 2013 12:11 PM
To: Brandt, Todd E
Cc: linux-ide@vger.kernel.org; linux-scsi@vger.kernel.org; linux-pm@vger.kernel.org; Jeff Garzik; Jens Axboe; Wysocki, Rafael J; Arjan van de Ven
Subject: Re: [PATCH v2 2/2] Adds new device resume mode in PM core, async plus non-blocking

On Fri, May 17, 2013 at 07:05:33PM +0000, Brandt, Todd E wrote:
> Updates the drivers/base/power subsystem to allow any devices which
> have registred as asynchronous and who have not registered "complete"
> callbacks to be non-blocking. i.e system resume can finish and return
> control to the user while these devices continue resuming.
>
> Changelog:
> v2:
>         - Updated patch submission. Incorporates comments from Tejun Heo.
>           Fixed comment format, removed camelcase. No functional changes.
>
> Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>
> ---
>  drivers/base/power/main.c |   20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 2b7f77d..1b16379 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -713,7 +713,6 @@ void dpm_resume(pm_message_t state)
>               put_device(dev);
>       }
>       mutex_unlock(&dpm_list_mtx);
> -     async_synchronize_full();
>       dpm_show_time(starttime, state, NULL);
>  }
>
> @@ -726,11 +725,14 @@ static void device_complete(struct device *dev, pm_message_t state)
>  {
>       void (*callback)(struct device *) = NULL;
>       char *info = NULL;
> +     bool hascb = false;

"hascb"?  Please spell out what this is.

>
>       if (dev->power.syscore)
>               return;
>
> -     device_lock(dev);
> + docomplete:
> +     if (hascb)
> +             device_lock(dev);
>
>       if (dev->pm_domain) {
>               info = "completing power domain ";
> @@ -751,13 +753,21 @@ static void device_complete(struct device *dev, pm_message_t state)
>               callback = dev->driver->pm->complete;
>       }
>
> +     /*
> +      * if a callback exists, lock the device and call it
> +      * otherwise don't even lock/unlock the device
> +      */
>       if (callback) {
> +             if (!hascb) {
> +                     hascb = true;
> +                     goto docomplete;

You want to jump backwards?  This isn't the scheduler, please, you can
do better here.

thanks,

greg k-h

Re: [PATCH v2 2/2] Adds new device resume mode in PM core, async plus non-blocking

[Alan Stern]

On Fri, 17 May 2013, Brandt, Todd E wrote:

> Updates the drivers/base/power subsystem to allow any devices which
> have registred as asynchronous and who have not registered "complete"
> callbacks to be non-blocking. i.e system resume can finish and return
> control to the user while these devices continue resuming.

This does not sound like a good idea.  The presence or absence of a 
"complete" callback has nothing to do with whether or not system resume 
can finish before the device is ready.  It is more closely related to 
whether the device's driver can register hot-plugged children below the 
device or needs to perform other actions after the children have been 
resumed.

The whole approach seems wrong -- it violates the implicit agreement 
that the kernel will not try to perform I/O through a device that isn't 
at full power.

A better approach would be to modify the SCSI disk driver to carry out 
the spin-up operation asynchronously with respect to the resume 
callback.  Then the disk could be reported as back to full power while 
the spin-up is taking place.

Alan Stern

RE: [PATCH v2 2/2] Adds new device resume mode in PM core, async plus non-blocking

[Brandt, Todd E]

Thanks for the feedback, I've taken things back to the drawing board in lieu of these issues and will be resubmitting once I have a patch that fits better into the formal power management architecture. Would you like to be CC'ed?

Todd Brandt
Linux Kernel Developer OTC, Hillsboro OR
https://opensource.intel.com/linux-wiki/ToddBrandt


________________________________________
From: Alan Stern [stern@rowland.harvard.edu]
Sent: Friday, May 17, 2013 12:44 PM
To: Brandt, Todd E
Cc: linux-ide@vger.kernel.org; linux-scsi@vger.kernel.org; linux-pm@vger.kernel.org; Jeff Garzik; Jens Axboe; Greg Kroah-Hartman; Wysocki, Rafael J; Arjan van de Ven
Subject: Re: [PATCH v2 2/2] Adds new device resume mode in PM core, async plus non-blocking

On Fri, 17 May 2013, Brandt, Todd E wrote:

> Updates the drivers/base/power subsystem to allow any devices which
> have registred as asynchronous and who have not registered "complete"
> callbacks to be non-blocking. i.e system resume can finish and return
> control to the user while these devices continue resuming.

This does not sound like a good idea.  The presence or absence of a
"complete" callback has nothing to do with whether or not system resume
can finish before the device is ready.  It is more closely related to
whether the device's driver can register hot-plugged children below the
device or needs to perform other actions after the children have been
resumed.

The whole approach seems wrong -- it violates the implicit agreement
that the kernel will not try to perform I/O through a device that isn't
at full power.

A better approach would be to modify the SCSI disk driver to carry out
the spin-up operation asynchronously with respect to the resume
callback.  Then the disk could be reported as back to full power while
the spin-up is taking place.

Alan Stern

RE: [PATCH v2 2/2] Adds new device resume mode in PM core, async plus non-blocking

[Alan Stern]

On Mon, 3 Jun 2013, Brandt, Todd E wrote:

> Thanks for the feedback, I've taken things back to the drawing board
> in lieu of these issues and will be resubmitting once I have a patch
> that fits better into the formal power management architecture. Would
> you like to be CC'ed?

No need, since I am subscribed to the linux-pm mailing list.

Alan Stern