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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Thu, Mar 5, 2020 at 5:49 PM Bartosz Golaszewski <brgl@xxxxxxxx> 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