Re: [PATCH] genalloc: add support of multiple gen_pools per device

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 4 Jun 2015 14:55:52 +0300 Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> wrote:

> This change adds two more exported interfaces to genalloc:
> 
> * devm_gen_pool_create_named() -- same as devm_gen_pool_create(), but
>   the created gen_pool object can be referenced by a given name,
> * dev_get_gen_pool_named() -- same as dev_get_gen_pool(), but allows
>   to get a previously registered particular gen_pool instance by name

The naming is inconsistent.  Either

	devm_gen_pool_create_named, dev_gen_pool_get_named

or

	devm_create_gen_pool_create, dev_get_gen_pool_named

> Also the change extends the logic of of_get_named_gen_pool(), if

and "of_get_named_gen_pool" is inconsistent with all the above!

Can we fix all this up please?

If there's a pattern, it is "subsytem-identifier_operation-on-it", so

	devm_gen_pool_named_create
	dev_gen_pool_named_get
	of_named_gen_pool_get

ie: it's big-endian.  The name starts out with the most significant
thing (subsystem identification) and fields in order of decreasing
significance.

Anyway, please have a think about it ;)

> there is no associated platform device with a given device node, it
> attempts to get a label property or device node name (= repeats MTD
> OF partition standard) and seeks for a named gen_pool registered by
> parent device node device.
> 
> The main idea of the change is to allow registration of independent
> gen_pools under the same umbrella device, say "partitions" on "storage
> device", the original functionality of one "partition" per "storage
> device" is untouched.
> 
> ...
>
>  /**
> + * devm_gen_pool_create_named - managed gen_pool_create
> + * @dev: device that provides the gen_pool
> + * @min_alloc_order: log base 2 of number of bytes each bitmap bit represents
> + * @nid: node id of the node the pool structure should be allocated on, or -1

Let's use "NUMA_NO_NODE" instead of a bare "-1".

> + * @name: name of a gen_pool within all gen_pool associated with the device
> + *
> + * Create a new special memory pool that can be used to manage special purpose
> + * memory not managed by the regular kmalloc/kfree interface. The pool will be
> + * automatically destroyed by the device management code.
> + */
> +struct gen_pool *devm_gen_pool_create_named(struct device *dev,
> +		int min_alloc_order, int nid, const char *name)
> +{
> +	struct gen_pool *pool;
> +
> +	pool = devm_gen_pool_create(dev, min_alloc_order, nid);
> +	if (pool)
> +		pool->name = name;

This requires that the caller perform management of the memory at
*name, which is a bit klunky.  It's more work for the caller to do and
it creates a dependency in the other direction: genpool requires that
the caller keep the storage alive.

So maybe it would be better to kstrdup() this string and make genpool
kfree() it when appropriate.


> +	return pool;
> +}
> +EXPORT_SYMBOL(devm_gen_pool_create_named);
> +
>
> ...
>
> +/**
> + * dev_get_gen_pool_named - Obtain the gen_pool (if any) for a device
> + * @dev: device to retrieve the gen_pool from
> + * @name: name of a gen_pool, addresses a particular gen_pool from device
> + *
> + * Returns the gen_pool for the device if one is present, or NULL.
> + */
> +struct gen_pool *dev_get_gen_pool_named(struct device *dev, const char *name)
> +{
> +	struct gen_pool **p = devres_find(dev, devm_gen_pool_release,
> +					  dev_gen_pool_match, (void *)name);
> +
> +	if (!p)
> +		return NULL;
> +	return *p;
> +}
> +EXPORT_SYMBOL_GPL(dev_get_gen_pool_named);

But we didn't do anything to prevent duplicated names.

>  #ifdef CONFIG_OF
>  /**
>   * of_get_named_gen_pool - find a pool by phandle property
> @@ -633,16 +689,30 @@ struct gen_pool *of_get_named_gen_pool(struct device_node *np,
>  	const char *propname, int index)
>  {
>  	struct platform_device *pdev;
> -	struct device_node *np_pool;
> +	struct device_node *np_pool, *parent;
> +	const char *name = NULL;
>  
>  	np_pool = of_parse_phandle(np, propname, index);
>  	if (!np_pool)
>  		return NULL;
> +
>  	pdev = of_find_device_by_node(np_pool);
> +	if (!pdev) {
> +		/* Check if named gen_pool is created by parent node device */
> +		parent = of_get_parent(np_pool);
> +		pdev = of_find_device_by_node(parent);
> +		of_node_put(parent);
> +
> +		of_property_read_string(np_pool, "label", &name);
> +		if (!name)
> +			name = np_pool->name;
> +	}
>  	of_node_put(np_pool);
> +
>  	if (!pdev)
>  		return NULL;
> -	return dev_get_gen_pool(&pdev->dev);
> +
> +	return dev_get_gen_pool_named(&pdev->dev, name);
>  }
>  EXPORT_SYMBOL_GPL(of_get_named_gen_pool);
>  #endif /* CONFIG_OF */