On Thu, Jun 21, 2018 at 12:32:59AM +0200, Rafael J. Wysocki wrote: > On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold <johan@xxxxxxxxxx> wrote: > > On Wed, Jun 20, 2018 at 02:54:10PM +0200, Rafael J. Wysocki wrote: > >> On Wednesday, June 20, 2018 2:23:46 PM CEST Johan Hovold wrote: > >> > On Wed, Jun 20, 2018 at 02:16:59AM -0700, Tony Lindgren wrote: > >> > > Hi, > >> > > > >> > > Adding Rafael and linux-pm to Cc as well. > >> > > > >> > > * Felipe Balbi <balbi@xxxxxxxxxx> [180619 01:23]: > >> > > > This is a direct consequence of not paying attention to the order of > >> > > > things. If driver were to assume that pm_domain->activate() would do the > >> > > > right thing for the device -- meaning that probe would run with an > >> > > > active device --, then we wouldn't need that pm_runtime_get() call on > >> > > > probe at all. Rather we would follow the sequence: > >> > > > > >> > > > pm_runtime_forbid() > >> > > > pm_runtime_set_active() > >> > > > pm_runtime_enable() > >> > > > > >> > > > /* do your probe routine */ > >> > > > > >> > > > pm_runtime_put_noidle() > >> > > > > >> > > > Then you remove you would need to call pm_runtime_get_noresume() to > >> > > > balance out the pm_runtime_put_noidle() there. > >> > > >> > > > (If you need to know why the pm_runtime_put_noidle(), remember that > >> > > > pm_runtime_set_active() increments the usage counter, so > >> > > > pm_runtime_put_noidle is basically allowing pm_runtime to happen as soon > >> > > > as userspace writes "auto" to /sys/..../power/control) > >> > > >> > That's not correct; pm_runtime_set_active() only increments the usage > >> > counter of a parent (under some circumstances), so unless you have bus > >> > code incrementing the usage counter before probe, the above > >> > pm_runtime_put_noidle() would actually introduce an imbalance. > >> > >> No, it wouldn't. It balances the incrementation in pm_runtime_forbid(). > > > > Right, but even if you take the whole sequence, which included > > pm_runtime_forbid(), consider what happens when pm_runtime_allow() is > > later called through sysfs (see below). > > > >> > And note that that's also the case even if you meant to say that > >> > *pm_runtime_forbid()* increments the usage counter (which it does). > >> > >> Why is it? > >> > >> Surely, after > >> > >> pm_runtime_forbid(dev); > >> pm_runtime_put_noidle(dev); > >> > >> the runtime PM usage counter of dev will be the same as before, won't it? > > > > Sure, but the imbalance, or rather inconsistent state, has already been > > introduced. > > > > Consider the following sequence of events: > > > > usage count > > 0 > > probe() > > pm_runtime_forbid() 1 > > pm_runtime_set_active() > > pm_runtime_enable() > > pm_runtime_put_noidle() 0 > > > > Here nothing is preventing the device from runtime suspending, despite > > runtime PM being forbidden. In fact, it will typically be suspended due > > to the pm_request_idle() in driver_probe_device(). If later we have: > > > > echo auto > power/control > > pm_runtime_allow() -1 > > OK, you have a point. > > After calling pm_runtime_forbid() the driver should allow user space > to unblock runtime PM for the device - or call pm_runtime_allow() > itself. Right, the usage count increment done by forbid() should only be balanced by allow(). > [cut] > > > > > And if runtime pm is later again forbidden: > > > > echo on > power/control > > pm_runtime_forbid() 0 > > > > then the device will not be resumed. > > But I don't quite see why that will be the case. rpm_resume() will > still be called and it doesn't look at the usage counter. You're right, I left out some important details and jumped to the conclusion. As you point out, the device will be unconditionally resumed, but due to the usage counter being zero the idle notification queued by rpm_resume() will typically suspend it straight away despite runtime pm being forbidden (cf. the idle notification issued by driver core after probe() returns). Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
