of_platform_depopulate can lead to a kernel error when calling release_resource()
The reason is that it is trying to release a resource that was not allocated
via insert_resource()
of_platform_depopulate()
of_platform_device_destroy()
platform_device_unregister(platform_device *pdev)
platform_device_del(platform_device *pdev)
for (i = 0; i < pdev->num_resources; i++)
release_resource()
of_platform_populate()
...
of_device_alloc()
pdev = platform_device_alloc()
# set pdev->resource, similar to platform_device_add_resources()
of_device_add(platform_device *pdev)
# similar to platform_device_add(), but note there's no
# insert_resource() in this path
device_add(&pdev->dev)
On this patchset:
base/platform has been fixed for an hypothetical condition where parent is
set but the platform is neither MEM or IO.
Then platform_device_alloc has been modified so it supports of and amba
devices.
Finally of_device_add has been modified to use platform_device_add().
v1: https://lkml.org/lkml/2015/4/20/435
v2: From: Bjorn Helgaas <[email protected]>
-Fix caller, not callee
https://lkml.org/lkml/2015/4/21/99
https://lkml.org/lkml/2015/4/21/100
v3: From: Rob Herring <[email protected]>
- Modify plaform_device_alloc to support of and ambda devices
https://lkml.org/lkml/2015/4/22/369
https://lkml.org/lkml/2015/4/22/370
https://lkml.org/lkml/2015/4/22/371
https://lkml.org/lkml/2015/4/22/374
https://lkml.org/lkml/2015/4/22/373
v4: From: Bjorn Helgaas <[email protected]>
-Remove WARN() patch
-Show conflicting resources
-Code Style
-Fix descriptions
From: Rob Herring <[email protected]>
-Fix descriptions
https://lkml.org/lkml/2015/4/23/254
https://lkml.org/lkml/2015/4/23/258
https://lkml.org/lkml/2015/4/23/257
https://lkml.org/lkml/2015/4/23/256
https://lkml.org/lkml/2015/4/23/255
v5: From: Rob Herring <[email protected]>
-Fix descriptions
Ricardo Ribalda Delgado (4):
base/platform: Only insert MEM and IO resources
base/platform: Continue on insert_resource() error
of/platform: Use platform_device interface
base/platform: Remove code duplication
drivers/base/platform.c | 84 ++++++++++++++++++++++++-------------------------
drivers/of/platform.c | 3 +-
2 files changed, 43 insertions(+), 44 deletions(-)
--
2.1.4
platform_device_del only checks the type of the resource in order to
call release_resource.
On the other hand, platform_device_add calls insert_resource for any
resource that has a parent.
Make both code branches balanced.
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
drivers/base/platform.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 063f0ab15259..46a56f694cec 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -341,19 +341,23 @@ int platform_device_add(struct platform_device *pdev)
for (i = 0; i < pdev->num_resources; i++) {
struct resource *p, *r = &pdev->resource[i];
+ unsigned long type = resource_type(r);
if (r->name == NULL)
r->name = dev_name(&pdev->dev);
+ if (!(type == IORESOURCE_MEM || type == IORESOURCE_IO))
+ continue;
+
p = r->parent;
if (!p) {
- if (resource_type(r) == IORESOURCE_MEM)
+ if (type == IORESOURCE_MEM)
p = &iomem_resource;
- else if (resource_type(r) == IORESOURCE_IO)
+ else if (type == IORESOURCE_IO)
p = &ioport_resource;
}
- if (p && insert_resource(p, r)) {
+ if (insert_resource(p, r)) {
dev_err(&pdev->dev, "failed to claim resource %d\n", i);
ret = -EBUSY;
goto failed;
--
2.1.4
insert_resource() can fail when the resource added overlaps
(partially or fully) with another.
Device tree and AMBA devices may contain resources that overlap, so they
could not call platform_device_add (see 02bbde7849e6 ('Revert "of:
use platform_device_add"'))"
On the other hand, device trees are released using
platform_device_unregister(). This function calls platform_device_del(),
which calls release_resource(), that crashes when the resource has not
been added with with insert_resource. This was not an issue when the
device tree could not be modified online, but this is not the case
anymore.
This patch let the flow continue when there is an insert error, after
notifying the user with a dev_err(). r->parent is set to NULL, so
platform_device_del() knows that the resource was not added, and
therefore it should not be released.
Acked-by: Rob Herring <[email protected]>
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
drivers/base/platform.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 46a56f694cec..5a29387e5ff6 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -332,7 +332,7 @@ int platform_device_add(struct platform_device *pdev)
*/
ret = ida_simple_get(&platform_devid_ida, 0, 0, GFP_KERNEL);
if (ret < 0)
- goto err_out;
+ return ret;
pdev->id = ret;
pdev->id_auto = true;
dev_set_name(&pdev->dev, "%s.%d.auto", pdev->name, pdev->id);
@@ -340,7 +340,7 @@ int platform_device_add(struct platform_device *pdev)
}
for (i = 0; i < pdev->num_resources; i++) {
- struct resource *p, *r = &pdev->resource[i];
+ struct resource *conflict, *p, *r = &pdev->resource[i];
unsigned long type = resource_type(r);
if (r->name == NULL)
@@ -357,11 +357,14 @@ int platform_device_add(struct platform_device *pdev)
p = &ioport_resource;
}
- if (insert_resource(p, r)) {
- dev_err(&pdev->dev, "failed to claim resource %d\n", i);
- ret = -EBUSY;
- goto failed;
- }
+ conflict = insert_resource_conflict(p, r);
+ if (!conflict)
+ continue;
+
+ dev_err(&pdev->dev,
+ "ignoring resource %pR (conflicts with %s %pR)\n",
+ r, conflict->name, conflict);
+ p->parent = NULL;
}
pr_debug("Registering platform device '%s'. Parent at %s\n",
@@ -371,7 +374,7 @@ int platform_device_add(struct platform_device *pdev)
if (ret == 0)
return ret;
- failed:
+ /* Failure path */
if (pdev->id_auto) {
ida_simple_remove(&platform_devid_ida, pdev->id);
pdev->id = PLATFORM_DEVID_AUTO;
@@ -381,11 +384,11 @@ int platform_device_add(struct platform_device *pdev)
struct resource *r = &pdev->resource[i];
unsigned long type = resource_type(r);
- if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
+ if ((type == IORESOURCE_MEM || type == IORESOURCE_IO) &&
+ r->parent)
release_resource(r);
}
- err_out:
return ret;
}
EXPORT_SYMBOL_GPL(platform_device_add);
@@ -414,7 +417,8 @@ void platform_device_del(struct platform_device *pdev)
struct resource *r = &pdev->resource[i];
unsigned long type = resource_type(r);
- if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
+ if ((type == IORESOURCE_MEM || type == IORESOURCE_IO) &&
+ r->parent)
release_resource(r);
}
}
--
2.1.4
of_platform_device_create_pdata() was using of_device_add() to create
the devices, but of_platform_device_destroy was using
platform_device_unregister() to free them.
of_device_add(), do not call insert_resource(), which initializes the
parent field of the resource structure, needed by release_resource(),
called by of_platform_device_destroy(). This leads to a NULL pointer
deference.
Instead of fixing the NULL pointer deference, what could hide other bugs,
this patch, replaces of_device_add() with platform_device_data().
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
v5: Fix comments by Rob Herring, thanks!
drivers/of/platform.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index ddf8e42c9367..236a8dfa83e6 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -184,8 +184,9 @@ static struct platform_device *of_platform_device_create_pdata(
dev->dev.bus = &platform_bus_type;
dev->dev.platform_data = platform_data;
of_dma_configure(&dev->dev, dev->dev.of_node);
+ dev->name = dev_name(&dev->dev);
- if (of_device_add(dev) != 0) {
+ if (platform_device_add(dev) != 0) {
of_dma_deconfigure(&dev->dev);
platform_device_put(dev);
goto err_clear_flag;
--
2.1.4
Failure path of platform_device_add was almost the same as
platform_device_del. Refactor same code in a function.
Acked-by: Rob Herring <[email protected]>
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
drivers/base/platform.c | 60 +++++++++++++++++++++----------------------------
1 file changed, 25 insertions(+), 35 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 5a29387e5ff6..cba8e0e83bfc 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -298,6 +298,25 @@ int platform_device_add_data(struct platform_device *pdev, const void *data,
}
EXPORT_SYMBOL_GPL(platform_device_add_data);
+static void platform_device_cleanout(struct platform_device *pdev, int n_res)
+{
+ int i;
+
+ if (pdev->id_auto) {
+ ida_simple_remove(&platform_devid_ida, pdev->id);
+ pdev->id = PLATFORM_DEVID_AUTO;
+ }
+
+ for (i = 0; i < n_res; i++) {
+ struct resource *r = &pdev->resource[i];
+ unsigned long type = resource_type(r);
+
+ if ((type == IORESOURCE_MEM || type == IORESOURCE_IO) &&
+ r->parent)
+ release_resource(r);
+ }
+}
+
/**
* platform_device_add - add a platform device to device hierarchy
* @pdev: platform device we're adding
@@ -371,23 +390,8 @@ int platform_device_add(struct platform_device *pdev)
dev_name(&pdev->dev), dev_name(pdev->dev.parent));
ret = device_add(&pdev->dev);
- if (ret == 0)
- return ret;
-
- /* Failure path */
- if (pdev->id_auto) {
- ida_simple_remove(&platform_devid_ida, pdev->id);
- pdev->id = PLATFORM_DEVID_AUTO;
- }
-
- while (--i >= 0) {
- struct resource *r = &pdev->resource[i];
- unsigned long type = resource_type(r);
-
- if ((type == IORESOURCE_MEM || type == IORESOURCE_IO) &&
- r->parent)
- release_resource(r);
- }
+ if (ret)
+ platform_device_cleanout(pdev, i);
return ret;
}
@@ -403,25 +407,11 @@ EXPORT_SYMBOL_GPL(platform_device_add);
*/
void platform_device_del(struct platform_device *pdev)
{
- int i;
-
- if (pdev) {
- device_del(&pdev->dev);
-
- if (pdev->id_auto) {
- ida_simple_remove(&platform_devid_ida, pdev->id);
- pdev->id = PLATFORM_DEVID_AUTO;
- }
-
- for (i = 0; i < pdev->num_resources; i++) {
- struct resource *r = &pdev->resource[i];
- unsigned long type = resource_type(r);
+ if (!pdev)
+ return;
- if ((type == IORESOURCE_MEM || type == IORESOURCE_IO) &&
- r->parent)
- release_resource(r);
- }
- }
+ device_del(&pdev->dev);
+ platform_device_cleanout(pdev, pdev->num_resources);
}
EXPORT_SYMBOL_GPL(platform_device_del);
--
2.1.4
On Tue, 26 May 2015 09:31:24 +0200
, Ricardo Ribalda Delgado <[email protected]>
wrote:
> insert_resource() can fail when the resource added overlaps
> (partially or fully) with another.
>
> Device tree and AMBA devices may contain resources that overlap, so they
> could not call platform_device_add (see 02bbde7849e6 ('Revert "of:
> use platform_device_add"'))"
>
> On the other hand, device trees are released using
> platform_device_unregister(). This function calls platform_device_del(),
> which calls release_resource(), that crashes when the resource has not
> been added with with insert_resource. This was not an issue when the
> device tree could not be modified online, but this is not the case
> anymore.
>
> This patch let the flow continue when there is an insert error, after
> notifying the user with a dev_err(). r->parent is set to NULL, so
> platform_device_del() knows that the resource was not added, and
> therefore it should not be released.
>
> Acked-by: Rob Herring <[email protected]>
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> ---
> drivers/base/platform.c | 26 +++++++++++++++-----------
> 1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 46a56f694cec..5a29387e5ff6 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -332,7 +332,7 @@ int platform_device_add(struct platform_device *pdev)
> */
> ret = ida_simple_get(&platform_devid_ida, 0, 0, GFP_KERNEL);
> if (ret < 0)
> - goto err_out;
> + return ret;
> pdev->id = ret;
> pdev->id_auto = true;
> dev_set_name(&pdev->dev, "%s.%d.auto", pdev->name, pdev->id);
> @@ -340,7 +340,7 @@ int platform_device_add(struct platform_device *pdev)
> }
>
> for (i = 0; i < pdev->num_resources; i++) {
> - struct resource *p, *r = &pdev->resource[i];
> + struct resource *conflict, *p, *r = &pdev->resource[i];
> unsigned long type = resource_type(r);
>
> if (r->name == NULL)
> @@ -357,11 +357,14 @@ int platform_device_add(struct platform_device *pdev)
> p = &ioport_resource;
> }
>
> - if (insert_resource(p, r)) {
> - dev_err(&pdev->dev, "failed to claim resource %d\n", i);
> - ret = -EBUSY;
> - goto failed;
> - }
> + conflict = insert_resource_conflict(p, r);
> + if (!conflict)
> + continue;
> +
> + dev_err(&pdev->dev,
> + "ignoring resource %pR (conflicts with %s %pR)\n",
> + r, conflict->name, conflict);
> + p->parent = NULL;
I'm pretty sure this is going to break some platforms. I described it in
my earlier email today, but I'll summarize here too since this is the
latest patch set.
Making this change allows the registration of devices to continue, but
it will still break device drivers that do a request_resource() on a
region that another device managed to claim with insert_resource(). The
only way around this is to not do insert_resource() at all, or to remove
the request_resource() from all drivers (not feasible).
I think we have to deal with it by making resource insertion optional.
I'd like to make the default be to do the insertion, and be able to
blacklist platforms that have issues.
g.
Hello Grant
On Thu, Jun 4, 2015 at 9:54 AM, Grant Likely <[email protected]> wrote:
>
> I'm pretty sure this is going to break some platforms. I described it in
> my earlier email today, but I'll summarize here too since this is the
> latest patch set.
The version that is in stable is also broken. Unloading the device
tree crashes the device completely.
>
> Making this change allows the registration of devices to continue, but
> it will still break device drivers that do a request_resource() on a
> region that another device managed to claim with insert_resource(). The
> only way around this is to not do insert_resource() at all, or to remove
> the request_resource() from all drivers (not feasible).
If we do not do insert_resource(), we will have the crash on
release_resource() and a lot! of code duplication.
>
> I think we have to deal with it by making resource insertion optional.
> I'd like to make the default be to do the insertion, and be able to
> blacklist platforms that have issues.
What about, making the request_resource a little bit more clever.
Something like:
If the resouce is not taken
return ok
if the resource is taken:
If the requester or the current owner of the resource are device tree devices
show a warning and continue.
else
return error
Wouldn't this fix the issue and guide the developers towards a proper
fix for their platform, instead of just encourage them to blacklist
their platform?
Thanks!
>
> g.
>
--
Ricardo Ribalda
On Thu, Jun 4, 2015 at 2:54 AM, Grant Likely <[email protected]> wrote:
> On Tue, 26 May 2015 09:31:24 +0200
> , Ricardo Ribalda Delgado <[email protected]>
> wrote:
>> insert_resource() can fail when the resource added overlaps
>> (partially or fully) with another.
>>
>> Device tree and AMBA devices may contain resources that overlap, so they
>> could not call platform_device_add (see 02bbde7849e6 ('Revert "of:
>> use platform_device_add"'))"
>>
>> On the other hand, device trees are released using
>> platform_device_unregister(). This function calls platform_device_del(),
>> which calls release_resource(), that crashes when the resource has not
>> been added with with insert_resource. This was not an issue when the
>> device tree could not be modified online, but this is not the case
>> anymore.
>>
>> This patch let the flow continue when there is an insert error, after
>> notifying the user with a dev_err(). r->parent is set to NULL, so
>> platform_device_del() knows that the resource was not added, and
>> therefore it should not be released.
>>
>> Acked-by: Rob Herring <[email protected]>
>> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
>> ---
>> drivers/base/platform.c | 26 +++++++++++++++-----------
>> 1 file changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> index 46a56f694cec..5a29387e5ff6 100644
>> --- a/drivers/base/platform.c
>> +++ b/drivers/base/platform.c
>> @@ -332,7 +332,7 @@ int platform_device_add(struct platform_device *pdev)
>> */
>> ret = ida_simple_get(&platform_devid_ida, 0, 0, GFP_KERNEL);
>> if (ret < 0)
>> - goto err_out;
>> + return ret;
>> pdev->id = ret;
>> pdev->id_auto = true;
>> dev_set_name(&pdev->dev, "%s.%d.auto", pdev->name, pdev->id);
>> @@ -340,7 +340,7 @@ int platform_device_add(struct platform_device *pdev)
>> }
>>
>> for (i = 0; i < pdev->num_resources; i++) {
>> - struct resource *p, *r = &pdev->resource[i];
>> + struct resource *conflict, *p, *r = &pdev->resource[i];
>> unsigned long type = resource_type(r);
>>
>> if (r->name == NULL)
>> @@ -357,11 +357,14 @@ int platform_device_add(struct platform_device *pdev)
>> p = &ioport_resource;
>> }
>>
>> - if (insert_resource(p, r)) {
>> - dev_err(&pdev->dev, "failed to claim resource %d\n", i);
>> - ret = -EBUSY;
>> - goto failed;
>> - }
>> + conflict = insert_resource_conflict(p, r);
>> + if (!conflict)
>> + continue;
>> +
>> + dev_err(&pdev->dev,
>> + "ignoring resource %pR (conflicts with %s %pR)\n",
>> + r, conflict->name, conflict);
>> + p->parent = NULL;
>
> I'm pretty sure this is going to break some platforms. I described it in
> my earlier email today, but I'll summarize here too since this is the
> latest patch set.
>
> Making this change allows the registration of devices to continue, but
> it will still break device drivers that do a request_resource() on a
> region that another device managed to claim with insert_resource(). The
> only way around this is to not do insert_resource() at all, or to remove
> the request_resource() from all drivers (not feasible).
Do we have some clue as to which platforms have problems? I seem to
recall some i.MX platform.
> I think we have to deal with it by making resource insertion optional.
> I'd like to make the default be to do the insertion, and be able to
> blacklist platforms that have issues.
Yes, otherwise we'll never put this issue to bed.
Rob