[PATCH 0/3] gpiolib: add reference counting to GPIO descriptors

[PATCH 0/3] gpiolib: add reference counting to GPIO descriptors

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This was: nvmem/gpio: fix resource management.

These are the remaining patches from the series aimed at fixing resource
management problems in nvmem.

The first one adds a new descriptor validation macro. The second uses kref to
add reference counting to GPIO descriptors. The last one fixes a potential
use-after-free problem in nvmem.

The last change should actually go into v5.6 but it depends on a new feature.
I'm not sure how to go about applying this.

Changes in patch 2/3 since the last submission:
- add a stub for when GPIOLIB is not selected
- use might_sleep() in gpiod_put() as we may not necessarily call gpiod_free()
  every time now (where it would have been called normally) depending on the
  reference count

Bartosz Golaszewski (3):
  gpiolib: provide VALIDATE_DESC_PTR() macro
  gpiolib: use kref in gpio_desc
  nvmem: increase the reference count of a gpio passed over config

 drivers/gpio/gpiolib.c        | 48 ++++++++++++++++++++++++++++++++---
 drivers/gpio/gpiolib.h        |  1 +
 drivers/nvmem/core.c          |  2 +-
 include/linux/gpio/consumer.h |  9 +++++++
 4 files changed, 55 insertions(+), 5 deletions(-)

-- 
2.25.0

[PATCH 1/3] gpiolib: provide VALIDATE_DESC_PTR() macro

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4d0106ceeba7..da8ffd40aa97 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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

[PATCH 2/3] gpiolib: use kref in gpio_desc

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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 <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c        | 40 +++++++++++++++++++++++++++++++----
 drivers/gpio/gpiolib.h        |  1 +
 include/linux/gpio/consumer.h |  9 ++++++++
 3 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index da8ffd40aa97..e97ea1e113a1 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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,41 @@ 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);
+	might_sleep();
+
+	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
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 3e0aab2945d8..51a92c43dd55 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -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);
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index bf2d017dd7b7..02b136884923 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -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);
 
@@ -237,6 +238,14 @@ gpiod_get_array_optional(struct device *dev, const char *con_id,
 	return NULL;
 }
 
+static inline struct gpio_desc *gpiod_ref(struct gpio_desc *desc)
+{
+	/* GPIO can never have been requested */
+	WARN_ON(desc);
+
+	return NULL;
+}
+
 static inline void gpiod_put(struct gpio_desc *desc)
 {
 	might_sleep();
-- 
2.25.0

[PATCH 3/3] nvmem: increase the reference count of a gpio passed over config

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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 <bgolaszewski@baylibre.com>
---
 drivers/nvmem/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index c05c4f4a7b9e..790ec9b5552e 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -350,7 +350,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

Re: [PATCH 2/3] gpiolib: use kref in gpio_desc

[Bartosz Golaszewski]

pon., 24 lut 2020 o 10:42 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> 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 <bgolaszewski@baylibre.com>

Linus,

This is in response to your suggestion under the previous version of this patch.

I refreshed my memory on device links and reference counting. I think
that device links are not the right tool for the problem I'm trying to
solve. You're right on the other hand about the need for reference
counting of gpiochip devices. Right now if we remove the chip with
GPIOs still requested the only thing that happens is a big splat:
"REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED".

We should probably have a kref on the gpiochip structure which would
be set to 1 when registering the chip, increased and decreased on
every operation such as requesting and releasing a GPIO respectively
and decreased by gpiochip_remove() too. That way if we call
gpiochip_remove() while some users are still holding GPIO descriptors
then the only thing that happens is: the reference count for this
gpiochip is decreased. Once the final consumer calls the appropriate
release routine and the reference count goes to 0, we'd call the
actual gpiochip release code. This is similar to what the clock
framework does IIRC.

This patch however addresses a different issue: I'd like to add
reference counting to descriptors associated with GPIOs requested by
consumers. The kref release function would not trigger a destruction
of the gpiochip - just releasing of the requested GPIO. In this
particular use-case: we can pass an already requested GPIO descriptor
to nvmem. I'd like the nvmem framework to be able to reference it and
then drop the reference once it's done with the line, so that the life
of this resource is not controlled only by the entity that initially
requested it.

In other words: we could use two kref objects: one for the gpiochip
and one for GPIO descriptors.

I hope that makes it more clear.

Best regards,
Bartosz

Re: [PATCH 2/3] gpiolib: use kref in gpio_desc

[Linus Walleij]

On Thu, Mar 5, 2020 at 5:49 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> I refreshed my memory on device links and reference counting. I think
> that device links are not the right tool for the problem I'm trying to
> solve.

OK, just check the below though so we are doing reference
counting in the right place.

> You're right on the other hand about the need for reference
> counting of gpiochip devices. Right now if we remove the chip with
> GPIOs still requested the only thing that happens is a big splat:
> "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED".
>
> We should probably have a kref on the gpiochip structure which would
> be set to 1 when registering the chip, increased and decreased on
> every operation such as requesting and releasing a GPIO respectively
> and decreased by gpiochip_remove() too. That way if we call
> gpiochip_remove() while some users are still holding GPIO descriptors
> then the only thing that happens is: the reference count for this
> gpiochip is decreased. Once the final consumer calls the appropriate
> release routine and the reference count goes to 0, we'd call the
> actual gpiochip release code. This is similar to what the clock
> framework does IIRC.

I don't think that is consistent with the device model: there is already
a struct device inside struct gpio_device which is what gets
created when the gpio_chip is registered.

The struct device inside struct gpio_device contains a
struct kobject.

The struct kobject contains a struct kref.

This kref is increased and decreased with get_device() and
put_device(). This is why in the gpiolib you have a bunch of
this:
get_device(&gdev->dev);
put_device(&gdev->dev);

This is used when creating any descriptor handle with
[devm_]gpiod_request(), linehandles or lineevents.

So it is already reference counted and there is no need to
introduce another reference counter on gpio_chips.

The reason why gpiochip_remove() right now
enforces removal and only prints a warning if you remove
a gpio_chip with requested GPIOs on it, is historical.

When I created the proper device and char device, the gpiolib
was really just a library (hence the name) not a driver framework.
Thus there was no real reference counting of anything
going on, and it was (as I perceived it) pretty common that misc
platforms just pulled out the GPIO chip underneath the drivers
using the GPIO lines.

If we would just block that, say refuse to perform the .remove
action on the module or platform (boardfile) code implementing
GPIO I was worried that we could cause serious regressions.

But I do not think this is a big problem?

Most drivers these days are using devm_gpiochip_add_data()
and that will not release the gpiochip until exactly this same
kref inside struct device inside gpio_device goes down to
zero.

If we should put effort anywhere I think it should be in
making more drivers use devm_gpiochip_add_data()
even if they are not adding any data, because that will make
sure the gpio_chip is not getting removed as long as there are
active consumers (which includes any kernel-internal
consumers or userspace consumers).

> This patch however addresses a different issue: I'd like to add
> reference counting to descriptors associated with GPIOs requested by
> consumers. The kref release function would not trigger a destruction
> of the gpiochip - just releasing of the requested GPIO. In this
> particular use-case: we can pass an already requested GPIO descriptor
> to nvmem. I'd like the nvmem framework to be able to reference it and
> then drop the reference once it's done with the line, so that the life
> of this resource is not controlled only by the entity that initially
> requested it.
>
> In other words: we could use two kref objects: one for the gpiochip
> and one for GPIO descriptors.

I do not think we need one for the gpiochip.

The other usecase I am somewhat following, but I am still in
a state of confusion on what is the best approach there.
I guess I have to re-read the patch.

Yours,
Linus Walleij

Re: [PATCH 2/3] gpiolib: use kref in gpio_desc

[Linus Walleij]

Hi Bartosz,

I'm struggling to figure out if this is the right way to count
references for gpio descriptors.

I cleared up the situation of why we don't want to add kref
to gpio_chip in the previous message: I think we got that covered.
(If I'm not wrong about it, and I am frequently wrong.)

This mail is about contrasting the suggested gpio_desc
kref with the existing managed resources, i.e. the
devm_* mechanisms.

devm_* macros are elusive because they do not use
reference counting at all.

Instead they put every devm_* requested resource with
a destruction function on a list associated with the struct
device. Functions get put on that list when we probe a
device driver, and the list is iterated and all release functions
are called when we exit .probe() with error or after calling the
optional .remove() function on the module. (More or less.)

This means anything devm_* managed lives and dies
with the device driver attaching to the device.
Documentation/driver-api/driver-model/devres.rst

If the intention of the patch is that this action is associated
with the detachment of the driver, then we are reinventing
the wheel we already invented.

E.g. to devm_* it doesn't really matter if someone else is
using a struct gpio_desc, or not, but if the current driver
is using it, it will be kept around until that driver detaches.
No reference counting needed for that.

So is this related to your problem or do I just get things
wrong?

Yours,
Linus Walleij

Re: [PATCH 2/3] gpiolib: use kref in gpio_desc

[Bartosz Golaszewski]

czw., 12 mar 2020 o 11:11 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Thu, Mar 5, 2020 at 5:49 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > I refreshed my memory on device links and reference counting. I think
> > that device links are not the right tool for the problem I'm trying to
> > solve.
>
> OK, just check the below though so we are doing reference
> counting in the right place.
>
> > You're right on the other hand about the need for reference
> > counting of gpiochip devices. Right now if we remove the chip with
> > GPIOs still requested the only thing that happens is a big splat:
> > "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED".
> >
> > We should probably have a kref on the gpiochip structure which would
> > be set to 1 when registering the chip, increased and decreased on
> > every operation such as requesting and releasing a GPIO respectively
> > and decreased by gpiochip_remove() too. That way if we call
> > gpiochip_remove() while some users are still holding GPIO descriptors
> > then the only thing that happens is: the reference count for this
> > gpiochip is decreased. Once the final consumer calls the appropriate
> > release routine and the reference count goes to 0, we'd call the
> > actual gpiochip release code. This is similar to what the clock
> > framework does IIRC.
>
> I don't think that is consistent with the device model: there is already
> a struct device inside struct gpio_device which is what gets
> created when the gpio_chip is registered.
>
> The struct device inside struct gpio_device contains a
> struct kobject.
>
> The struct kobject contains a struct kref.
>
> This kref is increased and decreased with get_device() and
> put_device(). This is why in the gpiolib you have a bunch of
> this:
> get_device(&gdev->dev);
> put_device(&gdev->dev);
>
> This is used when creating any descriptor handle with
> [devm_]gpiod_request(), linehandles or lineevents.
>
> So it is already reference counted and there is no need to
> introduce another reference counter on gpio_chips.
>

I think there's one significant detail missing here. While it's true
that the life-time of a device object is controlled by its reference
count, its registration with the driver model is not ie.
device_add/del() are called once per device as opposed to
get/put_device().

> The reason why gpiochip_remove() right now
> enforces removal and only prints a warning if you remove
> a gpio_chip with requested GPIOs on it, is historical.
>

Given the above I think that it wouldn't be possible to add reference
counting to gpio devices without a new kref if the task of the release
callback would be to call device_del() once the provider called its
unregister function and all consumers released requested resources.

> When I created the proper device and char device, the gpiolib
> was really just a library (hence the name) not a driver framework.
> Thus there was no real reference counting of anything
> going on, and it was (as I perceived it) pretty common that misc
> platforms just pulled out the GPIO chip underneath the drivers
> using the GPIO lines.
>
> If we would just block that, say refuse to perform the .remove
> action on the module or platform (boardfile) code implementing
> GPIO I was worried that we could cause serious regressions.
>
> But I do not think this is a big problem?
>
> Most drivers these days are using devm_gpiochip_add_data()
> and that will not release the gpiochip until exactly this same
> kref inside struct device inside gpio_device goes down to
> zero.
>

I believe this is not correct. The resources managed by devres are
released when the device is detached from a driver, not when the
device's reference count goes to 0. When the latter happens, the
device's specific (or its device_type's) release callback is called -
for gpiolib this is gpiodevice_release().

The kref inside struct device will not go down to zero until you call
device_del() (if you previously called device_add() that is which
increases the reference count by a couple points). But what I'm
thinking about is making the call to device_del() depend not on the
call to gpiochip_remove() but on the kref on the gpio device going
down to zero. As for the protection against module removal - this
should be handled by module_get/put().

I may be wrong of course but when looking at the code, this is what I
understand. Please let me know what you think.

Best regards
Bartosz Golaszewski

Re: [PATCH 2/3] gpiolib: use kref in gpio_desc

[Linus Walleij]

On Thu, Mar 12, 2020 at 7:25 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:

> I believe this is not correct. The resources managed by devres are
> released when the device is detached from a driver, not when the
> device's reference count goes to 0. When the latter happens, the
> device's specific (or its device_type's) release callback is called -
> for gpiolib this is gpiodevice_release().

Yeah you're right, I even point that out in my second letter :/

It's a bit of confusion for everyone (or it's just me).

> The kref inside struct device will not go down to zero until you call
> device_del() (if you previously called device_add() that is which
> increases the reference count by a couple points). But what I'm
> thinking about is making the call to device_del() depend not on the
> call to gpiochip_remove() but on the kref on the gpio device going
> down to zero. As for the protection against module removal - this
> should be handled by module_get/put().

Right. At the end of gpiochip_remove():

   cdev_device_del(&gdev->chrdev, &gdev->dev);
   put_device(&gdev->dev);

That last put_device() should in best case bring the refcount
to zero.

So the actual way we lifecycle GPIO chips is managed
resources using only devm_* but the reference count does work
too: reference count should normally land at zero since the
gpiochip_remove() call is ended with a call to
put_device() and that should (ideally) bring it to zero.

It's just that this doesn't really trigger anything.

I think there is no way out of the fact that we have to
forcefully remove the gpio_chip when devm_* destructors
kicks in: the driver is indeed getting removed at that
point.

In gpiochip_remove() we "numb" the chip so that any
gpio_desc:s currently in use will just fail silently and not crash,
since they are not backed by a driver any more. The descs
stay around until the consumer releases them, but if we probe the
same GPIO device again they will certainly not re-attach or
something.

Arguably it is a bit of policy. Would it make more sense to
have rmmod fail if the kref inside gdev->dev->kobj->kref
is != 1? I suppose that is what things like storage
drivers pretty much have to do.

The problem with that is that as soon as you have a consumer
that is compiled into the kernel it makes it impossible to
remove the gpio driver with rmmod.

I really needed to refresh this a bit, so the above is maybe
a bit therapeutic.

I don't really see how we could do things differently without
creating some other problem though.

Yours,
Linus Walleij

Re: [PATCH 2/3] gpiolib: use kref in gpio_desc

[Bartosz Golaszewski]

czw., 12 mar 2020 o 11:35 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> Hi Bartosz,
>
> I'm struggling to figure out if this is the right way to count
> references for gpio descriptors.
>
> I cleared up the situation of why we don't want to add kref
> to gpio_chip in the previous message: I think we got that covered.
> (If I'm not wrong about it, and I am frequently wrong.)
>
> This mail is about contrasting the suggested gpio_desc
> kref with the existing managed resources, i.e. the
> devm_* mechanisms.
>
> devm_* macros are elusive because they do not use
> reference counting at all.
>
> Instead they put every devm_* requested resource with
> a destruction function on a list associated with the struct
> device. Functions get put on that list when we probe a
> device driver, and the list is iterated and all release functions
> are called when we exit .probe() with error or after calling the
> optional .remove() function on the module. (More or less.)
>
> This means anything devm_* managed lives and dies
> with the device driver attaching to the device.
> Documentation/driver-api/driver-model/devres.rst
>
> If the intention of the patch is that this action is associated
> with the detachment of the driver, then we are reinventing
> the wheel we already invented.
>

In this case I was thinking about a situation where we pass a
requested descriptor to some other framework (nvmem in this case)
which internally doesn't know anything about who manages this resource
externally. Now we can of course simply not do anything about it and
expect the user (who passed us the descriptor) to handle the resources
correctly. But what happens if the user releases the descriptor not on
driver detach but somewhere else for whatever reason while nvmem
doesn't know about it? It may try to use the descriptor which will now
be invalid. Reference counting in this case would help IMHO.

Bart

> E.g. to devm_* it doesn't really matter if someone else is
> using a struct gpio_desc, or not, but if the current driver
> is using it, it will be kept around until that driver detaches.
> No reference counting needed for that.
>
> So is this related to your problem or do I just get things
> wrong?

Re: [PATCH 2/3] gpiolib: use kref in gpio_desc

[Bartosz Golaszewski]

pt., 13 mar 2020 o 09:44 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Thu, Mar 12, 2020 at 7:25 PM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
>
> > I believe this is not correct. The resources managed by devres are
> > released when the device is detached from a driver, not when the
> > device's reference count goes to 0. When the latter happens, the
> > device's specific (or its device_type's) release callback is called -
> > for gpiolib this is gpiodevice_release().
>
> Yeah you're right, I even point that out in my second letter :/
>
> It's a bit of confusion for everyone (or it's just me).
>

No, it is confusing and I only recently understood it while trying to
fix a memory leak in nvmem.

> > The kref inside struct device will not go down to zero until you call
> > device_del() (if you previously called device_add() that is which
> > increases the reference count by a couple points). But what I'm
> > thinking about is making the call to device_del() depend not on the
> > call to gpiochip_remove() but on the kref on the gpio device going
> > down to zero. As for the protection against module removal - this
> > should be handled by module_get/put().
>
> Right. At the end of gpiochip_remove():
>
>    cdev_device_del(&gdev->chrdev, &gdev->dev);
>    put_device(&gdev->dev);
>
> That last put_device() should in best case bring the refcount
> to zero.
>
> So the actual way we lifecycle GPIO chips is managed
> resources using only devm_* but the reference count does work
> too: reference count should normally land at zero since the
> gpiochip_remove() call is ended with a call to
> put_device() and that should (ideally) bring it to zero.
>
> It's just that this doesn't really trigger anything.
>

Not necessarily - if the new kref for GPIO device would be detached
from device reference counting and what it would trigger at release is
this:

   cdev_device_del(&gdev->chrdev, &gdev->dev);
   put_device(&gdev->dev);

Or to be even more clear: "getting" the gpiodevice would not be the
same as "getting" a device - in fact only when the gpiodevice kref
goes down to 0 do we put the reference to the device object.

> I think there is no way out of the fact that we have to
> forcefully remove the gpio_chip when devm_* destructors
> kicks in: the driver is indeed getting removed at that
> point.
>

There does seem to be a way around that though: the clock framework
does it by creating a clock "core" object which is reference counted
and if the provider is removed while consumers still hold references
to it, then it does a couple things to "numb" the provider (as you
nicely put it) like replacing all ops callbacks with NULL pointers but
keeps the structure alive until the consumers also give up all their
references.

That being said: I'm not saying this is necessary or even useful. I
started the discussion because I was under the impression I wasn't
clear enough when writing about reference counting for descriptors. If
nobody complains about the current implementation then let's not fix
something that's not broken.

Bartosz

> In gpiochip_remove() we "numb" the chip so that any
> gpio_desc:s currently in use will just fail silently and not crash,
> since they are not backed by a driver any more. The descs
> stay around until the consumer releases them, but if we probe the
> same GPIO device again they will certainly not re-attach or
> something.
>
> Arguably it is a bit of policy. Would it make more sense to
> have rmmod fail if the kref inside gdev->dev->kobj->kref
> is != 1? I suppose that is what things like storage
> drivers pretty much have to do.
>
> The problem with that is that as soon as you have a consumer
> that is compiled into the kernel it makes it impossible to
> remove the gpio driver with rmmod.
>
> I really needed to refresh this a bit, so the above is maybe
> a bit therapeutic.
>
> I don't really see how we could do things differently without
> creating some other problem though.
>
> Yours,
> Linus Walleij

Re: [PATCH 2/3] gpiolib: use kref in gpio_desc

[Bartosz Golaszewski]

pt., 13 mar 2020 o 16:04 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
>
> pt., 13 mar 2020 o 09:44 Linus Walleij <linus.walleij@linaro.org> napisał(a):
> >
> > On Thu, Mar 12, 2020 at 7:25 PM Bartosz Golaszewski
> > <bgolaszewski@baylibre.com> wrote:
> >
> > > I believe this is not correct. The resources managed by devres are
> > > released when the device is detached from a driver, not when the
> > > device's reference count goes to 0. When the latter happens, the
> > > device's specific (or its device_type's) release callback is called -
> > > for gpiolib this is gpiodevice_release().
> >
> > Yeah you're right, I even point that out in my second letter :/
> >
> > It's a bit of confusion for everyone (or it's just me).
> >
>
> No, it is confusing and I only recently understood it while trying to
> fix a memory leak in nvmem.
>
> > > The kref inside struct device will not go down to zero until you call
> > > device_del() (if you previously called device_add() that is which
> > > increases the reference count by a couple points). But what I'm
> > > thinking about is making the call to device_del() depend not on the
> > > call to gpiochip_remove() but on the kref on the gpio device going
> > > down to zero. As for the protection against module removal - this
> > > should be handled by module_get/put().
> >
> > Right. At the end of gpiochip_remove():
> >
> >    cdev_device_del(&gdev->chrdev, &gdev->dev);
> >    put_device(&gdev->dev);
> >
> > That last put_device() should in best case bring the refcount
> > to zero.
> >
> > So the actual way we lifecycle GPIO chips is managed
> > resources using only devm_* but the reference count does work
> > too: reference count should normally land at zero since the
> > gpiochip_remove() call is ended with a call to
> > put_device() and that should (ideally) bring it to zero.
> >
> > It's just that this doesn't really trigger anything.
> >
>
> Not necessarily - if the new kref for GPIO device would be detached
> from device reference counting and what it would trigger at release is
> this:
>
>    cdev_device_del(&gdev->chrdev, &gdev->dev);
>    put_device(&gdev->dev);
>
> Or to be even more clear: "getting" the gpiodevice would not be the
> same as "getting" a device - in fact only when the gpiodevice kref
> goes down to 0 do we put the reference to the device object.
>
> > I think there is no way out of the fact that we have to
> > forcefully remove the gpio_chip when devm_* destructors
> > kicks in: the driver is indeed getting removed at that
> > point.
> >
>
> There does seem to be a way around that though: the clock framework
> does it by creating a clock "core" object which is reference counted
> and if the provider is removed while consumers still hold references
> to it, then it does a couple things to "numb" the provider (as you
> nicely put it) like replacing all ops callbacks with NULL pointers but
> keeps the structure alive until the consumers also give up all their
> references.
>
> That being said: I'm not saying this is necessary or even useful. I
> started the discussion because I was under the impression I wasn't
> clear enough when writing about reference counting for descriptors. If
> nobody complains about the current implementation then let's not fix
> something that's not broken.
>
> Bartosz
>
> > In gpiochip_remove() we "numb" the chip so that any
> > gpio_desc:s currently in use will just fail silently and not crash,
> > since they are not backed by a driver any more. The descs
> > stay around until the consumer releases them, but if we probe the
> > same GPIO device again they will certainly not re-attach or
> > something.
> >
> > Arguably it is a bit of policy. Would it make more sense to
> > have rmmod fail if the kref inside gdev->dev->kobj->kref
> > is != 1? I suppose that is what things like storage
> > drivers pretty much have to do.
> >
> > The problem with that is that as soon as you have a consumer
> > that is compiled into the kernel it makes it impossible to
> > remove the gpio driver with rmmod.
> >
> > I really needed to refresh this a bit, so the above is maybe
> > a bit therapeutic.
> >
> > I don't really see how we could do things differently without
> > creating some other problem though.
> >
> > Yours,
> > Linus Walleij

Hi Linus,

what is your decision on this? Because if we don't merge this, then we
need to make sure nvmem doesn't call gpiod_put() for descriptors it
didn't obtain itself and we should probably fix it this week.

Bart

Re: [PATCH 2/3] gpiolib: use kref in gpio_desc

[Linus Walleij]

On Mon, Mar 23, 2020 at 9:44 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> Hi Linus,
>
> what is your decision on this? Because if we don't merge this, then we
> need to make sure nvmem doesn't call gpiod_put() for descriptors it
> didn't obtain itself and we should probably fix it this week.

I'm simply just overloaded right now, things related to how the world
looks etc. Also Torvalds writes in Documentation/process/management-style.rst
that if someone ask you to make a decision, you are screwed :/

Your decision is as good as mine, I'm not smarter in any
way so if it is urgent send me a pull request for the solution that seems best
to you, I trust you on this.

Yours,
Linus Walleij

Re: [PATCH 2/3] gpiolib: use kref in gpio_desc

[Bartosz Golaszewski]

śr., 25 mar 2020 o 12:16 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Mon, Mar 23, 2020 at 9:44 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > Hi Linus,
> >
> > what is your decision on this? Because if we don't merge this, then we
> > need to make sure nvmem doesn't call gpiod_put() for descriptors it
> > didn't obtain itself and we should probably fix it this week.
>
> I'm simply just overloaded right now, things related to how the world
> looks etc. Also Torvalds writes in Documentation/process/management-style.rst
> that if someone ask you to make a decision, you are screwed :/
>

I see, yeah the times are uncertain for sure. :(

> Your decision is as good as mine, I'm not smarter in any
> way so if it is urgent send me a pull request for the solution that seems best
> to you, I trust you on this.
>

There are no users right now of the broken interface, so I'll just
send a patch with an appropriate comment to Srinivas for the time
being and we can try to fix it in the next release.

Bartosz

Re: [PATCH 2/3] gpiolib: use kref in gpio_desc

[Linus Walleij]

On Fri, Mar 13, 2020 at 3:47 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> czw., 12 mar 2020 o 11:35 Linus Walleij <linus.walleij@linaro.org> napisał(a):

> In this case I was thinking about a situation where we pass a
> requested descriptor to some other framework (nvmem in this case)
> which internally doesn't know anything about who manages this resource
> externally. Now we can of course simply not do anything about it and
> expect the user (who passed us the descriptor) to handle the resources
> correctly. But what happens if the user releases the descriptor not on
> driver detach but somewhere else for whatever reason while nvmem
> doesn't know about it? It may try to use the descriptor which will now
> be invalid. Reference counting in this case would help IMHO.

I'm so confused because I keep believing it is reference counted
elsewhere.

struct gpio_desc *d always comes from the corresponding
struct gpio_device *descs array. This:

struct gpio_device {
        int                     id;
        struct device           dev;
(...)
        struct gpio_desc        *descs;
(...)

This array is allocated in gpiochip_add_data_with_key() like this:

        gdev->descs = kcalloc(chip->ngpio, sizeof(gdev->descs[0]), GFP_KERNEL);

Then it gets free:d in gpiodevice_release():

static void gpiodevice_release(struct device *dev)
{
        struct gpio_device *gdev = dev_get_drvdata(dev);
(...)
        kfree(gdev->descs);
        kfree(gdev);
}

This is the .release function for the gdev->dev, the device inside
struct gpio_device,
i.e. the same device that contains the descs in the first place. So it
is just living
and dying with the struct gpio_device.

struct gpio_device does *NOT* die in the devm_* destructor that gets called
when someone has e.g. added a gpiochip using devm_gpiochip_add_data().

I think the above observation is crucial: the lifetime of struct gpio_chip and
struct gpio_device are decoupled. When the struct gpio_chip dies, that
just "numbs" all gpio descriptors but they stay around along with the
struct gpio_device that contain them until the last
user is gone.

The struct gpio_device reference counted with the call to get_device(&gdev->dev)
in gpiod_request() which is on the path of gpiod_get_[index]().

If a consumer gets a gpio_desc using any gpiod_get* API this gets
increased and it gets decreased at every gpiod_put() or by the
managed resources.

So should you not rather exploit this fact and just add something
like:

void gpiod_reference(struct gpio_desc *desc)
{
    struct gpio_device *gdev;

    VALIDATE_DESC(desc);
    gdev = desc->gdev;
    get_device(&gdev->dev);
}

void gpiod_unreference(struct gpio_desc *desc)
{
    struct gpio_device *gdev;

    VALIDATE_DESC(desc);
    gdev = desc->gdev;
    put_device(&gdev->dev);
}

This should make sure the desc and the backing gpio_device
stays around until all references are gone.

NB: We also issue try_module_get() on the module that drives the
GPIO, which will make it impossible to unload that module while it
has active GPIOs. I think maybe the whole logic inside gpiod_request()
is needed to properly add an extra reference to a gpiod else someone
can (theoretically) pull out the module from below.

Yours,
Linus Walleij

Re: [PATCH 2/3] gpiolib: use kref in gpio_desc

[Bartosz Golaszewski]

czw., 26 mar 2020 o 21:50 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Fri, Mar 13, 2020 at 3:47 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > czw., 12 mar 2020 o 11:35 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> > In this case I was thinking about a situation where we pass a
> > requested descriptor to some other framework (nvmem in this case)
> > which internally doesn't know anything about who manages this resource
> > externally. Now we can of course simply not do anything about it and
> > expect the user (who passed us the descriptor) to handle the resources
> > correctly. But what happens if the user releases the descriptor not on
> > driver detach but somewhere else for whatever reason while nvmem
> > doesn't know about it? It may try to use the descriptor which will now
> > be invalid. Reference counting in this case would help IMHO.
>
> I'm so confused because I keep believing it is reference counted
> elsewhere.
>
> struct gpio_desc *d always comes from the corresponding
> struct gpio_device *descs array. This:
>
> struct gpio_device {
>         int                     id;
>         struct device           dev;
> (...)
>         struct gpio_desc        *descs;
> (...)
>
> This array is allocated in gpiochip_add_data_with_key() like this:
>
>         gdev->descs = kcalloc(chip->ngpio, sizeof(gdev->descs[0]), GFP_KERNEL);
>
> Then it gets free:d in gpiodevice_release():
>
> static void gpiodevice_release(struct device *dev)
> {
>         struct gpio_device *gdev = dev_get_drvdata(dev);
> (...)
>         kfree(gdev->descs);
>         kfree(gdev);
> }
>
> This is the .release function for the gdev->dev, the device inside
> struct gpio_device,
> i.e. the same device that contains the descs in the first place. So it
> is just living
> and dying with the struct gpio_device.
>
> struct gpio_device does *NOT* die in the devm_* destructor that gets called
> when someone has e.g. added a gpiochip using devm_gpiochip_add_data().
>
> I think the above observation is crucial: the lifetime of struct gpio_chip and
> struct gpio_device are decoupled. When the struct gpio_chip dies, that
> just "numbs" all gpio descriptors but they stay around along with the
> struct gpio_device that contain them until the last
> user is gone.
>
> The struct gpio_device reference counted with the call to get_device(&gdev->dev)
> in gpiod_request() which is on the path of gpiod_get_[index]().
>
> If a consumer gets a gpio_desc using any gpiod_get* API this gets
> increased and it gets decreased at every gpiod_put() or by the
> managed resources.
>
> So should you not rather exploit this fact and just add something
> like:
>
> void gpiod_reference(struct gpio_desc *desc)
> {
>     struct gpio_device *gdev;
>
>     VALIDATE_DESC(desc);
>     gdev = desc->gdev;
>     get_device(&gdev->dev);
> }
>
> void gpiod_unreference(struct gpio_desc *desc)
> {
>     struct gpio_device *gdev;
>
>     VALIDATE_DESC(desc);
>     gdev = desc->gdev;
>     put_device(&gdev->dev);
> }
>
> This should make sure the desc and the backing gpio_device
> stays around until all references are gone.
>
> NB: We also issue try_module_get() on the module that drives the
> GPIO, which will make it impossible to unload that module while it
> has active GPIOs. I think maybe the whole logic inside gpiod_request()
> is needed to properly add an extra reference to a gpiod else someone
> can (theoretically) pull out the module from below.
>

Thanks a lot for the detailed explanation. I'll make some time
(hopefully soon) to actually test this path and let you know if it
works as expected.

Best regards,
Bartosz Golaszewski

Re: [PATCH 2/3] gpiolib: use kref in gpio_desc

[Bartosz Golaszewski]

pon., 30 mar 2020 o 16:36 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
>
> czw., 26 mar 2020 o 21:50 Linus Walleij <linus.walleij@linaro.org> napisał(a):
> >
> > On Fri, Mar 13, 2020 at 3:47 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > czw., 12 mar 2020 o 11:35 Linus Walleij <linus.walleij@linaro.org> napisał(a):
> >
> > > In this case I was thinking about a situation where we pass a
> > > requested descriptor to some other framework (nvmem in this case)
> > > which internally doesn't know anything about who manages this resource
> > > externally. Now we can of course simply not do anything about it and
> > > expect the user (who passed us the descriptor) to handle the resources
> > > correctly. But what happens if the user releases the descriptor not on
> > > driver detach but somewhere else for whatever reason while nvmem
> > > doesn't know about it? It may try to use the descriptor which will now
> > > be invalid. Reference counting in this case would help IMHO.
> >
> > I'm so confused because I keep believing it is reference counted
> > elsewhere.
> >
> > struct gpio_desc *d always comes from the corresponding
> > struct gpio_device *descs array. This:
> >
> > struct gpio_device {
> >         int                     id;
> >         struct device           dev;
> > (...)
> >         struct gpio_desc        *descs;
> > (...)
> >
> > This array is allocated in gpiochip_add_data_with_key() like this:
> >
> >         gdev->descs = kcalloc(chip->ngpio, sizeof(gdev->descs[0]), GFP_KERNEL);
> >
> > Then it gets free:d in gpiodevice_release():
> >
> > static void gpiodevice_release(struct device *dev)
> > {
> >         struct gpio_device *gdev = dev_get_drvdata(dev);
> > (...)
> >         kfree(gdev->descs);
> >         kfree(gdev);
> > }
> >
> > This is the .release function for the gdev->dev, the device inside
> > struct gpio_device,
> > i.e. the same device that contains the descs in the first place. So it
> > is just living
> > and dying with the struct gpio_device.
> >
> > struct gpio_device does *NOT* die in the devm_* destructor that gets called
> > when someone has e.g. added a gpiochip using devm_gpiochip_add_data().
> >
> > I think the above observation is crucial: the lifetime of struct gpio_chip and
> > struct gpio_device are decoupled. When the struct gpio_chip dies, that
> > just "numbs" all gpio descriptors but they stay around along with the
> > struct gpio_device that contain them until the last
> > user is gone.
> >
> > The struct gpio_device reference counted with the call to get_device(&gdev->dev)
> > in gpiod_request() which is on the path of gpiod_get_[index]().
> >
> > If a consumer gets a gpio_desc using any gpiod_get* API this gets
> > increased and it gets decreased at every gpiod_put() or by the
> > managed resources.
> >
> > So should you not rather exploit this fact and just add something
> > like:
> >
> > void gpiod_reference(struct gpio_desc *desc)
> > {
> >     struct gpio_device *gdev;
> >
> >     VALIDATE_DESC(desc);
> >     gdev = desc->gdev;
> >     get_device(&gdev->dev);
> > }
> >
> > void gpiod_unreference(struct gpio_desc *desc)
> > {
> >     struct gpio_device *gdev;
> >
> >     VALIDATE_DESC(desc);
> >     gdev = desc->gdev;
> >     put_device(&gdev->dev);
> > }
> >
> > This should make sure the desc and the backing gpio_device
> > stays around until all references are gone.
> >
> > NB: We also issue try_module_get() on the module that drives the
> > GPIO, which will make it impossible to unload that module while it
> > has active GPIOs. I think maybe the whole logic inside gpiod_request()
> > is needed to properly add an extra reference to a gpiod else someone
> > can (theoretically) pull out the module from below.
> >
>
> Thanks a lot for the detailed explanation. I'll make some time
> (hopefully soon) to actually test this path and let you know if it
> works as expected.
>
> Best regards,
> Bartosz Golaszewski

Hi Linus,

So this "numbing down" of the chip works - in that I don't see any
splat in the above use-case but right now if nvmem takes an existing
GPIO descriptor over nvmem_config, then it will call gpiod_put() on it
and we'll do the same in the provider driver leading to the following
warning:

[  109.191755] ------------[ cut here ]------------
[  109.191787] WARNING: CPU: 0 PID: 207 at drivers/gpio/gpiolib.c:3097
release_nodes+0x1ac/0x1f8
[  109.191794] Modules linked in: at24
[  109.191975] CPU: 0 PID: 207 Comm: rmmod Not tainted
5.7.0-rc5-00001-g8c4cd0ae52ce-dirty #12
[  109.191982] Hardware name: Generic AM33XX (Flattened Device Tree)
[  109.192028] [<c01119ec>] (unwind_backtrace) from [<c010b9c0>]
(show_stack+0x10/0x14)
[  109.192050] [<c010b9c0>] (show_stack) from [<c05456b4>]
(dump_stack+0xc0/0xe0)
[  109.192076] [<c05456b4>] (dump_stack) from [<c0138b30>] (__warn+0xc0/0xf8)
[  109.192095] [<c0138b30>] (__warn) from [<c0138ec4>]
(warn_slowpath_fmt+0x58/0xb8)
[  109.192112] [<c0138ec4>] (warn_slowpath_fmt) from [<c0635938>]
(release_nodes+0x1ac/0x1f8)
[  109.192136] [<c0635938>] (release_nodes) from [<c0631d7c>]
(device_release_driver_internal+0xf8/0x1b8)
[  109.192154] [<c0631d7c>] (device_release_driver_internal) from
[<c0631eac>] (driver_detach+0x58/0xa8)
[  109.192172] [<c0631eac>] (driver_detach) from [<c0630ae0>]
(bus_remove_driver+0x4c/0xa4)
[  109.192191] [<c0630ae0>] (bus_remove_driver) from [<c01d80e0>]
(sys_delete_module+0x1bc/0x240)
[  109.192211] [<c01d80e0>] (sys_delete_module) from [<c0100260>]
(__sys_trace_return+0x0/0x20)

I'm not sure how to go about fixing it though. We could check in nvmem
if the descriptor was retrieved locally or passed from the nvmem
provider and the either put it or not respectively but this isn't very
clean.

Bart

Re: [PATCH 2/3] gpiolib: use kref in gpio_desc

[Linus Walleij]

On Thu, May 14, 2020 at 3:42 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> So this "numbing down" of the chip works - in that I don't see any
> splat in the above use-case but right now if nvmem takes an existing
> GPIO descriptor over nvmem_config, then it will call gpiod_put() on it
> and we'll do the same in the provider driver leading to the following
> warning:

Isn't that the WARN_ON(extra_checks) in gpiod_free()?
What part of the if() clause is causing this? I.e.:
if (desc && desc->gdev && gpiod_free_commit(desc)) ...

I suspect gpiod_free_commit() is causing it by returning nonzero.
We could essentially ignore that if and only if the gpio_chip
has been detached from the gpio_device.

This should fix the problem if I'm right.

Yours,
Linus Walleij