Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932213AbbFDXfh (ORCPT ); Thu, 4 Jun 2015 19:35:37 -0400 Received: from relay1.mentorg.com ([192.94.38.131]:58966 "EHLO relay1.mentorg.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932074AbbFDXff (ORCPT ); Thu, 4 Jun 2015 19:35:35 -0400 Message-ID: <5570E0C2.5030105@mentor.com> Date: Fri, 5 Jun 2015 02:35:30 +0300 From: Vladimir Zapolskiy User-Agent: Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Icedove/31.0 MIME-Version: 1.0 To: Andrew Morton CC: Philipp Zabel , Olof Johansson , Catalin Marinas , Will Deacon , Subject: Re: [PATCH] genalloc: add support of multiple gen_pools per device References: <1433418952-31749-1-git-send-email-vladimir_zapolskiy@mentor.com> <20150604153548.81ffa1de23a7a1954bcd4aad@linux-foundation.org> In-Reply-To: <20150604153548.81ffa1de23a7a1954bcd4aad@linux-foundation.org> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [137.202.0.76] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6883 Lines: 207 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 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/