[PATCH v1 0/7] gpiolib: Some cleanups

[PATCH v1 0/7] gpiolib: Some cleanups

[Andy Shevchenko]

Just a three groups of cleanups (some of them may be dependent to the previous
ones, but I am only aware about couple of patches, i.e. 3 & 4).

Andy Shevchenko (7):
  gpiolib: Print actual error when descriptor contains an error pointer
  gpiolib: Revert "Don't WARN on gpiod_put() for optional GPIO"
  gpiolib: Move validate_desc() and Co upper in the code
  gpiolib: Call validate_desc() when VALIDATE_DESC() can't be used
  gpiolib: Make taking gpio_lookup_lock consistent
  gpiolib: Convert to use guard()() for gpio_machine_hogs_mutex
  gpiolib: Remove redundant assignment of return variable

 drivers/gpio/gpiolib.c | 123 ++++++++++++++++++-----------------------
 1 file changed, 53 insertions(+), 70 deletions(-)

-- 
2.47.2

[PATCH v1 1/7] gpiolib: Print actual error when descriptor contains an error pointer

[Andy Shevchenko]

Print the actual error when descriptor contains an error pointer.
This might help debugging those rare cases.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index eb94428af6c0..0089745b381f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2445,7 +2445,7 @@ static int validate_desc(const struct gpio_desc *desc, const char *func)
 		return 0;
 
 	if (IS_ERR(desc)) {
-		pr_warn("%s: invalid GPIO (errorpointer)\n", func);
+		pr_warn("%s: invalid GPIO (errorpointer: %pe)\n", func, desc);
 		return PTR_ERR(desc);
 	}
 
-- 
2.47.2

Re: [PATCH v1 1/7] gpiolib: Print actual error when descriptor contains an error pointer

[Linus Walleij]

On Tue, Apr 15, 2025 at 1:11 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Print the actual error when descriptor contains an error pointer.
> This might help debugging those rare cases.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v1 2/7] gpiolib: Revert "Don't WARN on gpiod_put() for optional GPIO"

[Andy Shevchenko]

No need to double check the pointer for NULL since gpiod_free()
is using VALIDATE_DESC_VOID() which simply returns in that case.

This reverts commit 1d7765ba15aca68f3bc52f59434c1c34855bbb54.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0089745b381f..8ea5ddf4704d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -5156,8 +5156,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_array_optional);
  */
 void gpiod_put(struct gpio_desc *desc)
 {
-	if (desc)
-		gpiod_free(desc);
+	gpiod_free(desc);
 }
 EXPORT_SYMBOL_GPL(gpiod_put);
 
-- 
2.47.2

Re: [PATCH v1 2/7] gpiolib: Revert "Don't WARN on gpiod_put() for optional GPIO"

[Linus Walleij]

On Tue, Apr 15, 2025 at 1:11 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> No need to double check the pointer for NULL since gpiod_free()
> is using VALIDATE_DESC_VOID() which simply returns in that case.
>
> This reverts commit 1d7765ba15aca68f3bc52f59434c1c34855bbb54.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Well spotted!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v1 3/7] gpiolib: Move validate_desc() and Co upper in the code

[Andy Shevchenko]

Move validate_desc() and Co upper in the code to be able to use
in the further changes.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 62 +++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 8ea5ddf4704d..1fdf4d2ceb36 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -356,6 +356,37 @@ static int gpiochip_find_base_unlocked(u16 ngpio)
 	}
 }
 
+/*
+ * This descriptor validation needs to be inserted verbatim into each
+ * function taking a descriptor, so we need to use a preprocessor
+ * macro to avoid endless duplication. If the desc is NULL it is an
+ * optional GPIO and calls should just bail out.
+ */
+static int validate_desc(const struct gpio_desc *desc, const char *func)
+{
+	if (!desc)
+		return 0;
+
+	if (IS_ERR(desc)) {
+		pr_warn("%s: invalid GPIO (errorpointer: %pe)\n", func, desc);
+		return PTR_ERR(desc);
+	}
+
+	return 1;
+}
+
+#define VALIDATE_DESC(desc) do { \
+	int __valid = validate_desc(desc, __func__); \
+	if (__valid <= 0) \
+		return __valid; \
+	} while (0)
+
+#define VALIDATE_DESC_VOID(desc) do { \
+	int __valid = validate_desc(desc, __func__); \
+	if (__valid <= 0) \
+		return; \
+	} while (0)
+
 static int gpiochip_get_direction(struct gpio_chip *gc, unsigned int offset)
 {
 	int ret;
@@ -2433,37 +2464,6 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 	return ret;
 }
 
-/*
- * This descriptor validation needs to be inserted verbatim into each
- * function taking a descriptor, so we need to use a preprocessor
- * macro to avoid endless duplication. If the desc is NULL it is an
- * optional GPIO and calls should just bail out.
- */
-static int validate_desc(const struct gpio_desc *desc, const char *func)
-{
-	if (!desc)
-		return 0;
-
-	if (IS_ERR(desc)) {
-		pr_warn("%s: invalid GPIO (errorpointer: %pe)\n", func, desc);
-		return PTR_ERR(desc);
-	}
-
-	return 1;
-}
-
-#define VALIDATE_DESC(desc) do { \
-	int __valid = validate_desc(desc, __func__); \
-	if (__valid <= 0) \
-		return __valid; \
-	} while (0)
-
-#define VALIDATE_DESC_VOID(desc) do { \
-	int __valid = validate_desc(desc, __func__); \
-	if (__valid <= 0) \
-		return; \
-	} while (0)
-
 int gpiod_request(struct gpio_desc *desc, const char *label)
 {
 	int ret = -EPROBE_DEFER;
-- 
2.47.2

Re: [PATCH v1 3/7] gpiolib: Move validate_desc() and Co upper in the code

[Linus Walleij]

On Tue, Apr 15, 2025 at 1:11 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Move validate_desc() and Co upper in the code to be able to use
> in the further changes.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v1 4/7] gpiolib: Call validate_desc() when VALIDATE_DESC() can't be used

[Andy Shevchenko]

Call validate_desc() directly when VALIDATE_DESC() can't be used.
It will print additional information useful for debugging.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 1fdf4d2ceb36..6b307144e41a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -421,11 +421,8 @@ int gpiod_get_direction(struct gpio_desc *desc)
 	unsigned int offset;
 	int ret;
 
-	/*
-	 * We cannot use VALIDATE_DESC() as we must not return 0 for a NULL
-	 * descriptor like we usually do.
-	 */
-	if (IS_ERR_OR_NULL(desc))
+	ret = validate_desc(desc, __func__);
+	if (ret <= 0)
 		return -EINVAL;
 
 	CLASS(gpio_chip_guard, guard)(desc);
@@ -3992,13 +3989,10 @@ int gpiod_to_irq(const struct gpio_desc *desc)
 	struct gpio_device *gdev;
 	struct gpio_chip *gc;
 	int offset;
+	int ret;
 
-	/*
-	 * Cannot VALIDATE_DESC() here as gpiod_to_irq() consumer semantics
-	 * requires this function to not return zero on an invalid descriptor
-	 * but rather a negative error number.
-	 */
-	if (IS_ERR_OR_NULL(desc))
+	ret = validate_desc(desc, __func__);
+	if (ret <= 0)
 		return -EINVAL;
 
 	gdev = desc->gdev;
@@ -4010,13 +4004,12 @@ int gpiod_to_irq(const struct gpio_desc *desc)
 
 	offset = gpio_chip_hwgpio(desc);
 	if (gc->to_irq) {
-		int retirq = gc->to_irq(gc, offset);
+		ret = gc->to_irq(gc, offset);
+		if (ret)
+			return ret;
 
 		/* Zero means NO_IRQ */
-		if (!retirq)
-			return -ENXIO;
-
-		return retirq;
+		return -ENXIO;
 	}
 #ifdef CONFIG_GPIOLIB_IRQCHIP
 	if (gc->irq.chip) {
-- 
2.47.2

Re: [PATCH v1 4/7] gpiolib: Call validate_desc() when VALIDATE_DESC() can't be used

[Linus Walleij]

On Tue, Apr 15, 2025 at 1:11 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Call validate_desc() directly when VALIDATE_DESC() can't be used.
> It will print additional information useful for debugging.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

>         offset = gpio_chip_hwgpio(desc);
>         if (gc->to_irq) {
> -               int retirq = gc->to_irq(gc, offset);
> +               ret = gc->to_irq(gc, offset);
> +               if (ret)
> +                       return ret;
>
>                 /* Zero means NO_IRQ */
> -               if (!retirq)
> -                       return -ENXIO;
> -
> -               return retirq;
> +               return -ENXIO;

Totally unrelated change - but I'm fine with that :D

Yours,
Linus Walleij

Re: [PATCH v1 4/7] gpiolib: Call validate_desc() when VALIDATE_DESC() can't be used

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 10:44:18AM +0200, Linus Walleij wrote:
> On Tue, Apr 15, 2025 at 1:11 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > Call validate_desc() directly when VALIDATE_DESC() can't be used.
> > It will print additional information useful for debugging.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thank you!

...

> >         if (gc->to_irq) {
> > -               int retirq = gc->to_irq(gc, offset);
> > +               ret = gc->to_irq(gc, offset);
> > +               if (ret)
> > +                       return ret;
> >
> >                 /* Zero means NO_IRQ */
> > -               if (!retirq)
> > -                       return -ENXIO;
> > -
> > -               return retirq;
> > +               return -ENXIO;
> 
> Totally unrelated change - but I'm fine with that :D

It's not totally (it's a reuse of the same variable), but I can split it.
Bart, what do you prefer?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/7] gpiolib: Call validate_desc() when VALIDATE_DESC() can't be used

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 12:17:11PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 16, 2025 at 10:44:18AM +0200, Linus Walleij wrote:
> > On Tue, Apr 15, 2025 at 1:11 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:

...

> > >         if (gc->to_irq) {
> > > -               int retirq = gc->to_irq(gc, offset);
> > > +               ret = gc->to_irq(gc, offset);
> > > +               if (ret)
> > > +                       return ret;
> > >
> > >                 /* Zero means NO_IRQ */
> > > -               if (!retirq)
> > > -                       return -ENXIO;
> > > -
> > > -               return retirq;
> > > +               return -ENXIO;
> > 
> > Totally unrelated change - but I'm fine with that :D
> 
> It's not totally (it's a reuse of the same variable), but I can split it.
> Bart, what do you prefer?
Never mind, I will send a v2 soon.

Linus, I will leave your Rb tag in a split patch as it seems you've reviewed it
as a whole already.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 5/7] gpiolib: Make taking gpio_lookup_lock consistent

[Andy Shevchenko]

There are two ways to take a lock: plain call to the mutex_lock()
or using guard()() / scoped_guard(). The driver inconsistently uses
both. Make taking gpio_lookup_lock consistent.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6b307144e41a..cc12f274ccda 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4364,12 +4364,10 @@ void gpiod_add_lookup_tables(struct gpiod_lookup_table **tables, size_t n)
 {
 	unsigned int i;
 
-	mutex_lock(&gpio_lookup_lock);
+	guard(mutex)(&gpio_lookup_lock);
 
 	for (i = 0; i < n; i++)
 		list_add_tail(&tables[i]->list, &gpio_lookup_list);
-
-	mutex_unlock(&gpio_lookup_lock);
 }
 
 /**
@@ -4428,11 +4426,9 @@ void gpiod_remove_lookup_table(struct gpiod_lookup_table *table)
 	if (!table)
 		return;
 
-	mutex_lock(&gpio_lookup_lock);
+	guard(mutex)(&gpio_lookup_lock);
 
 	list_del(&table->list);
-
-	mutex_unlock(&gpio_lookup_lock);
 }
 EXPORT_SYMBOL_GPL(gpiod_remove_lookup_table);
 
-- 
2.47.2

Re: [PATCH v1 5/7] gpiolib: Make taking gpio_lookup_lock consistent

[Linus Walleij]

On Tue, Apr 15, 2025 at 1:11 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> There are two ways to take a lock: plain call to the mutex_lock()
> or using guard()() / scoped_guard(). The driver inconsistently uses
> both. Make taking gpio_lookup_lock consistent.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v1 6/7] gpiolib: Convert to use guard()() for gpio_machine_hogs_mutex

[Andy Shevchenko]

The driver uses guard()()/scoped_guard() for the rest of the synchronisation
calls. Convert to use the same for gpio_machine_hogs_mutex.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index cc12f274ccda..8d525e9d4319 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -916,14 +916,12 @@ static void machine_gpiochip_add(struct gpio_chip *gc)
 {
 	struct gpiod_hog *hog;
 
-	mutex_lock(&gpio_machine_hogs_mutex);
+	guard(mutex)(&gpio_machine_hogs_mutex);
 
 	list_for_each_entry(hog, &gpio_machine_hogs, list) {
 		if (!strcmp(gc->label, hog->chip_label))
 			gpiochip_machine_hog(gc, hog);
 	}
-
-	mutex_unlock(&gpio_machine_hogs_mutex);
 }
 
 static void gpiochip_setup_devs(void)
@@ -4440,7 +4438,7 @@ void gpiod_add_hogs(struct gpiod_hog *hogs)
 {
 	struct gpiod_hog *hog;
 
-	mutex_lock(&gpio_machine_hogs_mutex);
+	guard(mutex)(&gpio_machine_hogs_mutex);
 
 	for (hog = &hogs[0]; hog->chip_label; hog++) {
 		list_add_tail(&hog->list, &gpio_machine_hogs);
@@ -4454,8 +4452,6 @@ void gpiod_add_hogs(struct gpiod_hog *hogs)
 		if (gdev)
 			gpiochip_machine_hog(gpio_device_get_chip(gdev), hog);
 	}
-
-	mutex_unlock(&gpio_machine_hogs_mutex);
 }
 EXPORT_SYMBOL_GPL(gpiod_add_hogs);
 
@@ -4463,10 +4459,10 @@ void gpiod_remove_hogs(struct gpiod_hog *hogs)
 {
 	struct gpiod_hog *hog;
 
-	mutex_lock(&gpio_machine_hogs_mutex);
+	guard(mutex)(&gpio_machine_hogs_mutex);
+
 	for (hog = &hogs[0]; hog->chip_label; hog++)
 		list_del(&hog->list);
-	mutex_unlock(&gpio_machine_hogs_mutex);
 }
 EXPORT_SYMBOL_GPL(gpiod_remove_hogs);
 
-- 
2.47.2

Re: [PATCH v1 6/7] gpiolib: Convert to use guard()() for gpio_machine_hogs_mutex

[Linus Walleij]

On Tue, Apr 15, 2025 at 1:11 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> The driver uses guard()()/scoped_guard() for the rest of the synchronisation
> calls. Convert to use the same for gpio_machine_hogs_mutex.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v1 7/7] gpiolib: Remove redundant assignment of return variable

[Andy Shevchenko]

In some functions the returned variable is assigned to 0 and then
reassigned to the actual value. Remove redundant assignments.

In one case make it more clear that the assignment is not needed.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 8d525e9d4319..cca987addc0e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1015,7 +1015,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	struct gpio_device *gdev;
 	unsigned int desc_index;
 	int base = 0;
-	int ret = 0;
+	int ret;
 
 	/* Only allow one set() and one set_multiple(). */
 	if ((gc->set && gc->set_rv) ||
@@ -1040,11 +1040,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 
 	device_set_node(&gdev->dev, gpiochip_choose_fwnode(gc));
 
-	gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
-	if (gdev->id < 0) {
-		ret = gdev->id;
+	ret = ida_alloc(&gpio_ida, GFP_KERNEL);
+	if (ret < 0)
 		goto err_free_gdev;
-	}
+	gdev->id = ret;
 
 	ret = dev_set_name(&gdev->dev, GPIOCHIP_NAME "%d", gdev->id);
 	if (ret)
@@ -3075,7 +3074,7 @@ int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value)
  */
 int gpiod_enable_hw_timestamp_ns(struct gpio_desc *desc, unsigned long flags)
 {
-	int ret = 0;
+	int ret;
 
 	VALIDATE_DESC(desc);
 
@@ -3108,7 +3107,7 @@ EXPORT_SYMBOL_GPL(gpiod_enable_hw_timestamp_ns);
  */
 int gpiod_disable_hw_timestamp_ns(struct gpio_desc *desc, unsigned long flags)
 {
-	int ret = 0;
+	int ret;
 
 	VALIDATE_DESC(desc);
 
-- 
2.47.2

Re: [PATCH v1 7/7] gpiolib: Remove redundant assignment of return variable

[Linus Walleij]

On Tue, Apr 15, 2025 at 1:11 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> In some functions the returned variable is assigned to 0 and then
> reassigned to the actual value. Remove redundant assignments.
>
> In one case make it more clear that the assignment is not needed.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

This is actually really good, because if someone goofs up the code
so that assignment is actually needed then the compiler will
complain. (Arnd told me this.)

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij