| Message ID | 20250407-gpiod-is-equal-v1-1-7d85f568ae6e@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: don't compare raw GPIO descriptor pointers directly | expand |
On Mon, Apr 07, 2025 at 09:08:14AM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > There are users in the kernel that directly compare raw GPIO descriptor > pointers in order to determine whether they refer to the same physical > GPIO pin. This accidentally works like this but is not guaranteed by any > API contract. Let's provide a comparator function that hides the actual > logic. ... > +bool gpiod_is_equal(struct gpio_desc *desc, struct gpio_desc *other) > +{ > + return desc == other; I think it's better to have the one checked against NULL. That's how comparators make more sense, see, for example, 1b1bb7b29b10 ("drivers: base: Don't match devices with NULL of_node/fwnode/etc"). Ideally it should be IS_ERR_OR_NULL(), but we have here a principal disagreement, so this might be yet another (buggy) function in GPIOLIB. > +}
On Wed, Apr 9, 2025 at 4:32 PM Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > On Mon, Apr 07, 2025 at 09:08:14AM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > There are users in the kernel that directly compare raw GPIO descriptor > > pointers in order to determine whether they refer to the same physical > > GPIO pin. This accidentally works like this but is not guaranteed by any > > API contract. Let's provide a comparator function that hides the actual > > logic. > > ... > > > +bool gpiod_is_equal(struct gpio_desc *desc, struct gpio_desc *other) > > +{ > > + return desc == other; > > I think it's better to have the one checked against NULL. That's how > comparators make more sense, see, for example, 1b1bb7b29b10 ("drivers: > base: Don't match devices with NULL of_node/fwnode/etc"). > Yeah I guess it can be improved in a follow-up. > Ideally it should be IS_ERR_OR_NULL(), but we have here a principal disagreement, > so this might be yet another (buggy) function in GPIOLIB. > Our disagreement has nothing to do with this. In fact validating against IS_ERR() (and returning false) makes more sense than just accepting IS_ERR() in acting like it's a valid descriptor. I will send a follow-up tomorrow. Bartosz
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index b8197502a5ac..2e5b6982e76d 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -265,6 +265,20 @@ struct gpio_device *gpiod_to_gpio_device(struct gpio_desc *desc) } EXPORT_SYMBOL_GPL(gpiod_to_gpio_device); +/** + * gpiod_is_equal() - Check if two GPIO descriptors refer to the same pin. + * @desc: Descriptor to compare. + * @other: The second descriptor to compare against. + * + * Returns: + * True if the descriptors refer to the same physical pin. False otherwise. + */ +bool gpiod_is_equal(struct gpio_desc *desc, struct gpio_desc *other) +{ + return desc == other; +} +EXPORT_SYMBOL_GPL(gpiod_is_equal); + /** * gpio_device_get_base() - Get the base GPIO number allocated by this device * @gdev: GPIO device diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h index 45b651c05b9c..7355abadaef4 100644 --- a/include/linux/gpio/consumer.h +++ b/include/linux/gpio/consumer.h @@ -180,6 +180,8 @@ struct gpio_desc *devm_fwnode_gpiod_get_index(struct device *dev, enum gpiod_flags flags, const char *label); +bool gpiod_is_equal(struct gpio_desc *desc, struct gpio_desc *other); + #else /* CONFIG_GPIOLIB */ #include <linux/bug.h> @@ -547,6 +549,13 @@ struct gpio_desc *devm_fwnode_gpiod_get_index(struct device *dev, return ERR_PTR(-ENOSYS); } +static inline bool +gpiod_is_equal(struct gpio_desc *desc, struct gpio_desc *other) +{ + WARN_ON(desc || other); + return false; +} + #endif /* CONFIG_GPIOLIB */ #if IS_ENABLED(CONFIG_GPIOLIB) && IS_ENABLED(CONFIG_HTE)