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

[NeilBrown <neilb@suse.de>]

[-- 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 --]