[PATCH] PM: check for complete cb before device lock in dpm_complete

[PATCH] PM: check for complete cb before device lock in dpm_complete

[Todd Brandt]

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.

Signed-off-by: Todd Brandt <todd.e.brandt@linux.intel.com>
---
 drivers/base/power/main.c | 15 +++++++++++++++
 include/linux/pm.h        |  1 +
 2 files changed, 16 insertions(+)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 3d874ec..30eb16b 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -94,6 +94,7 @@ void device_pm_sleep_init(struct device *dev)
 	dev->power.is_suspended = false;
 	dev->power.is_noirq_suspended = false;
 	dev->power.is_late_suspended = false;
+	dev->power.no_complete = false;
 	init_completion(&dev->power.completion);
 	complete_all(&dev->power.completion);
 	dev->power.wakeup = NULL;
@@ -897,6 +898,9 @@ static void device_complete(struct device *dev, pm_message_t state)
 	if (dev->power.syscore)
 		return;
 
+	if (dev->power.no_complete)
+		goto Complete;
+
 	device_lock(dev);
 
 	if (dev->pm_domain) {
@@ -927,6 +931,7 @@ static void device_complete(struct device *dev, pm_message_t state)
 
 	device_unlock(dev);
 
+Complete:
 	pm_runtime_put(dev);
 }
 
@@ -1591,6 +1596,16 @@ static int device_prepare(struct device *dev, pm_message_t state)
 		trace_device_pm_callback_end(dev, ret);
 	}
 
+	/* check for the existence of a complete callback while its locked */
+	if ((dev->pm_domain && dev->pm_domain->ops.complete) ||
+		(dev->type && dev->type->pm && dev->type->pm->complete) ||
+		(dev->class && dev->class->pm && dev->class->pm->complete) ||
+		(dev->bus && dev->bus->pm && dev->bus->pm->complete) ||
+		(dev->driver && dev->driver->pm && dev->driver->pm->complete))
+		dev->power.no_complete = false;
+	else
+		dev->power.no_complete = true;
+
 	device_unlock(dev);
 
 	if (ret < 0) {
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 2d29c64..1e334cd 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -553,6 +553,7 @@ struct dev_pm_info {
 	bool			ignore_children:1;
 	bool			early_init:1;	/* Owned by the PM core */
 	bool			direct_complete:1;	/* Owned by the PM core */
+	bool			no_complete:1;	/* Owned by the PM core */
 	spinlock_t		lock;
 #ifdef CONFIG_PM_SLEEP
 	struct list_head	entry;
-- 
2.1.0

Re: [PATCH] PM: check for complete cb before device lock in dpm_complete

[Alan Stern]

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.

Alan Stern

Re: [PATCH] PM: check for complete cb before device lock in dpm_complete

[Todd E Brandt]

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 <todd.e.brandt@linux.intel.com>
---
 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);
 
-- 
2.1.0

Re: [PATCH] PM: check for complete cb before device lock in dpm_complete

[Alan Stern]

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 <todd.e.brandt@linux.intel.com>
> ---
>  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