Re: [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node

[Mark Rutland <mark.rutland@arm.com>]

On Fri, Mar 14, 2014 at 04:34:19PM +0000, Tomi Valkeinen wrote:
> On 14/03/14 18:04, Felipe Balbi wrote:
> > On Fri, Mar 14, 2014 at 02:07:45PM +0000, Mark Rutland wrote:
> >> On Fri, Mar 14, 2014 at 11:07:05AM +0000, Tomi Valkeinen wrote:
> >>> On 14/03/14 12:19, Tomi Valkeinen wrote:
> >>>> On 14/03/14 12:14, Mark Rutland wrote:
> >>>>
> >>>>> I can't see anything obviously wrong in platform_device_del. Do you have
> >>>>> a backtrace?
> >>>>
> >>>> Yes, below.
> >>>>
> >>>> I can see at least drivers/usb/dwc3/dwc3-exynos.c doing the exact same thing
> >>>> I do, so maybe I've got something wrong with the omapdss driver.
> >>>
> >>> Looks to me that the devices created by of_platform_populate() are not
> >>> unregisterable in all cases. The address resource created via
> >>> of_platform_populate() had NULL res->parent, which causes
> >>> release_resource to crash.
> >>
> >> Hmm. I can't see that unregistering such devices ever works as you say,
> >> given that __release_resource expects a non-NULL parent pointer. Either
> >> we should be setting the parent pointer when initialising devices from
> >> dt or we should teach __release_resource to not care. I'll have a go at
> >> fixing that.
> >>
> >> It looks like drivers/usb/dwc3/dwc3-exynos.c only unregisters the
> >> top-level device, not children. This top-level device has no
> >> IORESOURCE_{IO,MEM} resources judging by
> >> arch/arm/boot/dts/exynos5250.dtsi, which would explain why that driver
> >> isn't exploding: __release_resource will never get called.
> >>
> >> Anton, Felipe: 
> >>
> >> Does unregistering the parent ensure the children get cleaned up, or
> >> does it leave them dangling in the dwc3-exynos driver?
> > 
> > you should platform_device_unregister() for each children and
> > dwc3-exynos does that just fine:
> 
> Yes, that's what I do also, and it crashes. What Mark said above about
> unregistering never working for such devices is correct, but I don't
> know why he said dwc3-exynos only unregisters the top level device.

Because I'd failed to read the code correctly. It does indeed unregister
each of the children via platform_device_unregister.

Apologies for the confusion there.

> "such devices" above meaning devices with a 'reg' defined in the DT
> data, if I'm not mistaken.

Yes. As far as I can see, IORESOURCE_MEM resources instantiated from reg
entries in dt do not have their parent pointer initialised, and will
cause __release_resource to blow up. A quick inspection of resources on
my TC2 shows this to be the case -- the parent, sibling, and child
pointers are all NULL.

I'm at a loss to explain how the dwc3-exynos driver can call
platform_device_unregister on devices with such resource and not blow
up. Is anyone able to test dwc3_exynos_remove?

> So at the moment, I think of_platform_populate() and
> platform_device_unregister() combination in a driver is a bit risky.
> Work fine for certain cases, not for some other.

It certainly looks to be broken at the moment, but it should be fixable.

Thanks,
Mark.