usb: dwc3: of-simple: fix use-after-free on remove

usb: dwc3: of-simple: fix use-after-free on remove

[Roger Quadros]

On 18/06/18 12:51, Felipe Balbi wrote:
> 
> Hi,
> 
> Johan Hovold <johan@kernel.org> writes:
>> On Mon, Jun 18, 2018 at 12:33:44PM +0300, Felipe Balbi wrote:
>>
>>> Johan Hovold <johan@kernel.org> 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

usb: dwc3: of-simple: fix use-after-free on remove

[Alan Stern]

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc3: of-simple: fix use-after-free on remove

[Johan Hovold]

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@kernel.org> wrote:
> >> On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold <johan@kernel.org> 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@kernel.org> [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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc3: of-simple: fix use-after-free on remove

[Johan Hovold]

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 <rafael@kernel.org> wrote:
> > On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold <johan@kernel.org> 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@kernel.org> [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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc3: of-simple: fix use-after-free on remove

[Johan Hovold]

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 <johan@kernel.org> 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@kernel.org> [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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc3: of-simple: fix use-after-free on remove

[Roger Quadros]

On 21/06/18 01:55, Rafael J. Wysocki wrote:
> On Thu, Jun 21, 2018 at 12:32 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold <johan@kernel.org> 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@kernel.org> [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?

usb: dwc3: of-simple: fix use-after-free on remove

[Rafael J. Wysocki]

On Thu, Jun 21, 2018 at 12:32 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold <johan@kernel.org> 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@kernel.org> [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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc3: of-simple: fix use-after-free on remove

[Rafael J. Wysocki]

On Wed, Jun 20, 2018 at 5:46 PM, Johan Hovold <johan@kernel.org> 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@kernel.org> [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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc3: of-simple: fix use-after-free on remove

[Johan Hovold]

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@kernel.org> [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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc3: of-simple: fix use-after-free on remove

[Rafael J. Wysocki]

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@kernel.org> [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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc3: of-simple: fix use-after-free on remove

[Johan Hovold]

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@kernel.org> [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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc3: of-simple: fix use-after-free on remove

[Rafael J. Wysocki]

On Wed, Jun 20, 2018 at 1:05 PM, Felipe Balbi <balbi@kernel.org> wrote:
> "Rafael J. Wysocki" <rafael@kernel.org> writes:
>
>> On Wed, Jun 20, 2018 at 11:27 AM, Felipe Balbi <balbi@kernel.org> wrote:
>>>
>>> Hi,
>>>
>>> Tony Lindgren <tony@atomide.com> writes:
>>>> * Felipe Balbi <balbi@kernel.org> [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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc3: of-simple: fix use-after-free on remove

[Felipe Balbi]

"Rafael J. Wysocki" <rafael@kernel.org> writes:

> On Wed, Jun 20, 2018 at 11:27 AM, Felipe Balbi <balbi@kernel.org> wrote:
>>
>> Hi,
>>
>> Tony Lindgren <tony@atomide.com> writes:
>>> * Felipe Balbi <balbi@kernel.org> [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.

usb: dwc3: of-simple: fix use-after-free on remove

[Rafael J. Wysocki]

On Wed, Jun 20, 2018 at 11:27 AM, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Tony Lindgren <tony@atomide.com> writes:
>> * Felipe Balbi <balbi@kernel.org> [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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc3: of-simple: fix use-after-free on remove

[Felipe Balbi]

Hi,

Tony Lindgren <tony@atomide.com> writes:
> * Felipe Balbi <balbi@kernel.org> [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.

usb: dwc3: of-simple: fix use-after-free on remove

[Tony Lindgren]

Hi,

Adding Rafael and linux-pm to Cc as well.

* Felipe Balbi <balbi@kernel.org> [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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc3: of-simple: fix use-after-free on remove

[Tony Lindgren]

* Tero Kristo <t-kristo@ti.com> [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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc3: of-simple: fix use-after-free on remove

[Tero Kristo]

On 19/06/18 11:18, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc3: of-simple: fix use-after-free on remove

[Felipe Balbi]

Hi,

Roger Quadros <rogerq@ti.com> 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,
Allwinner, Mediatek, etc.


(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 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().

No, it doesn't. It has never been done; but how could it be?
platform_bus users aren't even ready to discuss methods to make sure
drivers can make assumptions about how PM should be implemented.

> 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."
I don't think that's saying what you think it's saying. That paragraph
is just telling you that pm_runtime doesn't know the state of the device
when probe is called, so it *always* defaults to suspended. If that's
not true, then you call pm_runtime_set_active().

Moreover, notice the little detail here:

"to ensure that the device is not put BACK to sleep during the probe"

It can only be put back to sleep if it was taken out of sleep before
hand. This is the same as calling pm_runtime_forbid() followed by
pm_runtime_set_active() followed by pm_runtime_enable(), here's
pm_runtime_forbid()'s implementation:

void pm_runtime_forbid(struct device *dev)
{
	spin_lock_irq(&dev->power.lock);
	if (!dev->power.runtime_auto)
		goto out;

	dev->power.runtime_auto = false;
	atomic_inc(&dev->power.usage_count);
	rpm_resume(dev, 0);

 out:
	spin_unlock_irq(&dev->power.lock);
}

> So looks like we can't assume that the device is "active" when probe()
> is called.

No, it's not a safe assumption for omap_device at least. I'm arguing
that it should be. There's no reason for you to not move
omap_device_enable() from _od_runtime_resume() to your pm_domain's
->activate() callback. Maybe not move, but copy it. Now you may need to
do something like below to make it work (clearly untested:

1 file changed, 14 insertions(+), 44 deletions(-)
arch/arm/mach-omap2/omap_device.c | 58 ++++++++++-----------------------------

modified   arch/arm/mach-omap2/omap_device.c
@@ -594,46 +594,22 @@ static int _od_fail_runtime_resume(struct device *dev)
 
 #endif
 
-#ifdef CONFIG_SUSPEND
-static int _od_suspend_noirq(struct device *dev)
+static void omap_device_activate(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct omap_device *od = to_omap_device(pdev);
-	int ret;
-
-	/* Don't attempt late suspend on a driver that is not bound */
-	if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER)
-		return 0;
-
-	ret = pm_generic_suspend_noirq(dev);
-
-	if (!ret && !pm_runtime_status_suspended(dev)) {
-		if (pm_generic_runtime_suspend(dev) == 0) {
-			omap_device_idle(pdev);
-			od->flags |= OMAP_DEVICE_SUSPENDED;
-		}
-	}
 
-	return ret;
+	od->flags &= ~OMAP_DEVICE_SUSPENDED;
+	omap_device_enable(pdev);
 }
 
-static int _od_resume_noirq(struct device *dev)
+static void omap_device_activate(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
-	struct omap_device *od = to_omap_device(pdev);
 
-	if (od->flags & OMAP_DEVICE_SUSPENDED) {
-		od->flags &= ~OMAP_DEVICE_SUSPENDED;
-		omap_device_enable(pdev);
-		pm_generic_runtime_resume(dev);
-	}
-
-	return pm_generic_resume_noirq(dev);
+	omap_device_idle(pdev);
+	od->flags |= OMAP_DEVICE_SUSPENDED;
 }
-#else
-#define _od_suspend_noirq NULL
-#define _od_resume_noirq NULL
-#endif
 
 struct dev_pm_domain omap_device_fail_pm_domain = {
 	.ops = {
@@ -647,9 +623,9 @@ struct dev_pm_domain omap_device_pm_domain = {
 		SET_RUNTIME_PM_OPS(_od_runtime_suspend, _od_runtime_resume,
 				   NULL)
 		USE_PLATFORM_PM_SLEEP_OPS
-		SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(_od_suspend_noirq,
-					      _od_resume_noirq)
 	}
+	.activate = omap_device_activate,
+	.dismiss = omap_device_dismiss,
 };
 
 /**
@@ -690,12 +666,9 @@ int omap_device_enable(struct platform_device *pdev)
 
 	od = to_omap_device(pdev);
 
-	if (od->_state == OMAP_DEVICE_STATE_ENABLED) {
-		dev_warn(&pdev->dev,
-			 "omap_device: %s() called from invalid state %d\n",
-			 __func__, od->_state);
-		return -EINVAL;
-	}
+	/* already enabled, just bail out */
+	if (od->_state == OMAP_DEVICE_STATE_ENABLED)
+		return 0;
 
 	ret = _omap_device_enable_hwmods(od);
 
@@ -721,12 +694,9 @@ int omap_device_idle(struct platform_device *pdev)
 
 	od = to_omap_device(pdev);
 
-	if (od->_state != OMAP_DEVICE_STATE_ENABLED) {
-		dev_warn(&pdev->dev,
-			 "omap_device: %s() called from invalid state %d\n",
-			 __func__, od->_state);
-		return -EINVAL;
-	}
+	/* if not enabled, bail out */
+	if (od->_state != OMAP_DEVICE_STATE_ENABLED)
+		return 0;
 
 	ret = _omap_device_idle_hwmods(od);
 
> Which means, we need to do
>
> pm_runtime_get_sync();
> enable optional clocks;

shouldn't be necessary with changes above.

usb: dwc3: of-simple: fix use-after-free on remove

[Roger Quadros]

+Tero, Tony and some TI folks

On 18/06/18 15:21, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>> On 18/06/18 12:51, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Johan Hovold <johan@kernel.org> writes:
>>>> On Mon, Jun 18, 2018 at 12:33:44PM +0300, Felipe Balbi wrote:
>>>>
>>>>> Johan Hovold <johan@kernel.org> 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

usb: dwc3: of-simple: fix use-after-free on remove

[Felipe Balbi]

Hi,

Roger Quadros <rogerq@ti.com> writes:
> On 18/06/18 12:51, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Johan Hovold <johan@kernel.org> writes:
>>> On Mon, Jun 18, 2018 at 12:33:44PM +0300, Felipe Balbi wrote:
>>>
>>>> Johan Hovold <johan@kernel.org> 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.

usb: dwc3: of-simple: fix use-after-free on remove

[Felipe Balbi]

Hi,

Johan Hovold <johan@kernel.org> writes:
> On Mon, Jun 18, 2018 at 12:33:44PM +0300, Felipe Balbi wrote:
>
>> Johan Hovold <johan@kernel.org> 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.

usb: dwc3: of-simple: fix use-after-free on remove

[Johan Hovold]

On Mon, Jun 18, 2018 at 12:33:44PM +0300, Felipe Balbi wrote:

> Johan Hovold <johan@kernel.org> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc3: of-simple: fix use-after-free on remove

[Felipe Balbi]

Hi,

Johan Hovold <johan@kernel.org> 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.

usb: dwc3: of-simple: fix use-after-free on remove

[Johan Hovold]

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc3: of-simple: fix use-after-free on remove

[Felipe Balbi]

Hi,

Johan Hovold <johan@kernel.org> writes:
> On Wed, Jun 13, 2018 at 12:39:18PM +0300, Felipe Balbi wrote:
>> Roger Quadros <rogerq@ti.com> 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.

usb: dwc3: of-simple: fix use-after-free on remove

[Johan Hovold]

On Wed, Jun 13, 2018 at 12:39:18PM +0300, Felipe Balbi wrote:
> Roger Quadros <rogerq@ti.com> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc3: of-simple: fix use-after-free on remove

[Felipe Balbi]

Roger Quadros <rogerq@ti.com> writes:

> On 13/06/18 11:05, Felipe Balbi wrote:
>> Roger Quadros <rogerq@ti.com> 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 <johan@kernel.org>
>>>> ---
>>>>
>>>> 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()

usb: dwc3: of-simple: fix use-after-free on remove

[Roger Quadros]

On 13/06/18 11:05, Felipe Balbi wrote:
> Roger Quadros <rogerq@ti.com> 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 <johan@kernel.org>
>>> ---
>>>
>>> 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

usb: dwc3: of-simple: fix use-after-free on remove

[Felipe Balbi]

Roger Quadros <rogerq@ti.com> 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 <johan@kernel.org>
>> ---
>> 
>> 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

usb: dwc3: of-simple: fix use-after-free on remove

[Roger Quadros]

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 <johan@kernel.org>
> ---
> 
> 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;
>  }
>

usb: dwc3: of-simple: fix use-after-free on remove

[Johan Hovold]

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 <johan@kernel.org>
> ---
> 
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: dwc3: of-simple: fix use-after-free on remove

[Johan Hovold]

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 <johan@kernel.org>
---

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);
 	pm_runtime_disable(dev);
+	pm_runtime_put_noidle(dev);
+	pm_runtime_set_suspended(dev);
 
 	return 0;
 }