[PATCH 0/3] nvmem: core: series with smaller refactorings

[PATCH 0/3] nvmem: core: series with smaller refactorings

[Heiner Kallweit]

Series with smaller refactorings of the nvmem core.

Heiner Kallweit (3):
  nvmem: core: remove member users from struct nvmem_device
  nvmem: core: add locking to nvmem_find_cell
  nvmem: core: remove nvmem_mutex

 drivers/nvmem/core.c | 37 +++++++++----------------------------
 1 file changed, 9 insertions(+), 28 deletions(-)

-- 
2.13.0

[PATCH 1/3] nvmem: core: remove member users from struct nvmem_device

[Heiner Kallweit]

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
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 <hkallweit1@gmail.com>
---
 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)
-- 
2.13.0

Re: [PATCH 1/3] nvmem: core: remove member users from struct nvmem_device

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



> 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 <hkallweit1@gmail.com>
> ---
>  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)
>

Re: [PATCH 1/3] nvmem: core: remove member users from struct nvmem_device

[Heiner Kallweit]

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.

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

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 <hkallweit1@gmail.com>
>> ---
>>  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)
>>
> 

Re: [PATCH 1/3] nvmem: core: remove member users from struct nvmem_device

[Srinivas Kandagatla]

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 <hkallweit1@gmail.com>
>>> ---
>>>   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)
>>>
>>
> 

[PATCH 2/3] nvmem: core: add locking to nvmem_find_cell

[Heiner Kallweit]

Adding entries to nvmem_cells and deleting entries from it is
protected by nvmem_cells_mutex. Therefore this mutex should
also protect iterating over the list.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/nvmem/core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 4e07f3f8..1aa6d25a 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -286,9 +286,15 @@ static struct nvmem_cell *nvmem_find_cell(const char *cell_id)
 {
 	struct nvmem_cell *p;
 
+	mutex_lock(&nvmem_cells_mutex);
+
 	list_for_each_entry(p, &nvmem_cells, node)
-		if (p && !strcmp(p->name, cell_id))
+		if (p && !strcmp(p->name, cell_id)) {
+			mutex_unlock(&nvmem_cells_mutex);
 			return p;
+		}
+
+	mutex_unlock(&nvmem_cells_mutex);
 
 	return NULL;
 }
-- 
2.13.0

Re: [PATCH 2/3] nvmem: core: add locking to nvmem_find_cell

[Srinivas Kandagatla]

On 04/06/17 12:01, Heiner Kallweit wrote:
> Adding entries to nvmem_cells and deleting entries from it is
> protected by nvmem_cells_mutex. Therefore this mutex should
> also protect iterating over the list.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/nvmem/core.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 4e07f3f8..1aa6d25a 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -286,9 +286,15 @@ static struct nvmem_cell *nvmem_find_cell(const char *cell_id)
>  {
>  	struct nvmem_cell *p;
>
> +	mutex_lock(&nvmem_cells_mutex);
> +
>  	list_for_each_entry(p, &nvmem_cells, node)
> -		if (p && !strcmp(p->name, cell_id))
> +		if (p && !strcmp(p->name, cell_id)) {
> +			mutex_unlock(&nvmem_cells_mutex);
>  			return p;
> +		}
> +
> +	mutex_unlock(&nvmem_cells_mutex);
>
Thanks for the patch, I will queue this up.

Thanks,
srini
>  	return NULL;
>  }
>

[PATCH 3/3] nvmem: core: remove nvmem_mutex

[Heiner Kallweit]

Mutex nvmem_mutex is used in __nvmem_device_get only and isn't needed
due to:

- of_nvmem_find just calls bus_find_device which doesn't need locking
- nvmem_find_cell is protected by nvmem_cells_mutex

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/nvmem/core.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 1aa6d25a..783eb431 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -55,7 +55,6 @@ struct nvmem_cell {
 	struct list_head	node;
 };
 
-static DEFINE_MUTEX(nvmem_mutex);
 static DEFINE_IDA(nvmem_ida);
 
 static LIST_HEAD(nvmem_cells);
@@ -538,14 +537,10 @@ static struct nvmem_device *__nvmem_device_get(struct device_node *np,
 {
 	struct nvmem_device *nvmem = NULL;
 
-	mutex_lock(&nvmem_mutex);
-
 	if (np) {
 		nvmem = of_nvmem_find(np);
-		if (!nvmem) {
-			mutex_unlock(&nvmem_mutex);
+		if (!nvmem)
 			return ERR_PTR(-EPROBE_DEFER);
-		}
 	} else {
 		struct nvmem_cell *cell = nvmem_find_cell(cell_id);
 
@@ -554,14 +549,10 @@ static struct nvmem_device *__nvmem_device_get(struct device_node *np,
 			*cellp = cell;
 		}
 
-		if (!nvmem) {
-			mutex_unlock(&nvmem_mutex);
+		if (!nvmem)
 			return ERR_PTR(-ENOENT);
-		}
 	}
 
-	mutex_unlock(&nvmem_mutex);
-
 	if (!try_module_get(nvmem->owner)) {
 		dev_err(&nvmem->dev,
 			"could not increase module refcount for cell %s\n",
-- 
2.13.0