Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Tue, Dec 12, 2023 at 3:32 PM Aaro Koskinen <aaro.koskinen@xxxxxx> wrote:
>
> Hi,
>
> On Tue, Dec 12, 2023 at 12:00:11PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Dec 12, 2023 at 11:12 AM Aaro Koskinen <aaro.koskinen@xxxxxx> wrote:
> > > On Mon, Dec 11, 2023 at 04:49:43PM +0100, Bartosz Golaszewski wrote:
> > > > Aaro: do you still have the HW to test this driver?
> > >
> > > Yes, and I still use it.
> > >
> > > > I understand the need to disable interrupts during writing or reading
> > > > data - when we are driving the clock line. But do we also absolutely
> > > > need to hold the spinlock when setting the direction of the data line
> > > > to input? Because if we don't then we could modify the last remaining
> > > > offender to not set GPIO direction with a spinlock held and finally make
> > > > gpiod_direction_*() functions sleepable.
> > >
> > > Hmm, I think it's required to be able to provide atomic xfer function.
> > > That is needed for e.g. for power off.
> >
> > By xfer: do you mean a request-response pair? Or just a single atomic,
> > one-way transfer of data?
>
> It's reading a register, then writing it back.
>
> A.

Well, I would consider it a valid use-case of changing line direction
where it can't sleep.

Maybe we need to have gpio_direction_input/output_raw_atomic() which
would have very simple semantics? Like no locking provided by gpiolib,
no additional flags interpreted (like open-drain, open-source, bias,
etc.) and no mutex held? It would be up to users who *really* need to
set direction in non-sleeping context to handle this correctly.

We could use it in this driver and possibly in any other which would
fail once we mark the original functions as "might_sleep()".

Bart