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

[Pantelis Antoniou]

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

Attachment: 0001-Link-platform-device-resources-properly.patch
Description: Binary data