lkml.org 
[lkml]   [2020]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH] gpiolib: don't call sleeping functions with a spinlock taken
    Hi Bartosz,

    On Fri, Mar 20, 2020 at 10:31 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
    > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
    >
    > We must not call pinctrl_gpio_can_use_line() with the gpio_lock taken
    > as it takes a mutex internally. Let's move the call before taking the
    > spinlock and store the return value.
    >
    > This isn't perfect - there's a moment between calling
    > pinctrl_gpio_can_use_line() and taking the spinlock where the situation
    > can change but it isn't a regression either: previously this part wasn't
    > protected at all and it only affects the information user-space is
    > seeing.
    >
    > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
    > Fixes: d2ac25798208 ("gpiolib: provide a dedicated function for setting lineinfo")
    > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

    Thanks for your patch!

    > --- a/drivers/gpio/gpiolib.c
    > +++ b/drivers/gpio/gpiolib.c
    > @@ -1154,8 +1154,19 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
    > struct gpioline_info *info)
    > {
    > struct gpio_chip *chip = desc->gdev->chip;
    > + bool ok_for_pinctrl;
    > unsigned long flags;
    >
    > + /*
    > + * This function takes a mutex so we must check this before taking
    > + * the spinlock.
    > + *
    > + * FIXME: find a non-racy way to retrieve this information. Maybe a
    > + * lock common to both frameworks?
    > + */
    > + ok_for_pinctrl = pinctrl_gpio_can_use_line(
    > + chip->base + info->line_offset);

    Note that this is now called unconditionally, while before it was only called
    if all previous steps in the ||-pipeline failed.

    > +
    > spin_lock_irqsave(&gpio_lock, flags);
    >
    > if (desc->name) {
    > @@ -1182,7 +1193,7 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
    > test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
    > test_bit(FLAG_EXPORT, &desc->flags) ||
    > test_bit(FLAG_SYSFS, &desc->flags) ||
    > - !pinctrl_gpio_can_use_line(chip->base + info->line_offset))
    > + !ok_for_pinctrl)
    > info->flags |= GPIOLINE_FLAG_KERNEL;
    > if (test_bit(FLAG_IS_OUT, &desc->flags))
    > info->flags |= GPIOLINE_FLAG_IS_OUT;

    Gr{oetje,eeting}s,

    Geert

    --
    Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

    In personal conversations with technical people, I call myself a hacker. But
    when I'm talking to journalists I just say "programmer" or something like that.
    -- Linus Torvalds
    \
     
     \ /
      Last update: 2020-03-20 12:00    [W:6.219 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site