Messages in this thread |  | | Subject | Re: [PATCH] platform: Fix platform device resource linking | From | Pantelis Antoniou <> | Date | Thu, 17 Jan 2013 19:27:21 +0200 |
| |
Hi Greg,
On Jan 17, 2013, at 7:07 PM, Greg Kroah-Hartman wrote:
> 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? >
Point.
>> 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? >
This is all part of a larger patchset; I guess you weren't directly CCed. The name of the patchset is 'Introducing Device Tree Overlays' and is a method of changing the live device tree and have the changes reflected to the kernel's state.
As I mentioned earlier, device tree platform devices were never removed up until now; the DT statically described the hardware of a board and there wasn't any way to remove a device.
As part of the Device Tree Overlay functionality, an overlay should be possible to be removed. The crash happens when a platform device created by DT is removed.
>> 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? >
Because while DT creates platform devices, it doesn't use the platform device methods to do so, rather than builds the platform device itself. This is something that was overlooked.
>> 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? >
It's not about a device parent, it's about a resource parent. In general resource handling in the DT world is a big WIP. One step to that direction is to have the resources properly linked as the rest of the kernel code expects.
>> 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. >
Looking at my mailer, it looks like the patch that uses this got dropped since it is such a small patch.
That is my mistake and apologize for the severe confusion.
The patch in question is attached; I will sent it along by itself too.
> thanks, > > greg k-h
Regards
-- Pantelis
[unhandled content-type:application/octet-stream] |  |