Messages in this thread | | | Date | Mon, 5 Dec 2011 16:09:25 +1100 | From | NeilBrown <> | Subject | Re: [PATCH 1/1] driver core: disable device's runtime pm during shutdown |
| |
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 [unhandled content-type:application/pgp-signature] | |