From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751504AbdFHG0U (ORCPT ); Thu, 8 Jun 2017 02:26:20 -0400 Received: from mail-wm0-f49.google.com ([74.125.82.49]:37205 "EHLO mail-wm0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750725AbdFHG0T (ORCPT ); Thu, 8 Jun 2017 02:26:19 -0400 Subject: Re: [PATCH] nvmem: core: add managed version of nvmem_register To: Heiner Kallweit Cc: Linux Kernel Mailing List References: <36f1f675-fe7c-fc6a-78ae-adc42a23e403@gmail.com> From: Srinivas Kandagatla Message-ID: Date: Thu, 8 Jun 2017 07:26:15 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/06/17 22:55, Heiner Kallweit wrote: > Am 07.06.2017 um 18:19 schrieb Srinivas Kandagatla: >> >> On 04/06/17 12:06, Heiner Kallweit wrote: >>> Add a device-managed version of nvmem_register. >>> >>> Signed-off-by: Heiner Kallweit >>> --- >>> Documentation/nvmem/nvmem.txt | 1 + >>> drivers/nvmem/core.c | 35 +++++++++++++++++++++++++++++++++++ >>> include/linux/nvmem-provider.h | 7 +++++++ >>> 3 files changed, 43 insertions(+) >>> >> >> Thanks for the patch, one comments.. >>> diff --git a/Documentation/nvmem/nvmem.txt b/Documentation/nvmem/nvmem.txt >>> index dbd40d87..b4ff7862 100644 >>> --- a/Documentation/nvmem/nvmem.txt >>> +++ b/Documentation/nvmem/nvmem.txt >>> @@ -37,6 +37,7 @@ and write the non-volatile memory. >>> A NVMEM provider can register with NVMEM core by supplying relevant >>> nvmem configuration to nvmem_register(), on success core would return a valid >>> nvmem_device pointer. >>> +devm_nvmem_register() is a device-managed version of nvmem_register. >>> >>> nvmem_unregister(nvmem) is used to unregister a previously registered provider. >>> >>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c >>> index 783eb431..55db219f 100644 >>> --- a/drivers/nvmem/core.c >>> +++ b/drivers/nvmem/core.c >>> @@ -531,6 +531,41 @@ int nvmem_unregister(struct nvmem_device *nvmem) >>> } >>> EXPORT_SYMBOL_GPL(nvmem_unregister); >>> >>> +static void devm_nvmem_release(struct device *dev, void *res) >>> +{ >>> + nvmem_unregister(*(struct nvmem_device **)res); >> >> nvmem_unregister() can fail, how are you going to deal with this error cases? >> > As stated in my answer to your other review comment: > Currently no caller of nvmem_unregister checks the return code. Currently all nvmem provider drivers check return code of unregister in remove path. > Checking the refcount I see more as a debug feature and I think making No, I don't think its a debug feature!! without this check we would end up dereferencing a freed pointer. > nvmem_unregister return void plus a WARN() if refcount != 0 would be better. WARN() would not be enough to stop the system to crash, in case the provider just unregisters with active users. > > Rgds, Heiner > >> >>> +} >>> + >>> +/** >>> + * devm_nvmem_register() - managed version of nvmem_register >>> + * >>> + * @config: nvmem device configuration with which nvmem device is created. >>> + * >>> + * Return: Will be an ERR_PTR() on error or a valid pointer to nvmem_device >>> + * on success. >>> + */ >>> + >>> +struct nvmem_device *devm_nvmem_register(const struct nvmem_config *config) >> >> For consistency reasons, devm versions of apis should always have dev at as first argument. >> >>> +{ >>> + struct nvmem_device *nv, **dr; >>> + >>> + dr = devres_alloc(devm_nvmem_release, sizeof(*dr), GFP_KERNEL); >>> + if (!dr) >>> + return ERR_PTR(-ENOMEM); >>> + >>> + nv = nvmem_register(config); >>> + if (IS_ERR(nv)) { >>> + devres_free(dr); >>> + return nv; >>> + } >>> + >>> + *dr = nv; >>> + devres_add(config->dev, dr); >>> + >>> + return nv; >>> +} >>> +EXPORT_SYMBOL_GPL(devm_nvmem_register); >>> + >> >