Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1158494 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] genalloc: add support of multiple gen_pools per device Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2015-06-04 14:00 +0200
    Re: [PATCH] genalloc: add support of multiple gen_pools per device Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2015-06-05 01:40 +0200

#1158494 — [PATCH] genalloc: add support of multiple gen_pools per device

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 <vladimir_zapolskiy@mentor.com>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1158881

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web