Message-ID: <CAMRc=McBWncrCcX87a3pYeZ3=uYGNhpSrK74hDP-XNYrT8WMMg@mail.gmail.com>
Date: Tue, 1 Apr 2025 10:57:39 +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 Tue, Apr 1, 2025 at 12:49 AM Linus Walleij <linus.walleij@...aro.org> wrote:
>
> On Mon, Mar 31, 2025 at 11:00 AM Bartosz Golaszewski <brgl@...ev.pl> wrote:
>
> > +Remove GPIOD_FLAGS_BIT_NONEXCLUSIVE
> > +
> > +This flag is an awful workaround that was created for some regulator
> > +corner-cases but got out of hand and is now used in at least 33 places
> > +treewide. Unlike what the intuition may tell users, it's not a reference
> > +counted mechanisms like what clocks or regulators use but just a raw access
> > +to the same GPIO descriptor from multiple places with no synchronization (other
> > +than what the underying driver offers). It doesn't even correctly support
> > +releasing the supposedly non-exclusive GPIOs. This whole thing should go and be
> > +replaced with a better solution - for exampe: using the relatively new power
> > +sequencing subsystem.
>
> Try to focus on the solution instead of writing so much about the problem.
> We mostly get the information that the GPIO maintainer dislikes them,
> not so much about why they exist and what can be done about them.
>

I'm sorry, after a second thought this does indeed sound too harsh but
when I realized that people started slapping this non-exclusive flag
on every problem - like when the GPIO ACPI code requests some GPIOs
(possibly erroneously), so driver authors just request it as
non-exclusive instead of investigating the actual problem, or some
codec drivers in sound/ which seem to not even need it and it looks
like a bad copy-paste, I got really frustrated that it's another thing
on the pile of stuff to fix. I will reword this entry.

> I would describe the actual problem and the actual solution,
> something like:
>
> "A problematic usecase for GPIOs is when two consumers want to use
> the same descriptor independently, as a consumer (in Linux a struct
> device) is generally assumed to have exclusive access to all of its
> resources, with a resource being defined as anything obtained behind
> a devm_* managed resource API such as devm_gpiod_get().
>

This is not a devres issue though. I don't think we should mention it
here. The real problem about this flag is that it effectively allows
two users to "fight" over a line's state. It would be better if it
would count the enable operations but this would go against the spirit
of the "gpiod_set_value()" interface, where you expect the value to be
actually set to what you request it to be. GPIOs should remain
exclusive and any "packaging" should happen in a higher layer.

This is one of those pesky resource ownership issues really.

> Providers such as regulators pose a special problem here since
> regulators instantiate several struct regulator_dev instances containing
> a struct device but using the *same* enable GPIO line: i.e. from a Linux
> point of view this enable line has a many-to-one ownership. You can
> imagine a 12V and a 5V regulator being turned on by the same GPIO
> line for example. The regulator resources need to have two different
> devices in Linux because they have different voltages, but they are enabled
> by the same GPIO.
>
> This breaks the devres resource assumptions:
>

The same thing would happen without devres. I think the regulator
framework should have some way of mediating GPIO use. There should
only really be a single gpiod_get() call for all enable-gpios of a
single regulator provider. I have it on my roadmap to look into it.
Whether the right approach is a GPIO quirk or a power sequence
provider binding to the top-level regulator provider, I don't know
yet.

> 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.

> A hack allows GPIO lines to be shared between several consumers
> with the flag GPIOD_FLAGS_BIT_NONEXCLUSIVE but this API is
> confusing and prone to abuse, as is the related devm_gpiod_unhinge() API.

This is another thing that should most likely be deprecated and
removed. It's clearly a case of papering over unclear ownership of a
resource. I believe that any "hand-over" of ownership is really asking
for trouble. The entity that does the "get" should also do the "put"
in almost all cases. Fortunately it's only limited to a few use-cases
in drivers/regulator/ so it's not nearly as bad. However, I don't
really see a close relationship between these two issues. How about
adding a task for that as well?

>
> A better solution to some of the problems is to use approaches such as
> the power sequencing subsystem, which avoids having several owners of
> a resource by strictly sequencing the order in which they are obtained
> and activated.
>
> For example a GPIO turning on the power for both wireless and bluetooth
> chips can be obtained and turned on in a power sequence such that this
> problem never occurs: it is always active when when it needs to be, it has
> just one owner (the power sequence itself, struct pwrseq_device, which
> contains a struct device) and the GPIO regulator driver is not used in this
> scenario."
>

So this bit is also unrelated - in the case of the wcn pwrseq driver,
there are two separate GPIOs for wifi and bluetooth. They just need a
delay between toggling one and the other. I would skip it.

Bart

Powered by blists - more mailing lists