From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753888AbbFDWfx (ORCPT ); Thu, 4 Jun 2015 18:35:53 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:43849 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752374AbbFDWfu (ORCPT ); Thu, 4 Jun 2015 18:35:50 -0400 Date: Thu, 4 Jun 2015 15:35:48 -0700 From: Andrew Morton To: Vladimir Zapolskiy Cc: Philipp Zabel , Olof Johansson , Catalin Marinas , Will Deacon , Subject: Re: [PATCH] genalloc: add support of multiple gen_pools per device Message-Id: <20150604153548.81ffa1de23a7a1954bcd4aad@linux-foundation.org> In-Reply-To: <1433418952-31749-1-git-send-email-vladimir_zapolskiy@mentor.com> References: <1433418952-31749-1-git-send-email-vladimir_zapolskiy@mentor.com> X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 4 Jun 2015 14:55:52 +0300 Vladimir Zapolskiy 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 */