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

login
register
mail settings
Submitter Brandt, Todd E
Date May 17, 2013, 7:05 p.m.
Message ID <11E08D716F0541429B7042699DD5C1A16B97BD8A@FMSMSX103.amr.corp.intel.com>
Download mbox | patch
Permalink /patch/244689/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Brandt, Todd E - May 17, 2013, 7:05 p.m.
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(-)

--
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
Greg KH - May 17, 2013, 7:11 p.m.
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
--
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
Brandt, Todd E - May 17, 2013, 7:29 p.m.
Hi, will do, thanks for the feedback. I used the goto to make the code addition smaller, but I obviously sacrificed code clarity.
Alan Stern - May 17, 2013, 7:44 p.m.
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

--
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
Brandt, Todd E - June 3, 2013, 9:10 p.m.
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
Alan Stern - June 4, 2013, 2:06 p.m.
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

--
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/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)) {