PM: check for complete cb before device lock in dpm_complete

Commit Message

Todd Brandt June 4, 2015, 10:08 p.m. UTC

On Fri, May 29, 2015 at 05:01:20PM -0400, Alan Stern wrote:
> On Fri, 29 May 2015, Todd Brandt wrote:
> 
> > In theory, if a device has no pm_ops complete callback it shouldn't have
> > to be locked in order to skip it in the dpm_complete call. This causes
> > problems when a device without a complete callback has already begun
> > operation after its resume cb is called. It can end up holding up the
> > system resume as the pm subsystem tries to get a device lock just to
> > check for a callback that isn't there.
> > 
> > This fixes an issue discovered on an Ivy Bridge laptop which has an
> > AlpsPS/2 GlidePoint touchpad connected to an i8042 serio bus. The resume
> > path ends up wasting a full second waiting for a device_lock on the psmouse
> > driver, only to then discover that it has no device_complete callback. The
> > alpa driver has already begun sending and receiving data after its resume
> > call was finished, which prevents the pm subsystem from getting the device
> > lock.
> 
> Why not simply move the device_lock() and device_unlock() calls inside
> the "if (callback) {...}" block in device_complete()?
> 
> Are you worried that the presence/absence of a callback might change 
> while the device is unlocked?  But that can happen with your patch too, 
> and there the window is much larger.

Hi Alan, I actually did that for my initial version of the patch but then
reconsidered. I was assuming someone would have an issue with reading
the callback while the device isn't locked. Below is the first version. It
has the exact same effect as the one in the top of the thread. Is this 
something that looks ok to you?

First version of the patch:

Signed-off-by: Todd Brandt <[e-mail]>
---
 drivers/base/power/main.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Comments

Alan Stern June 5, 2015, 3:04 p.m. UTC | #1

On Thu, 4 Jun 2015, Todd E Brandt wrote:

> Hi Alan, I actually did that for my initial version of the patch but then
> reconsidered. I was assuming someone would have an issue with reading
> the callback while the device isn't locked. Below is the first version. It
> has the exact same effect as the one in the top of the thread. Is this 
> something that looks ok to you?
> 
> First version of the patch:
> 
> Signed-off-by: Todd Brandt <[e-mail]>
> ---
>  drivers/base/power/main.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 3d874ec..b6a0e20 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -897,8 +897,6 @@ static void device_complete(struct device *dev, pm_message_t state)
>  	if (dev->power.syscore)
>  		return;
>  
> -	device_lock(dev);
> -
>  	if (dev->pm_domain) {
>  		info = "completing power domain ";
>  		callback = dev->pm_domain->ops.complete;
> @@ -918,12 +916,15 @@ static void device_complete(struct device *dev, pm_message_t state)
>  		callback = dev->driver->pm->complete;
>  	}
>  
> -	if (callback) {
> -		pm_dev_dbg(dev, state, info);
> -		trace_device_pm_callback_start(dev, info, state.event);
> -		callback(dev);
> -		trace_device_pm_callback_end(dev, 0);
> -	}
> +	if (!callback)
> +		return;
> +
> +	device_lock(dev);
> +
> +	pm_dev_dbg(dev, state, info);
> +	trace_device_pm_callback_start(dev, info, state.event);
> +	callback(dev);
> +	trace_device_pm_callback_end(dev, 0);
>  
>  	device_unlock(dev);

Well, this isn't quite right because we don't want to skip the 
pm_runtime_put(dev) that's just below the bottom of the patch.

More to the point, this does have a race with unregistration.  In
theory you could determine what "callback" is, and then another thread
could unregister the device (including its PM callbacks) before the
lock is acquired.  I don't know if that's liable to come up in
practice, although with asynchronous complete callbacks I suppose it
might.

A safer approach would work as follows: Figure out what "callback" is, 
and if it is NULL then skip the rest.  If it isn't, then lock the 
device, recompute "callback", and continue on like the existing 
routine.  This involves duplicating the computation of "callback", but 
that can be moved into a separate subroutine, so it's not so terrible.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
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 3d874ec..b6a0e20 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -897,8 +897,6 @@  static void device_complete(struct device *dev, pm_message_t state)
 	if (dev->power.syscore)
 		return;
 
-	device_lock(dev);
-
 	if (dev->pm_domain) {
 		info = "completing power domain ";
 		callback = dev->pm_domain->ops.complete;
@@ -918,12 +916,15 @@  static void device_complete(struct device *dev, pm_message_t state)
 		callback = dev->driver->pm->complete;
 	}
 
-	if (callback) {
-		pm_dev_dbg(dev, state, info);
-		trace_device_pm_callback_start(dev, info, state.event);
-		callback(dev);
-		trace_device_pm_callback_end(dev, 0);
-	}
+	if (!callback)
+		return;
+
+	device_lock(dev);
+
+	pm_dev_dbg(dev, state, info);
+	trace_device_pm_callback_start(dev, info, state.event);
+	callback(dev);
+	trace_device_pm_callback_end(dev, 0);
 
 	device_unlock(dev);