linux-kernel - Re: [PATCH 3/3] gpio: TODO: track the removal of GPIOD_FLAGS_BIT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAMRc=McKgfT1ZLVzsVbBJ5pFc3bwcGT4AyXG+V0JPP0z6ft3tg@mail.gmail.com>
Date: Mon, 7 Apr 2025 14:38:14 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH 3/3] gpio: TODO: track the removal of GPIOD_FLAGS_BIT_NONEXCLUSIVE

On Fri, Apr 4, 2025 at 11:02 AM Linus Walleij <linus.walleij@...aro.org> wrote:
>
> On Tue, Apr 1, 2025 at 10:57 AM Bartosz Golaszewski <brgl@...ev.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