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