Re: [PATCH v2 1/8] of: platform: Keep track of populated platform devices

[Sascha Hauer <s.hauer@pengutronix.de>]

On Wed, Sep 30, 2020 at 10:47:09AM +0200, Marco Felsch wrote:
> After checking the linux history I found no state where it was explicite
> allowed to register a platform device twice. In the early days there was
> an bug and in some cases linux did populated the same device again. This
> was fixed in 2014 by commit c6e126de43e7 ("of: Keep track of populated
> platform devices") and since then this wasn't possible anymore.
> 
> The for_each_device() solution had two issues:
>  1) We are looping over all devices each time
>     of_platform_device_create() gets called.
>  2) The logic around 'if (!dev->resource)' then 'continue' seems to be
>     broken. This suggest that we can populate a device without
>     resource(s) first and adding resource(s) later which isn't the case.
>     Instead the current logic will populate the same device again but
>     this time (maybe) with resources. So the caller can add the same
>     device as many times as possible without resources.
>  3) The device_node gets modified if the new device_node to add has the
>     same resources region.
> 
> Add a device reference to the device_node to track an registered device
> seems to:
>  1) Simplify the code.
>  2) Align the logic with the current linux state with the exception that
>     we are returning the already created device.
>  3) Avoid looping over each device all the time.
>  4) Fix the memory leak which can occure if of_platform_device_create()
>     gets called for the same device without any resources.
> 
> I added the missing check for amba devices too while on it.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> v2:
> - new patch
> 
>  drivers/of/platform.c | 51 +++++++++++++++++--------------------------
>  include/of.h          |  1 +
>  2 files changed, 21 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 21c7cce1a5..01de6f98af 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -101,11 +101,18 @@ struct device_d *of_platform_device_create(struct device_node *np,
>  	struct device_d *dev;
>  	struct resource *res = NULL, temp_res;
>  	resource_size_t resinval;
> -	int i, j, ret, num_reg = 0, match;
> +	int i, ret, num_reg = 0;
>  
>  	if (!of_device_is_available(np))
>  		return NULL;
>  
> +	/*
> +	 * Linux uses the OF_POPULATED flag to skip already populated/created
> +	 * devices.
> +	 */
> +	if (np->dev)
> +		return np->dev;
> +
>  	/* count the io resources */
>  	if (of_can_translate_address(np))
>  		while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> @@ -121,35 +128,6 @@ struct device_d *of_platform_device_create(struct device_node *np,
>  				return NULL;
>  			}
>  		}
> -
> -		/*
> -		 * A device may already be registered as platform_device.
> -		 * Instead of registering the same device again, just
> -		 * add this node to the existing device.
> -		 */
> -		for_each_device(dev) {
> -			if (!dev->resource)
> -				continue;
> -
> -			for (i = 0, match = 0; i < num_reg; i++)
> -				for (j = 0; j < dev->num_resources; j++)
> -					if (dev->resource[j].start ==
> -						res[i].start &&
> -					    dev->resource[j].end ==
> -						res[i].end) {
> -						match++;
> -						break;
> -					}
> -
> -			/* check if all address resources match */
> -			if (match == num_reg) {
> -				debug("connecting %s to %s\n",
> -					np->name, dev_name(dev));
> -				dev->device_node = np;
> -				free(res);
> -				return dev;
> -			}
> -		}

This code is for the case when platform code has registered a device
using platform_device_register() which is also in the device tree. When
the device tree gets populated afterwards then we would have two
devices for the same resources. The code you are removing here catches
this case and instead of registering a second device for the same
resources, the existing device only gets the pointer to the corresponding
node.

That said we can probably remove this code which was useful in the early
barebox device tree days, but the code you are adding doesn't replace
it. This should really be two patches.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox