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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 5 Dec 2011 16:09:25 +1100
From:	NeilBrown <neilb@...e.de>
To:	Chen Peter-B29397 <B29397@...escale.com>
Cc:	"Rafael J. Wysocki" <rjw@...k.pl>, Greg KH <greg@...ah.com>,
	"gregkh@...e.de" <gregkh@...e.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"stern@...land.harvard.edu" <stern@...land.harvard.edu>,
	"hzpeterchen@...il.com" <hzpeterchen@...il.com>,
	Igor Grinberg <grinberg@...pulab.co.il>
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@...escale.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

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)