Re: [PATCH] of: provide of_platform_unpopulate()

[Grant Likely <grant.likely@linaro.org>]

On Mon, 22 Jul 2013 16:16:07 -0500, Rob Herring <robherring2@gmail.com> wrote:
> On 07/21/2013 06:44 PM, Grant Likely wrote:
> > On Sun, Jul 21, 2013 at 9:48 PM, Rob Herring <robherring2@gmail.com> wrote:
> >> On 07/21/2013 09:42 AM, Rob Herring wrote:
> >>> On 07/19/2013 01:14 PM, Sebastian Andrzej Siewior wrote:
> >>>> So I called of_platform_populate() on a device to get each child device
> >>>> probed and on rmmod and I need to reverse its doing. After a quick grep
> >>>> I did what others did as well and rmmod ended in:
> >>>>
> >>>> | Unable to handle kernel NULL pointer dereference at virtual address 00000018
> >>>> | PC is at release_resource+0x18/0x80
> >>>> | Process rmmod (pid: 2005, stack limit = 0xedc30238)
> >>>> | [<c003add0>] (release_resource+0x18/0x80) from [<c0300e08>] (platform_device_del+0x78/0xac)
> >>>> | [<c0300e08>] (platform_device_del+0x78/0xac) from [<c0301358>] (platform_device_unregister+0xc/0x18)
> >>>>
> >>>> The problem is that platform_device_del() "releases" each ressource in its
> >>>> tree. This does not work on platform_devices created by OF becuase they
> >>>> were never added via insert_resource(). As a consequence old->parent in
> >>>> __release_resource() is NULL and we explode while accessing ->child.
> >>>> So I either I do something completly wrong _or_ nobody here tested the
> >>>> rmmod path of their driver.
> >>>
> >>> Wouldn't the correct fix be to call insert_resource somehow? The problem
> >>> I have is that while of_platform_populate is all about parsing the DT
> >>> and creating devices, the removal side has nothing to do with DT. So
> >>> this should not be in the DT code. I think the core device code should
> >>> be able to handle removal if the device creation side is done correctly.
> >>>
> >>> It looks to me like of_device_add either needs to call
> >>> platform_device_add rather than device_add. I think the device name
> >>> setting in platform_device_add should be a nop. If not, a check that the
> >>> name is already set could be added.
> >>>
> >>
> >> BTW, it looks like Grant has attempted this already:
> > 
> > Yup, things broke badly. Unfortunately the of_platform_device and
> > platform_device history doesn't treat resources in the same way. I
> > would like to merge the code, but I haven't been able to figure out a
> > clean way to do it. Looks like we do need the unpopulate function.
> 
> Was there more breakage than imx6 and amba devices? Your first version
> had a fallback case for powerpc. Couldn't we do just allow that for more
> than just powerpc? I'd much rather see some work-around within the core
> DT code with a warning to prevent more proliferation than putting this
> into drivers.

It's tricky stuff. I've not figured out a solution I'm happy with.
Trying to figure out when to apply a work around is hard because the
resource reservation makes assumptions about the memory range layout
that doesn't match the assumptions made by device tree code.

One /possible/ option is to not add the resources to the devices at all
when the device is registered and instead resolve them right at bind
time. Jean Christophe proposed doing this already to solve a different
problem; obtaining resources that require other drivers to be probed
first. If the resources are resolved at .probe() time, then the resource
registration problem should also go away.

The downside to that approach is that it makes each deferred probe more
expensive; potentially a *lot* more expensive depending on how much work
the xlate functions have to do. It would be worth prototyping though to
see how well it works.

g.