Re: [RESEND PATCH v6 5/7] gpiolib: provide a dedicated function for setting lineinfo

[Bartosz Golaszewski]

pon., 16 mar 2020 o 17:21 Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> napisaÅ(a):
>
> Hi Bartosz,
>
> On Tue, Feb 11, 2020 at 10:21 AM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
> > From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> > We'll soon be filling out the gpioline_info structure in multiple
> > places. Add a separate function that given a gpio_desc sets all relevant
> > fields.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>
> This is now commit d2ac25798208fb85 ("gpiolib: provide a dedicated
> function for setting lineinfo") in gpio/for-next.
>
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1147,6 +1147,60 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> >         return ret;
> >  }
> >
> > +static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
> > +                                 struct gpioline_info *info)
> > +{
> > +       struct gpio_chip *chip = desc->gdev->chip;
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&gpio_lock, flags);
>
> spinlock taken
>
> > +
> > +       if (desc->name) {
> > +               strncpy(info->name, desc->name, sizeof(info->name));
> > +               info->name[sizeof(info->name) - 1] = '\0';
> > +       } else {
> > +               info->name[0] = '\0';
> > +       }
> > +
> > +       if (desc->label) {
> > +               strncpy(info->consumer, desc->label, sizeof(info->consumer));
> > +               info->consumer[sizeof(info->consumer) - 1] = '\0';
> > +       } else {
> > +               info->consumer[0] = '\0';
> > +       }
> > +
> > +       /*
> > +        * Userspace only need to know that the kernel is using this GPIO so
> > +        * it can't use it.
> > +        */
> > +       info->flags = 0;
> > +       if (test_bit(FLAG_REQUESTED, &desc->flags) ||
> > +           test_bit(FLAG_IS_HOGGED, &desc->flags) ||
> > +           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))
>
> pinctrl_gpio_can_use_line(), and pinctrl_get_device_gpio_range() called
> from it, call mutex_lock():
>
>     BUG: sleeping function called from invalid context at
> kernel/locking/mutex.c:281
>     in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 652, name: lsgpio
>     CPU: 1 PID: 652 Comm: lsgpio Not tainted
> 5.6.0-rc1-koelsch-00008-gd2ac25798208fb85 #755
>     Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
>     [<c020e3f0>] (unwind_backtrace) from [<c020a5b8>] (show_stack+0x10/0x14)
>     [<c020a5b8>] (show_stack) from [<c07d31b4>] (dump_stack+0x88/0xa8)
>     [<c07d31b4>] (dump_stack) from [<c0241318>] (___might_sleep+0xf8/0x168)
>     [<c0241318>] (___might_sleep) from [<c07ec13c>] (mutex_lock+0x24/0x7c)
>     [<c07ec13c>] (mutex_lock) from [<c046f47c>]
> (pinctrl_get_device_gpio_range+0x1c/0xb4)
>     [<c046f47c>] (pinctrl_get_device_gpio_range) from [<c046f5e8>]
> (pinctrl_gpio_can_use_line+0x24/0x88)
>     [<c046f5e8>] (pinctrl_gpio_can_use_line) from [<c0478bd0>]
> (gpio_ioctl+0x270/0x584)
>     [<c0478bd0>] (gpio_ioctl) from [<c03194c0>] (vfs_ioctl+0x20/0x38)
>
> Reproducer is "lsgpio" with CONFIG_DEBUG_ATOMIC_SLEEP=y.
>

Hi Geert,

thanks for the report. I added the locking around this code because it
seemed wrong to me that this part wasn't protected in any way before.
Now the question is how do we deal with this.

Linus: do you think it's safe to move the call to
pinctrl_gpio_can_use_line() before the spinlock is taken? I'd say yes
but I'm not sure if I'm not missing some inter-framework intricacies.

Best regards,
Bartosz Golaszewski