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
Also the change extends the logic of of_get_named_gen_pool(), if
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.
Signed-off-by: Vladimir Zapolskiy <[email protected]>
---
include/linux/genalloc.h | 6 ++++
lib/genalloc.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 78 insertions(+), 2 deletions(-)
diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
index 1ccaab4..213645f 100644
--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -59,6 +59,8 @@ struct gen_pool {
genpool_algo_t algo; /* allocation function */
void *data;
+
+ const char *name;
};
/*
@@ -117,8 +119,12 @@ extern unsigned long gen_pool_first_fit_order_align(unsigned long *map,
extern unsigned long gen_pool_best_fit(unsigned long *map, unsigned long size,
unsigned long start, unsigned int nr, void *data);
+extern struct gen_pool *devm_gen_pool_create_named(struct device *dev,
+ int min_alloc_order, int nid, const char *name);
extern struct gen_pool *devm_gen_pool_create(struct device *dev,
int min_alloc_order, int nid);
+extern struct gen_pool *dev_get_gen_pool_named(struct device *dev,
+ const char *name);
extern struct gen_pool *dev_get_gen_pool(struct device *dev);
bool addr_in_gen_pool(struct gen_pool *pool, unsigned long start,
diff --git a/lib/genalloc.c b/lib/genalloc.c
index d214866..3300071 100644
--- a/lib/genalloc.c
+++ b/lib/genalloc.c
@@ -602,6 +602,30 @@ struct gen_pool *devm_gen_pool_create(struct device *dev, int min_alloc_order,
EXPORT_SYMBOL(devm_gen_pool_create);
/**
+ * 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
+ * @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;
+
+ return pool;
+}
+EXPORT_SYMBOL(devm_gen_pool_create_named);
+
+/**
* dev_get_gen_pool - Obtain the gen_pool (if any) for a device
* @dev: device to retrieve the gen_pool from
*
@@ -618,6 +642,38 @@ struct gen_pool *dev_get_gen_pool(struct device *dev)
}
EXPORT_SYMBOL_GPL(dev_get_gen_pool);
+static int dev_gen_pool_match(struct device *dev, void *res, void *data)
+{
+ struct gen_pool **p = res;
+
+ /* Always get a match, if no data supplied */
+ if (!data)
+ return 1;
+
+ if (!(*p)->name)
+ return 0;
+
+ return !strcmp((*p)->name, data);
+}
+
+/**
+ * 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);
+
#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 */
--
2.1.4
On Thu, 4 Jun 2015 14:55:52 +0300 Vladimir Zapolskiy <[email protected]> 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 */
Hello Andrew,
thank you for review.
On 05.06.2015 01:35, Andrew Morton wrote:
> On Thu, 4 Jun 2015 14:55:52 +0300 Vladimir Zapolskiy <[email protected]> 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!
>
This naming is based on the naming of existing interfaces found in
lib/genalloc.c:
devm_gen_pool_create() --> devm_gen_pool_create_named()
dev_get_gen_pool() --> dev_get_gen_pool_named()
of_get_named_gen_pool() --- this is untouched.
Just "_named" suffix has been added as you can see.
> Can we fix all this up please?
Sure, but since the inconsistent naming is already present in the
kernel, I suppose another preceding cross domain patch is needed,
relatively small though.
> 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 ;)
What would be your opinion on the following naming proposal:
devm_gen_pool_create() --> devm_gen_pool_create(),
devm_gen_pool_create_named()
No changes, and "named" flavour of a new gen_pool create interface goes
to the suffix position.
dev_get_gen_pool() --> dev_gen_pool_get(),
dev_gen_pool_get_named()
And the last one
of_get_named_gen_pool() --> of_gen_pool_get()
Here "named" reminds of need to provide a phandle name, not gen_pool
name as above, to avoid confusion I would propose to drop it.
If it is okay from your point of view, I'll send another non-functional
patch against linux-next to rename existing interfaces.
>> 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".
Makes sense, thank you.
>> + * @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.
>
I agree, will fix it.
>> + 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.
Like with MTD OF partitions used as a template technically it is
possible to register several gen_pools under the same name, when
requested the first found one is returned to a client, correctness of
one to one mapping is offloaded to the register party, for instance if
of_get_named_gen_pool() is supposed to be used, then DTS description is
expected to be consistent.
Is it good enough or better to add a name uniqueness check? In my
opinion the latter case may require to change error return value of
devm_gen_pool_create() from NULL to ERR_PTR().
>> #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 */
>
--
With best wishes,
Vladimir
On Fri, 5 Jun 2015 02:35:30 +0300 Vladimir Zapolskiy <[email protected]> wrote:
>
> > 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 ;)
>
> What would be your opinion on the following naming proposal:
>
> devm_gen_pool_create() --> devm_gen_pool_create(),
> devm_gen_pool_create_named()
>
> No changes, and "named" flavour of a new gen_pool create interface goes
> to the suffix position.
>
> dev_get_gen_pool() --> dev_gen_pool_get(),
> dev_gen_pool_get_named()
>
> And the last one
>
> of_get_named_gen_pool() --> of_gen_pool_get()
>
> Here "named" reminds of need to provide a phandle name, not gen_pool
> name as above, to avoid confusion I would propose to drop it.
>
> If it is okay from your point of view, I'll send another non-functional
> patch against linux-next to rename existing interfaces.
Sounds good.
> >> +/**
> >> + * 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.
>
> Like with MTD OF partitions used as a template technically it is
> possible to register several gen_pools under the same name, when
> requested the first found one is returned to a client, correctness of
> one to one mapping is offloaded to the register party, for instance if
> of_get_named_gen_pool() is supposed to be used, then DTS description is
> expected to be consistent.
Well, if we go this way then your "first found one" becomes part of
the dev_get_gen_pool_named() interface definition and should be
documented and maintained. Most-recently-registered or
least-recently-registered?
But either way, does the interface make sense? Or it is an error
condition? If it's an error condition then we should check for it
rather than silently ignoring it.
> Is it good enough or better to add a name uniqueness check? In my
> opinion the latter case may require to change error return value of
> devm_gen_pool_create() from NULL to ERR_PTR().
It sounds that way.