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
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 ebf034b..6028681 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.
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 6028681..2e7e904 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
of_platform_device_destroy().
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.
This patch, replaces of_device_add() with platform_device_add().
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
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 a01f57c..f011f57 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -183,8 +183,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.
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 2e7e904..152d84d 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
Ping?
On Thu, Apr 23, 2015 at 3:58 PM, Ricardo Ribalda Delgado
<[email protected]> wrote:
> 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
>
> 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
>
--
Ricardo Ribalda
On Thu, Apr 23, 2015 at 8:58 AM, Ricardo Ribalda Delgado
<[email protected]> wrote:
> 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.
Does this actually solve anything? What resources have parents besides
mem or io?
Rob
>
> 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 ebf034b..6028681 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
>
Hello Rob
On Wed, May 13, 2015 at 3:56 PM, Rob Herring <[email protected]> wrote:
>
> Does this actually solve anything? What resources have parents besides
> mem or io?
It is code cleanup for the later patches. del_resource checks if the
resource is mem or io.
We either add the check here, or remove the check in del_
Regards!
On Thu, Apr 23, 2015 at 8:58 AM, Ricardo Ribalda Delgado
<[email protected]> wrote:
> Failure path of platform_device_add was almost the same as
> platform_device_del. Refactor same code in a function.
>
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
Clean-ups should come first in the series. Otherwise:
Acked-by: Rob Herring <[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 2e7e904..152d84d 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 Thu, Apr 23, 2015 at 8:58 AM, Ricardo Ribalda Delgado
<[email protected]> wrote:
> of_platform_device_create_pdata() was using of_device_add() to create
> the devices, but of_platform_device_destroy was using
> of_platform_device_destroy().
You mean platform_device_destroy?
> 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.
>
> This patch, replaces of_device_add() with platform_device_add().
Can you please include a pointer to the previous attempt and explain
how this is different.
>
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
Acked-by: Rob Herring <[email protected]>
> ---
> 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 a01f57c..f011f57 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -183,8 +183,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
>
On Thu, Apr 23, 2015 at 8:58 AM, 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.
>
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
Acked-by: Rob Herring <[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 6028681..2e7e904 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
>
On Thu, 23 Apr 2015 15:58:11 +0200
, Ricardo Ribalda Delgado <[email protected]>
wrote:
> 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 ebf034b..6028681 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;
This patch is correct in that it makes things balanced, but I don't
think it is the right behaviour. I've just posted a patch that does it
the other way around, based on a patch that Pantelis posted doing the
same thing, but without refactoring at the same time.
Instead of filtering out the non-MEM/IO resources, the new code checks
the parent pointer on removal, because that is the safe test of if a
resource has been registered in the first place.
g.
On Wed, 13 May 2015 09:03:31 -0500
, Rob Herring <[email protected]>
wrote:
> On Thu, Apr 23, 2015 at 8:58 AM, Ricardo Ribalda Delgado
> <[email protected]> wrote:
> > Failure path of platform_device_add was almost the same as
> > platform_device_del. Refactor same code in a function.
> >
> > Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
>
> Clean-ups should come first in the series. Otherwise:
>
> Acked-by: Rob Herring <[email protected]>
I disagree in this case, but only because the bug fix needs to be fixed
immediately, but the refactoring can wait for v4.1. Ricardo, if you can
confirm my patch solves the problem, then I'll push it to Linus, and you
can respin this patch to match.
g.
>
> > ---
> > 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 2e7e904..152d84d 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
> >