[3/3] gpio: TODO: track the removal of GPIOD_FLAGS_BIT_NONEXCLUSIVE

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This flag should be replaced by a better mechanism that counts the users
and properly manages the resources. The pwrseq subsystem is a good
candidate. GPIOs themselves should remain a unique resource. Add a task
for tracking the removal of GPIOD_FLAGS_BIT_NONEXCLUSIVE.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/TODO | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

On Tue, Apr 1, 2025 at 10:57 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> > If several providers with their own struct device is using one
> > and the same GPIO line, the devres consumer is unclear: which
> > struct device should own the GPIO line?
> >
>
> Well, other subsystems just use reference-counted resources in this
> case but see above - this is not a good fit for GPIOs.

So to rehash, for example clocks and regulators are by definition the
equivalent to NONEXCLUSIVE, that is their default behaviour.

Two devices can surely request the same clock.

They can independently issue clk_enable() and clk_disable(),
and the semantics is a reference count increase/decrease.

They can have the same phandle in the device tree.

But GPIOs can not. They can only have one owner.

Technically this is because the only reference count we have in a gpio
descriptor is the boolean flag FLAG_REQUESTED, and it
happens like this in gpiod_request_commit():

        if (test_and_set_bit(FLAG_REQUESTED, &desc->flags))
                return -EBUSY;

This semantic is in a way natural because what would you do when
two owners make something a GPIO cannot do, such as
one does gpiod_set_value(1) and the other does gpiod_set_value(0)?

This issue does not exist in resources such as clocks or
regulators that only do enable/disable and that is why GPIOs
are different from other resources.

Then we can think of solutions to that.

One way would be to add a new type of refcounted GPIO
descriptor for this specific usecase, like (OTOMH):

struct gpio_desc_shared {
    struct gpio_desc *gpiod;
    struct device *devs[MAX_OWNERS];
    u32 use_count;
};

struct gpio_desc_shared *gpiod_shared_get(struct device *dev ...);
void gpiod_shared_put(struct gpio_desc_shared *gds);

int gpiod_shared_enable(struct gpio_desc_shared *gds);
int gpiod_shared_disable(struct gpio_desc_shared *gds);

So this compound struct will not be able to set value
directly.

The gpiod inside that shared descriptor need to be obtained with
some gpiolib-internal quirk, not with gpiod_request().

It will issue gpiod_set_value(1) on the first enable and
gpiod_set_value(0) on last disable its internal descriptor.

If existing valid users are switched over to that then the
NONEXCLUSIVE stuff can be deleted.

Yours,
Linus Walleij

On Fri, Apr 4, 2025 at 11:02 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Apr 1, 2025 at 10:57 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > > If several providers with their own struct device is using one
> > > and the same GPIO line, the devres consumer is unclear: which
> > > struct device should own the GPIO line?
> > >
> >
> > Well, other subsystems just use reference-counted resources in this
> > case but see above - this is not a good fit for GPIOs.
>
> So to rehash, for example clocks and regulators are by definition the
> equivalent to NONEXCLUSIVE, that is their default behaviour.
>
> Two devices can surely request the same clock.
>
> They can independently issue clk_enable() and clk_disable(),
> and the semantics is a reference count increase/decrease.
>
> They can have the same phandle in the device tree.
>
> But GPIOs can not. They can only have one owner.
>
> Technically this is because the only reference count we have in a gpio
> descriptor is the boolean flag FLAG_REQUESTED, and it
> happens like this in gpiod_request_commit():
>
>         if (test_and_set_bit(FLAG_REQUESTED, &desc->flags))
>                 return -EBUSY;
>
> This semantic is in a way natural because what would you do when
> two owners make something a GPIO cannot do, such as
> one does gpiod_set_value(1) and the other does gpiod_set_value(0)?
>

Or even better: one goes gpiod_direction_output(desc, 1), the other
goes gpiod_direction_input()!

One goes gpiod_set_config(OPEN_DRAIN), the other gpiod_set_config(OPEN_SOURCE)!!

There's just no good way of allowing multiple users to work with the
same line in a safe and sane way.

> This issue does not exist in resources such as clocks or
> regulators that only do enable/disable and that is why GPIOs
> are different from other resources.
>
> Then we can think of solutions to that.
>
> One way would be to add a new type of refcounted GPIO
> descriptor for this specific usecase, like (OTOMH):
>
> struct gpio_desc_shared {
>     struct gpio_desc *gpiod;
>     struct device *devs[MAX_OWNERS];
>     u32 use_count;
> };
>
> struct gpio_desc_shared *gpiod_shared_get(struct device *dev ...);
> void gpiod_shared_put(struct gpio_desc_shared *gds);
>
> int gpiod_shared_enable(struct gpio_desc_shared *gds);
> int gpiod_shared_disable(struct gpio_desc_shared *gds);
>
> So this compound struct will not be able to set value
> directly.
>
> The gpiod inside that shared descriptor need to be obtained with
> some gpiolib-internal quirk, not with gpiod_request().
>
> It will issue gpiod_set_value(1) on the first enable and
> gpiod_set_value(0) on last disable its internal descriptor.
>
> If existing valid users are switched over to that then the
> NONEXCLUSIVE stuff can be deleted.
>

I don't agree with this. I could possibly live with that being used
exclusively in lower-level core subsystem code (for instance:
regulator/core.c) but, in this form, this still requires drivers - who
have no business knowing whether the GPIO they use is shared - to use
the right API. Not to mention that once you make an interface
available, people will be very eager to abuse it. IMO this should be
approached from the other side.

The closest thing to making the sharing opaque to consumers is
providing a pwrseq-backed, faux GPIO chip that allows a very limited
set of operations on GPIOs - get, get_value, set_value - and return an
error on others. A value set would actually be equivalent to "enable
high" and be refcounted by pwrseq. I have something in mind but this
cycle, I already have a lot on my plate. I will get to it eventually
and come up with some code to back my idea.

In any case: the GPIO sharing logic should be hidden, I just need to
figure out how to make it possible to be notified about when the value
change actually happens as per Mark's requirement.

Let me reiterate: a random ethernet PHY driver should not have to call
gpiod_get_shared() or anything similar - why would it? It can be used
in all kinds of situations, whether the GPIO is shared is none of its
business.

Bartosz

On Tue, Apr 15, 2025 at 10:44 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Mon, Apr 7, 2025 at 2:38 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > I don't agree with this. I could possibly live with that being used
> > exclusively in lower-level core subsystem code (for instance:
> > regulator/core.c) but, in this form, this still requires drivers - who
> > have no business knowing whether the GPIO they use is shared - to use
> > the right API. Not to mention that once you make an interface
> > available, people will be very eager to abuse it. IMO this should be
> > approached from the other side.
> >
> > The closest thing to making the sharing opaque to consumers is
> > providing a pwrseq-backed, faux GPIO chip that allows a very limited
> > set of operations on GPIOs - get, get_value, set_value - and return an
> > error on others. A value set would actually be equivalent to "enable
> > high" and be refcounted by pwrseq. I have something in mind but this
> > cycle, I already have a lot on my plate. I will get to it eventually
> > and come up with some code to back my idea.
> >
> > In any case: the GPIO sharing logic should be hidden, I just need to
> > figure out how to make it possible to be notified about when the value
> > change actually happens as per Mark's requirement.
> >
> > Let me reiterate: a random ethernet PHY driver should not have to call
> > gpiod_get_shared() or anything similar - why would it? It can be used
> > in all kinds of situations, whether the GPIO is shared is none of its
> > business.
>
> I get your point, it's just that I don't see how pwrseq solves it, so I
> would have to see it.
>
> I think a bit of my problem is with the name, as in how would a
> power seqeunce solve the problem of a GPIO that is shared for
> something not power or reset for example.
>

Sigh... It may end up being the same story as with BPF. I named it
pwrseq because a power sequence was what I was working with but it
also deals with shared resources.

> But maybe all the existing cases we have are shared power or
> reset :D
>

No, unfortunately not, though for the cases of shared reset-gpios we
now have the reset-gpio.c driver and its implicit sharing of GPIOs.

> I could think of a shared LED GPIO (this LED should be on if
> any consumers A...X are active) but I just made that up.
>

Actually, I have something in mind that may allow me to avoid using
pwrseq. Give me a couple weeks though as I have other priorities ATM.

Bart