Message ID | 20231219004158.12405-4-warthog618@gmail.com |
---|---|
State | New |
Headers | show |
Series | gpiolib: cdev: relocate debounce_period_us | expand |
On Tue, Dec 19, 2023 at 1:42 AM Kent Gibson <warthog618@gmail.com> wrote: > > Reduce the time holding the gpio_lock by snapshotting the desc flags, > rather than testing them individually while holding the lock. > > Accept that the calculation of the used field is inherently racy, and > only check the availability of the line from pinctrl if other checks > pass, so avoiding the check for lines that are otherwise in use. > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > Reviewed-by: Andy Shevchenko <andy@kernel.org> > --- > drivers/gpio/gpiolib-cdev.c | 74 ++++++++++++++++++------------------- > 1 file changed, 36 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > index aecc4241b6c8..9f5104d7532f 100644 > --- a/drivers/gpio/gpiolib-cdev.c > +++ b/drivers/gpio/gpiolib-cdev.c > @@ -2399,74 +2399,72 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc, > struct gpio_v2_line_info *info) > { > struct gpio_chip *gc = desc->gdev->chip; > - bool ok_for_pinctrl; > - unsigned long flags; > + unsigned long dflags; > > memset(info, 0, sizeof(*info)); > info->offset = gpio_chip_hwgpio(desc); > > - /* > - * 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(gc, info->offset); > + scoped_guard(spinlock_irqsave, &gpio_lock) { > + if (desc->name) > + strscpy(info->name, desc->name, sizeof(info->name)); > > - spin_lock_irqsave(&gpio_lock, flags); > + if (desc->label) > + strscpy(info->consumer, desc->label, > + sizeof(info->consumer)); > > - if (desc->name) > - strscpy(info->name, desc->name, sizeof(info->name)); > - > - if (desc->label) > - strscpy(info->consumer, desc->label, sizeof(info->consumer)); > + dflags = READ_ONCE(desc->flags); > + } > > /* > - * Userspace only need to know that the kernel is using this GPIO so > - * it can't use it. > + * Userspace only need know that the kernel is using this GPIO so it > + * can't use it. > + * The calculation of the used flag is slightly racy, as it may read > + * desc, gc and pinctrl state without a lock covering all three at > + * once. Worst case if the line is in transition and the calculation > + * is inconsistent then it looks to the user like they performed the > + * read on the other side of the transition - but that can always > + * happen. > + * The definitive test that a line is available to userspace is to > + * request 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) || > + if (test_bit(FLAG_REQUESTED, &dflags) || > + test_bit(FLAG_IS_HOGGED, &dflags) || > + test_bit(FLAG_USED_AS_IRQ, &dflags) || > + test_bit(FLAG_EXPORT, &dflags) || > + test_bit(FLAG_SYSFS, &dflags) || > !gpiochip_line_is_valid(gc, info->offset) || > - !ok_for_pinctrl) > + !pinctrl_gpio_can_use_line(gc, info->offset)) > info->flags |= GPIO_V2_LINE_FLAG_USED; > > - if (test_bit(FLAG_IS_OUT, &desc->flags)) > + if (test_bit(FLAG_IS_OUT, &dflags)) > info->flags |= GPIO_V2_LINE_FLAG_OUTPUT; > else > info->flags |= GPIO_V2_LINE_FLAG_INPUT; > > - if (test_bit(FLAG_ACTIVE_LOW, &desc->flags)) > + if (test_bit(FLAG_ACTIVE_LOW, &dflags)) One more nit: you no longer have to use atomic bitops here, you can switch to the ones without guarantees for better performance. Bart > info->flags |= GPIO_V2_LINE_FLAG_ACTIVE_LOW; > > - if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) > + if (test_bit(FLAG_OPEN_DRAIN, &dflags)) > info->flags |= GPIO_V2_LINE_FLAG_OPEN_DRAIN; > - if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) > + if (test_bit(FLAG_OPEN_SOURCE, &dflags)) > info->flags |= GPIO_V2_LINE_FLAG_OPEN_SOURCE; > > - if (test_bit(FLAG_BIAS_DISABLE, &desc->flags)) > + if (test_bit(FLAG_BIAS_DISABLE, &dflags)) > info->flags |= GPIO_V2_LINE_FLAG_BIAS_DISABLED; > - if (test_bit(FLAG_PULL_DOWN, &desc->flags)) > + if (test_bit(FLAG_PULL_DOWN, &dflags)) > info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN; > - if (test_bit(FLAG_PULL_UP, &desc->flags)) > + if (test_bit(FLAG_PULL_UP, &dflags)) > info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_UP; > > - if (test_bit(FLAG_EDGE_RISING, &desc->flags)) > + if (test_bit(FLAG_EDGE_RISING, &dflags)) > info->flags |= GPIO_V2_LINE_FLAG_EDGE_RISING; > - if (test_bit(FLAG_EDGE_FALLING, &desc->flags)) > + if (test_bit(FLAG_EDGE_FALLING, &dflags)) > info->flags |= GPIO_V2_LINE_FLAG_EDGE_FALLING; > > - if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &desc->flags)) > + if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &dflags)) > info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME; > - else if (test_bit(FLAG_EVENT_CLOCK_HTE, &desc->flags)) > + else if (test_bit(FLAG_EVENT_CLOCK_HTE, &dflags)) > info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE; > - > - spin_unlock_irqrestore(&gpio_lock, flags); > } > > struct gpio_chardev_data { > -- > 2.39.2 >
On Tue, Dec 19, 2023 at 10:30 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Tue, Dec 19, 2023 at 1:42 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > Reduce the time holding the gpio_lock by snapshotting the desc flags, > > rather than testing them individually while holding the lock. > > > > Accept that the calculation of the used field is inherently racy, and > > only check the availability of the line from pinctrl if other checks > > pass, so avoiding the check for lines that are otherwise in use. > > > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > > Reviewed-by: Andy Shevchenko <andy@kernel.org> > > --- > > drivers/gpio/gpiolib-cdev.c | 74 ++++++++++++++++++------------------- > > 1 file changed, 36 insertions(+), 38 deletions(-) > > > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > > index aecc4241b6c8..9f5104d7532f 100644 > > --- a/drivers/gpio/gpiolib-cdev.c > > +++ b/drivers/gpio/gpiolib-cdev.c > > @@ -2399,74 +2399,72 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc, > > struct gpio_v2_line_info *info) > > { > > struct gpio_chip *gc = desc->gdev->chip; > > - bool ok_for_pinctrl; > > - unsigned long flags; > > + unsigned long dflags; > > > > memset(info, 0, sizeof(*info)); > > info->offset = gpio_chip_hwgpio(desc); > > > > - /* > > - * 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(gc, info->offset); > > + scoped_guard(spinlock_irqsave, &gpio_lock) { > > + if (desc->name) > > + strscpy(info->name, desc->name, sizeof(info->name)); > > > > - spin_lock_irqsave(&gpio_lock, flags); > > + if (desc->label) > > + strscpy(info->consumer, desc->label, > > + sizeof(info->consumer)); > > > > - if (desc->name) > > - strscpy(info->name, desc->name, sizeof(info->name)); > > - > > - if (desc->label) > > - strscpy(info->consumer, desc->label, sizeof(info->consumer)); > > + dflags = READ_ONCE(desc->flags); > > + } > > > > /* > > - * Userspace only need to know that the kernel is using this GPIO so > > - * it can't use it. > > + * Userspace only need know that the kernel is using this GPIO so it > > + * can't use it. > > + * The calculation of the used flag is slightly racy, as it may read > > + * desc, gc and pinctrl state without a lock covering all three at > > + * once. Worst case if the line is in transition and the calculation > > + * is inconsistent then it looks to the user like they performed the > > + * read on the other side of the transition - but that can always > > + * happen. > > + * The definitive test that a line is available to userspace is to > > + * request 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) || > > + if (test_bit(FLAG_REQUESTED, &dflags) || > > + test_bit(FLAG_IS_HOGGED, &dflags) || > > + test_bit(FLAG_USED_AS_IRQ, &dflags) || > > + test_bit(FLAG_EXPORT, &dflags) || > > + test_bit(FLAG_SYSFS, &dflags) || > > !gpiochip_line_is_valid(gc, info->offset) || > > - !ok_for_pinctrl) > > + !pinctrl_gpio_can_use_line(gc, info->offset)) > > info->flags |= GPIO_V2_LINE_FLAG_USED; > > > > - if (test_bit(FLAG_IS_OUT, &desc->flags)) > > + if (test_bit(FLAG_IS_OUT, &dflags)) > > info->flags |= GPIO_V2_LINE_FLAG_OUTPUT; > > else > > info->flags |= GPIO_V2_LINE_FLAG_INPUT; > > > > - if (test_bit(FLAG_ACTIVE_LOW, &desc->flags)) > > + if (test_bit(FLAG_ACTIVE_LOW, &dflags)) > > One more nit: you no longer have to use atomic bitops here, you can > switch to the ones without guarantees for better performance. -ENEVERMIND, there's no non-atomic test_bit(). I'll go ahead and apply this one too. Bart > > Bart > > > info->flags |= GPIO_V2_LINE_FLAG_ACTIVE_LOW; > > > > - if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) > > + if (test_bit(FLAG_OPEN_DRAIN, &dflags)) > > info->flags |= GPIO_V2_LINE_FLAG_OPEN_DRAIN; > > - if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) > > + if (test_bit(FLAG_OPEN_SOURCE, &dflags)) > > info->flags |= GPIO_V2_LINE_FLAG_OPEN_SOURCE; > > > > - if (test_bit(FLAG_BIAS_DISABLE, &desc->flags)) > > + if (test_bit(FLAG_BIAS_DISABLE, &dflags)) > > info->flags |= GPIO_V2_LINE_FLAG_BIAS_DISABLED; > > - if (test_bit(FLAG_PULL_DOWN, &desc->flags)) > > + if (test_bit(FLAG_PULL_DOWN, &dflags)) > > info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN; > > - if (test_bit(FLAG_PULL_UP, &desc->flags)) > > + if (test_bit(FLAG_PULL_UP, &dflags)) > > info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_UP; > > > > - if (test_bit(FLAG_EDGE_RISING, &desc->flags)) > > + if (test_bit(FLAG_EDGE_RISING, &dflags)) > > info->flags |= GPIO_V2_LINE_FLAG_EDGE_RISING; > > - if (test_bit(FLAG_EDGE_FALLING, &desc->flags)) > > + if (test_bit(FLAG_EDGE_FALLING, &dflags)) > > info->flags |= GPIO_V2_LINE_FLAG_EDGE_FALLING; > > > > - if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &desc->flags)) > > + if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &dflags)) > > info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME; > > - else if (test_bit(FLAG_EVENT_CLOCK_HTE, &desc->flags)) > > + else if (test_bit(FLAG_EVENT_CLOCK_HTE, &dflags)) > > info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE; > > - > > - spin_unlock_irqrestore(&gpio_lock, flags); > > } > > > > struct gpio_chardev_data { > > -- > > 2.39.2 > >
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index aecc4241b6c8..9f5104d7532f 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -2399,74 +2399,72 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc, struct gpio_v2_line_info *info) { struct gpio_chip *gc = desc->gdev->chip; - bool ok_for_pinctrl; - unsigned long flags; + unsigned long dflags; memset(info, 0, sizeof(*info)); info->offset = gpio_chip_hwgpio(desc); - /* - * 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(gc, info->offset); + scoped_guard(spinlock_irqsave, &gpio_lock) { + if (desc->name) + strscpy(info->name, desc->name, sizeof(info->name)); - spin_lock_irqsave(&gpio_lock, flags); + if (desc->label) + strscpy(info->consumer, desc->label, + sizeof(info->consumer)); - if (desc->name) - strscpy(info->name, desc->name, sizeof(info->name)); - - if (desc->label) - strscpy(info->consumer, desc->label, sizeof(info->consumer)); + dflags = READ_ONCE(desc->flags); + } /* - * Userspace only need to know that the kernel is using this GPIO so - * it can't use it. + * Userspace only need know that the kernel is using this GPIO so it + * can't use it. + * The calculation of the used flag is slightly racy, as it may read + * desc, gc and pinctrl state without a lock covering all three at + * once. Worst case if the line is in transition and the calculation + * is inconsistent then it looks to the user like they performed the + * read on the other side of the transition - but that can always + * happen. + * The definitive test that a line is available to userspace is to + * request 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) || + if (test_bit(FLAG_REQUESTED, &dflags) || + test_bit(FLAG_IS_HOGGED, &dflags) || + test_bit(FLAG_USED_AS_IRQ, &dflags) || + test_bit(FLAG_EXPORT, &dflags) || + test_bit(FLAG_SYSFS, &dflags) || !gpiochip_line_is_valid(gc, info->offset) || - !ok_for_pinctrl) + !pinctrl_gpio_can_use_line(gc, info->offset)) info->flags |= GPIO_V2_LINE_FLAG_USED; - if (test_bit(FLAG_IS_OUT, &desc->flags)) + if (test_bit(FLAG_IS_OUT, &dflags)) info->flags |= GPIO_V2_LINE_FLAG_OUTPUT; else info->flags |= GPIO_V2_LINE_FLAG_INPUT; - if (test_bit(FLAG_ACTIVE_LOW, &desc->flags)) + if (test_bit(FLAG_ACTIVE_LOW, &dflags)) info->flags |= GPIO_V2_LINE_FLAG_ACTIVE_LOW; - if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) + if (test_bit(FLAG_OPEN_DRAIN, &dflags)) info->flags |= GPIO_V2_LINE_FLAG_OPEN_DRAIN; - if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) + if (test_bit(FLAG_OPEN_SOURCE, &dflags)) info->flags |= GPIO_V2_LINE_FLAG_OPEN_SOURCE; - if (test_bit(FLAG_BIAS_DISABLE, &desc->flags)) + if (test_bit(FLAG_BIAS_DISABLE, &dflags)) info->flags |= GPIO_V2_LINE_FLAG_BIAS_DISABLED; - if (test_bit(FLAG_PULL_DOWN, &desc->flags)) + if (test_bit(FLAG_PULL_DOWN, &dflags)) info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN; - if (test_bit(FLAG_PULL_UP, &desc->flags)) + if (test_bit(FLAG_PULL_UP, &dflags)) info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_UP; - if (test_bit(FLAG_EDGE_RISING, &desc->flags)) + if (test_bit(FLAG_EDGE_RISING, &dflags)) info->flags |= GPIO_V2_LINE_FLAG_EDGE_RISING; - if (test_bit(FLAG_EDGE_FALLING, &desc->flags)) + if (test_bit(FLAG_EDGE_FALLING, &dflags)) info->flags |= GPIO_V2_LINE_FLAG_EDGE_FALLING; - if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &desc->flags)) + if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &dflags)) info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME; - else if (test_bit(FLAG_EVENT_CLOCK_HTE, &desc->flags)) + else if (test_bit(FLAG_EVENT_CLOCK_HTE, &dflags)) info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE; - - spin_unlock_irqrestore(&gpio_lock, flags); } struct gpio_chardev_data {