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? >>> 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? -- cheers, -roger Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki -- 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
