Re: [PATCH] platform: Fix platform device resource linking

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Jan 17, 2013 at 06:50:19PM +0200, Pantelis Antoniou wrote:
> 
> On Jan 17, 2013, at 6:31 PM, Greg Kroah-Hartman wrote:
> 
> > On Fri, Jan 04, 2013 at 12:43:46AM +0200, Pantelis Antoniou wrote:
> >> Hi Greg,
> >> 
> >> On Jan 4, 2013, at 12:40 AM, Greg Kroah-Hartman wrote:
> >> 
> >>> On Fri, Jan 04, 2013 at 12:31:10AM +0200, Pantelis Antoniou wrote:
> >>>> Platform device removal uncovered a number of problems with
> >>>> the way resources are handled in the core platform code.
> >>>> 
> >>>> Resources now form child/parent linkages and this requires
> >>>> proper linking of the resources. On top of that the OF core
> >>>> directly creates it's own platform devices. Simplify things
> >>>> by providing helper functions that manage the linking properly.
> >>>> 
> >>>> Two functions are provided:
> >>>> 
> >>>> platform_device_link_resources(), which links all the
> >>>> linkable resources (if not already linked).
> >>>> 
> >>>> and platform_device_unlink_resources(), which unlinks all the
> >>>> resources.
> >>> 
> >>> Who would call these functions, and why?
> >>> 
> >>> And why have we never seen problems with removing platform devices
> >>> previously?
> >>> 
> >> 
> >> Have you tried removing devices that were created via DT and 
> >> not using platform data?
> > 
> > Don't you think that answering two questions with another question as
> > something that isn't very helpful?  :)
> > 
> > Dropped from my queue, please resend when you can provide the needed
> > information.
> > 
> > thanks,
> > 
> 
> That's not very nice, but anyway...

What would you have me do if you were in my shoes?

> In a nutshell, we have to exercise the platform device subsystem, in ways
> that never happened before, so all sorts of weird bugs that no-one has seen
> before.

Why do you have to do this?  What are you doing that is so different
from everyone else?  What drivers are you using that trigger this type
of thing?

> In that case, the code path for creating platform devices from DT is
> not the same as the one that is used when creating platform device from
> a board file.

Why not?

> Take a look at platform_device_add() in drivers/base/platform.c and
> drivers/of/device.c
> 
> platform_device_add() properly links the resources by using insert_resource(),
> while of_device_add() doesn't bother with it. Now when we try to unregister
> the device everything will is fine in the platform device case, since the resources
> are linked properly. In the DT case we will crash spectacularly in 
> __release_resource at the first line (p = &old->parent->child), since no-one bothered
> to fill in the parent pointer.

So, isn't that a bug in the DT case?  A device always has to have a
parent, as you have found out.  Hm, maybe not "root" devices, but you
don't have many of those, right?

> That's what the patches do; first the code in platform_device_add() that perform the
> resource linking is factored as a separate function (platform_device_link_resources).
> 
> The platform_device_unlink_resources() function, just makes things more clearer. 

But you added a new function that no one calls, which is what I am
objecting to.

thanks,

greg k-h