nvmem: core: series with smaller refactorings

PATCH In review Heiner Kallweit 2 participants 8 messages Jun 4, 2017 – Jun 8, 2017 NVMEM framework

lore

Share & tools

lore

Heiner Kallweit

author

2017-06-04 10:48 UTC #0 lore raw

cover nvmem: core: series with smaller refactorings

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(-)

Signature · 1 line

2.13.0

Heiner Kallweit

author

2017-06-04 11:02 UTC #1 lore raw

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

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 <[email protected]>

---
 drivers/nvmem/core.c | 16 ----------------
 1 file changed, 16 deletions(-)

drivers/nvmem/core.c +0 ~~-16~~

index 8c830a80..4e07f3f8 100644
@@ -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)

Signature · 1 line

2.13.0

Srinivas Kandagatla

2017-06-07 15:30 UTC #2 lore raw

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

Quoted 2 lines · 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

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

Quoted 65 lines

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 <[email protected]>

---
 drivers/nvmem/core.c | 16 ----------------
 1 file changed, 16 deletions(-)

drivers/nvmem/core.c +0 ~~-16~~

index 8c830a80..4e07f3f8 100644
@@ -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)

Heiner Kallweit

author

2017-06-07 21:55 UTC #3 lore raw

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

Am 07.06.2017 um 17:30 schrieb Srinivas Kandagatla:

Quoted 10 lines · Heiner Kallweit

Quoted 2 lines
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

Quoted 68 lines

Quoted 67 lines

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 <[email protected]>

---
 drivers/nvmem/core.c | 16 ----------------
 1 file changed, 16 deletions(-)

drivers/nvmem/core.c +0 ~~-16~~

index 8c830a80..4e07f3f8 100644
@@ -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)

Srinivas Kandagatla

2017-06-08 06:34 UTC #4 lore raw

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

Quoted 14 lines · Heiner Kallweit

Am 07.06.2017 um 17:30 schrieb Srinivas Kandagatla:
Quoted 10 lines · Heiner Kallweit
Quoted 2 lines
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:

Quoted 7 lines

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.

Quoted 3 lines

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.

Quoted 8 lines

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

Quoted 73 lines

Rgds, Heiner

Quoted 68 lines

Quoted 67 lines

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 <[email protected]>

---
  drivers/nvmem/core.c | 16 ----------------
  1 file changed, 16 deletions(-)

drivers/nvmem/core.c +0 ~~-16~~

index 8c830a80..4e07f3f8 100644
@@ -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)

Heiner Kallweit

author

2017-06-04 11:02 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/nvmem/core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/nvmem/core.c +7 -1

index 4e07f3f8..1aa6d25a 100644
@@ -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;
 }

Signature · 1 line

2.13.0

Srinivas Kandagatla

2017-06-07 15:31 UTC #6 lore raw

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

Quoted 18 lines · 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 <[email protected]>
---
 drivers/nvmem/core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/nvmem/core.c +1 -0
index 4e07f3f8..1aa6d25a 100644
@@ -286,9 +286,15 @@ static struct nvmem_cell *nvmem_find_cell(const char *cell_id)
 {
 	struct nvmem_cell *p;
 
+	mutex_lock(&nvmem_cells_mutex);

drivers/nvmem/core.c static struct nvmem_cell *nvmem_find_cell(const char *cell_id) · line 298

Quoted 10 lines

drivers/nvmem/core.c +6 -1

+
 	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

Quoted 3 lines

drivers/nvmem/core.c +0 -0
 	return NULL;
 }

Heiner Kallweit

author

2017-06-04 11:02 UTC #7 lore raw

PATCH 3/3 nvmem: core: remove nvmem_mutex

In reply to 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 <[email protected]>

---
 drivers/nvmem/core.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

drivers/nvmem/core.c +2 ~~-11~~

index 1aa6d25a..783eb431 100644
@@ -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",

Signature · 1 line

2.13.0