Skip to contentSkip to conversation

nvmem/gpio: fix resource management

PATCH Superseded Bartosz Golaszewski 4 participants 16 messages Feb 18, 2020 – Feb 20, 2020

Subsystems (3)
Stable branch GPIO NVMEM framework

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Bartosz Golaszewski

author
#0 lore raw

v2 cover nvmem/gpio: fix resource management

From: Bartosz Golaszewski <[email protected]>

This series addresses a couple problems with memory management in nvmem core.

First we fix two earlier memory leaks - this is obvious stable material. Next we extend the GPIO framework to use reference counting for GPIO descriptors. We then use it to fix the resource management problem with the write-protect pin.

Finally we add some readability tweaks.

While the memory leak with wp-gpios is now in mainline - I'm not sure how to go about applying the kref patch. This is theoretically a new feature but it's also the cleanest way of fixing the problem.

v1 -> v2:
- make gpiod_ref() helper return
- reorganize the series for easier merging
- fix another memory leak

Bartosz Golaszewski (6):
nvmem: fix memory leak in error path
nvmem: fix another memory leak in error path
gpiolib: use kref in gpio_desc
nvmem: increase the reference count of a gpio passed over config
nvmem: remove a stray newline in nvmem_register()
nvmem: add a newline for readability

Khouloud Touil (1):
nvmem: release the write-protect pin

 drivers/gpio/gpiolib.c        | 30 +++++++++++++++++++++++++++---
 drivers/gpio/gpiolib.h        |  1 +
 drivers/nvmem/core.c          | 18 +++++++++++-------
 include/linux/gpio/consumer.h |  1 +
 4 files changed, 40 insertions(+), 10 deletions(-)
Signature · 1 line
2.25.0
author
#1 lore raw

PATCH v2 1/7 nvmem: fix memory leak in error path

From: Bartosz Golaszewski <[email protected]>

We need to remove the ida mapping when returning from nvmem_register() with an error.

Cc <[email protected]>
Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/nvmem/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/nvmem/core.c +3 -1
index ef326f243f36..b0be03d5f240 100644
@@ -353,7 +353,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 		nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
 						    GPIOD_OUT_HIGH);
 	if (IS_ERR(nvmem->wp_gpio))
-		return ERR_CAST(nvmem->wp_gpio);
+		goto err_ida_remove;
 
 
 	kref_init(&nvmem->refcnt);

@@ -430,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	device_del(&nvmem->dev);
 err_put_device:
 	put_device(&nvmem->dev);
+err_ida_remove:
+	ida_simple_remove(&nvmem_ida, nvmem->id);
 
 	return ERR_PTR(rval);
 }
Signature · 1 line
2.25.0
author
#2 lore raw

PATCH v2 2/7 nvmem: fix another memory leak in error path

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

The nvmem struct is only freed on the first error check after its allocation and leaked after that. Fix it with a new label.

Cc <[email protected]>
Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/nvmem/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/nvmem/core.c +4 -4
index b0be03d5f240..c9b3f4047154 100644
@@ -343,10 +343,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 		return ERR_PTR(-ENOMEM);
 
 	rval  = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
-	if (rval < 0) {
-		kfree(nvmem);
-		return ERR_PTR(rval);
-	}
+	if (rval < 0)
+		goto err_free_nvmem;
 	if (config->wp_gpio)
 		nvmem->wp_gpio = config->wp_gpio;
 	else

@@ -432,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	put_device(&nvmem->dev);
 err_ida_remove:
 	ida_simple_remove(&nvmem_ida, nvmem->id);
+err_free_nvmem:
+	kfree(nvmem);
 
 	return ERR_PTR(rval);
 }
Signature · 1 line
2.25.0
#3 lore raw

Re: [PATCH v2 2/7] nvmem: fix another memory leak in error path

Quoted 7 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

The nvmem struct is only freed on the first error check after its allocation and leaked after that. Fix it with a new label.

Cc <[email protected]>
Signed-off-by Bartosz Golaszewski <[email protected]>

looks like 1/7 introduced the bug and 2/7 fixes it. IMO, you should 1/7 and 2/7 should be single patch.

--srini

Quoted 31 lines
---
  drivers/nvmem/core.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)
drivers/nvmem/core.c +4 -4
index b0be03d5f240..c9b3f4047154 100644
@@ -343,10 +343,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
  		return ERR_PTR(-ENOMEM);
 
  	rval  = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
-	if (rval < 0) {
-		kfree(nvmem);
-		return ERR_PTR(rval);
-	}
+	if (rval < 0)
+		goto err_free_nvmem;
  	if (config->wp_gpio)
  		nvmem->wp_gpio = config->wp_gpio;
  	else

@@ -432,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
  	put_device(&nvmem->dev);
  err_ida_remove:
  	ida_simple_remove(&nvmem_ida, nvmem->id);
+err_free_nvmem:
+	kfree(nvmem);
 
  	return ERR_PTR(rval);
  }
#4 lore raw

Re: [PATCH v2 2/7] nvmem: fix another memory leak in error path

wt., 18 lut 2020 o 10:47 Srinivas Kandagatla
<[email protected]> napisał(a):

Quoted 15 lines · Bartosz Golaszewski
Quoted 7 lines

From: Bartosz Golaszewski <[email protected]>

The nvmem struct is only freed on the first error check after its allocation and leaked after that. Fix it with a new label.

Cc <[email protected]>
Signed-off-by Bartosz Golaszewski <[email protected]>

looks like 1/7 introduced the bug and 2/7 fixes it. IMO, you should 1/7 and 2/7 should be single patch.

The bug already exists in mainline - the nvmem object is only freed if ida_simple_get() fails, but any subsequent error leads to a memory leak. In other words: it doesn't matter if it's a single patch or two.

Bart

#5 lore raw

Re: [PATCH v2 2/7] nvmem: fix another memory leak in error path

In reply to Bartosz Golaszewski
Quoted 29 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

The nvmem struct is only freed on the first error check after its allocation and leaked after that. Fix it with a new label.

Cc <[email protected]>
Signed-off-by Bartosz Golaszewski <[email protected]>
---
  drivers/nvmem/core.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)
drivers/nvmem/core.c +2 -4
index b0be03d5f240..c9b3f4047154 100644
@@ -343,10 +343,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
  		return ERR_PTR(-ENOMEM);
 
  	rval  = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
-	if (rval < 0) {
-		kfree(nvmem);
-		return ERR_PTR(rval);
-	}
+	if (rval < 0)
+		goto err_free_nvmem;
  	if (config->wp_gpio)
  		nvmem->wp_gpio = config->wp_gpio;
  	else

drivers/nvmem/core.c struct nvmem_device *nvmem_register(const struct nvmem_config *config) · line 434

Quoted 5 lines
drivers/nvmem/core.c +2 -0
@@ -432,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
  	put_device(&nvmem->dev);
  err_ida_remove:
  	ida_simple_remove(&nvmem_ida, nvmem->id);
+err_free_nvmem:
+	kfree(nvmem);

This is not correct fix to start with, if the device has already been intialized before jumping here then nvmem would be freed as part of nvmem_release().

So the bug was actually introduced by adding err_ida_remove label.

You can free nvmem at that point but not at any point after that as device core would be holding a reference to it.

--srini

Quoted 4 lines
drivers/nvmem/core.c +0 -0
  	return ERR_PTR(rval);
  }
author
#6 lore raw

Re: [PATCH v2 2/7] nvmem: fix another memory leak in error path

wt., 18 lut 2020 o 10:56 Srinivas Kandagatla
<[email protected]> napisał(a):

Quoted 43 lines · Bartosz Golaszewski
Quoted 29 lines

From: Bartosz Golaszewski <[email protected]>

The nvmem struct is only freed on the first error check after its allocation and leaked after that. Fix it with a new label.

Cc <[email protected]>
Signed-off-by Bartosz Golaszewski <[email protected]>
---
  drivers/nvmem/core.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)
drivers/nvmem/core.c +2 -4
index b0be03d5f240..c9b3f4047154 100644
@@ -343,10 +343,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
              return ERR_PTR(-ENOMEM);
 
      rval  = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
-     if (rval < 0) {
-             kfree(nvmem);
-             return ERR_PTR(rval);
-     }
+     if (rval < 0)
+             goto err_free_nvmem;
      if (config->wp_gpio)
              nvmem->wp_gpio = config->wp_gpio;
      else

drivers/nvmem/core.c struct nvmem_device *nvmem_register(const struct nvmem_config *config) · line 434

Quoted 5 lines
drivers/nvmem/core.c +2 -0
@@ -432,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
      put_device(&nvmem->dev);
  err_ida_remove:
      ida_simple_remove(&nvmem_ida, nvmem->id);
+err_free_nvmem:
+     kfree(nvmem);

This is not correct fix to start with, if the device has already been intialized before jumping here then nvmem would be freed as part of nvmem_release().

Quoted 5 lines

So the bug was actually introduced by adding err_ida_remove label.

You can free nvmem at that point but not at any point after that as device core would be holding a reference to it.

OK I see - I missed the release() callback assignment. Frankly: I find this split of resource management responsibility confusing. Since the users are expected to call nvmem_unregister() anyway - wouldn't it be more clear to just free all resources there? What is the advantage of defining the release() callback for device type here?

Bartosz

#7 lore raw

Re: [PATCH v2 2/7] nvmem: fix another memory leak in error path

Quoted 51 lines · Bartosz Golaszewski

wt., 18 lut 2020 o 10:56 Srinivas Kandagatla
<[email protected]> napisał(a):

Quoted 43 lines · Bartosz Golaszewski
Quoted 29 lines

From: Bartosz Golaszewski <[email protected]>

The nvmem struct is only freed on the first error check after its allocation and leaked after that. Fix it with a new label.

Cc <[email protected]>
Signed-off-by Bartosz Golaszewski <[email protected]>
---
   drivers/nvmem/core.c | 8 ++++----
   1 file changed, 4 insertions(+), 4 deletions(-)
drivers/nvmem/core.c +2 -4
index b0be03d5f240..c9b3f4047154 100644
@@ -343,10 +343,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
               return ERR_PTR(-ENOMEM);
 
       rval  = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
-     if (rval < 0) {
-             kfree(nvmem);
-             return ERR_PTR(rval);
-     }
+     if (rval < 0)
+             goto err_free_nvmem;
       if (config->wp_gpio)
               nvmem->wp_gpio = config->wp_gpio;
       else

drivers/nvmem/core.c struct nvmem_device *nvmem_register(const struct nvmem_config *config) · line 434

Quoted 5 lines
drivers/nvmem/core.c +2 -0
@@ -432,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
       put_device(&nvmem->dev);
   err_ida_remove:
       ida_simple_remove(&nvmem_ida, nvmem->id);
+err_free_nvmem:
+     kfree(nvmem);

This is not correct fix to start with, if the device has already been intialized before jumping here then nvmem would be freed as part of nvmem_release().

Quoted 5 lines

So the bug was actually introduced by adding err_ida_remove label.

You can free nvmem at that point but not at any point after that as device core would be holding a reference to it.

Quoted 5 lines

OK I see - I missed the release() callback assignment. Frankly: I find this split of resource management responsibility confusing. Since the users are expected to call nvmem_unregister() anyway - wouldn't it be more clear to just free all resources there? What is the advantage of defining the release() callback for device type here?

Because we are using dev pointer from nvmem structure, and this dev pointer should be valid until release callback is invoked.

Freeing nvmem at any early stage would make dev pointer invalid and device core would dereference it!

--srini

Quoted 3 lines

Bartosz

#8 lore raw

Re: [PATCH v2 2/7] nvmem: fix another memory leak in error path

wt., 18 lut 2020 o 11:11 Srinivas Kandagatla
<[email protected]> napisał(a):

Quoted 61 lines · Bartosz Golaszewski
Quoted 51 lines

wt., 18 lut 2020 o 10:56 Srinivas Kandagatla
<[email protected]> napisał(a):

Quoted 43 lines · Bartosz Golaszewski
Quoted 29 lines

From: Bartosz Golaszewski <[email protected]>

The nvmem struct is only freed on the first error check after its allocation and leaked after that. Fix it with a new label.

Cc <[email protected]>
Signed-off-by Bartosz Golaszewski <[email protected]>
---
   drivers/nvmem/core.c | 8 ++++----
   1 file changed, 4 insertions(+), 4 deletions(-)
drivers/nvmem/core.c +2 -4
index b0be03d5f240..c9b3f4047154 100644
@@ -343,10 +343,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
               return ERR_PTR(-ENOMEM);
 
       rval  = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
-     if (rval < 0) {
-             kfree(nvmem);
-             return ERR_PTR(rval);
-     }
+     if (rval < 0)
+             goto err_free_nvmem;
       if (config->wp_gpio)
               nvmem->wp_gpio = config->wp_gpio;
       else

drivers/nvmem/core.c struct nvmem_device *nvmem_register(const struct nvmem_config *config) · line 434

Quoted 5 lines
drivers/nvmem/core.c +2 -0
@@ -432,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
       put_device(&nvmem->dev);
   err_ida_remove:
       ida_simple_remove(&nvmem_ida, nvmem->id);
+err_free_nvmem:
+     kfree(nvmem);

This is not correct fix to start with, if the device has already been intialized before jumping here then nvmem would be freed as part of nvmem_release().

Quoted 5 lines

So the bug was actually introduced by adding err_ida_remove label.

You can free nvmem at that point but not at any point after that as device core would be holding a reference to it.

Quoted 5 lines

OK I see - I missed the release() callback assignment. Frankly: I find this split of resource management responsibility confusing. Since the users are expected to call nvmem_unregister() anyway - wouldn't it be more clear to just free all resources there? What is the advantage of defining the release() callback for device type here?

Quoted 6 lines

Because we are using dev pointer from nvmem structure, and this dev pointer should be valid until release callback is invoked.

Freeing nvmem at any early stage would make dev pointer invalid and device core would dereference it!

Ok, let me brew up a v3 with that in mind.

Bart

author
#9 lore raw

PATCH v2 3/7 gpiolib: use kref in gpio_desc

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

GPIO descriptors are freed by consumers using gpiod_put(). The name of this function suggests some reference counting is going on but it's not true.

Use kref to actually introduce reference counting for gpio_desc objects. Add a corresponding gpiod_get() helper for increasing the reference count.

This doesn't change anything for already existing (correct) drivers but allows us to keep track of GPIO descs used by multiple users.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/gpio/gpiolib.c        | 30 +++++++++++++++++++++++++++---
 drivers/gpio/gpiolib.h        |  1 +
 include/linux/gpio/consumer.h |  1 +
 3 files changed, 29 insertions(+), 3 deletions(-)
drivers/gpio/gpiolib.c +27 -3
index 4d0106ceeba7..582425e9b449 100644
@@ -2798,6 +2798,8 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 		goto done;
 	}
 
+	kref_init(&desc->ref);
+
 	if (chip->request) {
 		/* chip->request may sleep */
 		spin_unlock_irqrestore(&gpio_lock, flags);

@@ -2933,6 +2935,13 @@ void gpiod_free(struct gpio_desc *desc)
 	}
 }
 
+static void gpiod_free_kref(struct kref *ref)
+{
+	struct gpio_desc *desc = container_of(ref, struct gpio_desc, ref);
+
+	gpiod_free(desc);
+}
+
 /**
  * gpiochip_is_requested - return string iff signal was requested
  * @chip: controller managing the signal

@@ -5067,18 +5076,33 @@ struct gpio_descs *__must_check gpiod_get_array_optional(struct device *dev,
 EXPORT_SYMBOL_GPL(gpiod_get_array_optional);
 
 /**
- * gpiod_put - dispose of a GPIO descriptor
- * @desc:	GPIO descriptor to dispose of
+ * gpiod_put - decrease the reference count of a GPIO descriptor
+ * @desc:	GPIO descriptor to unref
  *
  * No descriptor can be used after gpiod_put() has been called on it.
  */
 void gpiod_put(struct gpio_desc *desc)
 {
 	if (desc)
-		gpiod_free(desc);
+		kref_put(&desc->ref, gpiod_free_kref);
 }
 EXPORT_SYMBOL_GPL(gpiod_put);
 
+/**
+ * gpiod_ref - increase the reference count of a GPIO descriptor
+ * @desc:	GPIO descriptor to reference
+ *
+ * Returns the same gpio_desc after increasing the reference count.
+ */
+struct gpio_desc *gpiod_ref(struct gpio_desc *desc)
+{
+	if (desc)
+		kref_get(&desc->ref);
+
+	return desc;
+}
+EXPORT_SYMBOL_GPL(gpiod_ref);
+
 /**
  * gpiod_put_array - dispose of multiple GPIO descriptors
  * @descs:	struct gpio_descs containing an array of descriptors
drivers/gpio/gpiolib.h +1 -0
index 3e0aab2945d8..51a92c43dd55 100644
@@ -119,6 +119,7 @@ struct gpio_desc {
 	const char		*label;
 	/* Name of the GPIO */
 	const char		*name;
+	struct kref		ref;
 };
 
 int gpiod_request(struct gpio_desc *desc, const char *label);
include/linux/gpio/consumer.h +1 -0
index bf2d017dd7b7..c7b5fb3d9d64 100644
@@ -81,6 +81,7 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
 struct gpio_descs *__must_check gpiod_get_array_optional(struct device *dev,
 							const char *con_id,
 							enum gpiod_flags flags);
+struct gpio_desc *gpiod_ref(struct gpio_desc *desc);
 void gpiod_put(struct gpio_desc *desc);
 void gpiod_put_array(struct gpio_descs *descs);
Signature · 1 line
2.25.0
author
#10 lore raw

PATCH v2 4/7 nvmem: increase the reference count of a gpio passed over config

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

We can obtain the write-protect GPIO in nvmem_register() by requesting it ourselves or by storing the gpio_desc passed in nvmem_config. In the latter case we need to increase the reference count so that it gets freed correctly.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/nvmem/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvmem/core.c +1 -1
index c9b3f4047154..c4a6c044b020 100644
@@ -346,7 +346,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	if (rval < 0)
 		goto err_free_nvmem;
 	if (config->wp_gpio)
-		nvmem->wp_gpio = config->wp_gpio;
+		nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
 	else
 		nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
 						    GPIOD_OUT_HIGH);
Signature · 1 line
2.25.0
build bot
#11 lore raw

Re: [PATCH v2 4/7] nvmem: increase the reference count of a gpio passed over config

I love your patch! Perhaps something to improve:

Hi Bartosz,

I love your patch! Perhaps something to improve:

[auto build test WARNING on gpio/for-next]
[also build test WARNING on linus/master v5.6-rc2]
[cannot apply to next-20200219]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Bartosz-Golaszewski/nvmem-gpio-fix-resource-management/20200220-045651
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: nios2-3c120_defconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 7.5.0

reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=nios2

If you fix the issue, kindly add following tag

Reported-by kbuild test robot <[email protected]>

All warnings (new ones prefixed by >>):

drivers/nvmem/core.c: In function 'nvmem_register':
drivers/nvmem/core.c:349:20: error: implicit declaration of function 'gpiod_ref'; did you mean 'gpiod_get'? [-Werror=implicit-function-declaration]

      nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
                       ^~~~~~~~~
                       gpiod_get
>> drivers/nvmem/core.c:349:18: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
      nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
                     ^

cc1: some warnings being treated as errors

vim +349 drivers/nvmem/core.c

322
323 /**
324 * nvmem_register() - Register a nvmem device for given nvmem_config.
325 * Also creates an binary entry in /sys/bus/nvmem/devices/dev-name/nvmem
326 *
327 * @config: nvmem device configuration with which nvmem device is created.
328 *
329 * Return: Will be an ERR_PTR() on error or a valid pointer to nvmem_device
330 * on success.
331 */
332
333 struct nvmem_device *nvmem_register(const struct nvmem_config *config)
334 {
335 struct nvmem_device *nvmem;
336 int rval;
337
338 if (!config->dev)
339 return ERR_PTR(-EINVAL);
340
341 nvmem = kzalloc(sizeof(*nvmem), GFP_KERNEL);
342 if (!nvmem)
343 return ERR_PTR(-ENOMEM);
344
345 rval = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
346 if (rval < 0)
347 goto err_free_nvmem;
348 if (config->wp_gpio)

 > 349			nvmem->wp_gpio = gpiod_ref(config->wp_gpio);

350 else
351 nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
352 GPIOD_OUT_HIGH);
353 if (IS_ERR(nvmem->wp_gpio))
354 goto err_ida_remove;
355
356
357 kref_init(&nvmem->refcnt);
358 INIT_LIST_HEAD(&nvmem->cells);
359
360 nvmem->id = rval;
361 nvmem->owner = config->owner;
362 if (!nvmem->owner && config->dev->driver)
363 nvmem->owner = config->dev->driver->owner;
364 nvmem->stride = config->stride ?: 1;
365 nvmem->word_size = config->word_size ?: 1;
366 nvmem->size = config->size;
367 nvmem->dev.type = &nvmem_provider_type;
368 nvmem->dev.bus = &nvmem_bus_type;
369 nvmem->dev.parent = config->dev;
370 nvmem->priv = config->priv;
371 nvmem->type = config->type;
372 nvmem->reg_read = config->reg_read;
373 nvmem->reg_write = config->reg_write;
374 if (!config->no_of_node)
375 nvmem->dev.of_node = config->dev->of_node;
376
377 if (config->id == -1 && config->name) {
378 dev_set_name(&nvmem->dev, "%s", config->name);
379 } else {
380 dev_set_name(&nvmem->dev, "%s%d",
381 config->name ? : "nvmem",
382 config->name ? config->id : nvmem->id);
383 }
384
385 nvmem->read_only = device_property_present(config->dev, "read-only") ||
386 config->read_only || !nvmem->reg_write;
387
388 nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config);
389
390 device_initialize(&nvmem->dev);
391
392 dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
393
394 rval = device_add(&nvmem->dev);
395 if (rval)
396 goto err_put_device;
397
398 if (config->compat) {
399 rval = nvmem_sysfs_setup_compat(nvmem, config);
400 if (rval)
401 goto err_device_del;
402 }
403
404 if (config->cells) {
405 rval = nvmem_add_cells(nvmem, config->cells, config->ncells);
406 if (rval)
407 goto err_teardown_compat;
408 }
409
410 rval = nvmem_add_cells_from_table(nvmem);
411 if (rval)
412 goto err_remove_cells;
413
414 rval = nvmem_add_cells_from_of(nvmem);
415 if (rval)
416 goto err_remove_cells;
417
418 blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
419
420 return nvmem;
421
422 err_remove_cells:
423 nvmem_device_remove_all_cells(nvmem);
424 err_teardown_compat:
425 if (config->compat)
426 nvmem_sysfs_remove_compat(nvmem, config);
427 err_device_del:
428 device_del(&nvmem->dev);
429 err_put_device:
430 put_device(&nvmem->dev);
431 err_ida_remove:
432 ida_simple_remove(&nvmem_ida, nvmem->id);
433 err_free_nvmem:
434 kfree(nvmem);
435
436 return ERR_PTR(rval);
437 }
438 EXPORT_SYMBOL_GPL(nvmem_register);
439

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

author
#12 lore raw

PATCH v2 5/7 nvmem: release the write-protect pin

In reply to Bartosz Golaszewski

From: Khouloud Touil <[email protected]>

Put the write-protect GPIO descriptor in nvmem_release() and in the error path in nvmem_register().

Fixes 2a127da461a9 ("nvmem: add support for the write-protect pin")
Reported-by [email protected]
Signed-off-by Khouloud Touil <[email protected]>

[Bartosz:
- also put the gpio in error path in nvmem_register(),
- tweak the commit message: the GPIO is not auto-released]

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/nvmem/core.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/nvmem/core.c +2 -0
index c4a6c044b020..e47152e9db34 100644
@@ -72,6 +72,7 @@ static void nvmem_release(struct device *dev)
 	struct nvmem_device *nvmem = to_nvmem_device(dev);
 
 	ida_simple_remove(&nvmem_ida, nvmem->id);
+	gpiod_put(nvmem->wp_gpio);
 	kfree(nvmem);
 }
 

@@ -428,6 +429,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	device_del(&nvmem->dev);
 err_put_device:
 	put_device(&nvmem->dev);
+	gpiod_put(nvmem->wp_gpio);
 err_ida_remove:
 	ida_simple_remove(&nvmem_ida, nvmem->id);
 err_free_nvmem:
Signature · 1 line
2.25.0
#13 lore raw

Re: [PATCH v2 5/7] nvmem: release the write-protect pin

drivers/nvmem/core.c struct nvmem_device *nvmem_register(const struct nvmem_config *config) · line 435

Quoted 8 lines · Bartosz Golaszewski
drivers/nvmem/core.c +1 -0
@@ -428,6 +429,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
  	device_del(&nvmem->dev);
  err_put_device:
  	put_device(&nvmem->dev);
+	gpiod_put(nvmem->wp_gpio);
  err_ida_remove:
  	ida_simple_remove(&nvmem_ida, nvmem->id);
  err_free_nvmem:

This is also redundant as explained in my previous comments about release()

--srini

Quoted 1 line
author
#14 lore raw

PATCH v2 6/7 nvmem: remove a stray newline in nvmem_register()

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Two newlines are unnecessary - remove one.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/nvmem/core.c | 1 -
 1 file changed, 1 deletion(-)
drivers/nvmem/core.c +0 -1
index e47152e9db34..5e7d46eccaf6 100644
@@ -354,7 +354,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	if (IS_ERR(nvmem->wp_gpio))
 		goto err_ida_remove;
 
-
 	kref_init(&nvmem->refcnt);
 	INIT_LIST_HEAD(&nvmem->cells);
Signature · 1 line
2.25.0
author
#15 lore raw

PATCH v2 7/7 nvmem: add a newline for readability

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Visibly separate the GPIO request from the previous operation in the code with a newline.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/nvmem/core.c | 1 +
 1 file changed, 1 insertion(+)
drivers/nvmem/core.c +1 -0
index 5e7d46eccaf6..ec955db4673c 100644
@@ -346,6 +346,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	rval  = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
 	if (rval < 0)
 		goto err_free_nvmem;
+
 	if (config->wp_gpio)
 		nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
 	else
Signature · 1 line
2.25.0