Re: [PATCH 3/5] omap: Properly handle resources for omap_devices

[Pantelis Antoniou <panto@antoniou-consulting.com>]

Hi Kevin,

On Aug 9, 2013, at 7:35 PM, Kevin Hilman wrote:

> Pantelis Antoniou <panto@antoniou-consulting.com> writes:
> 
>> Hi
>> On Aug 9, 2013, at 6:16 PM, Kevin Hilman wrote:
>> 
>>> Pantelis Antoniou <panto@antoniou-consulting.com> writes:
>>> 
>>>> Hi Kevin,
>>>> 
>>>> On Aug 7, 2013, at 9:45 PM, Kevin Hilman wrote:
>>>> 
>>>>> [fixing address for Benoit]
>>>>> 
>>>>> Pantelis Antoniou <panto@antoniou-consulting.com> writes:
>>>>> 
>>>>>> omap_device relies on the platform notifier callbacks managing resources
>>>>>> behind the scenes. The resources were not properly linked causing crashes
>>>>>> when removing the device.
>>>>>> 
>>>>>> Rework the resource modification code so that linking is performed properly,
>>>>>> and make sure that no resources that have no parent (which can happen for DMA
>>>>>> & IRQ resources) are ever left for cleanup by the core resource layer.
>>>>>> 
>>>>>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
>>>>> 
>>>>> This one failed my "took more than 15 minutes to understand" test.  The
>>>>> changelog is rather vague (especially about what "properly" means), and
>>>>> the combination of moving code and changing it makes the patch rather
>>>>> clunky to read, so I remain a bit confused about what the actual problem
>>>>> is.  Please elaborate.
>>>>> 
>>>>> Also, could you share a crash dump as well as details about how to
>>>>> reproduce this problem?
>>>>> 
>>>>> Thanks,
>>>>> 
>>>>> Kevin
>>>> 
>>>> It's the full patchset that fixes the problem:
>>>> 
>>>> Let me illustrate:
>>>> 
>>>> The kernel I use is located at:
>>>> 
>>>> git@github.com:pantoniou/linux-beagle-track-mainline.git
>>>> branch: merge-20130806 (there are topic branches for other stuff too)
>>> 
>>> Sorry, I don't have the time to go through a bunch of out of tree
>>> branches to figure this out.  Can you create a simpler test case to
>>> reproduce this?  e.g. Does this happen when building the serial driver
>>> as a module and then removing it?  If not, why not?
>> 
>> What 'bunch of out of tree branches'? There is a single merge branch against
>> current mainline. 
> 
> And That branch contains multiple other branches (as you stated yourself
> above), many of which look to be out of tree:
> 
> $ git log --oneline --merges  panto/master..panto/merge-20130806
> 031297e Merge branch 'lcdc-fixes' into merge-20130806
> 44cf018 Merge branch 'usb-fixes' into merge-20130806
> 3cc739b Merge branch 'tscadc' into merge-20130806
> f0ec35d Merge branch 'capemgr' into merge-20130806
> 4d87712 Merge branch 'general-fixes' into merge-20130806
> 04535fb Merge branch 'pinctrl-fixes' into merge-20130806
> 5115b55 Merge branch 'i2c-fixes' into merge-20130806
> e96edf5 Merge branch 'capes' into merge-20130806
> 1eb1779 Merge branch 'dts-fixes' into merge-20130806
> ca12149 Merge branch 'mmc-fixes' into merge-20130806
> 41ed7a0 Merge branch 'pdev-fixes' into merge-20130806
> 2ba9995 Merge branch 'of-fixes' into merge-20130806
> 38b5cb4 Merge branch 'dtc-overlays' into merge-20130806
> 167648d Merge branch 'dtc-fixes' into merge-20130806
> 2f7de02 Merge branch 'reset' into merge-20130806
> 
> $ git log --oneline --no-merges panto/master..panto/merge-20130806 |wc -l
> 85
> 

Yes, but they have nothing to do with the problem at hand; the only
branch that matters i pdev-fixes.

>> And no you don't trigger this by removing a module.
>> 
>> platform_driver_unregister() != platform_device_register().
>> 
>> I'll try to figure out something simpler, but it's pretty damn obvious that
>> something is not right there. 
> 
> I agree something does not seem right, and never said otherwise.  I'm
> simply looking for an easy way to reproduce it with mainline.  
> 
> Since it's so "damn obvious", I'll accept the insult and agree to being
> a moron.  Please educate me by making it even more obvious with a way to
> reproduce it against mainline.
> 

You can't reproduce it via rmmod'ing a module; it's not the same code path.
That you need to do is to call platform_device_unregister() for the affected
device on a DT enabled board.

The only platform that triggers it currently is the beaglebone with DT
overlays support.

To get it to trigger on something simpler, I'll have to write a pretty
elaborate module where I register the device and then immediately unregister
it.

>> Do you think fixing problems in that part of platform device removal
>> was something I did because I didn't have other things to do?
> 
> Settle down, I said nothing of the sort.
> 
> On the contrary, I assumed you were being a generous citizen of the
> community and contributing back your fixes, and as a developer of this
> part of the kernel, I'm trying to help.  If I didn't care, I wouldn't
> have responded in the first place.
> 

OK

>> It is broken and I'm trying to get it fixed. Are you?
> 
> I was, but your accusatory tone is reducing my motivation to help.
> 

I guess that's a fair accusation; sorry for coming out a bit strong here.

Anyway, I'll see what it takes to come up with a test module that exhibits the issue
easier.

> Kevin
> 

Regards

-- Pantelis