Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Thu, 21 Jun 2018, Roger Quadros wrote: > >>> 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? Look, there has been a lot of confusion in this email thread. Let's get some things straightened out before it goes much farther. There are only a very few reasons for ever calling pm_runtime_forbid() or pm_runtime_allow() at any time other than just before the device is registered. In fact, I can only think of one reason at the moment: If a device belongs to a class which is well known to have excellent support for runtime PM, a driver might want to call pm_runtime_allow(). But in general, a driver should not call these routines. The decision about whether or not a device should be allowed to go into runtime suspend is a policy matter and therefore should be decided by the user, not by the kernel. Furthermore, these calls merely set a default value; the default can be overridden by the user at any time and therefore a driver cannot depend on these functions for anything. Another point of confusion involves balancing pm_runtime_get_* and pm_runtime_put_* calls. They should always end up in balance, and at any moment there never should be more put's than get's except in one very particular circumstance: The bus system may guarantee to invoke probe callbacks after performing an extra get, and to perform an extra put after invoking remove callbacks (the PCI subsystem does this). This can be useful when a lot of drivers don't support runtime PM; the extra get insures that the PM core will never try to suspend the device while such a driver is bound to it. A driver that _does_ support runtime PM would do an extra pm_runtime_put at the end of its probe routine and an extra pm_runtime_get_sync in its remove routine; this will allow runtime PM to work while keeping the counts non-negative. That's the only situation I know of where it's reasonable to have more puts than gets. The PCI subsystem does this, and to be perfectly honest, I do not remember why local_pci_probe() recommends that drivers call pm_runtime_put_noidle rather than pm_runtime_put. On the face of it, this would allow the PM usage counter to go to 0 without triggering an immediate runtime suspend. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
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 > > wrote: > >> On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold 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 [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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Thu, Jun 21, 2018 at 12:55:26AM +0200, Rafael J. Wysocki wrote: > On Thu, Jun 21, 2018 at 12:32 AM, Rafael J. Wysocki wrote: > > On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold 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 [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. > 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(). That seems to be the case, but I'm not sure of much of what PCI is doing that can be applied here (e.g. OMAP platform devices), where unbound devices should always be powered off and were I assume we want to have runtime pm allowed by default, for example. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
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 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 [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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On 21/06/18 01:55, Rafael J. Wysocki wrote: > On Thu, Jun 21, 2018 at 12:32 AM, Rafael J. Wysocki wrote: >> On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold 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 [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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Thu, Jun 21, 2018 at 12:32 AM, Rafael J. Wysocki wrote: > On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold 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 [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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold 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 [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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
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 [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 then the device remains suspended, but we get a negative usage count. This does not seem to play well with neither runtime pm (consider a synchronous get followed by a put, which will fail to again suspend the device) or, for example, system suspend, which tries to prevent runtime pm by incrementing the usage count. And if runtime pm is later again forbidden: echo on > power/control pm_runtime_forbid() 0 then the device will not be resumed. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
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 [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(). > 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? -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
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 [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. 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). Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Wed, Jun 20, 2018 at 1:05 PM, Felipe Balbi wrote: > "Rafael J. Wysocki" writes: > >> On Wed, Jun 20, 2018 at 11:27 AM, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Tony Lindgren writes: * Felipe Balbi [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() >> >> Why do you need the _forbid() thing? >> >> Does the default user space control setting need to be changed? > > wouldn't that race with a udev rule writing "auto" power/control as soon > as device is added? Why would it? > (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) I wonder if we could also remove the need for drivers to call pm_runtime_putnoidle() at the end of the probe? If we had pm_runtime_init_enabled() implemented. >>> >>> probably not. We want to block runtime pm during probe, until the device >>> is fully initialized, the only way to do that is to increment rpm usage >>> counter. >> >> That or enable it only at the end. But I guess you want to >> runtime-resume it earlier, don't you? > > well, if arch implements pm_domain->activate() and guarantees that > device is already running, with clocks stable during probe, then > enabling runtime pm at the end is probably the best idea. But if you call pm_runtime_set_active() at any point, you're assuming that the device is active anyway. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
"Rafael J. Wysocki" writes: > On Wed, Jun 20, 2018 at 11:27 AM, Felipe Balbi wrote: >> >> Hi, >> >> Tony Lindgren writes: >>> * Felipe Balbi [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() > > Why do you need the _forbid() thing? > > Does the default user space control setting need to be changed? wouldn't that race with a udev rule writing "auto" power/control as soon as device is added? (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) >>> >>> I wonder if we could also remove the need for drivers to call >>> pm_runtime_putnoidle() at the end of the probe? If we had >>> pm_runtime_init_enabled() implemented. >> >> probably not. We want to block runtime pm during probe, until the device >> is fully initialized, the only way to do that is to increment rpm usage >> counter. > > That or enable it only at the end. But I guess you want to > runtime-resume it earlier, don't you? well, if arch implements pm_domain->activate() and guarantees that device is already running, with clocks stable during probe, then enabling runtime pm at the end is probably the best idea. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Wed, Jun 20, 2018 at 11:27 AM, Felipe Balbi wrote: > > Hi, > > Tony Lindgren writes: >> * Felipe Balbi [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() Why do you need the _forbid() thing? Does the default user space control setting need to be changed? >>> 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. >> >> How about let's create some prettier interface for the above runtime PM >> trickery? >> >> How about something like pm_runtime_init_enabled() for the above >> sequence? And then have a separate helper for every other use case? Come on. >> It might be then able to do the trick even if activate is not >> implemented.. >> >> Right now it has the feeling of "oh well we can't get runtime PM to >> work so let's bypass it with activate call and then trick runtime PM >> to start in enabled mode" :) > > no strong feelings about that either way. It's only 3 lines of code > anyway. I feel like that's a minor cosmetic change. > >>> (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) >> >> I wonder if we could also remove the need for drivers to call >> pm_runtime_putnoidle() at the end of the probe? If we had >> pm_runtime_init_enabled() implemented. > > probably not. We want to block runtime pm during probe, until the device > is fully initialized, the only way to do that is to increment rpm usage > counter. That or enable it only at the end. But I guess you want to runtime-resume it earlier, don't you? -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
Hi, Tony Lindgren writes: > * Felipe Balbi [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. > > How about let's create some prettier interface for the above runtime PM > trickery? > > How about something like pm_runtime_init_enabled() for the above > sequence? > > It might be then able to do the trick even if activate is not > implemented.. > > Right now it has the feeling of "oh well we can't get runtime PM to > work so let's bypass it with activate call and then trick runtime PM > to start in enabled mode" :) no strong feelings about that either way. It's only 3 lines of code anyway. I feel like that's a minor cosmetic change. >> (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) > > I wonder if we could also remove the need for drivers to call > pm_runtime_putnoidle() at the end of the probe? If we had > pm_runtime_init_enabled() implemented. probably not. We want to block runtime pm during probe, until the device is fully initialized, the only way to do that is to increment rpm usage counter. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
Hi, Adding Rafael and linux-pm to Cc as well. * Felipe Balbi [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. How about let's create some prettier interface for the above runtime PM trickery? How about something like pm_runtime_init_enabled() for the above sequence? It might be then able to do the trick even if activate is not implemented.. Right now it has the feeling of "oh well we can't get runtime PM to work so let's bypass it with activate call and then trick runtime PM to start in enabled mode" :) > Anyway, with an assumption like this, after all platform_devices are > converted over, the assumption can be moved into the bus code and, low > and behold, to enable runtime pm for your driver, all you have to is > implement your callbacks and add pm_runtime_put_noidle() to probe and > pm_runtime_get_noresume() to remove (apart from, of course, making sure > the device isn't allowed to runtime_suspend when it's actually busy). > > Do you see the end goal? It certainly would be nice to make runtime PM generic for drivers :) > (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) I wonder if we could also remove the need for drivers to call pm_runtime_putnoidle() at the end of the probe? If we had pm_runtime_init_enabled() implemented. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
* Tero Kristo [180619 12:13]: > > TI SoC arm64 PM is done via genpd attached to each domain, which in turn > passes control messages to the PM firmware when transitions happen. See > drivers/soc/ti_sci_pm_domains.c for details. > > Not quite sure about the discussion following later, but there is work > ongoing to get rid of both omap_hwmod and omap_device, and replace this with > a proper bus driver for OMAP architectures. It might solve the issues being > talked about, or then not. Tony, any comments on that? As long as drivers keep working we should add the callbacks suggested by Felipe if that allows making drivers generic. Then changing the driver probes can be done one driver at a time hopefully. > Anyway, main reason that omap devices are actually suspended on HW level > during probe is the aggressive power management applied on the device, and > based on the documentation this is actually a valid thing to do. This makes > sure that all devices get to idle if they are not used (no driver probed > etc.), and will allow the SoC to idle core powerdomain. What Felipe is suggesting is adding the callbacks that won't change the runtime PM usecount. They just enable the device for probe, then bus code can disable it again on failed probe. In the device driver probe things in theory should work then both the old way and the PCI style way Felipe wants. Needs to be tested of course :) Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On 19/06/18 11:18, Felipe Balbi wrote: Hi, Roger Quadros writes: I suggest merging this fix for 4.18-rc, and then Roger can rework the driver so that it works also on OMAP. omap has its own glue layer for several reasons. If you're talking about Keystone devices, then okay, I understand. But in that case, this would mean Keystone is copying the same arguably broken PM domain design from OMAP and it would be best not to propagate that idea. Maybe so. I'm not sure what Roger's use case is, but perhaps the omap glue driver could be used instead. unlikely. Keystone devices are very different from OMAP family. But we'll see what Roger says. Well, I was considering to use of-simple for the AM654 SoC [1] but now I'm of the opinion that it might be better to add a new glue layer driver why isn't dwc3-keystone.c enough? It is doing too many things than we really need to for AM654. I'm assuming you meant you really don't need for AM654. That can be made optional with the use of a different compatible, right? for that because - it needs to poke a few registers in the wrapper region dwc3-keystone.c does that already - it doesn't really need the driver to enable any clock Seems to me you're trying to port omap_device to arm64... It isn't an omap_device but is very similar to how it is done on keystone. Fair enough - it needs a pm_runtime_get_sync() to be done in probe this really shouldn't be necessary. Keystone doesn't rely on all the omap_device legacy. At least it didn't use to. Could it be that you're just missing a struct dev_pm_domain definition for arm64? I don't think so. If I had missed it it wouldn't enable at all. arm64 doesn't define own pm_domain. I haven't seen how you guys implemented your PM for arm64 (is there a publically accessible version somewhere?), but I'd say you should take the opportunity to remove this relying on pm_runtime_get_sync() calls from probe and just do what everybody else does; namely: enable clocks on probe, pm_runtime_set_active, etc. This is something Tero can comment on. TI SoC arm64 PM is done via genpd attached to each domain, which in turn passes control messages to the PM firmware when transitions happen. See drivers/soc/ti_sci_pm_domains.c for details. Not quite sure about the discussion following later, but there is work ongoing to get rid of both omap_hwmod and omap_device, and replace this with a proper bus driver for OMAP architectures. It might solve the issues being talked about, or then not. Tony, any comments on that? Anyway, main reason that omap devices are actually suspended on HW level during probe is the aggressive power management applied on the device, and based on the documentation this is actually a valid thing to do. This makes sure that all devices get to idle if they are not used (no driver probed etc.), and will allow the SoC to idle core powerdomain. -Tero -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
Hi,
Roger Quadros writes:
>>> I suggest merging this fix for 4.18-rc, and then Roger can rework the
>>> driver so that it works also on OMAP.
>>
>> omap has its own glue layer for several reasons. If you're talking about
>> Keystone devices, then okay, I understand. But in that case, this would
>> mean Keystone is copying the same arguably broken PM domain design from
>> OMAP and it would be best not to propagate that idea.
>
> Maybe so. I'm not sure what Roger's use case is, but perhaps the omap
> glue driver could be used instead.
unlikely. Keystone devices are very different from OMAP family. But
we'll see what Roger says.
>>>
>>> Well, I was considering to use of-simple for the AM654 SoC [1] but now
>>> I'm of the opinion that it might be better to add a new glue layer driver
>>
>> why isn't dwc3-keystone.c enough?
>
> It is doing too many things than we really need to for AM654.
I'm assuming you meant you really don't need for AM654. That can be made
optional with the use of a different compatible, right?
>>> for that because
>>> - it needs to poke a few registers in the wrapper region
>>
>> dwc3-keystone.c does that already
>>
>>> - it doesn't really need the driver to enable any clock
>>
>> Seems to me you're trying to port omap_device to arm64...
>
> It isn't an omap_device but is very similar to how it is done on
> keystone.
Fair enough
>>> - it needs a pm_runtime_get_sync() to be done in probe
>>
>> this really shouldn't be necessary. Keystone doesn't rely on all the
>> omap_device legacy. At least it didn't use to. Could it be that you're
>> just missing a struct dev_pm_domain definition for arm64?
>
> I don't think so. If I had missed it it wouldn't enable at all.
arm64 doesn't define own pm_domain.
>> I haven't seen how you guys implemented your PM for arm64 (is there a
>> publically accessible version somewhere?), but I'd say you should take
>> the opportunity to remove this relying on pm_runtime_get_sync() calls
>> from probe and just do what everybody else does; namely: enable clocks
>> on probe, pm_runtime_set_active, etc.
>
> This is something Tero can comment on.
>
>>
>> This helps drivers being able to make assumptions about devices being
>> enabled during probe. pm_runtime becomes easier to implement generically
>> too.
>>
>
> You keep mentioning that PM domain design is broken on OMAP. Could
> you please clarify what is broken? Is it the fact that the bus code
> doesn't enable the device before probe and that we have to do a
> pm_runtime_sync() in probe?
it's the fact that we need to rely on pm_runtime_get() to enable the
device from probe. Just consider for a second the situation this
creates:
probe() calls pm_runtime_enable() and pm_runtime_get*(). That will
eventually go back and call our own ->runtime_resume() callback. This
means that driver must make sure that *set_drvdata() is done before
pm_runtime_get() and driver must make sure that running
->runtime_resume() from probe() is okay in every case. What drivers end
up doing, is nonsense like below:
static int musb_runtime_resume(struct device *dev)
{
struct musb *musb = dev_to_musb(dev);
unsigned long flags;
int error;
/*
* When pm_runtime_get_sync called for the first time in driver
* init, some of the structure is still not initialized which is
* used in restore function. But clock needs to be
* enabled before any register access, so
* pm_runtime_get_sync has to be called.
* Also context restore without save does not make
* any sense
*/
if (!musb->is_initialized)
return 0;
[...]
}
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.
Anyway, with an assumption like this, after all platform_devices are
converted over, the assumption can be moved into the bus code and, low
and behold, to enable runtime pm for your driver, all you have to is
implement your callbacks and add pm_runtime_put_noidle() to probe and
pm_runtime_get_noresume() to remove (apart from, of course, making sure
the device isn't allowed to runtime_suspend when it's actually busy).
Do you see the end goal?
Now, what omap_device did, even if accidentally, was create these
different approach which makes it more difficult to write a generic
driver that works for OMAPs as well as Exynos, PCI, Snapdragon,
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
+Tero, Tony and some TI folks On 18/06/18 15:21, Felipe Balbi wrote: > > Hi, > > Roger Quadros writes: >> On 18/06/18 12:51, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Johan Hovold writes: On Mon, Jun 18, 2018 at 12:33:44PM +0300, Felipe Balbi wrote: > Johan Hovold writes: >> I suggest merging this fix for 4.18-rc, and then Roger can rework the >> driver so that it works also on OMAP. > > omap has its own glue layer for several reasons. If you're talking about > Keystone devices, then okay, I understand. But in that case, this would > mean Keystone is copying the same arguably broken PM domain design from > OMAP and it would be best not to propagate that idea. Maybe so. I'm not sure what Roger's use case is, but perhaps the omap glue driver could be used instead. >>> >>> unlikely. Keystone devices are very different from OMAP family. But >>> we'll see what Roger says. >>> >> >> Well, I was considering to use of-simple for the AM654 SoC [1] but now >> I'm of the opinion that it might be better to add a new glue layer driver > > why isn't dwc3-keystone.c enough? It is doing too many things than we really need to for AM654. > >> for that because >> - it needs to poke a few registers in the wrapper region > > dwc3-keystone.c does that already > >> - it doesn't really need the driver to enable any clock > > Seems to me you're trying to port omap_device to arm64... It isn't an omap_device but is very similar to how it is done on keystone. > >> - it needs a pm_runtime_get_sync() to be done in probe > > this really shouldn't be necessary. Keystone doesn't rely on all the > omap_device legacy. At least it didn't use to. Could it be that you're > just missing a struct dev_pm_domain definition for arm64? I don't think so. If I had missed it it wouldn't enable at all. > > I haven't seen how you guys implemented your PM for arm64 (is there a > publically accessible version somewhere?), but I'd say you should take > the opportunity to remove this relying on pm_runtime_get_sync() calls > from probe and just do what everybody else does; namely: enable clocks > on probe, pm_runtime_set_active, etc. This is something Tero can comment on. > > This helps drivers being able to make assumptions about devices being > enabled during probe. pm_runtime becomes easier to implement generically > too. > You keep mentioning that PM domain design is broken on OMAP. Could you please clarify what is broken? Is it the fact that the bus code doesn't enable the device before probe and that we have to do a pm_runtime_sync() in probe? I tried to discuss this here [1] but looks like you missed it. Re-iterating here. Platform bus doesn't seem to enable the device as part of of_platform_populate(). see __device_attach() and driver_probe_device() in drivers/base/dd.c It does a pm_runtime_get_sync() on dev->parent but not on dev. Also, from section 5 of Documentation/power/runtime_pm.txt "In addition to that, the initial runtime PM status of all devices is 'suspended', but it need not reflect the actual physical state of the device. Thus, if the device is initially active (i.e. it is able to process I/O), its runtime PM status must be changed to 'active', with the help of pm_runtime_set_active(), before pm_runtime_enable() is called for the device." "Note, if the device may execute pm_runtime calls during the probe (such as if it is registers with a subsystem that may call back in) then the pm_runtime_get_sync() call paired with a pm_runtime_put() call will be appropriate to ensure that the device is not put back to sleep during the probe. This can happen with systems such as the network device layer." So looks like we can't assume that the device is "active" when probe() is called. Which means, we need to do pm_runtime_get_sync(); enable optional clocks; [1] https://lkml.org/lkml/2018/6/13/265 -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
Hi, Roger Quadros writes: > On 18/06/18 12:51, Felipe Balbi wrote: >> >> Hi, >> >> Johan Hovold writes: >>> On Mon, Jun 18, 2018 at 12:33:44PM +0300, Felipe Balbi wrote: >>> Johan Hovold writes: >>> > I suggest merging this fix for 4.18-rc, and then Roger can rework the > driver so that it works also on OMAP. omap has its own glue layer for several reasons. If you're talking about Keystone devices, then okay, I understand. But in that case, this would mean Keystone is copying the same arguably broken PM domain design from OMAP and it would be best not to propagate that idea. >>> >>> Maybe so. I'm not sure what Roger's use case is, but perhaps the omap >>> glue driver could be used instead. >> >> unlikely. Keystone devices are very different from OMAP family. But >> we'll see what Roger says. >> > > Well, I was considering to use of-simple for the AM654 SoC [1] but now > I'm of the opinion that it might be better to add a new glue layer driver why isn't dwc3-keystone.c enough? > for that because > - it needs to poke a few registers in the wrapper region dwc3-keystone.c does that already > - it doesn't really need the driver to enable any clock Seems to me you're trying to port omap_device to arm64... > - it needs a pm_runtime_get_sync() to be done in probe this really shouldn't be necessary. Keystone doesn't rely on all the omap_device legacy. At least it didn't use to. Could it be that you're just missing a struct dev_pm_domain definition for arm64? I haven't seen how you guys implemented your PM for arm64 (is there a publically accessible version somewhere?), but I'd say you should take the opportunity to remove this relying on pm_runtime_get_sync() calls from probe and just do what everybody else does; namely: enable clocks on probe, pm_runtime_set_active, etc. This helps drivers being able to make assumptions about devices being enabled during probe. pm_runtime becomes easier to implement generically too. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On 18/06/18 12:51, Felipe Balbi wrote: > > Hi, > > Johan Hovold writes: >> On Mon, Jun 18, 2018 at 12:33:44PM +0300, Felipe Balbi wrote: >> >>> Johan Hovold writes: >> I suggest merging this fix for 4.18-rc, and then Roger can rework the driver so that it works also on OMAP. >>> >>> omap has its own glue layer for several reasons. If you're talking about >>> Keystone devices, then okay, I understand. But in that case, this would >>> mean Keystone is copying the same arguably broken PM domain design from >>> OMAP and it would be best not to propagate that idea. >> >> Maybe so. I'm not sure what Roger's use case is, but perhaps the omap >> glue driver could be used instead. > > unlikely. Keystone devices are very different from OMAP family. But > we'll see what Roger says. > Well, I was considering to use of-simple for the AM654 SoC [1] but now I'm of the opinion that it might be better to add a new glue layer driver for that because - it needs to poke a few registers in the wrapper region - it doesn't really need the driver to enable any clock - it needs a pm_runtime_get_sync() to be done in probe [1] - https://lkml.org/lkml/2018/6/5/44 -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
Hi,
Johan Hovold writes:
> On Wed, Jun 13, 2018 at 12:39:18PM +0300, Felipe Balbi wrote:
>> Roger Quadros writes:
>
>> > I'm still trying to get my head around this.
>> >
>> > in probe() we do
>> > {
>> >enable all clocks;
>> > pm_runtime_set_active(dev);
>> > pm_runtime_enable(dev);
>> > pm_runtime_get_sync(dev);
>> > }
>> >
>> > How will runtime suspend work at all?
>> > We're holding a positive RPM count in probe().
>>
>> echo auto > /path/to/dwc3/power/control
>
> That makes no difference, user space cannot modify the always-active
> behaviour given that probe returns with a positive usage count.
>
>> Granted, that get_sync() would've been better as a pm_runtime_forbid()
>
> Yeah, that would allow user space some control, albeit in a way that
> may override a user space configuration (as the platform device would
> already have been registered).
>
> Why are you trying to prevent runtime pm in the first place? Shouldn't
> the device be allowed to suspend when it has no active child by
> default?
I don't use this glue layer, actually. As long as there are no
regressions, you can change it to your heart's content. I still it's
best to start with pm runtime blocked and let userspace decide what and
when should have pm runtime enabled.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
Hi, Johan Hovold writes: > On Mon, Jun 18, 2018 at 12:33:44PM +0300, Felipe Balbi wrote: > >> Johan Hovold writes: > >> > I suggest merging this fix for 4.18-rc, and then Roger can rework the >> > driver so that it works also on OMAP. >> >> omap has its own glue layer for several reasons. If you're talking about >> Keystone devices, then okay, I understand. But in that case, this would >> mean Keystone is copying the same arguably broken PM domain design from >> OMAP and it would be best not to propagate that idea. > > Maybe so. I'm not sure what Roger's use case is, but perhaps the omap > glue driver could be used instead. unlikely. Keystone devices are very different from OMAP family. But we'll see what Roger says. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Mon, Jun 18, 2018 at 12:33:44PM +0300, Felipe Balbi wrote: > Johan Hovold writes: > > I suggest merging this fix for 4.18-rc, and then Roger can rework the > > driver so that it works also on OMAP. > > omap has its own glue layer for several reasons. If you're talking about > Keystone devices, then okay, I understand. But in that case, this would > mean Keystone is copying the same arguably broken PM domain design from > OMAP and it would be best not to propagate that idea. Maybe so. I'm not sure what Roger's use case is, but perhaps the omap glue driver could be used instead. Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
Hi, Johan Hovold writes: > On Mon, Jun 18, 2018 at 11:15:41AM +0300, Felipe Balbi wrote: > >> I don't use this glue layer, actually. As long as there are no >> regressions, you can change it to your heart's content. I still it's >> best to start with pm runtime blocked and let userspace decide what and >> when should have pm runtime enabled. > > I don't use it either, I just noticed the use-after-free in remove() > (and that probe was returning with a positive runtime PM usage count). > > I suggest merging this fix for 4.18-rc, and then Roger can rework the > driver so that it works also on OMAP. omap has its own glue layer for several reasons. If you're talking about Keystone devices, then okay, I understand. But in that case, this would mean Keystone is copying the same arguably broken PM domain design from OMAP and it would be best not to propagate that idea. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Mon, Jun 18, 2018 at 11:15:41AM +0300, Felipe Balbi wrote: > I don't use this glue layer, actually. As long as there are no > regressions, you can change it to your heart's content. I still it's > best to start with pm runtime blocked and let userspace decide what and > when should have pm runtime enabled. I don't use it either, I just noticed the use-after-free in remove() (and that probe was returning with a positive runtime PM usage count). I suggest merging this fix for 4.18-rc, and then Roger can rework the driver so that it works also on OMAP. Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Wed, Jun 13, 2018 at 12:39:18PM +0300, Felipe Balbi wrote:
> Roger Quadros writes:
> > I'm still trying to get my head around this.
> >
> > in probe() we do
> > {
> > enable all clocks;
> > pm_runtime_set_active(dev);
> > pm_runtime_enable(dev);
> > pm_runtime_get_sync(dev);
> > }
> >
> > How will runtime suspend work at all?
> > We're holding a positive RPM count in probe().
>
> echo auto > /path/to/dwc3/power/control
That makes no difference, user space cannot modify the always-active
behaviour given that probe returns with a positive usage count.
> Granted, that get_sync() would've been better as a pm_runtime_forbid()
Yeah, that would allow user space some control, albeit in a way that
may override a user space configuration (as the platform device would
already have been registered).
Why are you trying to prevent runtime pm in the first place? Shouldn't
the device be allowed to suspend when it has no active child by default?
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
Roger Quadros writes:
> On 13/06/18 11:05, Felipe Balbi wrote:
>> Roger Quadros writes:
>>
>>> Hi Johan,
>>>
>>> On 31/05/18 17:45, Johan Hovold wrote:
The clocks have already been explicitly disabled and put as part of
remove() so the runtime suspend callback must not be run when balancing
the runtime PM usage count before returning.
Fixes: 16adc674d0d6 ("usb: dwc3: add generic OF glue layer")
Signed-off-by: Johan Hovold
---
Changes in v2
- balance usage count only after disabling runtime PM to avoid racing
with pm_runtime_suspend() as suggested by Alan
drivers/usb/dwc3/dwc3-of-simple.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
b/drivers/usb/dwc3/dwc3-of-simple.c
index cb2ee96fd3e8..048922d549dd 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -165,8 +165,9 @@ static int dwc3_of_simple_remove(struct
platform_device *pdev)
reset_control_put(simple->resets);
- pm_runtime_put_sync(dev);
>>>
>>> Wasn't this call there to balance out the pm_runtime_get_sync() call in
>>> probe()?
>>> The pm_runtime_get_sync() call is still there in probe().
>>
>> that's now balanced by put_noidle below
>>
>>
>
> OK.
>
> I'm still trying to get my head around this.
>
> in probe() we do
> {
> enable all clocks;
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
> pm_runtime_get_sync(dev);
> }
>
> How will runtime suspend work at all?
> We're holding a positive RPM count in probe().
echo auto > /path/to/dwc3/power/control
Granted, that get_sync() would've been better as a pm_runtime_forbid()
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On 13/06/18 11:05, Felipe Balbi wrote:
> Roger Quadros writes:
>
>> Hi Johan,
>>
>> On 31/05/18 17:45, Johan Hovold wrote:
>>> The clocks have already been explicitly disabled and put as part of
>>> remove() so the runtime suspend callback must not be run when balancing
>>> the runtime PM usage count before returning.
>>>
>>> Fixes: 16adc674d0d6 ("usb: dwc3: add generic OF glue layer")
>>> Signed-off-by: Johan Hovold
>>> ---
>>>
>>> Changes in v2
>>> - balance usage count only after disabling runtime PM to avoid racing
>>>with pm_runtime_suspend() as suggested by Alan
>>>
>>>
>>> drivers/usb/dwc3/dwc3-of-simple.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
>>> b/drivers/usb/dwc3/dwc3-of-simple.c
>>> index cb2ee96fd3e8..048922d549dd 100644
>>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
>>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>>> @@ -165,8 +165,9 @@ static int dwc3_of_simple_remove(struct platform_device
>>> *pdev)
>>>
>>> reset_control_put(simple->resets);
>>>
>>> - pm_runtime_put_sync(dev);
>>
>> Wasn't this call there to balance out the pm_runtime_get_sync() call in
>> probe()?
>> The pm_runtime_get_sync() call is still there in probe().
>
> that's now balanced by put_noidle below
>
>
OK.
I'm still trying to get my head around this.
in probe() we do
{
enable all clocks;
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
pm_runtime_get_sync(dev);
}
How will runtime suspend work at all?
We're holding a positive RPM count in probe().
This was pointed out by Johan as well in [1]
[1] https://lkml.org/lkml/2018/5/28/1705
--
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
Roger Quadros writes:
> Hi Johan,
>
> On 31/05/18 17:45, Johan Hovold wrote:
>> The clocks have already been explicitly disabled and put as part of
>> remove() so the runtime suspend callback must not be run when balancing
>> the runtime PM usage count before returning.
>>
>> Fixes: 16adc674d0d6 ("usb: dwc3: add generic OF glue layer")
>> Signed-off-by: Johan Hovold
>> ---
>>
>> Changes in v2
>> - balance usage count only after disabling runtime PM to avoid racing
>>with pm_runtime_suspend() as suggested by Alan
>>
>>
>> drivers/usb/dwc3/dwc3-of-simple.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
>> b/drivers/usb/dwc3/dwc3-of-simple.c
>> index cb2ee96fd3e8..048922d549dd 100644
>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>> @@ -165,8 +165,9 @@ static int dwc3_of_simple_remove(struct platform_device
>> *pdev)
>>
>> reset_control_put(simple->resets);
>>
>> -pm_runtime_put_sync(dev);
>
> Wasn't this call there to balance out the pm_runtime_get_sync() call in
> probe()?
> The pm_runtime_get_sync() call is still there in probe().
that's now balanced by put_noidle below
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
Hi Johan,
On 31/05/18 17:45, Johan Hovold wrote:
> The clocks have already been explicitly disabled and put as part of
> remove() so the runtime suspend callback must not be run when balancing
> the runtime PM usage count before returning.
>
> Fixes: 16adc674d0d6 ("usb: dwc3: add generic OF glue layer")
> Signed-off-by: Johan Hovold
> ---
>
> Changes in v2
> - balance usage count only after disabling runtime PM to avoid racing
>with pm_runtime_suspend() as suggested by Alan
>
>
> drivers/usb/dwc3/dwc3-of-simple.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
> b/drivers/usb/dwc3/dwc3-of-simple.c
> index cb2ee96fd3e8..048922d549dd 100644
> --- a/drivers/usb/dwc3/dwc3-of-simple.c
> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> @@ -165,8 +165,9 @@ static int dwc3_of_simple_remove(struct platform_device
> *pdev)
>
> reset_control_put(simple->resets);
>
> - pm_runtime_put_sync(dev);
Wasn't this call there to balance out the pm_runtime_get_sync() call in probe()?
The pm_runtime_get_sync() call is still there in probe().
> pm_runtime_disable(dev);
> + pm_runtime_put_noidle(dev);
> + pm_runtime_set_suspended(dev);
>
> return 0;
> }
>
--
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Thu, May 31, 2018 at 04:45:52PM +0200, Johan Hovold wrote:
> The clocks have already been explicitly disabled and put as part of
> remove() so the runtime suspend callback must not be run when balancing
> the runtime PM usage count before returning.
>
> Fixes: 16adc674d0d6 ("usb: dwc3: add generic OF glue layer")
> Signed-off-by: Johan Hovold
> ---
>
> Changes in v2
Forgot to add a v2 prefix to the subject, sorry.
> - balance usage count only after disabling runtime PM to avoid racing
>with pm_runtime_suspend() as suggested by Alan
And this really should be a s/pm_runtime_suspend()/a runtime suspend
request/.
I blame the north European heat wave. ;)
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
