The register and unregister paths for platform_devices use different
tests to chose which resources to process. Register uses the value of
both parent & type, but unregister relies solely on type, which can
result in some resources not being unregistered, and an oops when an
unregistered resource is attempted to be removed. The oops issue is
particularly a problem for devicetree users because resources are not
registered in that path.
Add a test that exposes the problem for devicetree users, and then fix
the problem.
Add a single resource to the test bus device to exercise the platform
bus code a little more. This isn't strictly a devicetree test, but it is
a corner case that the devicetree runs into. Until we've got platform
device unittests, it can live here. It doesn't need to be an explicit
text because the kernel will oops when it is wrong.
Cc: Pantelis Antoniou <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Ricardo Ribalda Delgado <[email protected]>
Signed-off-by: Grant Likely <[email protected]>
---
drivers/of/unittest.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 18016341d5a9..0a27b38c3041 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -753,6 +753,11 @@ static void __init of_unittest_match_node(void)
}
}
+static struct resource test_bus_res = {
+ .start = 0xfffffff8,
+ .end = 0xfffffff9,
+ .flags = IORESOURCE_MEM,
+};
static const struct platform_device_info test_bus_info = {
.name = "unittest-bus",
};
@@ -795,6 +800,7 @@ static void __init of_unittest_platform_populate(void)
if (rc)
return;
test_bus->dev.of_node = np;
+ platform_device_add_resources(test_bus, &test_bus_res, 1);
of_platform_populate(np, match, NULL, &test_bus->dev);
for_each_child_of_node(np, child) {
--
2.1.4
The unregister path of platform_device is broken. On registration, it
will register all resources with either a parent already set, or
type==IORESOURCE_{IO,MEM}. However, on unregister it will release
everything with type==IORESOURCE_{IO,MEM}, but ignore the others. There
are also cases where resources don't get registered in the first place,
like with devices created by of_platform_populate()*.
Fix the unregister path to be symmetrical with the register path by
checking the parent pointer instead of the type field to decide which
resources to unregister. This is safe because the upshot of the
registration path algorithm is that registered resources have a parent
pointer, and non-registered resources do not.
* It can be argued that of_platform_populate() should be registering
it's resources, and they argument has some merit. However, there are
quite a few platforms that end up broken if we try to do that due to
overlapping resources in the device tree. Until that is fixed, we need
to solve the immediate problem.
Cc: Pantelis Antoniou <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Ricardo Ribalda Delgado <[email protected]>
Signed-off-by: Grant Likely <[email protected]>
---
drivers/base/platform.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index ebf034b97278..7403de94832c 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -375,9 +375,7 @@ int platform_device_add(struct platform_device *pdev)
while (--i >= 0) {
struct resource *r = &pdev->resource[i];
- unsigned long type = resource_type(r);
-
- if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
+ if (r->parent)
release_resource(r);
}
@@ -408,9 +406,7 @@ void platform_device_del(struct platform_device *pdev)
for (i = 0; i < pdev->num_resources; i++) {
struct resource *r = &pdev->resource[i];
- unsigned long type = resource_type(r);
-
- if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
+ if (r->parent)
release_resource(r);
}
}
--
2.1.4
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()
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
6) You complaint that that series can break miss behaved platforms
7) I send a couple of patches that fix your problem and leaves the
window open to blacklist the platforms that miss behave.
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
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 solution has been reviewed by a bunch of people, removes code
duplication and afaik, is tested, does not break any platform, and I
believe that is closer to an scenario when we can remove
of_device_add() and all the devices behave similarly.
Best Regards
On Sun, Jun 7, 2015 at 4:20 PM, Grant Likely <[email protected]> wrote:
> The unregister path of platform_device is broken. On registration, it
> will register all resources with either a parent already set, or
> type==IORESOURCE_{IO,MEM}. However, on unregister it will release
> everything with type==IORESOURCE_{IO,MEM}, but ignore the others. There
> are also cases where resources don't get registered in the first place,
> like with devices created by of_platform_populate()*.
>
> Fix the unregister path to be symmetrical with the register path by
> checking the parent pointer instead of the type field to decide which
> resources to unregister. This is safe because the upshot of the
> registration path algorithm is that registered resources have a parent
> pointer, and non-registered resources do not.
>
> * It can be argued that of_platform_populate() should be registering
> it's resources, and they argument has some merit. However, there are
> quite a few platforms that end up broken if we try to do that due to
> overlapping resources in the device tree. Until that is fixed, we need
> to solve the immediate problem.
>
> Cc: Pantelis Antoniou <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Ricardo Ribalda Delgado <[email protected]>
> Signed-off-by: Grant Likely <[email protected]>
> ---
> drivers/base/platform.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index ebf034b97278..7403de94832c 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -375,9 +375,7 @@ int platform_device_add(struct platform_device *pdev)
>
> while (--i >= 0) {
> struct resource *r = &pdev->resource[i];
> - unsigned long type = resource_type(r);
> -
> - if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
> + if (r->parent)
> release_resource(r);
> }
>
> @@ -408,9 +406,7 @@ void platform_device_del(struct platform_device *pdev)
>
> for (i = 0; i < pdev->num_resources; i++) {
> struct resource *r = &pdev->resource[i];
> - unsigned long type = resource_type(r);
> -
> - if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
> + if (r->parent)
> release_resource(r);
> }
> }
> --
> 2.1.4
>
--
Ricardo Ribalda
Hi Ricardo, Grant,
> On Jun 7, 2015, at 21:13 , Ricardo Ribalda Delgado <[email protected]> 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()
> 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
> 6) You complaint that that series can break miss behaved platforms
> 7) I send a couple of patches that fix your problem and leaves the
> window open to blacklist the platforms that miss behave.
>
> 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
>
>
> 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 solution has been reviewed by a bunch of people, removes code
> duplication and afaik, is tested, does not break any platform, and I
> believe that is closer to an scenario when we can remove
> of_device_add() and all the devices behave similarly.
>
>
> Best Regards
>
Either patch fixes (or rather papers over) the problem.
The way I understand it is that we have two issues.
1. The rather obvious crash on device removal.
2. The of_platform_populate (and the subsequent call to
of_platform_device_create_pdata()) not registering the resources
due to bugs in platforms that do overlapping regions.
#1 is fixed but #2 is rather touchy.
I would like to eventually fix those platforms; starting with a
nice fat warning when using overlapping regions would be nice.
The problem is how to deal with range definitions that overlap.
Does anyone remember what does the spec say regarding those?
If the way they are used now in those platform is wrong we should
mark them in some manner in the DT so that the idiom does not propagate.
If they are valid, so be it, we have to fix it in driver core somehow.
Regards
— Pantelis
>
>
> On Sun, Jun 7, 2015 at 4:20 PM, Grant Likely <[email protected]> wrote:
>> The unregister path of platform_device is broken. On registration, it
>> will register all resources with either a parent already set, or
>> type==IORESOURCE_{IO,MEM}. However, on unregister it will release
>> everything with type==IORESOURCE_{IO,MEM}, but ignore the others. There
>> are also cases where resources don't get registered in the first place,
>> like with devices created by of_platform_populate()*.
>>
>> Fix the unregister path to be symmetrical with the register path by
>> checking the parent pointer instead of the type field to decide which
>> resources to unregister. This is safe because the upshot of the
>> registration path algorithm is that registered resources have a parent
>> pointer, and non-registered resources do not.
>>
>> * It can be argued that of_platform_populate() should be registering
>> it's resources, and they argument has some merit. However, there are
>> quite a few platforms that end up broken if we try to do that due to
>> overlapping resources in the device tree. Until that is fixed, we need
>> to solve the immediate problem.
>>
>> Cc: Pantelis Antoniou <[email protected]>
>> Cc: Wolfram Sang <[email protected]>
>> Cc: Rob Herring <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: Ricardo Ribalda Delgado <[email protected]>
>> Signed-off-by: Grant Likely <[email protected]>
>> ---
>> drivers/base/platform.c | 8 ++------
>> 1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> index ebf034b97278..7403de94832c 100644
>> --- a/drivers/base/platform.c
>> +++ b/drivers/base/platform.c
>> @@ -375,9 +375,7 @@ int platform_device_add(struct platform_device *pdev)
>>
>> while (--i >= 0) {
>> struct resource *r = &pdev->resource[i];
>> - unsigned long type = resource_type(r);
>> -
>> - if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
>> + if (r->parent)
>> release_resource(r);
>> }
>>
>> @@ -408,9 +406,7 @@ void platform_device_del(struct platform_device *pdev)
>>
>> for (i = 0; i < pdev->num_resources; i++) {
>> struct resource *r = &pdev->resource[i];
>> - unsigned long type = resource_type(r);
>> -
>> - if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
>> + if (r->parent)
>> release_resource(r);
>> }
>> }
>> --
>> 2.1.4
>>
>
>
>
> --
> Ricardo Ribalda
Hello Pantelis
On Mon, Jun 8, 2015 at 10:14 AM, Pantelis Antoniou
<[email protected]> wrote:
>>
>
> Either patch fixes (or rather papers over) the problem.
>
> The way I understand it is that we have two issues.
>
> 1. The rather obvious crash on device removal.
> 2. The of_platform_populate (and the subsequent call to
> of_platform_device_create_pdata()) not registering the resources
> due to bugs in platforms that do overlapping regions.
>
>
> #1 is fixed but #2 is rather touchy.
I believe that after my last patchset also #2 is fixed.
The only missing part is applying the shared flag to ONLY the broken
OF platforms, not to all.
We even have a warning on overlapping resources, that should help the
development
https://git.kernel.org/cgit/linux/kernel/git/gregkh/driver-core.git/tree/drivers/base/platform.c?h=driver-core-testing#n379
Hopefully one day we can merge of and platform :)
>
> I would like to eventually fix those platforms; starting with a
> nice fat warning when using overlapping regions would be nice.
>
> The problem is how to deal with range definitions that overlap.
> Does anyone remember what does the spec say regarding those?
>
> If the way they are used now in those platform is wrong we should
> mark them in some manner in the DT so that the idiom does not propagate.
We could easily find out which DT have overlapping resources on the tree...
Regars!
Hi Ricardo,
Comments below...
On Sun, 7 Jun 2015 20:13:15 +0200
, Ricardo Ribalda Delgado <[email protected]>
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.
> 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.
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.
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.
>
> 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.
> 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.
> that solution has been reviewed by a bunch of people, removes code
> duplication and afaik, is tested, does not break any platform, and I
> believe that is closer to an scenario when we can remove
> of_device_add() and all the devices behave similarly.
>
>
> Best Regards
>
>
>
> On Sun, Jun 7, 2015 at 4:20 PM, Grant Likely <[email protected]> wrote:
> > The unregister path of platform_device is broken. On registration, it
> > will register all resources with either a parent already set, or
> > type==IORESOURCE_{IO,MEM}. However, on unregister it will release
> > everything with type==IORESOURCE_{IO,MEM}, but ignore the others. There
> > are also cases where resources don't get registered in the first place,
> > like with devices created by of_platform_populate()*.
> >
> > Fix the unregister path to be symmetrical with the register path by
> > checking the parent pointer instead of the type field to decide which
> > resources to unregister. This is safe because the upshot of the
> > registration path algorithm is that registered resources have a parent
> > pointer, and non-registered resources do not.
> >
> > * It can be argued that of_platform_populate() should be registering
> > it's resources, and they argument has some merit. However, there are
> > quite a few platforms that end up broken if we try to do that due to
> > overlapping resources in the device tree. Until that is fixed, we need
> > to solve the immediate problem.
> >
> > Cc: Pantelis Antoniou <[email protected]>
> > Cc: Wolfram Sang <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: Ricardo Ribalda Delgado <[email protected]>
> > Signed-off-by: Grant Likely <[email protected]>
> > ---
> > drivers/base/platform.c | 8 ++------
> > 1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index ebf034b97278..7403de94832c 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -375,9 +375,7 @@ int platform_device_add(struct platform_device *pdev)
> >
> > while (--i >= 0) {
> > struct resource *r = &pdev->resource[i];
> > - unsigned long type = resource_type(r);
> > -
> > - if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
> > + if (r->parent)
> > release_resource(r);
> > }
> >
> > @@ -408,9 +406,7 @@ void platform_device_del(struct platform_device *pdev)
> >
> > for (i = 0; i < pdev->num_resources; i++) {
> > struct resource *r = &pdev->resource[i];
> > - unsigned long type = resource_type(r);
> > -
> > - if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
> > + if (r->parent)
> > release_resource(r);
> > }
> > }
> > --
> > 2.1.4
> >
>
>
>
> --
> Ricardo Ribalda
Hello Grant
On Mon, Jun 8, 2015 at 8:47 PM, Grant Likely <[email protected]> wrote:
> Hi Ricardo,
>
> Comments below...
>
> On Sun, 7 Jun 2015 20:13:15 +0200
> , Ricardo Ribalda Delgado <[email protected]>
> 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
On Sun, Jun 7, 2015 at 9:20 AM, Grant Likely <[email protected]> wrote:
> Add a single resource to the test bus device to exercise the platform
> bus code a little more. This isn't strictly a devicetree test, but it is
> a corner case that the devicetree runs into. Until we've got platform
> device unittests, it can live here. It doesn't need to be an explicit
> text because the kernel will oops when it is wrong.
Isn't the unittest oops'ing a problem?
> Cc: Pantelis Antoniou <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Ricardo Ribalda Delgado <[email protected]>
> Signed-off-by: Grant Likely <[email protected]>
> ---
> drivers/of/unittest.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index 18016341d5a9..0a27b38c3041 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -753,6 +753,11 @@ static void __init of_unittest_match_node(void)
> }
> }
>
> +static struct resource test_bus_res = {
> + .start = 0xfffffff8,
> + .end = 0xfffffff9,
> + .flags = IORESOURCE_MEM,
> +};
> static const struct platform_device_info test_bus_info = {
> .name = "unittest-bus",
> };
> @@ -795,6 +800,7 @@ static void __init of_unittest_platform_populate(void)
> if (rc)
> return;
> test_bus->dev.of_node = np;
> + platform_device_add_resources(test_bus, &test_bus_res, 1);
A comment here as to the purpose of this would be useful.
Rob
>
> of_platform_populate(np, match, NULL, &test_bus->dev);
> for_each_child_of_node(np, child) {
> --
> 2.1.4
>
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
<[email protected]> wrote:
> Hello Grant
>
>
> On Mon, Jun 8, 2015 at 8:47 PM, Grant Likely <[email protected]> wrote:
>> Hi Ricardo,
>>
>> Comments below...
>>
>> On Sun, 7 Jun 2015 20:13:15 +0200
>> , Ricardo Ribalda Delgado <[email protected]>
>> 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
On Mon, 8 Jun 2015 22:09:13 +0200
, Ricardo Ribalda Delgado <[email protected]>
wrote:
> Hello Grant
>
>
> On Mon, Jun 8, 2015 at 8:47 PM, Grant Likely <[email protected]> wrote:
> > Hi Ricardo,
> >
> > Comments below...
> >
> > On Sun, 7 Jun 2015 20:13:15 +0200
> > , Ricardo Ribalda Delgado <[email protected]>
> > 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.
Not exactly. There is a bug in the platform subsystem: Register and
unregister are not symmetrical, which when combined with the defect in
the OF code results in an oops. It is appropriate to fix the
non-symmetrical code path.
> >> 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.
No, it's better to drop the whole series. There are still issues and it
will conflict with merging the bugfix for v4.1.
> >
> > 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?
register path:
Insert all resources with a parent, if no parent and type == MEM or
IO, then use iomem_resource/ioport_resource as the parent. At the end of
registration, each resource with a parent assigned will be inserted.
unregister path:
Without patch 2/3: Remove all resources with type == MEM or IO. If it
hasn't been inserted then the kernel will oops. Neglects to remove
non-MEM/IO resources.
With patch 2/3: Remove all resources that have a parent and type == MEM
or IO. Neglects to remove non-MEM/IO resources that were inserted in
the register path.
In both the with and without cases the remove behaviour doesn't not
strictly reverse the insert behaviour, which is not what we want.
I do realize that patch 1/3 of the series stops inserting non-MEM/IO
resources in the register path, but do you know if it is safe to change
that behaviour? There are users who use set parent explicitly, and don't
depend on the default IO and MEM resource roots. For example, I was able
to quickly find devices setting their own root with:
$ git grep '\.parent = .*res'
arch/arm/mach-sa1100/neponset.c: sa1111_resources[0].parent = sa1111_res;
arch/arm/mach-sa1100/neponset.c: smc91x_resources[0].parent = smc91x_res;
arch/arm/mach-sa1100/neponset.c: smc91x_resources[1].parent = smc91x_res;
arch/ia64/sn/kernel/io_init.c: res[0].parent = &ioport_resource;
arch/ia64/sn/kernel/io_init.c: res[1].parent = &iomem_resource;
arch/mips/pci/pci-ar2315.c: apc->mem_res.parent = res;
arch/mips/pci/pci-ar71xx.c: apc->io_res.parent = res;
arch/mips/pci/pci-ar71xx.c: apc->mem_res.parent = res;
arch/mips/pci/pci-ar724x.c: apc->io_res.parent = res;
arch/mips/pci/pci-ar724x.c: apc->mem_res.parent = res;
arch/mips/pci/pci-rc32434.c: .parent = &rc32434_res_pci_mem1,
drivers/mfd/mfd-core.c: res[r].parent = cell->resources[r].parent;
drivers/uwb/whci.c: umc->resource.parent = &card->pci->resource[bar];
And that was just a quick search. All of those examples are still type
MEM/IO, so it isn't a definitive answer. Due-diligence still needs to be
done before patch 1/3 would be acceptable.
In the mean time, the simple bug fix is by far the least risky option.
> > 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.
That series is not in mainline. It is not applied yet. We don't merge
things that aren't ready.
g.
On Mon, 8 Jun 2015 15:16:39 -0500
, Rob Herring <[email protected]>
wrote:
> On Sun, Jun 7, 2015 at 9:20 AM, Grant Likely <[email protected]> wrote:
> > Add a single resource to the test bus device to exercise the platform
> > bus code a little more. This isn't strictly a devicetree test, but it is
> > a corner case that the devicetree runs into. Until we've got platform
> > device unittests, it can live here. It doesn't need to be an explicit
> > text because the kernel will oops when it is wrong.
>
> Isn't the unittest oops'ing a problem?
Yes, but an oops while running unittests automatically means unittests
have failed. :-)
> > Cc: Pantelis Antoniou <[email protected]>
> > Cc: Wolfram Sang <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: Ricardo Ribalda Delgado <[email protected]>
> > Signed-off-by: Grant Likely <[email protected]>
> > ---
> > drivers/of/unittest.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> > index 18016341d5a9..0a27b38c3041 100644
> > --- a/drivers/of/unittest.c
> > +++ b/drivers/of/unittest.c
> > @@ -753,6 +753,11 @@ static void __init of_unittest_match_node(void)
> > }
> > }
> >
> > +static struct resource test_bus_res = {
> > + .start = 0xfffffff8,
> > + .end = 0xfffffff9,
> > + .flags = IORESOURCE_MEM,
> > +};
> > static const struct platform_device_info test_bus_info = {
> > .name = "unittest-bus",
> > };
> > @@ -795,6 +800,7 @@ static void __init of_unittest_platform_populate(void)
> > if (rc)
> > return;
> > test_bus->dev.of_node = np;
> > + platform_device_add_resources(test_bus, &test_bus_res, 1);
>
> A comment here as to the purpose of this would be useful.
I'll add this:
/*
* Add a dummy resource to the test bus node after it is
* registered to catch problems with un-inserted resources. The
* DT code doesn't insert the resources, and it has caused the
* kernel to oops in the past. This makes sure the same bug
* doesn't crop up again.
*/
g.
On Tue, Jun 9, 2015 at 4:00 AM, Grant Likely <[email protected]> wrote:
> On Mon, 8 Jun 2015 22:09:13 +0200
> , Ricardo Ribalda Delgado <[email protected]> wrote:
[...]
>>
>> > 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.
>
> No, it's better to drop the whole series. There are still issues and it
> will conflict with merging the bugfix for v4.1.
>
Multiple platforms stopped booting in next-20150609 as discovered by
kernelci.org[1], and was bisected down to commit b6d2233f2916
(of/platform: Use platform_device interface).
I'll leave you guys to sort out whether that patch or the whole series
should be reverted, but I can confirm that reverting that patch on top
of next-20150609 gets things booting again.
Kevin
[1] http://kernelci.org/boot/all/job/next/kernel/next-20150609/
Hi Kevin, Hi Grant, Hi Greg
Although I do not agree with everything exposed by Grant, I understand
his concerns as a Maintainer with future support of the code. Also
there is no point in wasting more energy in discussing than in coding.
So, in order to make everybody life a bit easier I think a good plan here is:
1) revert the whole serie :(
2) apply Grant patch (you can add my Acked-by) and propose his
inclusion for back-porting in 4.0
3) I post a patch series with the cleanout missing in his patch. Not
needed to be backported, but nice it is merged soon, since it has been
already reviewed by Bjorn and Rob.
4) I post a patch series with the resource flag and "Use
platform_device interface". And we can disscuss if this is good
sollution or not
Could be nice to be able to test the code on 4) on the "broken platforms".
Do we all agree on this plan?
Thanks and Kevin sorry for wasting your time.
On Wed, Jun 10, 2015 at 2:22 AM, Kevin Hilman <[email protected]> wrote:
> On Tue, Jun 9, 2015 at 4:00 AM, Grant Likely <[email protected]> wrote:
>> On Mon, 8 Jun 2015 22:09:13 +0200
>> , Ricardo Ribalda Delgado <[email protected]> wrote:
>
> [...]
>
>>>
>>> > 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.
>>
>> No, it's better to drop the whole series. There are still issues and it
>> will conflict with merging the bugfix for v4.1.
>>
>
> Multiple platforms stopped booting in next-20150609 as discovered by
> kernelci.org[1], and was bisected down to commit b6d2233f2916
> (of/platform: Use platform_device interface).
>
> I'll leave you guys to sort out whether that patch or the whole series
> should be reverted, but I can confirm that reverting that patch on top
> of next-20150609 gets things booting again.
>
> Kevin
>
> [1] http://kernelci.org/boot/all/job/next/kernel/next-20150609/
--
Ricardo Ribalda
+Tony
On Wed, Jun 10, 2015 at 2:11 AM, Ricardo Ribalda Delgado
<[email protected]> wrote:
> Hi Kevin, Hi Grant, Hi Greg
>
> Although I do not agree with everything exposed by Grant, I understand
> his concerns as a Maintainer with future support of the code. Also
> there is no point in wasting more energy in discussing than in coding.
>
> So, in order to make everybody life a bit easier I think a good plan here is:
>
> 1) revert the whole serie :(
> 2) apply Grant patch (you can add my Acked-by) and propose his
> inclusion for back-porting in 4.0
> 3) I post a patch series with the cleanout missing in his patch. Not
> needed to be backported, but nice it is merged soon, since it has been
> already reviewed by Bjorn and Rob.
> 4) I post a patch series with the resource flag and "Use
> platform_device interface". And we can disscuss if this is good
> sollution or not
>
>
> Could be nice to be able to test the code on 4) on the "broken platforms".
That is what kernelci.org is for, but we should try to understand how
these platforms broke and have testcases.
> Do we all agree on this plan?
Yes. I don't know that the shared flag is a good idea, but we can
discuss that later.
> Thanks and Kevin sorry for wasting your time.
I've looked at some of the failures. Armada 370 looks normal AFAICT,
but the network device evidently does not probe. i.MX6 has no log, but
IIRC it is what failed previously on Grant's last attempt. For OMAP4,
I found the overlapping region here:
omap4_padconf_core: scm@100000 {
compatible = "ti,omap4-scm-padconf-core",
"simple-bus";
#address-cells = <1>;
#size-cells = <1>;
ranges = <0 0x100000 0x1000>;
omap4_pmx_core: pinmux@40 {
compatible = "ti,omap4-padconf",
"pinctrl-single";
reg = <0x40 0x0196>;
#address-cells = <1>;
#size-cells = <0>;
#interrupt-cells = <1>;
interrupt-controller;
pinctrl-single,register-width = <16>;
pinctrl-single,function-mask = <0x7fff>;
};
omap4_padconf_global: omap4_padconf_global@5a0 {
compatible = "syscon";
reg = <0x5a0 0x170>;
#address-cells = <1>;
#size-cells = <1>;
pbias_regulator: pbias_regulator {
compatible = "ti,pbias-omap";
reg = <0x60 0x4>;
0x60 is within the pinmux range of 0x40-0x1d6.
But why is the regulator a sub node here instead of omap4_pmx_core?
syscon =
<&omap4_padconf_global>;
This seems to indicate that 0x60 is supposed to be an offset from
0x5a0. That would require a ranges property in the parent. Is this an
error?
pbias_mmc_reg: pbias_mmc_omap4 {
regulator-name
= "pbias_mmc_omap4";
regulator-min-microvolt = <1800000>;
regulator-max-microvolt = <3000000>;
};
};
};
};
>
> On Wed, Jun 10, 2015 at 2:22 AM, Kevin Hilman <[email protected]> wrote:
>> On Tue, Jun 9, 2015 at 4:00 AM, Grant Likely <[email protected]> wrote:
>>> On Mon, 8 Jun 2015 22:09:13 +0200
>>> , Ricardo Ribalda Delgado <[email protected]> wrote:
>>
>> [...]
>>
>>>>
>>>> > 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.
>>>
>>> No, it's better to drop the whole series. There are still issues and it
>>> will conflict with merging the bugfix for v4.1.
>>>
>>
>> Multiple platforms stopped booting in next-20150609 as discovered by
>> kernelci.org[1], and was bisected down to commit b6d2233f2916
>> (of/platform: Use platform_device interface).
>>
>> I'll leave you guys to sort out whether that patch or the whole series
>> should be reverted, but I can confirm that reverting that patch on top
>> of next-20150609 gets things booting again.
>>
>> Kevin
>>
>> [1] http://kernelci.org/boot/all/job/next/kernel/next-20150609/
>
>
>
> --
> Ricardo Ribalda
On Wed, Jun 10, 2015 at 12:11 AM, Ricardo Ribalda Delgado
<[email protected]> wrote:
> Hi Kevin, Hi Grant, Hi Greg
>
> Although I do not agree with everything exposed by Grant, I understand
> his concerns as a Maintainer with future support of the code. Also
> there is no point in wasting more energy in discussing than in coding.
>
> So, in order to make everybody life a bit easier I think a good plan here is:
>
> 1) revert the whole serie :(
IMO, the should be done ASAP so these failures are not masking other
problems. So Greg doesn't have to wade through this whole thread to
figure out which patches to revert, I'd recommend sending him the list
of commits to revert.
At this stage of the cycle (merge window opening soon, maintainers
trying to stabilize their stuff for Linus) we want to see linux-next
approaching some level of stability.
Kevin
Hello Greg
On Wed, Jun 10, 2015 at 4:38 PM, Kevin Hilman <[email protected]> wrote:
> On Wed, Jun 10, 2015 at 12:11 AM, Ricardo Ribalda Delgado
> At this stage of the cycle (merge window opening soon, maintainers
> trying to stabilize their stuff for Linus) we want to see linux-next
> approaching some level of stability.
Please revert
base/platform: Only insert MEM and IO resources
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=36d4b29260753ad78b1ce4363145332c02519adc
base/platform: Continue on insert_resource() error
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=e50e69d1ac4232af0b6890f16929bf5ceee81538
of/platform: Use platform_device interface
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=b6d2233f2916fa9338786aeab2e936c5a07e4d0c
base/platform: Remove code duplication
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=6d9d4b1469b0d9748145e168fc9ec585e1f3f4b0
This patch from Grant needs to be applied:
[PATCH 2/2] drivercore: Fix unregistration path of platform devices
I dont know if it should go throw your tree or Grants devicetree, but
it needs to be backported to 4.0
Thanks!
>
> Kevin
--
Ricardo Ribalda
On Wed, Jun 10, 2015 at 04:46:33PM +0200, Ricardo Ribalda Delgado wrote:
> Hello Greg
>
> On Wed, Jun 10, 2015 at 4:38 PM, Kevin Hilman <[email protected]> wrote:
> > On Wed, Jun 10, 2015 at 12:11 AM, Ricardo Ribalda Delgado
>
> > At this stage of the cycle (merge window opening soon, maintainers
> > trying to stabilize their stuff for Linus) we want to see linux-next
> > approaching some level of stability.
>
> Please revert
>
> base/platform: Only insert MEM and IO resources
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=36d4b29260753ad78b1ce4363145332c02519adc
> base/platform: Continue on insert_resource() error
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=e50e69d1ac4232af0b6890f16929bf5ceee81538
> of/platform: Use platform_device interface
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=b6d2233f2916fa9338786aeab2e936c5a07e4d0c
> base/platform: Remove code duplication
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=6d9d4b1469b0d9748145e168fc9ec585e1f3f4b0
Revert these in the reverse order, right?
And git commit ids would be all that I need, not a URL, can't do much
with that :)
I'll go do that now...
thanks,
greg k-h
On Wed, Jun 10, 2015 at 04:46:33PM +0200, Ricardo Ribalda Delgado wrote:
> Hello Greg
>
> On Wed, Jun 10, 2015 at 4:38 PM, Kevin Hilman <[email protected]> wrote:
> > On Wed, Jun 10, 2015 at 12:11 AM, Ricardo Ribalda Delgado
>
> > At this stage of the cycle (merge window opening soon, maintainers
> > trying to stabilize their stuff for Linus) we want to see linux-next
> > approaching some level of stability.
>
> Please revert
>
> base/platform: Only insert MEM and IO resources
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=36d4b29260753ad78b1ce4363145332c02519adc
> base/platform: Continue on insert_resource() error
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=e50e69d1ac4232af0b6890f16929bf5ceee81538
> of/platform: Use platform_device interface
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=b6d2233f2916fa9338786aeab2e936c5a07e4d0c
> base/platform: Remove code duplication
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=6d9d4b1469b0d9748145e168fc9ec585e1f3f4b0
Now reverted.
> This patch from Grant needs to be applied:
>
> [PATCH 2/2] drivercore: Fix unregistration path of platform devices
I need some acks before I apply anything else as this is a total mess.
greg k-h
On Wed, Jun 10, 2015 at 4:40 PM, Greg Kroah-Hartman
<[email protected]> wrote:
> On Wed, Jun 10, 2015 at 04:46:33PM +0200, Ricardo Ribalda Delgado wrote:
>> Hello Greg
>>
>> On Wed, Jun 10, 2015 at 4:38 PM, Kevin Hilman <[email protected]> wrote:
>> > On Wed, Jun 10, 2015 at 12:11 AM, Ricardo Ribalda Delgado
>>
>> > At this stage of the cycle (merge window opening soon, maintainers
>> > trying to stabilize their stuff for Linus) we want to see linux-next
>> > approaching some level of stability.
>>
>> Please revert
>>
>> base/platform: Only insert MEM and IO resources
>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=36d4b29260753ad78b1ce4363145332c02519adc
>> base/platform: Continue on insert_resource() error
>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=e50e69d1ac4232af0b6890f16929bf5ceee81538
>> of/platform: Use platform_device interface
>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=b6d2233f2916fa9338786aeab2e936c5a07e4d0c
>> base/platform: Remove code duplication
>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=6d9d4b1469b0d9748145e168fc9ec585e1f3f4b0
>
> Now reverted.
>
>> This patch from Grant needs to be applied:
>>
>> [PATCH 2/2] drivercore: Fix unregistration path of platform devices
>
> I need some acks before I apply anything else as this is a total mess.
Yes please. Rob, Pantelis, Wolfram. Can you test my patch and provides acks?
g.
Hi Grant,
> On Jun 10, 2015, at 20:11 , Grant Likely <[email protected]> wrote:
>
> On Wed, Jun 10, 2015 at 4:40 PM, Greg Kroah-Hartman
> <[email protected]> wrote:
>> On Wed, Jun 10, 2015 at 04:46:33PM +0200, Ricardo Ribalda Delgado wrote:
>>> Hello Greg
>>>
>>> On Wed, Jun 10, 2015 at 4:38 PM, Kevin Hilman <[email protected]> wrote:
>>>> On Wed, Jun 10, 2015 at 12:11 AM, Ricardo Ribalda Delgado
>>>
>>>> At this stage of the cycle (merge window opening soon, maintainers
>>>> trying to stabilize their stuff for Linus) we want to see linux-next
>>>> approaching some level of stability.
>>>
>>> Please revert
>>>
>>> base/platform: Only insert MEM and IO resources
>>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=36d4b29260753ad78b1ce4363145332c02519adc
>>> base/platform: Continue on insert_resource() error
>>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=e50e69d1ac4232af0b6890f16929bf5ceee81538
>>> of/platform: Use platform_device interface
>>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=b6d2233f2916fa9338786aeab2e936c5a07e4d0c
>>> base/platform: Remove code duplication
>>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=6d9d4b1469b0d9748145e168fc9ec585e1f3f4b0
>>
>> Now reverted.
>>
>>> This patch from Grant needs to be applied:
>>>
>>> [PATCH 2/2] drivercore: Fix unregistration path of platform devices
>>
>> I need some acks before I apply anything else as this is a total mess.
>
> Yes please. Rob, Pantelis, Wolfram. Can you test my patch and provides acks?
>
Will do.
> g.
Regards
— Pantelis
> >> This patch from Grant needs to be applied:
> >>
> >> [PATCH 2/2] drivercore: Fix unregistration path of platform devices
> >
> > I need some acks before I apply anything else as this is a total mess.
>
> Yes please. Rob, Pantelis, Wolfram. Can you test my patch and provides acks?
Ah, this is the mail I forgot to write yesterday, sorry. I am on travel
and can not test before Monday, I am afraid. Will that do?
Thanks a lot for working on this, though! Much appreciated.
Hello
Tested-by: Ricardo Ribalda Delgado <[email protected]>
Some runtime warnings, but no oops
[ 46.348911] Trying to free nonexistent resource
<00000000b0050000-00000000b005ffff>
[ 46.351979] Trying to free nonexistent resource
<00000000b9000000-00000000b900ffff>
[ 46.351993] Trying to free nonexistent resource
<00000000b0000000-00000000b000ffff>
[ 46.352003] Trying to free nonexistent resource
<00000000b9000000-00000000b900ffff>
[ 46.352009] Trying to free nonexistent resource
<00000000b0000000-00000000b000ffff>
Thanks!
On Sun, Jun 7, 2015 at 4:20 PM, Grant Likely <[email protected]> wrote:
> The unregister path of platform_device is broken. On registration, it
> will register all resources with either a parent already set, or
> type==IORESOURCE_{IO,MEM}. However, on unregister it will release
> everything with type==IORESOURCE_{IO,MEM}, but ignore the others. There
> are also cases where resources don't get registered in the first place,
> like with devices created by of_platform_populate()*.
>
> Fix the unregister path to be symmetrical with the register path by
> checking the parent pointer instead of the type field to decide which
> resources to unregister. This is safe because the upshot of the
> registration path algorithm is that registered resources have a parent
> pointer, and non-registered resources do not.
>
> * It can be argued that of_platform_populate() should be registering
> it's resources, and they argument has some merit. However, there are
> quite a few platforms that end up broken if we try to do that due to
> overlapping resources in the device tree. Until that is fixed, we need
> to solve the immediate problem.
>
> Cc: Pantelis Antoniou <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Ricardo Ribalda Delgado <[email protected]>
> Signed-off-by: Grant Likely <[email protected]>
> ---
> drivers/base/platform.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index ebf034b97278..7403de94832c 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -375,9 +375,7 @@ int platform_device_add(struct platform_device *pdev)
>
> while (--i >= 0) {
> struct resource *r = &pdev->resource[i];
> - unsigned long type = resource_type(r);
> -
> - if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
> + if (r->parent)
> release_resource(r);
> }
>
> @@ -408,9 +406,7 @@ void platform_device_del(struct platform_device *pdev)
>
> for (i = 0; i < pdev->num_resources; i++) {
> struct resource *r = &pdev->resource[i];
> - unsigned long type = resource_type(r);
> -
> - if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
> + if (r->parent)
> release_resource(r);
> }
> }
> --
> 2.1.4
>
--
Ricardo Ribalda
On Sun, Jun 07, 2015 at 03:20:10PM +0100, Grant Likely wrote:
> Add a single resource to the test bus device to exercise the platform
> bus code a little more. This isn't strictly a devicetree test, but it is
> a corner case that the devicetree runs into. Until we've got platform
> device unittests, it can live here. It doesn't need to be an explicit
> text because the kernel will oops when it is wrong.
>
> Cc: Pantelis Antoniou <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Ricardo Ribalda Delgado <[email protected]>
> Signed-off-by: Grant Likely <[email protected]>
Thanks for doing this!
Tested-by: Wolfram Sang <[email protected]>
On Sun, Jun 07, 2015 at 03:20:11PM +0100, Grant Likely wrote:
> The unregister path of platform_device is broken. On registration, it
> will register all resources with either a parent already set, or
> type==IORESOURCE_{IO,MEM}. However, on unregister it will release
> everything with type==IORESOURCE_{IO,MEM}, but ignore the others. There
> are also cases where resources don't get registered in the first place,
> like with devices created by of_platform_populate()*.
>
> Fix the unregister path to be symmetrical with the register path by
> checking the parent pointer instead of the type field to decide which
> resources to unregister. This is safe because the upshot of the
> registration path algorithm is that registered resources have a parent
> pointer, and non-registered resources do not.
>
> * It can be argued that of_platform_populate() should be registering
> it's resources, and they argument has some merit. However, there are
> quite a few platforms that end up broken if we try to do that due to
> overlapping resources in the device tree. Until that is fixed, we need
> to solve the immediate problem.
>
> Cc: Pantelis Antoniou <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Ricardo Ribalda Delgado <[email protected]>
> Signed-off-by: Grant Likely <[email protected]>
Tested-by: Wolfram Sang <[email protected]>
* Rob Herring <[email protected]> [150610 07:06]:
>
> I've looked at some of the failures. Armada 370 looks normal AFAICT,
> but the network device evidently does not probe. i.MX6 has no log, but
> IIRC it is what failed previously on Grant's last attempt. For OMAP4,
> I found the overlapping region here:
>
> omap4_padconf_core: scm@100000 {
> compatible = "ti,omap4-scm-padconf-core",
> "simple-bus";
> #address-cells = <1>;
> #size-cells = <1>;
> ranges = <0 0x100000 0x1000>;
>
> omap4_pmx_core: pinmux@40 {
> compatible = "ti,omap4-padconf",
> "pinctrl-single";
> reg = <0x40 0x0196>;
> #address-cells = <1>;
> #size-cells = <0>;
> #interrupt-cells = <1>;
> interrupt-controller;
> pinctrl-single,register-width = <16>;
> pinctrl-single,function-mask = <0x7fff>;
> };
>
> omap4_padconf_global: omap4_padconf_global@5a0 {
> compatible = "syscon";
> reg = <0x5a0 0x170>;
> #address-cells = <1>;
> #size-cells = <1>;
>
> pbias_regulator: pbias_regulator {
> compatible = "ti,pbias-omap";
> reg = <0x60 0x4>;
>
> 0x60 is within the pinmux range of 0x40-0x1d6.
>
> But why is the regulator a sub node here instead of omap4_pmx_core?
I don't think the reg entry is in use here as the pbias_regulator uses
syscon_regmap_lookup_by_phandle via syscon.
> syscon =
> <&omap4_padconf_global>;
>
> This seems to indicate that 0x60 is supposed to be an offset from
> 0x5a0. That would require a ranges property in the parent. Is this an
> error?
Yeah we should add ranges to padconf_global so drivers not using syscon
can just do of_ioremap for a dedicated range of registers within the
padconf_global. That area has things like PHYs, regulators and clocks.
Regards,
Tony
Hello
> Tested-by: Wolfram Sang <[email protected]>
This patch now have a bunch of tested-by and without it, the current
version of linux-next crash.
Who will take it? Grant, Greg? Please make sure to notify
linux-stable since at least 4.1
Thanks!
--
Ricardo Ribalda
ping?
Hello Grant, Hello Greg
Is there any planned timeframe for applying this patch into someones tree?
Thanks!
On Tue, Jun 23, 2015 at 7:12 PM, Ricardo Ribalda Delgado
<[email protected]> wrote:
> Hello
>
>> Tested-by: Wolfram Sang <[email protected]>
>
> This patch now have a bunch of tested-by and without it, the current
> version of linux-next crash.
>
> Who will take it? Grant, Greg? Please make sure to notify
> linux-stable since at least 4.1
>
> Thanks!
>
>
> --
> Ricardo Ribalda
--
Ricardo Ribalda
ping?
On Thu, Jul 16, 2015 at 10:33 PM, Ricardo Ribalda Delgado
<[email protected]> wrote:
> ping?
>
> Hello Grant, Hello Greg
>
> Is there any planned timeframe for applying this patch into someones tree?
>
>
> Thanks!
>
> On Tue, Jun 23, 2015 at 7:12 PM, Ricardo Ribalda Delgado
> <[email protected]> wrote:
>> Hello
>>
>>> Tested-by: Wolfram Sang <[email protected]>
>>
>> This patch now have a bunch of tested-by and without it, the current
>> version of linux-next crash.
>>
>> Who will take it? Grant, Greg? Please make sure to notify
>> linux-stable since at least 4.1
>>
>> Thanks!
>>
>>
>> --
>> Ricardo Ribalda
>
>
>
> --
> Ricardo Ribalda
--
Ricardo Ribalda
On Thu, Jul 16, 2015 at 3:33 PM, Ricardo Ribalda Delgado
<[email protected]> wrote:
> ping?
>
> Hello Grant, Hello Greg
>
> Is there any planned timeframe for applying this patch into someones tree?
Greg is out ATM. I'll pick this up for 4.3.
Rob
>
>
> Thanks!
>
> On Tue, Jun 23, 2015 at 7:12 PM, Ricardo Ribalda Delgado
> <[email protected]> wrote:
>> Hello
>>
>>> Tested-by: Wolfram Sang <[email protected]>
>>
>> This patch now have a bunch of tested-by and without it, the current
>> version of linux-next crash.
>>
>> Who will take it? Grant, Greg? Please make sure to notify
>> linux-stable since at least 4.1
>>
>> Thanks!
>>
>>
>> --
>> Ricardo Ribalda
>
>
>
> --
> Ricardo Ribalda
thanks!
Please, remember that we need to push it also to stable :)
On Sun, Aug 23, 2015 at 11:52 PM, Rob Herring <[email protected]> wrote:
> On Thu, Jul 16, 2015 at 3:33 PM, Ricardo Ribalda Delgado
> <[email protected]> wrote:
>> ping?
>>
>> Hello Grant, Hello Greg
>>
>> Is there any planned timeframe for applying this patch into someones tree?
>
> Greg is out ATM. I'll pick this up for 4.3.
>
> Rob
>
>>
>>
>> Thanks!
>>
>> On Tue, Jun 23, 2015 at 7:12 PM, Ricardo Ribalda Delgado
>> <[email protected]> wrote:
>>> Hello
>>>
>>>> Tested-by: Wolfram Sang <[email protected]>
>>>
>>> This patch now have a bunch of tested-by and without it, the current
>>> version of linux-next crash.
>>>
>>> Who will take it? Grant, Greg? Please make sure to notify
>>> linux-stable since at least 4.1
>>>
>>> Thanks!
>>>
>>>
>>> --
>>> Ricardo Ribalda
>>
>>
>>
>> --
>> Ricardo Ribalda
--
Ricardo Ribalda