[PATCH v4 0/4] Fix null pointer deference when calling of_platform_depopulate

[PATCH v4 0/4] Fix null pointer deference when calling of_platform_depopulate

[Ricardo Ribalda Delgado]

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 <bhelgaas@google.com>
     -Fix caller, not callee

    https://lkml.org/lkml/2015/4/21/99
    https://lkml.org/lkml/2015/4/21/100

v3: From: Rob Herring <robherring2@gmail.com>
      - 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 <bhelgaas@google.com>
     -Remove WARN() patch
     -Show conflicting resources
     -Code Style
     -Fix descriptions

    From: Rob Herring <robherring2@gmail.com>
       -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

[PATCH v4 1/4] base/platform: Only insert MEM and IO resources

[Ricardo Ribalda Delgado]

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 <ricardo.ribalda@gmail.com>
---
 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

[PATCH v4 2/4] base/platform: Continue on insert_resource() error

[Ricardo Ribalda Delgado]

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 <ricardo.ribalda@gmail.com>
---
 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

[PATCH v4 3/4] of/platform: Use platform_device interface

[Ricardo Ribalda Delgado]

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 <ricardo.ribalda@gmail.com>
---
 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

[PATCH v4 4/4] base/platform: Remove code duplication

[Ricardo Ribalda Delgado]

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 <ricardo.ribalda@gmail.com>
---
 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

Re: [PATCH v4 0/4] Fix null pointer deference when calling of_platform_depopulate

[Ricardo Ribalda Delgado]

Ping?

On Thu, Apr 23, 2015 at 3:58 PM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> 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 <bhelgaas@google.com>
>      -Fix caller, not callee
>
>     https://lkml.org/lkml/2015/4/21/99
>     https://lkml.org/lkml/2015/4/21/100
>
> v3: From: Rob Herring <robherring2@gmail.com>
>       - 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 <bhelgaas@google.com>
>      -Remove WARN() patch
>      -Show conflicting resources
>      -Code Style
>      -Fix descriptions
>
>     From: Rob Herring <robherring2@gmail.com>
>        -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

Re: [PATCH v4 1/4] base/platform: Only insert MEM and IO resources

[Rob Herring]

On Thu, Apr 23, 2015 at 8:58 AM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> 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 <ricardo.ribalda@gmail.com>
> ---
>  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
>

Re: [PATCH v4 1/4] base/platform: Only insert MEM and IO resources

[Ricardo Ribalda Delgado]

Hello Rob

On Wed, May 13, 2015 at 3:56 PM, Rob Herring <robherring2@gmail.com> 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!

Re: [PATCH v4 4/4] base/platform: Remove code duplication

[Rob Herring]

On Thu, Apr 23, 2015 at 8:58 AM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> 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 <ricardo.ribalda@gmail.com>

Clean-ups should come first in the series. Otherwise:

Acked-by: Rob Herring <robh@kernel.org>

> ---
>  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
>

Re: [PATCH v4 3/4] of/platform: Use platform_device interface

[Rob Herring]

On Thu, Apr 23, 2015 at 8:58 AM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> 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 <ricardo.ribalda@gmail.com>

Acked-by: Rob Herring <robh@kernel.org>

> ---
>  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
>

Re: [PATCH v4 2/4] base/platform: Continue on insert_resource() error

[Rob Herring]

On Thu, Apr 23, 2015 at 8:58 AM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> 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 <ricardo.ribalda@gmail.com>

Acked-by: Rob Herring <robh@kernel.org>

> ---
>  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
>

Re: [PATCH v4 1/4] base/platform: Only insert MEM and IO resources

[Grant Likely]

On Thu, 23 Apr 2015 15:58:11 +0200
, Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
 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 <ricardo.ribalda@gmail.com>
> ---
>  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.

Re: [PATCH v4 4/4] base/platform: Remove code duplication

[Grant Likely]

On Wed, 13 May 2015 09:03:31 -0500
, Rob Herring <robherring2@gmail.com>
 wrote:
> On Thu, Apr 23, 2015 at 8:58 AM, Ricardo Ribalda Delgado
> <ricardo.ribalda@gmail.com> 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 <ricardo.ribalda@gmail.com>
> 
> Clean-ups should come first in the series. Otherwise:
> 
> Acked-by: Rob Herring <robh@kernel.org>

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
> >