On Thu, Jun 21, 2018 at 12:32 AM, Rafael J. Wysocki <rafael@xxxxxxxxxx> 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. The confusion regarding the pm_runtime_put_noidle() at the end may come from the special requirement of the PCI bus type as per the comment in local_pci_probe(). -- 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
