[lkml]   [2013]   [Jan]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] platform: Fix platform device resource linking
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?


>> 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


-- Pantelis

[unhandled content-type:application/octet-stream]
 \ /
  Last update: 2013-01-17 19:01    [W:0.063 / U:0.240 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site