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 [off-list ref] writes:

quoted

On 18/06/18 12:51, Felipe Balbi wrote:

quoted

Hi,

Johan Hovold [off-list ref] writes:

quoted

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

quoted

Johan Hovold [off-list ref] writes:

quoted

quoted

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.

quoted

for that because
- it needs to poke a few registers in the wrapper region

dwc3-keystone.c does that already

quoted

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

quoted

- 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