nvmem/gpio: fix resource management

From: Bartosz Golaszewski <[email protected]>

Hi Srinivas,

sorry for the lack of proper QA last time. This time the code is tested on real HW (Beagle Bone Black with an ACME cape) including the error path.

--

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

First we fix a memory leak introduced in this release cycle. 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.

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

v2 -> v3:
- drop incorrect patches
- add a patch adding a comment about resource management
- extend the GPIO kref patch: only increment the reference count if the descriptor is associated with a requested line

v3 -> v4:
- fixed the return value in error path in nvmem_register()
- dropped patches already applied to the nvmem tree
- dropped the patch adding the comment about resource management

v4 -> v5:
- don't reference nvmem once it's freed
- add GPIO descriptor validation in gpiod_ref()

Bartosz Golaszewski (4):
nvmem: fix memory leak in error path
gpiolib: provide VALIDATE_DESC_PTR() macro
gpiolib: use kref in gpio_desc
nvmem: increase the reference count of a gpio passed over config

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

 drivers/gpio/gpiolib.c        | 46 ++++++++++++++++++++++++++++++++---
 drivers/gpio/gpiolib.h        |  1 +
 drivers/nvmem/core.c          | 11 ++++++---
 include/linux/gpio/consumer.h |  1 +
 4 files changed, 52 insertions(+), 7 deletions(-)

2.25.0

From: Bartosz Golaszewski <[email protected]>

We need to free the ida mapping and nvmem struct if the write-protect GPIO lookup fails.

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

index 503da67dde06..2758d90d63b7 100644
@@ -353,8 +353,12 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	else
 		nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
 						    GPIOD_OUT_HIGH);
-	if (IS_ERR(nvmem->wp_gpio))
-		return ERR_CAST(nvmem->wp_gpio);
+	if (IS_ERR(nvmem->wp_gpio)) {
+		ida_simple_remove(&nvmem_ida, nvmem->id);
+		rval = PTR_ERR(nvmem->wp_gpio);
+		kfree(nvmem);
+		return ERR_PTR(rval);
+	}
 
 	kref_init(&nvmem->refcnt);
 	INIT_LIST_HEAD(&nvmem->cells);

2.25.0

Applied this and 5/5

Thanks,
srini

From: Bartosz Golaszewski <[email protected]>

We're about to add a public GPIO function that takes a descriptor as argument and returns a pointer. Add a corresponding macro wrapping the validate_desc() function that returns an ERR_PTR() on error.

---
 drivers/gpio/gpiolib.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index 4d0106ceeba7..da8ffd40aa97 100644
@@ -2864,6 +2864,14 @@ static int validate_desc(const struct gpio_desc *desc, const char *func)
 		return; \
 	} while (0)
 
+#define VALIDATE_DESC_PTR(desc) do { \
+	int __valid = validate_desc(desc, __func__); \
+	if (__valid < 0) \
+		return ERR_PTR(__valid); \
+	if (__valid == 0) \
+		return NULL; \
+	} while (0)
+
 int gpiod_request(struct gpio_desc *desc, const char *label)
 {
 	int ret = -EPROBE_DEFER;

2.25.0

(I expect to get this back as a pull request or something later.)

Yours,
Linus Walleij

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.

---
 drivers/gpio/gpiolib.c        | 38 +++++++++++++++++++++++++++++++----
 drivers/gpio/gpiolib.h        |  1 +
 include/linux/gpio/consumer.h |  1 +
 3 files changed, 36 insertions(+), 4 deletions(-)

index da8ffd40aa97..a00e28ca8a49 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);

@@ -2941,6 +2943,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

@@ -5075,18 +5084,39 @@ 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);
+	VALIDATE_DESC_VOID(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)
+{
+	VALIDATE_DESC_PTR(desc);
+
+	if (!test_bit(FLAG_REQUESTED, &desc->flags)) {
+		pr_warn("gpiolib: unable to increase the reference count of unrequested GPIO descriptor\n");
+		return 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

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

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

2.25.0

I'm having some trouble figuring out if we might be reinventing a wheel here.

A while back there was a proposed patch to add device links between GPIO producers and consumers, so that a GPIO chip won't be dropped while there are active consumers.

(I don't remember who sent the patch.)

We have a similar functionality in pin control if the .link_consumers property is set on the pincontrol device. I was thinking about making that compulsory at one point.

The device links use a kref already existing in struct device and would in this case be the kref in the struct device for the struct gpio_device.

So if that existed, gpiod_ref could just grab another device_link_add().

Maybe we should just add device links between all GPIO consumers (devices) and struct gpio_device:s struct device and implement it like this so we don't have to back out of this later?

C.f. commit
commit 036f394dd77f8117346874151793ec38967d843f
pinctrl: Enable device link creation for pin control

(...)

pt., 28 lut 2020 o 23:33 Linus Walleij <[email protected]> napisał(a):

I was always under the impression that device links are aimed mostly at runtime PM.

Yes, definitely looks like it's done with PM in mind. Maybe we should do what nvmem does? Define a struct device_type for GPIO chips with an appropriate release() callback and use get_device() and put_device()? Although nvmem seems to use kref for cells and device reference counting somewhat separately - maybe that's something to address too.

Yeah this is fixed in the next version (with a different subject since it no longer concerns nvmem that much).

Bart

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.

---
 drivers/nvmem/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 2758d90d63b7..4a4ef2049022 100644
@@ -349,7 +349,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	}
 
 	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);

2.25.0

From: Khouloud Touil <[email protected]>

Put the write-protect GPIO descriptor in nvmem_release() so that it can be automatically released when the associated device's reference count drops to 0.

---
 drivers/nvmem/core.c | 1 +
 1 file changed, 1 insertion(+)

index 4a4ef2049022..790ec9b5552e 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);
 }
 

2.25.0