On Thu, Jun 21, 2018 at 11:17:36AM +0300, Roger Quadros wrote: > On 21/06/18 01:55, Rafael J. Wysocki wrote: > > 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 > > Can you call pm_runtime_forbid() before pm_runtime_enable()? > Wouldn't it fail with -EACCES as dev->power.disable_depth > 0? No, pm_runtime_forbid() manipulates the usage count directly and doesn't check for errors from rpm_resume(), so this actually "works". That doesn't mean anyone should be doing it, though (e.g. for the below reasons). > >>> 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(). > > OK. So it is the PCI bus which is behaving odd here and > pm_runtime_put_noidle() needs to be done only if its a PCI device, correct? Yeah, the pm_runtime_put_noidle() would be used to balance the unconditional runtime resume by the bus code in case a PCI driver implements runtime pm. 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
