Hello Andy,
On Thu, Aug 03, 2023 at 02:51:40PM +0300, Andy Shevchenko wrote:
> On Thu, Aug 03, 2023 at 11:42:12AM +0200, Uwe Kleine-König wrote:
> > On Sun, Jul 30, 2023 at 12:07:33PM +0200, Bartosz Golaszewski wrote:
>
> ...
>
> > - the locking scheme in gpiod_request_commit() looks strange. gpio_lock
> > is released and retaken possibly several times. I wonder what it
> > actually protects there. Maybe doing
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index edab00c9cb3c..496b1cebba58 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2064,13 +2064,11 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
> > goto out_free_unlock;
> > }
> > }
> > + spin_unlock_irqrestore(&gpio_lock, flags);
> > if (gc->get_direction) {
> > /* gc->get_direction may sleep */
> > - spin_unlock_irqrestore(&gpio_lock, flags);
> > gpiod_get_direction(desc);
> > - spin_lock_irqsave(&gpio_lock, flags);
> > }
> > - spin_unlock_irqrestore(&gpio_lock, flags);
> > return 0;
> >
> > out_free_unlock:
> >
> > simplifies the code and given that gpiod_get_direction() rechecks
> > gc->get_direction unlocked I don't think we'd loose anything here.
>
> Wouldn't this break sleeping bus accesses (I2C gpio expanders, etc)?
This question is too short for me to understand what you think. The
only difference my patch does is that gc->get_direction is checked
without holding the lock and a lock+unlock pair. I don't see how this is
relevant to sleeping bus accesses.
lock()
...
if (A) {
unlock()
something()
lock()
}
unlock()
is nearly identical to:
lock()
...
unlock()
if (A) {
something()
}
lock()
unlock()
which in turn is nearly identical to
lock()
...
unlock()
if (A) {
something()
}
. But I might well miss something, as the "nearly"s above sometimes are
relevant.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
