On Thu, 4 Jun 2015 14:55:52 +0300 Vladimir Zapolskiy
<vladimir_zapols...@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 */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
