| Message ID | 20250225-retval-fixes-v1-0-078c4c98517a@linaro.org |
|---|---|
| Headers | show |
| Series | gpiolib: fix bugs in retval sanitization | expand |
On Tue, Feb 25, 2025 at 12:56:25PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > gpiochip_get() is called only in two places: in gpio_chip_get_value() > and in gpiochip_get_multiple() where the existence of the gc->get() > callback is already checked. It makes sense to unduplicate the check by > moving it one level up the stack. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On Tue, Feb 25, 2025 at 12:56:23PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > During chip registration we should neither check the return value of > gc->get_direction() nor hold the SRCU lock when calling it. The former > is because pin controllers may have pins set to alternate functions and > return errors from their get_direction() callbacks. That's alright - we > should default to the safe INPUT state and not bail-out. The latter is > not needed because we haven't registered the chip yet so there's nothing > to protect against dynamic removal. In fact: we currently hit a lockdep > splat. Revert to calling the gc->get_direction() callback directly not > not checking its value. ... I think the below code deserves a commit (as a summary of the above commit message). > + if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) > + assign_bit(FLAG_IS_OUT, &desc->flags, > + !gc->get_direction(gc, desc_index)); > + else > assign_bit(FLAG_IS_OUT, > &desc->flags, !gc->direction_input); Otherwise LGTM, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On Tue, Feb 25, 2025 at 03:43:29PM +0100, Bartosz Golaszewski wrote: > On Tue, Feb 25, 2025 at 2:22 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Feb 25, 2025 at 12:56:23PM +0100, Bartosz Golaszewski wrote: > > > During chip registration we should neither check the return value of > > > gc->get_direction() nor hold the SRCU lock when calling it. The former > > > is because pin controllers may have pins set to alternate functions and > > > return errors from their get_direction() callbacks. That's alright - we > > > should default to the safe INPUT state and not bail-out. The latter is > > > not needed because we haven't registered the chip yet so there's nothing > > > to protect against dynamic removal. In fact: we currently hit a lockdep > > > splat. Revert to calling the gc->get_direction() callback directly not > > > not checking its value. ... > > I think the below code deserves a commit (as a summary of the above commit > > message). > > Can you rephrase? I'm not getting this one. Ah, s/commit/comment/ > > > + if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) > > > + assign_bit(FLAG_IS_OUT, &desc->flags, > > > + !gc->get_direction(gc, desc_index)); > > > + else > > > assign_bit(FLAG_IS_OUT, > > > &desc->flags, !gc->direction_input); > > > > Otherwise LGTM, > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Here's a set of fixes to issues spotted in next after queuing the series adding return value sanitization to GPIO core. Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> --- Bartosz Golaszewski (3): gpiolib: don't use gpiochip_get_direction() when registering a chip gpiolib: use a more explicit retval logic in gpiochip_get_direction() gpiolib: don't double-check the gc->get callback's existence drivers/gpio/gpiolib.c | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-) --- base-commit: 0226d0ce98a477937ed295fb7df4cc30b46fc304 change-id: 20250225-retval-fixes-a1a09a1e3a08 Best regards,