From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751490AbdFHGeW (ORCPT ); Thu, 8 Jun 2017 02:34:22 -0400 Received: from mail-wm0-f42.google.com ([74.125.82.42]:34971 "EHLO mail-wm0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750822AbdFHGeU (ORCPT ); Thu, 8 Jun 2017 02:34:20 -0400 Subject: Re: [PATCH 1/3] nvmem: core: remove member users from struct nvmem_device To: Heiner Kallweit , Linux Kernel Mailing List References: <32c019d9-656d-6214-af2a-bef7e91dc1de@gmail.com> <8dd4f326-9751-911a-0567-62413c7756f6@linaro.org> <8db3067b-66ca-e8f4-d974-256ede5eeef5@gmail.com> From: Srinivas Kandagatla Message-ID: Date: Thu, 8 Jun 2017 07:34:17 +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: <8db3067b-66ca-e8f4-d974-256ede5eeef5@gmail.com> 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:51, Heiner Kallweit wrote: > Am 07.06.2017 um 17:30 schrieb Srinivas Kandagatla: >> >> >> On 04/06/17 12:01, Heiner Kallweit wrote: >>> Member users is used only to check whether we're allowed to remove >>> the module. So in case of built-in it's not used at all and in case >> >> nvmem providers doesn't have to be independent drivers, providers could be part of the other driver which can dynamically register and unregister nvmem providers. For example at24 and at25 drivers. >> >> This patch will break such cases !! >> > Thanks for the quick review. > I don't think this patch breaks e.g. at24 / at25. Let me try to explain: > > at24 / at25 set themself as owner in struct nvmem_device and nvmem_unregister > is called from at24/25_remove only. These remove callbacks are called only if > all references to the respective module have been released. > > In current kernel code I don't see any nvmem use broken by the proposed patch. > However in general you're right, there may be future use cases where > nvmem_unregister isn't called only from a remove callback. Yes, the patch would not break the exiting code, but as said it would break a feature which was considered while writing the code. > > If the refcount isn't zero when calling nvmem_unregister then there's a bigger > problem, I don't think there's any normal use case where this can happen. Yes I understand chances of this error path is slim but it would crash the system if it hits this path, so this safety check is in place. > Instead of just returning -EBUSY I think a WARN() would be appropriate. > Currently no caller of nvmem_unregister checks the return code anyway. > My opinion would be that the refcount here is more a debug feature. > > > Whilst we're talking about nvmem_unregister: > I think the device_del() at the end should be a device_unregister(). > Else we miss put_device as second part of destroying a device. These issues have already been addressed in https://patchwork.kernel.org/patch/9685559/ https://patchwork.kernel.org/patch/9685561/ https://patchwork.kernel.org/patch/9729235/ --srini > > Rgds, Heiner > > >> >> >>> that owner is a module we have the module refcount for the same >>> purpose already. Whenever users is incremented the owner's refcount >>> is incremented too. Therefore users isn't needed. >>> >>> Signed-off-by: Heiner Kallweit >>> --- >>> drivers/nvmem/core.c | 16 ---------------- >>> 1 file changed, 16 deletions(-) >>> >>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c >>> index 8c830a80..4e07f3f8 100644 >>> --- a/drivers/nvmem/core.c >>> +++ b/drivers/nvmem/core.c >>> @@ -33,7 +33,6 @@ struct nvmem_device { >>> int word_size; >>> int ncells; >>> int id; >>> - int users; >>> size_t size; >>> bool read_only; >>> int flags; >>> @@ -517,13 +516,6 @@ EXPORT_SYMBOL_GPL(nvmem_register); >>> */ >>> int nvmem_unregister(struct nvmem_device *nvmem) >>> { >>> - mutex_lock(&nvmem_mutex); >>> - if (nvmem->users) { >>> - mutex_unlock(&nvmem_mutex); >>> - return -EBUSY; >>> - } >>> - mutex_unlock(&nvmem_mutex); >>> - >>> if (nvmem->flags & FLAG_COMPAT) >>> device_remove_bin_file(nvmem->base_dev, &nvmem->eeprom); >>> >>> @@ -562,7 +554,6 @@ static struct nvmem_device *__nvmem_device_get(struct device_node *np, >>> } >>> } >>> >>> - nvmem->users++; >>> mutex_unlock(&nvmem_mutex); >>> >>> if (!try_module_get(nvmem->owner)) { >>> @@ -570,10 +561,6 @@ static struct nvmem_device *__nvmem_device_get(struct device_node *np, >>> "could not increase module refcount for cell %s\n", >>> nvmem->name); >>> >>> - mutex_lock(&nvmem_mutex); >>> - nvmem->users--; >>> - mutex_unlock(&nvmem_mutex); >>> - >>> return ERR_PTR(-EINVAL); >>> } >>> >>> @@ -583,9 +570,6 @@ static struct nvmem_device *__nvmem_device_get(struct device_node *np, >>> static void __nvmem_device_put(struct nvmem_device *nvmem) >>> { >>> module_put(nvmem->owner); >>> - mutex_lock(&nvmem_mutex); >>> - nvmem->users--; >>> - mutex_unlock(&nvmem_mutex); >>> } >>> >>> static int nvmem_match(struct device *dev, void *data) >>> >> >