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
0001-Link-platform-device-resources-properly.patch
Description: Binary data
