[PATCH 1/1] driver core: disable device's runtime pm during shutdown

[PATCH 1/1] driver core: disable device's runtime pm during shutdown

[Peter Chen]

There may be an issue when the user issue "reboot/shutdown" command, then
the device has shut down its hardware, after that, this runtime-pm featured
device's driver will probably be scheduled to do its suspend routine,
and at its suspend routine, it may access hardware, but the device has
already shutdown physically, then the system hang may be occurred.

I ran out this issue using an auto-suspend supported USB devices, like
3G modem, keyboard. The usb runtime suspend routine may be scheduled
after the usb controller has been shut down, and the usb runtime suspend
routine will try to suspend its roothub(controller), it will access
register, then the system hang occurs as the controller is shutdown.
Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/base/core.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index bc8729d..78445f4 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -22,6 +22,7 @@
 #include <linux/kallsyms.h>
 #include <linux/mutex.h>
 #include <linux/async.h>
+#include <linux/pm_runtime.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -1742,6 +1743,8 @@ void device_shutdown(void)
 		 */
 		list_del_init(&dev->kobj.entry);
 		spin_unlock(&devices_kset->list_lock);
+		/* Disable all device's runtime power management */
+		pm_runtime_disable(dev);
 
 		if (dev->bus && dev->bus->shutdown) {
 			dev_dbg(dev, "shutdown\n");
-- 
1.7.1

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[Ming Lei]

Hi,

On Mon, Nov 14, 2011 at 8:43 AM, Peter Chen <peter.chen@freescale.com> wrote:
> There may be an issue when the user issue "reboot/shutdown" command, then
> the device has shut down its hardware, after that, this runtime-pm featured
> device's driver will probably be scheduled to do its suspend routine,
> and at its suspend routine, it may access hardware, but the device has
> already shutdown physically, then the system hang may be occurred.
>
> I ran out this issue using an auto-suspend supported USB devices, like
> 3G modem, keyboard. The usb runtime suspend routine may be scheduled
> after the usb controller has been shut down, and the usb runtime suspend
> routine will try to suspend its roothub(controller), it will access
> register, then the system hang occurs as the controller is shutdown.
> Signed-off-by: Peter Chen <peter.chen@freescale.com>

This patch should make many guys happy about shutdown/reboot function,
at least fix the reboot issue on my panda board, maybe CC stable@kernel.org
is needed, :-)

> ---
>  drivers/base/core.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index bc8729d..78445f4 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -22,6 +22,7 @@
>  #include <linux/kallsyms.h>
>  #include <linux/mutex.h>
>  #include <linux/async.h>
> +#include <linux/pm_runtime.h>
>
>  #include "base.h"
>  #include "power/power.h"
> @@ -1742,6 +1743,8 @@ void device_shutdown(void)
>                 */
>                list_del_init(&dev->kobj.entry);
>                spin_unlock(&devices_kset->list_lock);
> +               /* Disable all device's runtime power management */
> +               pm_runtime_disable(dev);
>
>                if (dev->bus && dev->bus->shutdown) {
>                        dev_dbg(dev, "shutdown\n");
> --
> 1.7.1
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

thanks,
-- 
Ming Lei

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[Rafael J. Wysocki]

On Monday, November 14, 2011, Ming Lei wrote:
> Hi,
> 
> On Mon, Nov 14, 2011 at 8:43 AM, Peter Chen <peter.chen@freescale.com> wrote:
> > There may be an issue when the user issue "reboot/shutdown" command, then
> > the device has shut down its hardware, after that, this runtime-pm featured
> > device's driver will probably be scheduled to do its suspend routine,
> > and at its suspend routine, it may access hardware, but the device has
> > already shutdown physically, then the system hang may be occurred.
> >
> > I ran out this issue using an auto-suspend supported USB devices, like
> > 3G modem, keyboard. The usb runtime suspend routine may be scheduled
> > after the usb controller has been shut down, and the usb runtime suspend
> > routine will try to suspend its roothub(controller), it will access
> > register, then the system hang occurs as the controller is shutdown.
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> 
> This patch should make many guys happy about shutdown/reboot function,
> at least fix the reboot issue on my panda board, maybe CC stable@kernel.org
> is needed, :-)

I'm taking this as an ACK.

Rafael


> > ---
> >  drivers/base/core.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index bc8729d..78445f4 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/kallsyms.h>
> >  #include <linux/mutex.h>
> >  #include <linux/async.h>
> > +#include <linux/pm_runtime.h>
> >
> >  #include "base.h"
> >  #include "power/power.h"
> > @@ -1742,6 +1743,8 @@ void device_shutdown(void)
> >                 */
> >                list_del_init(&dev->kobj.entry);
> >                spin_unlock(&devices_kset->list_lock);
> > +               /* Disable all device's runtime power management */
> > +               pm_runtime_disable(dev);
> >
> >                if (dev->bus && dev->bus->shutdown) {
> >                        dev_dbg(dev, "shutdown\n");
> > --
> > 1.7.1
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> 
> thanks,
> 

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[Rafael J. Wysocki]

On Monday, November 14, 2011, Peter Chen wrote:
> There may be an issue when the user issue "reboot/shutdown" command, then
> the device has shut down its hardware, after that, this runtime-pm featured
> device's driver will probably be scheduled to do its suspend routine,
> and at its suspend routine, it may access hardware, but the device has
> already shutdown physically, then the system hang may be occurred.
> 
> I ran out this issue using an auto-suspend supported USB devices, like
> 3G modem, keyboard. The usb runtime suspend routine may be scheduled
> after the usb controller has been shut down, and the usb runtime suspend
> routine will try to suspend its roothub(controller), it will access
> register, then the system hang occurs as the controller is shutdown.
> Signed-off-by: Peter Chen <peter.chen@freescale.com>

Greg, do you want me to take this one?

Rafael


> ---
>  drivers/base/core.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index bc8729d..78445f4 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -22,6 +22,7 @@
>  #include <linux/kallsyms.h>
>  #include <linux/mutex.h>
>  #include <linux/async.h>
> +#include <linux/pm_runtime.h>
>  
>  #include "base.h"
>  #include "power/power.h"
> @@ -1742,6 +1743,8 @@ void device_shutdown(void)
>  		 */
>  		list_del_init(&dev->kobj.entry);
>  		spin_unlock(&devices_kset->list_lock);
> +		/* Disable all device's runtime power management */
> +		pm_runtime_disable(dev);
>  
>  		if (dev->bus && dev->bus->shutdown) {
>  			dev_dbg(dev, "shutdown\n");
> 

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[Greg KH]

On Mon, Nov 14, 2011 at 11:27:37PM +0100, Rafael J. Wysocki wrote:
> On Monday, November 14, 2011, Peter Chen wrote:
> > There may be an issue when the user issue "reboot/shutdown" command, then
> > the device has shut down its hardware, after that, this runtime-pm featured
> > device's driver will probably be scheduled to do its suspend routine,
> > and at its suspend routine, it may access hardware, but the device has
> > already shutdown physically, then the system hang may be occurred.
> > 
> > I ran out this issue using an auto-suspend supported USB devices, like
> > 3G modem, keyboard. The usb runtime suspend routine may be scheduled
> > after the usb controller has been shut down, and the usb runtime suspend
> > routine will try to suspend its roothub(controller), it will access
> > register, then the system hang occurs as the controller is shutdown.
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> 
> Greg, do you want me to take this one?

Please do:
	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

thanks,

greg k-h

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[Rafael J. Wysocki]

On Tuesday, November 15, 2011, Greg KH wrote:
> On Mon, Nov 14, 2011 at 11:27:37PM +0100, Rafael J. Wysocki wrote:
> > On Monday, November 14, 2011, Peter Chen wrote:
> > > There may be an issue when the user issue "reboot/shutdown" command, then
> > > the device has shut down its hardware, after that, this runtime-pm featured
> > > device's driver will probably be scheduled to do its suspend routine,
> > > and at its suspend routine, it may access hardware, but the device has
> > > already shutdown physically, then the system hang may be occurred.
> > > 
> > > I ran out this issue using an auto-suspend supported USB devices, like
> > > 3G modem, keyboard. The usb runtime suspend routine may be scheduled
> > > after the usb controller has been shut down, and the usb runtime suspend
> > > routine will try to suspend its roothub(controller), it will access
> > > register, then the system hang occurs as the controller is shutdown.
> > > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > 
> > Greg, do you want me to take this one?
> 
> Please do:
> 	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

Done, thanks!

Rafael

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2337 bytes --]

On Wed, 16 Nov 2011 00:16:08 +0100 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Tuesday, November 15, 2011, Greg KH wrote:
> > On Mon, Nov 14, 2011 at 11:27:37PM +0100, Rafael J. Wysocki wrote:
> > > On Monday, November 14, 2011, Peter Chen wrote:
> > > > There may be an issue when the user issue "reboot/shutdown" command, then
> > > > the device has shut down its hardware, after that, this runtime-pm featured
> > > > device's driver will probably be scheduled to do its suspend routine,
> > > > and at its suspend routine, it may access hardware, but the device has
> > > > already shutdown physically, then the system hang may be occurred.
> > > > 
> > > > I ran out this issue using an auto-suspend supported USB devices, like
> > > > 3G modem, keyboard. The usb runtime suspend routine may be scheduled
> > > > after the usb controller has been shut down, and the usb runtime suspend
> > > > routine will try to suspend its roothub(controller), it will access
> > > > register, then the system hang occurs as the controller is shutdown.
> > > > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > > 
> > > Greg, do you want me to take this one?
> > 
> > Please do:
> > 	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> 
> Done, thanks!
> 

Hi,
 this patches causes a problem for me.

Specifically it makes it impossible to power-down a device which uses twl4030
for power control on an omap3 processor.

To perform the shutdown we need to send a command over the i2c bus.
The relevant bus is called omap_i2c.1 and this is normally in suspend mode.
When a request is sent, omap_i2c_xfer uses pm_runtime_get_sync to wake it up,
performs the transfer, then calls pm_runtime_put to let it go back to sleep.

So it is asleep when the new pm_runtime_disable() call is made, so it stays
asleep, omap_i2c_xfer cannot wake it, the transfer doesn't happen and the
system doesn't get powered off.

So here is a device that should *not* have pm disabled at shutdown.

So I feel this fix is a little too heavy-handed.
I don't fully understand the problem scenario described above but it seems to
me that if the auto-suspend timer can fire after the hardware has been shut
down, then maybe the hardware-shutdown should be disabling that timer.  Maybe?

Suggestions?

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[Alan Stern]

On Mon, 5 Dec 2011, NeilBrown wrote:

> Hi,
>  this patches causes a problem for me.
> 
> Specifically it makes it impossible to power-down a device which uses twl4030
> for power control on an omap3 processor.
> 
> To perform the shutdown we need to send a command over the i2c bus.
> The relevant bus is called omap_i2c.1 and this is normally in suspend mode.
> When a request is sent, omap_i2c_xfer uses pm_runtime_get_sync to wake it up,
> performs the transfer, then calls pm_runtime_put to let it go back to sleep.
> 
> So it is asleep when the new pm_runtime_disable() call is made, so it stays
> asleep, omap_i2c_xfer cannot wake it, the transfer doesn't happen and the
> system doesn't get powered off.

In other words, to perform the system shutdown you need to send a 
command over the i2c bus after the bus controller's shutdown routine 
has been called?

> So here is a device that should *not* have pm disabled at shutdown.

Or maybe it shouldn't be shut down at all.

> So I feel this fix is a little too heavy-handed.
> I don't fully understand the problem scenario described above but it seems to
> me that if the auto-suspend timer can fire after the hardware has been shut
> down, then maybe the hardware-shutdown should be disabling that timer.  Maybe?

That's not robust.  The timer can be restarted, and there are other 
ways of initiating runtime PM besides the timer.

> Suggestions?

Can the shutdown routine for the i2c controller simply call
pm_runtime_enable()?

Alan Stern

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2902 bytes --]

On Sun, 4 Dec 2011 21:42:02 -0500 (EST) Alan Stern
<stern@rowland.harvard.edu> wrote:

> On Mon, 5 Dec 2011, NeilBrown wrote:
> 
> > Hi,
> >  this patches causes a problem for me.
> > 
> > Specifically it makes it impossible to power-down a device which uses twl4030
> > for power control on an omap3 processor.
> > 
> > To perform the shutdown we need to send a command over the i2c bus.
> > The relevant bus is called omap_i2c.1 and this is normally in suspend mode.
> > When a request is sent, omap_i2c_xfer uses pm_runtime_get_sync to wake it up,
> > performs the transfer, then calls pm_runtime_put to let it go back to sleep.
> > 
> > So it is asleep when the new pm_runtime_disable() call is made, so it stays
> > asleep, omap_i2c_xfer cannot wake it, the transfer doesn't happen and the
> > system doesn't get powered off.
> 
> In other words, to perform the system shutdown you need to send a 
> command over the i2c bus after the bus controller's shutdown routine 
> has been called?

Correct.
Though in this case the i2c bus doesn't have a shutdown routine so it doesn't
cause a problem.  It is only the inability to pm_resume that is a problem.


> 
> > So here is a device that should *not* have pm disabled at shutdown.
> 
> Or maybe it shouldn't be shut down at all.

Maybe not.  Current 'device_shutdown()' shuts down every device.
I guess having a "don't shutdown at power-off" flag could be used to solve
the problem.

> 
> > So I feel this fix is a little too heavy-handed.
> > I don't fully understand the problem scenario described above but it seems to
> > me that if the auto-suspend timer can fire after the hardware has been shut
> > down, then maybe the hardware-shutdown should be disabling that timer.  Maybe?
> 
> That's not robust.  The timer can be restarted, and there are other 
> ways of initiating runtime PM besides the timer.

Still, shouldn't they notice that the hardware has been shutdown and so not
do anything?

> 
> > Suggestions?
> 
> Can the shutdown routine for the i2c controller simply call
> pm_runtime_enable()?

No.
The shutdown routine is in drivers/mfd/twl4030-power.c (not in mainline
currently but there are various patches floating around that add
twl4030_poweroff, and this is the obviously-correct place for the code).
It has a 'struct i2c_adapter' device.
The device that needs to have pm_runtime_enable called on it is a 'struct
platform_device' that is linked through the dev_data of the i2c_adapter.

i.e. only omap-i2c specific code knows how to find the device that needs to
be enabled.  The twl4030 code doesn't.

So even if we had a used_for_shutdown flag in dev_pm_info I don't know how we
would manage to set it for the right device.... unless we had a dev_pm_ops
method to ask a device to set it on all devices it depends on.  Sounds messy.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[Ming Lei]

Hi,

On Mon, Dec 5, 2011 at 5:56 AM, NeilBrown <neilb@suse.de> wrote:
>
> Hi,
>  this patches causes a problem for me.
>
> Specifically it makes it impossible to power-down a device which uses twl4030
> for power control on an omap3 processor.
>
> To perform the shutdown we need to send a command over the i2c bus.
> The relevant bus is called omap_i2c.1 and this is normally in suspend mode.
> When a request is sent, omap_i2c_xfer uses pm_runtime_get_sync to wake it up,
> performs the transfer, then calls pm_runtime_put to let it go back to sleep.
>
> So it is asleep when the new pm_runtime_disable() call is made, so it stays
> asleep, omap_i2c_xfer cannot wake it, the transfer doesn't happen and the
> system doesn't get powered off.
>
> So here is a device that should *not* have pm disabled at shutdown.
>
> So I feel this fix is a little too heavy-handed.

Maybe the device's runtime PM should not be disabled if
there is no ->shutdown defined in its driver, how about the blew?

diff --git a/drivers/base/core.c b/drivers/base/core.c
index d8b3d89..ca30659 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1743,14 +1743,16 @@ void device_shutdown(void)
 		 */
 		list_del_init(&dev->kobj.entry);
 		spin_unlock(&devices_kset->list_lock);
-		/* Disable all device's runtime power management */
-		pm_runtime_disable(dev);

+		/* Disable the device's runtime power management if
+		 * it is to be shutdown*/
 		if (dev->bus && dev->bus->shutdown) {
 			dev_dbg(dev, "shutdown\n");
+			pm_runtime_disable(dev);
 			dev->bus->shutdown(dev);
 		} else if (dev->driver && dev->driver->shutdown) {
 			dev_dbg(dev, "shutdown\n");
+			pm_runtime_disable(dev);
 			dev->driver->shutdown(dev);
 		}
 		put_device(dev);


> I don't fully understand the problem scenario described above but it seems to
> me that if the auto-suspend timer can fire after the hardware has been shut
> down, then maybe the hardware-shutdown should be disabling that timer.  Maybe?
>
> Suggestions?
>
> Thanks,
> NeilBrown


thanks,
-- 
Ming Lei

RE: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[Chen Peter-B29397]

> >  this patches causes a problem for me.
> >
> > Specifically it makes it impossible to power-down a device which uses
> twl4030
> > for power control on an omap3 processor.
> >
> > To perform the shutdown we need to send a command over the i2c bus.
> > The relevant bus is called omap_i2c.1 and this is normally in suspend
> mode.
> > When a request is sent, omap_i2c_xfer uses pm_runtime_get_sync to wake
> it up,
> > performs the transfer, then calls pm_runtime_put to let it go back to
> sleep.
> >
> > So it is asleep when the new pm_runtime_disable() call is made, so it
> stays
> > asleep, omap_i2c_xfer cannot wake it, the transfer doesn't happen and
> the
> > system doesn't get powered off.
> >
> > So here is a device that should *not* have pm disabled at shutdown.
> >
> > So I feel this fix is a little too heavy-handed.
> 
> Maybe the device's runtime PM should not be disabled if
> there is no ->shutdown defined in its driver, how about the blew?
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d8b3d89..ca30659 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1743,14 +1743,16 @@ void device_shutdown(void)
>  		 */
>  		list_del_init(&dev->kobj.entry);
>  		spin_unlock(&devices_kset->list_lock);
> -		/* Disable all device's runtime power management */
> -		pm_runtime_disable(dev);
> 
> +		/* Disable the device's runtime power management if
> +		 * it is to be shutdown*/
>  		if (dev->bus && dev->bus->shutdown) {
>  			dev_dbg(dev, "shutdown\n");
> +			pm_runtime_disable(dev);
>  			dev->bus->shutdown(dev);
>  		} else if (dev->driver && dev->driver->shutdown) {
>  			dev_dbg(dev, "shutdown\n");
> +			pm_runtime_disable(dev);
>  			dev->driver->shutdown(dev);
>  		}
>  		put_device(dev);
> 
> 
Yes, it is the solution for Neil's case, but I am not sure for all.
Is it possible to resume driver's itself at driver's shutdown?

> > I don't fully understand the problem scenario described above but it
> seems to
> > me that if the auto-suspend timer can fire after the hardware has been
> shut
> > down, then maybe the hardware-shutdown should be disabling that
> timer.  Maybe?
> >
> > Suggestions?
> >
> > Thanks,
> > NeilBrown
> 
> 
> thanks,
> --
> Ming Lei

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[Ming Lei]

Hi,

On Mon, Dec 5, 2011 at 12:42 PM, Chen Peter-B29397 <B29397@freescale.com> wrote:
>
>
>> >  this patches causes a problem for me.
>> >
>> > Specifically it makes it impossible to power-down a device which uses
>> twl4030
>> > for power control on an omap3 processor.
>> >
>> > To perform the shutdown we need to send a command over the i2c bus.
>> > The relevant bus is called omap_i2c.1 and this is normally in suspend
>> mode.
>> > When a request is sent, omap_i2c_xfer uses pm_runtime_get_sync to wake
>> it up,
>> > performs the transfer, then calls pm_runtime_put to let it go back to
>> sleep.
>> >
>> > So it is asleep when the new pm_runtime_disable() call is made, so it
>> stays
>> > asleep, omap_i2c_xfer cannot wake it, the transfer doesn't happen and
>> the
>> > system doesn't get powered off.
>> >
>> > So here is a device that should *not* have pm disabled at shutdown.
>> >
>> > So I feel this fix is a little too heavy-handed.
>>
>> Maybe the device's runtime PM should not be disabled if
>> there is no ->shutdown defined in its driver, how about the blew?
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index d8b3d89..ca30659 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -1743,14 +1743,16 @@ void device_shutdown(void)
>>                */
>>               list_del_init(&dev->kobj.entry);
>>               spin_unlock(&devices_kset->list_lock);
>> -             /* Disable all device's runtime power management */
>> -             pm_runtime_disable(dev);
>>
>> +             /* Disable the device's runtime power management if
>> +              * it is to be shutdown*/
>>               if (dev->bus && dev->bus->shutdown) {
>>                       dev_dbg(dev, "shutdown\n");
>> +                     pm_runtime_disable(dev);
>>                       dev->bus->shutdown(dev);
>>               } else if (dev->driver && dev->driver->shutdown) {
>>                       dev_dbg(dev, "shutdown\n");
>> +                     pm_runtime_disable(dev);
>>                       dev->driver->shutdown(dev);
>>               }
>>               put_device(dev);
>>
>>
> Yes, it is the solution for Neil's case, but I am not sure for all.
> Is it possible to resume driver's itself at driver's shutdown?

Looks like this way is still not good. Maybe we should put all
device into active state first, then set its runtime pm as disabled.

thanks,
-- 
Ming Lei

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[Ming Lei]

Hi,

On Mon, Dec 5, 2011 at 1:12 PM, Ming Lei <tom.leiming@gmail.com> wrote:
> Hi,
>
> On Mon, Dec 5, 2011 at 12:42 PM, Chen Peter-B29397 <B29397@freescale.com> wrote:
>>
>>
>>> >  this patches causes a problem for me.
>>> >
>>> > Specifically it makes it impossible to power-down a device which uses
>>> twl4030
>>> > for power control on an omap3 processor.
>>> >
>>> > To perform the shutdown we need to send a command over the i2c bus.
>>> > The relevant bus is called omap_i2c.1 and this is normally in suspend
>>> mode.
>>> > When a request is sent, omap_i2c_xfer uses pm_runtime_get_sync to wake
>>> it up,
>>> > performs the transfer, then calls pm_runtime_put to let it go back to
>>> sleep.
>>> >
>>> > So it is asleep when the new pm_runtime_disable() call is made, so it
>>> stays
>>> > asleep, omap_i2c_xfer cannot wake it, the transfer doesn't happen and
>>> the
>>> > system doesn't get powered off.
>>> >
>>> > So here is a device that should *not* have pm disabled at shutdown.
>>> >
>>> > So I feel this fix is a little too heavy-handed.
>>>
>>> Maybe the device's runtime PM should not be disabled if
>>> there is no ->shutdown defined in its driver, how about the blew?
>>>
>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>> index d8b3d89..ca30659 100644
>>> --- a/drivers/base/core.c
>>> +++ b/drivers/base/core.c
>>> @@ -1743,14 +1743,16 @@ void device_shutdown(void)
>>>                */
>>>               list_del_init(&dev->kobj.entry);
>>>               spin_unlock(&devices_kset->list_lock);
>>> -             /* Disable all device's runtime power management */
>>> -             pm_runtime_disable(dev);
>>>
>>> +             /* Disable the device's runtime power management if
>>> +              * it is to be shutdown*/
>>>               if (dev->bus && dev->bus->shutdown) {
>>>                       dev_dbg(dev, "shutdown\n");
>>> +                     pm_runtime_disable(dev);
>>>                       dev->bus->shutdown(dev);
>>>               } else if (dev->driver && dev->driver->shutdown) {
>>>                       dev_dbg(dev, "shutdown\n");
>>> +                     pm_runtime_disable(dev);
>>>                       dev->driver->shutdown(dev);
>>>               }
>>>               put_device(dev);
>>>
>>>
>> Yes, it is the solution for Neil's case, but I am not sure for all.
>> Is it possible to resume driver's itself at driver's shutdown?
>
> Looks like this way is still not good. Maybe we should put all
> device into active state first, then set its runtime pm as disabled.

Below patch should a simple fix on the issue, but it may prolong
shutdown/reboot time.


diff --git a/drivers/base/core.c b/drivers/base/core.cindex
d8b3d89..96b266c 100644--- a/drivers/base/core.c+++
b/drivers/base/core.c@@ -1743,8 +1743,9 @@ void device_shutdown(void)
		 */ 		list_del_init(&dev->kobj.entry);
		spin_unlock(&devices_kset->list_lock);-		/* Disable all device's
runtime power management */-		pm_runtime_disable(dev);++		/* put
device into active state and forbid runtime pm
*/+		pm_runtime_forbid(dev);  		if (dev->bus && dev->bus->shutdown) {
			dev_dbg(dev, "shutdown\n");

thanks,
-- 
Ming Lei

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[Ming Lei]

Hi,

On Mon, Dec 5, 2011 at 5:01 PM, Ming Lei <tom.leiming@gmail.com> wrote:

>>>
>>>
>>>> >  this patches causes a problem for me.
>>>> >
>>>> > Specifically it makes it impossible to power-down a device which uses
>>>> twl4030
>>>> > for power control on an omap3 processor.
>>>> >
>>>> > To perform the shutdown we need to send a command over the i2c bus.
>>>> > The relevant bus is called omap_i2c.1 and this is normally in suspend
>>>> mode.
>>>> > When a request is sent, omap_i2c_xfer uses pm_runtime_get_sync to wake
>>>> it up,
>>>> > performs the transfer, then calls pm_runtime_put to let it go back to
>>>> sleep.
>>>> >
>>>> > So it is asleep when the new pm_runtime_disable() call is made, so it
>>>> stays
>>>> > asleep, omap_i2c_xfer cannot wake it, the transfer doesn't happen and
>>>> the
>>>> > system doesn't get powered off.
>>>> >
>>>> > So here is a device that should *not* have pm disabled at shutdown.
>>>> >
>>>> > So I feel this fix is a little too heavy-handed.
>>>>
>>>> Maybe the device's runtime PM should not be disabled if
>>>> there is no ->shutdown defined in its driver, how about the blew?
>>>>
>>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>>> index d8b3d89..ca30659 100644
>>>> --- a/drivers/base/core.c
>>>> +++ b/drivers/base/core.c
>>>> @@ -1743,14 +1743,16 @@ void device_shutdown(void)
>>>>                */
>>>>               list_del_init(&dev->kobj.entry);
>>>>               spin_unlock(&devices_kset->list_lock);
>>>> -             /* Disable all device's runtime power management */
>>>> -             pm_runtime_disable(dev);
>>>>
>>>> +             /* Disable the device's runtime power management if
>>>> +              * it is to be shutdown*/
>>>>               if (dev->bus && dev->bus->shutdown) {
>>>>                       dev_dbg(dev, "shutdown\n");
>>>> +                     pm_runtime_disable(dev);
>>>>                       dev->bus->shutdown(dev);
>>>>               } else if (dev->driver && dev->driver->shutdown) {
>>>>                       dev_dbg(dev, "shutdown\n");
>>>> +                     pm_runtime_disable(dev);
>>>>                       dev->driver->shutdown(dev);
>>>>               }
>>>>               put_device(dev);
>>>>
>>>>
>>> Yes, it is the solution for Neil's case, but I am not sure for all.
>>> Is it possible to resume driver's itself at driver's shutdown?
>>
>> Looks like this way is still not good. Maybe we should put all
>> device into active state first, then set its runtime pm as disabled.
>
> Below patch should a simple fix on the issue, but it may prolong
> shutdown/reboot time.
>
>
> diff --git a/drivers/base/core.c b/drivers/base/core.cindex
> d8b3d89..96b266c 100644--- a/drivers/base/core.c+++
> b/drivers/base/core.c@@ -1743,8 +1743,9 @@ void device_shutdown(void)
>                 */             list_del_init(&dev->kobj.entry);
>                spin_unlock(&devices_kset->list_lock);-         /* Disable all device's
> runtime power management */-            pm_runtime_disable(dev);++              /* put
> device into active state and forbid runtime pm
> */+             pm_runtime_forbid(dev);                 if (dev->bus && dev->bus->shutdown) {
>                        dev_dbg(dev, "shutdown\n");
,
Sorry, the above is line wrapped badly, see the below:

diff --git a/drivers/base/core.c b/drivers/base/core.c
index d8b3d89..96b266c 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1743,8 +1743,9 @@ void device_shutdown(void)
 		 */
 		list_del_init(&dev->kobj.entry);
 		spin_unlock(&devices_kset->list_lock);
-		/* Disable all device's runtime power management */
-		pm_runtime_disable(dev);
+
+		/* put device into active state and forbit runtime pm */
+		pm_runtime_forbid(dev);

 		if (dev->bus && dev->bus->shutdown) {
 			dev_dbg(dev, "shutdown\n");


thanks,
-- 
Ming Lei

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[Alan Stern]

On Mon, 5 Dec 2011, Ming Lei wrote:

> Sorry, the above is line wrapped badly, see the below:
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d8b3d89..96b266c 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1743,8 +1743,9 @@ void device_shutdown(void)
>  		 */
>  		list_del_init(&dev->kobj.entry);
>  		spin_unlock(&devices_kset->list_lock);
> -		/* Disable all device's runtime power management */
> -		pm_runtime_disable(dev);
> +
> +		/* put device into active state and forbit runtime pm */
> +		pm_runtime_forbid(dev);
> 
>  		if (dev->bus && dev->bus->shutdown) {
>  			dev_dbg(dev, "shutdown\n");

We don't want to put devices into the active state when it's not 
necessary.  A better approach would be:

		/* Don't allow any more runtime suspends */
		pm_runtime_get_noresume(dev);
		pm_runtime_barrier(dev);

Alan Stern

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2540 bytes --]

On Mon, 5 Dec 2011 11:02:38 -0500 (EST) Alan Stern
<stern@rowland.harvard.edu> wrote:

> On Mon, 5 Dec 2011, Ming Lei wrote:
> 
> > Sorry, the above is line wrapped badly, see the below:
> > 
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index d8b3d89..96b266c 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -1743,8 +1743,9 @@ void device_shutdown(void)
> >  		 */
> >  		list_del_init(&dev->kobj.entry);
> >  		spin_unlock(&devices_kset->list_lock);
> > -		/* Disable all device's runtime power management */
> > -		pm_runtime_disable(dev);
> > +
> > +		/* put device into active state and forbit runtime pm */
> > +		pm_runtime_forbid(dev);
> > 
> >  		if (dev->bus && dev->bus->shutdown) {
> >  			dev_dbg(dev, "shutdown\n");
> 
> We don't want to put devices into the active state when it's not 
> necessary.  A better approach would be:
> 
> 		/* Don't allow any more runtime suspends */
> 		pm_runtime_get_noresume(dev);
> 		pm_runtime_barrier(dev);
> 
> Alan Stern

That sounds like a reasonable approach if we really need to do something at
this level.  But is this the only place that ->shutdown methods are called
from?  If they are called from elsewhere, would those places need the
same pm_runtime protection?

BTW I was wrong when I said that only calling pm_runtime_disable if there was
a ->shutdown function would not work for me. i.e. the following patch does
solve my particular issue (though I'm not sure it is "right").
I was getting confused by the two different devices: the i2c device and the
platform device.
The i2c device has a ->shutdown which does nothing, but doesn't need to wake
up.
The platform device is the one which needs to wake up, but it doesn't have a
->shutdown function is this patch causes it not have pm_runtime disabled.

Thanks,
NeilBrown

diff --git a/drivers/base/core.c b/drivers/base/core.c
index d8b3d89..b9aa5d2 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1743,13 +1743,13 @@ void device_shutdown(void)
 		 */
 		list_del_init(&dev->kobj.entry);
 		spin_unlock(&devices_kset->list_lock);
-		/* Disable all device's runtime power management */
-		pm_runtime_disable(dev);
 
 		if (dev->bus && dev->bus->shutdown) {
+			pm_runtime_disable(dev);
 			dev_dbg(dev, "shutdown\n");
 			dev->bus->shutdown(dev);
 		} else if (dev->driver && dev->driver->shutdown) {
+			pm_runtime_disable(dev);
 			dev_dbg(dev, "shutdown\n");
 			dev->driver->shutdown(dev);
 		}

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[Alan Stern]

On Tue, 6 Dec 2011, NeilBrown wrote:

> > We don't want to put devices into the active state when it's not 
> > necessary.  A better approach would be:
> > 
> > 		/* Don't allow any more runtime suspends */
> > 		pm_runtime_get_noresume(dev);
> > 		pm_runtime_barrier(dev);
> > 
> > Alan Stern
> 
> That sounds like a reasonable approach if we really need to do something at
> this level.  But is this the only place that ->shutdown methods are called
> from?  If they are called from elsewhere, would those places need the
> same pm_runtime protection?

I don't know if shutdown methods are called from anywhere else, but 
they shouldn't be.  The kerneldoc for struct bus_type plainly says:

* @shutdown:	Called at shut-down time to quiesce the device.

> BTW I was wrong when I said that only calling pm_runtime_disable if there was
> a ->shutdown function would not work for me. i.e. the following patch does
> solve my particular issue (though I'm not sure it is "right").
> I was getting confused by the two different devices: the i2c device and the
> platform device.
> The i2c device has a ->shutdown which does nothing, but doesn't need to wake
> up.
> The platform device is the one which needs to wake up, but it doesn't have a
> ->shutdown function is this patch causes it not have pm_runtime disabled.
> 
> Thanks,
> NeilBrown
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d8b3d89..b9aa5d2 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1743,13 +1743,13 @@ void device_shutdown(void)
>  		 */
>  		list_del_init(&dev->kobj.entry);
>  		spin_unlock(&devices_kset->list_lock);
> -		/* Disable all device's runtime power management */
> -		pm_runtime_disable(dev);
>  
>  		if (dev->bus && dev->bus->shutdown) {
> +			pm_runtime_disable(dev);
>  			dev_dbg(dev, "shutdown\n");
>  			dev->bus->shutdown(dev);
>  		} else if (dev->driver && dev->driver->shutdown) {
> +			pm_runtime_disable(dev);
>  			dev_dbg(dev, "shutdown\n");
>  			dev->driver->shutdown(dev);
>  		}

Still, it's quite conceivable that a shutdown routine might want to 
resume a device that had been runtime-suspended.  Disabling runtime PM 
for that device would prevent the routine from doing its work.

The original problem the $SUBJECT patch was meant to solve was that a
runtime-PM suspend method was called after the shutdown routine had
run.  Doing a runtime_pm_get_noresume() ought to solve this.

There still remains the possibility of a runtime resume method being
called after the shutdown routine.  So far nobody has complained about
that happening except you -- and your complaint was that it didn't
work, not that it shouldn't happen.  But if necessary, individual
drivers could add pm_runtime_disable() calls to their shutdown
routines.

Alan Stern

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[Rafael J. Wysocki]

On Monday, December 05, 2011, Alan Stern wrote:
> On Tue, 6 Dec 2011, NeilBrown wrote:
> 
> > > We don't want to put devices into the active state when it's not 
> > > necessary.  A better approach would be:
> > > 
> > > 		/* Don't allow any more runtime suspends */
> > > 		pm_runtime_get_noresume(dev);
> > > 		pm_runtime_barrier(dev);

That's exactly what we do during system suspend and I _really_ think it's
not a good idea to do anything different (given that the original "disable"
idea didn't work), because that may result in serious amount of confusion.

> > 
> > That sounds like a reasonable approach if we really need to do something at
> > this level.

We do.

> > But is this the only place that ->shutdown methods are called
> > from?  If they are called from elsewhere, would those places need the
> > same pm_runtime protection?
> 
> I don't know if shutdown methods are called from anywhere else, but 
> they shouldn't be.  The kerneldoc for struct bus_type plainly says:
> 
> * @shutdown:	Called at shut-down time to quiesce the device.

It's a bug to run the shutdown callback in any other situation.

> > BTW I was wrong when I said that only calling pm_runtime_disable if there was
> > a ->shutdown function would not work for me. i.e. the following patch does
> > solve my particular issue (though I'm not sure it is "right").
> > I was getting confused by the two different devices: the i2c device and the
> > platform device.
> > The i2c device has a ->shutdown which does nothing, but doesn't need to wake
> > up.
> > The platform device is the one which needs to wake up, but it doesn't have a
> > ->shutdown function is this patch causes it not have pm_runtime disabled.
> > 
> > Thanks,
> > NeilBrown
> > 
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index d8b3d89..b9aa5d2 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -1743,13 +1743,13 @@ void device_shutdown(void)
> >  		 */
> >  		list_del_init(&dev->kobj.entry);
> >  		spin_unlock(&devices_kset->list_lock);
> > -		/* Disable all device's runtime power management */
> > -		pm_runtime_disable(dev);
> >  
> >  		if (dev->bus && dev->bus->shutdown) {
> > +			pm_runtime_disable(dev);
> >  			dev_dbg(dev, "shutdown\n");
> >  			dev->bus->shutdown(dev);
> >  		} else if (dev->driver && dev->driver->shutdown) {
> > +			pm_runtime_disable(dev);
> >  			dev_dbg(dev, "shutdown\n");
> >  			dev->driver->shutdown(dev);
> >  		}
> 
> Still, it's quite conceivable that a shutdown routine might want to 
> resume a device that had been runtime-suspended.  Disabling runtime PM 
> for that device would prevent the routine from doing its work.
> 
> The original problem the $SUBJECT patch was meant to solve was that a
> runtime-PM suspend method was called after the shutdown routine had
> run.  Doing a runtime_pm_get_noresume() ought to solve this.

That's correct.

> There still remains the possibility of a runtime resume method being
> called after the shutdown routine.  So far nobody has complained about
> that happening except you -- and your complaint was that it didn't
> work, not that it shouldn't happen.  But if necessary, individual
> drivers could add pm_runtime_disable() calls to their shutdown
> routines.

I agree.

The whole problem is that .shutdown() had been there before the PM
callbacks were added (let alone runtime PM) and it pretty much duplicates
.pm->poweroff().  Also, it doesn't handle the case when the _noirq()
callbacks are necessary (although it's very hard to trigger in practice, if
possible at all).

Anyway, since .shutdown() is quite analogous to .pm->poweroff(), the core
should treat them both consistently and do the same to synchronize with
runtime PM before calling them.  Whatever is needed beyond that, should be
done by drivers and subsystems.

So, Alan, can you please post a patch implementing your proposed approach
against the current Linus' tree?

Rafael

[PATCH] Driver core: leave runtime PM enabled during system shutdown

[Alan Stern]

Disabling all runtime PM during system shutdown turns out not to be a
good idea, because some devices may need to be woken up from a
low-power state at that time.

The whole point of disabling runtime PM for system shutdown was to
prevent untimely runtime-suspend method calls.  This patch (as1504)
accomplishes the same result by incrementing the usage count for each
device and waiting for ongoing runtime-PM callbacks to finish.  This
is what we already do during system suspend and hibernation, which
makes sense since the shutdown method is pretty much a legacy analog
of the pm->poweroff method.

This fixes a regression on some OMAP systems.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Reported-by: NeilBrown <neilb@suse.de>
CC: Chen Peter-B29397 <B29397@freescale.com>

---

 drivers/base/core.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: usb-3.2/drivers/base/core.c
===================================================================
--- usb-3.2.orig/drivers/base/core.c
+++ usb-3.2/drivers/base/core.c
@@ -1743,8 +1743,10 @@ void device_shutdown(void)
 		 */
 		list_del_init(&dev->kobj.entry);
 		spin_unlock(&devices_kset->list_lock);
-		/* Disable all device's runtime power management */
-		pm_runtime_disable(dev);
+
+		/* Don't allow any more runtime suspends */
+		pm_runtime_get_noresume(dev);
+		pm_runtime_barrier(dev);
 
 		if (dev->bus && dev->bus->shutdown) {
 			dev_dbg(dev, "shutdown\n");

Re: [PATCH] Driver core: leave runtime PM enabled during system shutdown

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1815 bytes --]

On Tue, 6 Dec 2011 10:26:14 -0500 (EST) Alan Stern
<stern@rowland.harvard.edu> wrote:

> Disabling all runtime PM during system shutdown turns out not to be a
> good idea, because some devices may need to be woken up from a
> low-power state at that time.
> 
> The whole point of disabling runtime PM for system shutdown was to
> prevent untimely runtime-suspend method calls.  This patch (as1504)
> accomplishes the same result by incrementing the usage count for each
> device and waiting for ongoing runtime-PM callbacks to finish.  This
> is what we already do during system suspend and hibernation, which
> makes sense since the shutdown method is pretty much a legacy analog
> of the pm->poweroff method.
> 
> This fixes a regression on some OMAP systems.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Reported-by: NeilBrown <neilb@suse.de>
> CC: Chen Peter-B29397 <B29397@freescale.com>
> 
> ---
> 
>  drivers/base/core.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Index: usb-3.2/drivers/base/core.c
> ===================================================================
> --- usb-3.2.orig/drivers/base/core.c
> +++ usb-3.2/drivers/base/core.c
> @@ -1743,8 +1743,10 @@ void device_shutdown(void)
>  		 */
>  		list_del_init(&dev->kobj.entry);
>  		spin_unlock(&devices_kset->list_lock);
> -		/* Disable all device's runtime power management */
> -		pm_runtime_disable(dev);
> +
> +		/* Don't allow any more runtime suspends */
> +		pm_runtime_get_noresume(dev);
> +		pm_runtime_barrier(dev);
>  
>  		if (dev->bus && dev->bus->shutdown) {
>  			dev_dbg(dev, "shutdown\n");


Thanks a lot.
I can confirm that this definitely fixes my problem.
  Tested-by: NeilBrown <neilb@suse.de>

Thanks for your help!

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] Driver core: leave runtime PM enabled during system shutdown

[Greg KH]

On Tue, Dec 06, 2011 at 10:26:14AM -0500, Alan Stern wrote:
> Disabling all runtime PM during system shutdown turns out not to be a
> good idea, because some devices may need to be woken up from a
> low-power state at that time.
> 
> The whole point of disabling runtime PM for system shutdown was to
> prevent untimely runtime-suspend method calls.  This patch (as1504)
> accomplishes the same result by incrementing the usage count for each
> device and waiting for ongoing runtime-PM callbacks to finish.  This
> is what we already do during system suspend and hibernation, which
> makes sense since the shutdown method is pretty much a legacy analog
> of the pm->poweroff method.
> 
> This fixes a regression on some OMAP systems.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Reported-by: NeilBrown <neilb@suse.de>
> CC: Chen Peter-B29397 <B29397@freescale.com>

Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

Rafael, you will take this in your tree, right?

Re: [PATCH] Driver core: leave runtime PM enabled during system shutdown

[Rafael J. Wysocki]

On Tuesday, December 06, 2011, Greg KH wrote:
> On Tue, Dec 06, 2011 at 10:26:14AM -0500, Alan Stern wrote:
> > Disabling all runtime PM during system shutdown turns out not to be a
> > good idea, because some devices may need to be woken up from a
> > low-power state at that time.
> > 
> > The whole point of disabling runtime PM for system shutdown was to
> > prevent untimely runtime-suspend method calls.  This patch (as1504)
> > accomplishes the same result by incrementing the usage count for each
> > device and waiting for ongoing runtime-PM callbacks to finish.  This
> > is what we already do during system suspend and hibernation, which
> > makes sense since the shutdown method is pretty much a legacy analog
> > of the pm->poweroff method.
> > 
> > This fixes a regression on some OMAP systems.
> > 
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > Reported-by: NeilBrown <neilb@suse.de>
> > CC: Chen Peter-B29397 <B29397@freescale.com>
> 
> Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> 
> Rafael, you will take this in your tree, right?

Yes, I will, thanks!

Re: [PATCH] Driver core: leave runtime PM enabled during system shutdown

[Rafael J. Wysocki]

On Tuesday, December 06, 2011, Alan Stern wrote:
> Disabling all runtime PM during system shutdown turns out not to be a
> good idea, because some devices may need to be woken up from a
> low-power state at that time.
> 
> The whole point of disabling runtime PM for system shutdown was to
> prevent untimely runtime-suspend method calls.  This patch (as1504)
> accomplishes the same result by incrementing the usage count for each
> device and waiting for ongoing runtime-PM callbacks to finish.  This
> is what we already do during system suspend and hibernation, which
> makes sense since the shutdown method is pretty much a legacy analog
> of the pm->poweroff method.
> 
> This fixes a regression on some OMAP systems.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Reported-by: NeilBrown <neilb@suse.de>
> CC: Chen Peter-B29397 <B29397@freescale.com>

Greg, do you have any objections against this patch and, if you don't,
would you mind if I took it (it fixes one that went in through my tree).

Thanks,
Rafael


> ---
> 
>  drivers/base/core.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Index: usb-3.2/drivers/base/core.c
> ===================================================================
> --- usb-3.2.orig/drivers/base/core.c
> +++ usb-3.2/drivers/base/core.c
> @@ -1743,8 +1743,10 @@ void device_shutdown(void)
>  		 */
>  		list_del_init(&dev->kobj.entry);
>  		spin_unlock(&devices_kset->list_lock);
> -		/* Disable all device's runtime power management */
> -		pm_runtime_disable(dev);
> +
> +		/* Don't allow any more runtime suspends */
> +		pm_runtime_get_noresume(dev);
> +		pm_runtime_barrier(dev);
>  
>  		if (dev->bus && dev->bus->shutdown) {
>  			dev_dbg(dev, "shutdown\n");
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2561 bytes --]

On Mon, 5 Dec 2011 11:26:40 +0800 Ming Lei <tom.leiming@gmail.com> wrote:

> Hi,
> 
> On Mon, Dec 5, 2011 at 5:56 AM, NeilBrown <neilb@suse.de> wrote:
> >
> > Hi,
> >  this patches causes a problem for me.
> >
> > Specifically it makes it impossible to power-down a device which uses twl4030
> > for power control on an omap3 processor.
> >
> > To perform the shutdown we need to send a command over the i2c bus.
> > The relevant bus is called omap_i2c.1 and this is normally in suspend mode.
> > When a request is sent, omap_i2c_xfer uses pm_runtime_get_sync to wake it up,
> > performs the transfer, then calls pm_runtime_put to let it go back to sleep.
> >
> > So it is asleep when the new pm_runtime_disable() call is made, so it stays
> > asleep, omap_i2c_xfer cannot wake it, the transfer doesn't happen and the
> > system doesn't get powered off.
> >
> > So here is a device that should *not* have pm disabled at shutdown.
> >
> > So I feel this fix is a little too heavy-handed.
> 
> Maybe the device's runtime PM should not be disabled if
> there is no ->shutdown defined in its driver, how about the blew?

Thanks, but that won't actually help.

dev->bus->shutdown is i2c_device_shutdown so there is a shutdown method.
However i2c_device_shutdown just finds the driver can calls
driver->shutdown(), and that is the 'shutdown' that is NULL.

Thanks,
NeilBrown



> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d8b3d89..ca30659 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1743,14 +1743,16 @@ void device_shutdown(void)
>  		 */
>  		list_del_init(&dev->kobj.entry);
>  		spin_unlock(&devices_kset->list_lock);
> -		/* Disable all device's runtime power management */
> -		pm_runtime_disable(dev);
> 
> +		/* Disable the device's runtime power management if
> +		 * it is to be shutdown*/
>  		if (dev->bus && dev->bus->shutdown) {
>  			dev_dbg(dev, "shutdown\n");
> +			pm_runtime_disable(dev);
>  			dev->bus->shutdown(dev);
>  		} else if (dev->driver && dev->driver->shutdown) {
>  			dev_dbg(dev, "shutdown\n");
> +			pm_runtime_disable(dev);
>  			dev->driver->shutdown(dev);
>  		}
>  		put_device(dev);
> 
> 
> > I don't fully understand the problem scenario described above but it seems to
> > me that if the auto-suspend timer can fire after the hardware has been shut
> > down, then maybe the hardware-shutdown should be disabling that timer.  Maybe?
> >
> > Suggestions?
> >
> > Thanks,
> > NeilBrown
> 
> 
> thanks,


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

RE: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[Chen Peter-B29397]

 
> > > So it is asleep when the new pm_runtime_disable() call is made, so it
> stays
> > > asleep, omap_i2c_xfer cannot wake it, the transfer doesn't happen and
> the
> > > system doesn't get powered off.
> > >
> > > So here is a device that should *not* have pm disabled at shutdown.
> > >
> > > So I feel this fix is a little too heavy-handed.
> >
> > Maybe the device's runtime PM should not be disabled if
> > there is no ->shutdown defined in its driver, how about the blew?
> 
> Thanks, but that won't actually help.
> 
> dev->bus->shutdown is i2c_device_shutdown so there is a shutdown method.
> However i2c_device_shutdown just finds the driver can calls
> driver->shutdown(), and that is the 'shutdown' that is NULL.
> 
Then, i2c should be registered before than twl4030 as a platform device, since
twl4030 is an i2c device. Just let i2c shut down later than twl4030's. 

> Thanks,
> NeilBrown
> 
> 
> 

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1603 bytes --]

On Mon, 5 Dec 2011 05:53:27 +0000 Chen Peter-B29397 <B29397@freescale.com>
wrote:

> 
>  
> > > > So it is asleep when the new pm_runtime_disable() call is made, so it
> > stays
> > > > asleep, omap_i2c_xfer cannot wake it, the transfer doesn't happen and
> > the
> > > > system doesn't get powered off.
> > > >
> > > > So here is a device that should *not* have pm disabled at shutdown.
> > > >
> > > > So I feel this fix is a little too heavy-handed.
> > >
> > > Maybe the device's runtime PM should not be disabled if
> > > there is no ->shutdown defined in its driver, how about the blew?
> > 
> > Thanks, but that won't actually help.
> > 
> > dev->bus->shutdown is i2c_device_shutdown so there is a shutdown method.
> > However i2c_device_shutdown just finds the driver can calls
> > driver->shutdown(), and that is the 'shutdown' that is NULL.
> > 
> Then, i2c should be registered before than twl4030 as a platform device, since
> twl4030 is an i2c device. Just let i2c shut down later than twl4030's. 

It almost certainly is, but that is totally irrelevant.

The problem has nothing to do with ordering and nothing much to do with
->shutdown() being called.

The problem is simply that pm_runtime_disable() is being called on all
devices, and that stops devices that are asleep from waking up.

I think the intention of putting that call in was to stop devices that are
awake from going to sleep, and maybe that is justified.  But stopping devices
that are asleep from waking up isn't.

NeilBrown


> 
> > Thanks,
> > NeilBrown
> > 
> > 
> > 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

RE: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[Chen Peter-B29397]

 
> > > stays
> > > > > asleep, omap_i2c_xfer cannot wake it, the transfer doesn't happen
> and
> > > the
> > > > > system doesn't get powered off.
> > > > >
> > > > > So here is a device that should *not* have pm disabled at
> shutdown.
> > > > >
> > > > > So I feel this fix is a little too heavy-handed.
> > > >
> > > > Maybe the device's runtime PM should not be disabled if
> > > > there is no ->shutdown defined in its driver, how about the blew?
> > >
> > > Thanks, but that won't actually help.
> > >
> > > dev->bus->shutdown is i2c_device_shutdown so there is a shutdown
> method.
> > > However i2c_device_shutdown just finds the driver can calls
> > > driver->shutdown(), and that is the 'shutdown' that is NULL.
> > >
> > Then, i2c should be registered before than twl4030 as a platform device,
> since
> > twl4030 is an i2c device. Just let i2c shut down later than twl4030's.
> 
> It almost certainly is, but that is totally irrelevant.
> 
> The problem has nothing to do with ordering and nothing much to do with
> ->shutdown() being called.
> 
> The problem is simply that pm_runtime_disable() is being called on all
> devices, and that stops devices that are asleep from waking up.
> 
Please correct me if I am wrong
1. This change only affects when the user issues "reboot" or "shutdown"
2. The pm_runtime_disable(dev) is only be called before this "dev" is
coming to shutdown
3. If i2c bus/driver->shutdown is not called, its runtime pm is still enabled.
4. You may issue i2c xfer after i2c bus/driver->shutdown is called.  

> I think the intention of putting that call in was to stop devices that
> are
> awake from going to sleep, and maybe that is justified.  But stopping
> devices
> that are asleep from waking up isn't.
> 
> NeilBrown
> 
> 
> >
> > > Thanks,
> > > NeilBrown
> > >
> > >
> > >

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2621 bytes --]

On Mon, 5 Dec 2011 08:32:27 +0000 Chen Peter-B29397 <B29397@freescale.com>
wrote:

>  
> > > > stays
> > > > > > asleep, omap_i2c_xfer cannot wake it, the transfer doesn't happen
> > and
> > > > the
> > > > > > system doesn't get powered off.
> > > > > >
> > > > > > So here is a device that should *not* have pm disabled at
> > shutdown.
> > > > > >
> > > > > > So I feel this fix is a little too heavy-handed.
> > > > >
> > > > > Maybe the device's runtime PM should not be disabled if
> > > > > there is no ->shutdown defined in its driver, how about the blew?
> > > >
> > > > Thanks, but that won't actually help.
> > > >
> > > > dev->bus->shutdown is i2c_device_shutdown so there is a shutdown
> > method.
> > > > However i2c_device_shutdown just finds the driver can calls
> > > > driver->shutdown(), and that is the 'shutdown' that is NULL.
> > > >
> > > Then, i2c should be registered before than twl4030 as a platform device,
> > since
> > > twl4030 is an i2c device. Just let i2c shut down later than twl4030's.
> > 
> > It almost certainly is, but that is totally irrelevant.
> > 
> > The problem has nothing to do with ordering and nothing much to do with
> > ->shutdown() being called.
> > 
> > The problem is simply that pm_runtime_disable() is being called on all
> > devices, and that stops devices that are asleep from waking up.
> > 
> Please correct me if I am wrong
> 1. This change only affects when the user issues "reboot" or "shutdown"

well "halt -p" or "poweroff", but yes.

> 2. The pm_runtime_disable(dev) is only be called before this "dev" is
> coming to shutdown

Yes, but the important point is that is being called before pm_power_off is
called.

> 3. If i2c bus/driver->shutdown is not called, its runtime pm is still enabled.

I think you mean "disabled" ??

Yes, but I don't see how that is relevant.

> 4. You may issue i2c xfer after i2c bus/driver->shutdown is called.  

Yes, but again I cannot see the relevance.

NeilBrown


> 
> > I think the intention of putting that call in was to stop devices that
> > are
> > awake from going to sleep, and maybe that is justified.  But stopping
> > devices
> > that are asleep from waking up isn't.
> > 
> > NeilBrown
> > 
> > 
> > >
> > > > Thanks,
> > > > NeilBrown
> > > >
> > > >
> > > >
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

RE: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[Chen Peter-B29397]

 
> 
> Hi,
>  this patches causes a problem for me.
> 
> Specifically it makes it impossible to power-down a device which uses
> twl4030
> for power control on an omap3 processor.
> 
> To perform the shutdown we need to send a command over the i2c bus.
> The relevant bus is called omap_i2c.1 and this is normally in suspend
> mode.
> When a request is sent, omap_i2c_xfer uses pm_runtime_get_sync to wake it
> up,
> performs the transfer, then calls pm_runtime_put to let it go back to
> sleep.
> 
> So it is asleep when the new pm_runtime_disable() call is made, so it
> stays
> asleep, omap_i2c_xfer cannot wake it, the transfer doesn't happen and the
> system doesn't get powered off.
> 
> So here is a device that should *not* have pm disabled at shutdown.
> 
> So I feel this fix is a little too heavy-handed.
> I don't fully understand the problem scenario described above but it
> seems to
> me that if the auto-suspend timer can fire after the hardware has been
> shut
> down, then maybe the hardware-shutdown should be disabling that timer.
> Maybe?
> 
Oh, I am sorry to cause your problem. I think it may not be easy to handle this
kinds of problem well. 

In my opinion, it is better to handle shutdown/suspend SYNC at device drivers.
Since the pm core is hard to know driver's shutdown is finished, and vice versa.

1. Driver needs to has relationship between suspend/shutdown, like usb host,
hcd needs to know downstream port's suspend, and usb core needs to know hcd's shutdown. 

2. At driver's shutdown
static void xxx_driver_shutdown(struct platform_device *pdev)
{
	if (supports_autosuspend(&pdev->dev) {
		pm_runtime_cancel_pending(&pdev->dev);
		wait_xxx_driver_suspend(pdev); /* need to sync with driver's suspend */
	}
	real_shutdown();		
}

> Suggestions?
> 
> Thanks,
> NeilBrown

Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2588 bytes --]

On Mon, 5 Dec 2011 03:29:24 +0000 Chen Peter-B29397 <B29397@freescale.com>
wrote:

>  
> > 
> > Hi,
> >  this patches causes a problem for me.
> > 
> > Specifically it makes it impossible to power-down a device which uses
> > twl4030
> > for power control on an omap3 processor.
> > 
> > To perform the shutdown we need to send a command over the i2c bus.
> > The relevant bus is called omap_i2c.1 and this is normally in suspend
> > mode.
> > When a request is sent, omap_i2c_xfer uses pm_runtime_get_sync to wake it
> > up,
> > performs the transfer, then calls pm_runtime_put to let it go back to
> > sleep.
> > 
> > So it is asleep when the new pm_runtime_disable() call is made, so it
> > stays
> > asleep, omap_i2c_xfer cannot wake it, the transfer doesn't happen and the
> > system doesn't get powered off.
> > 
> > So here is a device that should *not* have pm disabled at shutdown.
> > 
> > So I feel this fix is a little too heavy-handed.
> > I don't fully understand the problem scenario described above but it
> > seems to
> > me that if the auto-suspend timer can fire after the hardware has been
> > shut
> > down, then maybe the hardware-shutdown should be disabling that timer.
> > Maybe?
> > 
> Oh, I am sorry to cause your problem. I think it may not be easy to handle this
> kinds of problem well. 
> 
> In my opinion, it is better to handle shutdown/suspend SYNC at device drivers.
> Since the pm core is hard to know driver's shutdown is finished, and vice versa.
> 
> 1. Driver needs to has relationship between suspend/shutdown, like usb host,
> hcd needs to know downstream port's suspend, and usb core needs to know hcd's shutdown. 
> 
> 2. At driver's shutdown
> static void xxx_driver_shutdown(struct platform_device *pdev)
> {
> 	if (supports_autosuspend(&pdev->dev) {
> 		pm_runtime_cancel_pending(&pdev->dev);
> 		wait_xxx_driver_suspend(pdev); /* need to sync with driver's suspend */
> 	}
> 	real_shutdown();		
> }
>

I think that makes sense to me.
It might be reasonable to call pm_runtime_barrier() in device_shutdown() to
make it a bit easier for the ->shutdown function, but the final
synchronisation should probably happen right there where you suggest.

It seems that driver->shutdown() is called from other places than just
device_shutdown(). If a ->driver_shutdown function was allowed to assume that
pm_runtime has been disabled, all of those call points would need to disable
pm_runtime, which doesn't seem like the right way to go.

Thanks,
NeilBrown



Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]