[PATCH 0/2] gpio: don't compare raw GPIO descriptor pointers directly

[PATCH 0/2] gpio: don't compare raw GPIO descriptor pointers directly

[Bartosz Golaszewski]

Handling of shared GPIOs in the kernel needs some improvements. Let's
start with a simple change of not comparing GPIO descriptor pointers
directly as there's nothing that guarantees that the same physical pin
will always be represented by a single GPIO descriptor obtained by
calling gpiod_get().

For merging: I suggest to take patch 1/2 through the GPIO tree and
provide an immutable tag for the regulator tree.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
Bartosz Golaszewski (2):
      gpio: provide gpiod_is_equal()
      regulator: don't compare raw GPIO descriptor pointers

 drivers/gpio/gpiolib.c        | 14 ++++++++++++++
 drivers/regulator/core.c      |  2 +-
 include/linux/gpio/consumer.h |  9 +++++++++
 3 files changed, 24 insertions(+), 1 deletion(-)
---
base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
change-id: 20250402-gpiod-is-equal-4e28bb8ab842

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[PATCH 1/2] gpio: provide gpiod_is_equal()

[Bartosz Golaszewski]

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.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c        | 14 ++++++++++++++
 include/linux/gpio/consumer.h |  9 +++++++++
 2 files changed, 23 insertions(+)

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)

-- 
2.45.2

[PATCH 2/2] regulator: don't compare raw GPIO descriptor pointers

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

There's no API contract saying that two GPIO descriptor pointers
obtained with a call to gpiod_get() (or one of the variants), that refer
to the same physical GPIO pin, always point to the same structure. Use
the dedicated comparator function.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/regulator/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 90629a756693..7a248dc8d2e2 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2617,7 +2617,7 @@ static int regulator_ena_gpio_request(struct regulator_dev *rdev,
 	mutex_lock(&regulator_list_mutex);
 
 	list_for_each_entry(pin, &regulator_ena_gpio_list, list) {
-		if (pin->gpiod == gpiod) {
+		if (gpiod_is_equal(pin->gpiod, gpiod)) {
 			rdev_dbg(rdev, "GPIO is already used\n");
 			goto update_ena_gpio_to_rdev;
 		}

-- 
2.45.2

Re: [PATCH 0/2] gpio: don't compare raw GPIO descriptor pointers directly

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 210 bytes --]

On Mon, Apr 07, 2025 at 09:08:13AM +0200, Bartosz Golaszewski wrote:

> For merging: I suggest to take patch 1/2 through the GPIO tree and
> provide an immutable tag for the regulator tree.

Sounds good to me.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] gpio: provide gpiod_is_equal()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 494 bytes --]

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.

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: (subset) [PATCH 0/2] gpio: don't compare raw GPIO descriptor pointers directly

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Mon, 07 Apr 2025 09:08:13 +0200, Bartosz Golaszewski wrote:
> Handling of shared GPIOs in the kernel needs some improvements. Let's
> start with a simple change of not comparing GPIO descriptor pointers
> directly as there's nothing that guarantees that the same physical pin
> will always be represented by a single GPIO descriptor obtained by
> calling gpiod_get().
> 
> For merging: I suggest to take patch 1/2 through the GPIO tree and
> provide an immutable tag for the regulator tree.
> 
> [...]

Applied, thanks!

[1/2] gpio: provide gpiod_is_equal()
      https://git.kernel.org/brgl/linux/c/265daffe788aa1cc5925d0afcde4fe6e99c66638

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH 1/2] gpio: provide gpiod_is_equal()

[Andy Shevchenko]

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.

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] gpio: provide gpiod_is_equal()

[Bartosz Golaszewski]

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

Re: (subset) [PATCH 0/2] gpio: don't compare raw GPIO descriptor pointers directly

[Mark Brown]

On Mon, 07 Apr 2025 09:08:13 +0200, Bartosz Golaszewski wrote:
> Handling of shared GPIOs in the kernel needs some improvements. Let's
> start with a simple change of not comparing GPIO descriptor pointers
> directly as there's nothing that guarantees that the same physical pin
> will always be represented by a single GPIO descriptor obtained by
> calling gpiod_get().
> 
> For merging: I suggest to take patch 1/2 through the GPIO tree and
> provide an immutable tag for the regulator tree.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[2/2] regulator: don't compare raw GPIO descriptor pointers
      commit: aaf6223ea2a1ff9316a81bf851fd5a0e82635b60

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark