Message-ID: <CAPybu_1QYHh7Kv1mnm8qpA9abx_XNMgt8x5n5Y-7k2HNqrqruQ@mail.gmail.com>
Date:	Mon, 8 Jun 2015 22:47:38 +0200
From:	Ricardo Ribalda Delgado <ricardo.ribalda@...il.com>
To:	Grant Likely <grant.likely@...aro.org>
Cc:	Rob Herring <robherring2@...il.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
	Wolfram Sang <wsa@...-dreams.de>,
	Rob Herring <robh@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH 2/2] drivercore: Fix unregistration path of platform devices

Hello

Regarding the two problems

1) The  immediate bug fix  for dt unload

I agree that we should use the simplest possible patch for
backporting, but I believe that Grant patch does not differ too much
from

https://git.kernel.org/cgit/linux/kernel/git/gregkh/driver-core.git/commit/?h=driver-core-next&id=e50e69d1ac4232af0b6890f16929bf5ceee81538

which is already in driver-core-next and throws a nice warning message
for debugging.  I believe that this is the patch that should be
backported.


2) Not adding resources:

Until we found a solution for the platforms that are broken we only
need to revert
https://git.kernel.org/cgit/linux/kernel/git/gregkh/driver-core.git/commit/?h=driver-core-next&id=b6d2233f2916fa9338786aeab2e936c5a07e4d0c

We get a lot of code cleanout (already reviewed) by doing this.


I think reverting the whole series is not the best solution specially
being a v5 :)

Anyway whatever we decide I have some hardware where I can run tests if needed


Regards and sorry for the flood!

On Mon, Jun 8, 2015 at 10:09 PM, Ricardo Ribalda Delgado
<ricardo.ribalda@...il.com> wrote:
> Hello Grant
>
>
> On Mon, Jun 8, 2015 at 8:47 PM, Grant Likely <grant.likely@...aro.org> wrote:
>> Hi Ricardo,
>>
>> Comments below...
>>
>> On Sun, 7 Jun 2015 20:13:15 +0200
>> , Ricardo Ribalda Delgado <ricardo.ribalda@...il.com>
>>  wrote:
>>> Hello Grant
>>>
>>> I would ask you to go through all the discussion related to this bug.
>>> Here is a summary (please anyone involved correct me if I am wrong)
>>>
>>> 1) I send a patch to fix the oops if release resource is executed with
>>> a resource without parent
>>> 2) Bjorn says that we should fix the issue of the problem, which he
>>> pointed out being that we use platform_device_del() after using
>>> of_device_add()
>>
>> Bjorn's comments on v3 of your patchset were correct. The proposed bug
>> fix hacked the __release_resource function directly, when the bug is in
>> the platform_bus_type code.
>>
>
> The bug is not in the platform subsystem but in the of subsystem. Your
> patch fixes it in the platform subsystem, so it is as bad as fixing it
> directly on the resource interface.
>
>
>>> 3) I resend a patchset to use platform_devide_add()
>>> 4) 3 series of cleanouts after the help from Rob and Bjorn
>>> 5) Greg adds the series (v5) to his device core tree
>>
>> The series is still wrong.
>>
>> Greg, please drop Ricardo's series. It isn't correct and it will cause
>> breakage.
>
> The series can be kept, only
>
> patch "of/platform: Use platform_device interface"
>
> needs to be reverted.
>
>>
>> There are two issues that need to be delt with:
>>
>> First, there is the immediate bug fix which should go to Linus before
>> v4.1. I believe my patch handles it correctly. I've included a test
>> case, but I would like to have acks from Rob and Pantelis before merging
>> it. Ricardo's v5 patch 2/4 comes close to solving it, but it still
>> doesn't make the unregister path symmetric with the register path.
>
> Could you please be more specific. what is not symmetric after
> applying the patchset?
>
>
>> Second, there is the issue of making devicetree platform_devices request
>> resources.  That's harder, and we are *NOT* ready to merge anything. Nor
>> is it a time critical issue.
>>
>>> 6) You complaint that that series can break miss behaved platforms
>>
>> Yes, because it will.
>>
>>> 7) I send a couple of patches that fix your problem and leaves the
>>> window open to blacklist the platforms that miss behave.
>>
>> I've replied to that series. It isn't a good solution either.
>
> I have also replied, please provide a testcase and we will figure it
> if it is not handled properly. So far it works fine on my tests.
>
>>>
>>> now you send a patch that takes us to back to step 1), and adds some
>>> code that is already merged into gregk's
>>> https://git.kernel.org/cgit/linux/kernel/git/gregkh/driver-core.git/tree/drivers/base/platform.c?h=driver-core-testing#n314
>>
>> My patch is different. In v3 __release_resource was hacked directly. By
>> v5 you were fixing platform_device_{add,del}, which is the right thing,
>> but still isn't symmetric. My patch I think handles the bug fix
>> correctly.
>
> There is no need to apply your patch, that behaviour is already
> impletented in my patchset. If we want to pospone the non registry of
> resources on of devices we just need to revert
>
> "of/platform: Use platform_device interface"
>
> I believe reverting 1 patch is patch is better than reverting 4
> reviewed patches and applying a new one.
>
>>
>>> Wouldn't you agree that it will be a better solution to give your
>>> feedback regarding https://lkml.org/lkml/2015/6/5/246 and fix this
>>> issue together?
>>
>> That I've done. I'm not happy with it. Sorry.
>
> No worries :), but we need to find another sollution. And if we can
> remove all the duplicated code in /of we will have much less bugs in
> the future.
>
>
> Regards



-- 
Ricardo Ribalda
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists