[PATCH v2 00/10] gpio/pinctrl: replace gpiochip_is_requested() with a safer interface

[PATCH v2 00/10] gpio/pinctrl: replace gpiochip_is_requested() with a safer interface

[Bartosz Golaszewski]

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

While reworking the locking in GPIOLIB I realized that locking the
descriptor with users still calling gpiochip_is_requested() will still
be buggy as it returns a pointer to a string that can be freed whenever
the descriptor is released. Let's provide a safer alternative in the
form of a function that returns a copy of the label.

Use it in all drivers and remove gpiochip_is_requested().

I plan to provide this series in an immutable branch for the pinctrl and
baytrail trees to pull.

v1 -> v2:
- use DEFINE_CLASS() to register a destructor for making sure that the
  duplicated label doesn't leak out of the for_each loops even with
  break;

Bartosz Golaszewski (10):
  gpiolib: provide gpiochip_dup_line_label()
  gpio: wm831x: use gpiochip_dup_line_label()
  gpio: wm8994: use gpiochip_dup_line_label()
  gpio: stmpe: use gpiochip_dup_line_label()
  pinctrl: abx500: use gpiochip_dup_line_label()
  pinctrl: nomadik: use gpiochip_dup_line_label()
  pinctrl: baytrail: use gpiochip_dup_line_label()
  pinctrl: sppctl: use gpiochip_dup_line_label()
  gpiolib: use gpiochip_dup_line_label() in for_each helpers
  gpiolib: remove gpiochip_is_requested()

 drivers/gpio/gpio-stmpe.c                 |  6 +++-
 drivers/gpio/gpio-wm831x.c                | 14 ++++++---
 drivers/gpio/gpio-wm8994.c                | 13 +++++---
 drivers/gpio/gpiolib.c                    | 37 ++++++++++++++---------
 drivers/pinctrl/intel/pinctrl-baytrail.c  | 11 ++++---
 drivers/pinctrl/nomadik/pinctrl-abx500.c  |  9 ++++--
 drivers/pinctrl/nomadik/pinctrl-nomadik.c |  6 +++-
 drivers/pinctrl/sunplus/sppctl.c          | 10 +++---
 include/linux/gpio/driver.h               | 37 +++++++++++++++++------
 9 files changed, 95 insertions(+), 48 deletions(-)

-- 
2.40.1

[PATCH v2 01/10] gpiolib: provide gpiochip_dup_line_label()

[Bartosz Golaszewski]

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

gpiochip_is_requested() not only has a misleading name but it returns
a pointer to a string that is freed when the descriptor is released.

Provide a new helper meant to replace it, which returns a copy of the
label string instead.

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a5faaea6915d..8e932e6a6a8d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2400,6 +2400,35 @@ const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset)
 }
 EXPORT_SYMBOL_GPL(gpiochip_is_requested);
 
+/**
+ * gpiochip_dup_line_label - Get a copy of the consumer label.
+ * @gc: GPIO chip controlling this line.
+ * @offset: Hardware offset of the line.
+ *
+ * Returns:
+ * Pointer to a copy of the consumer label if the line is requested or NULL
+ * if it's not. If a valid pointer was returned, it must be freed using
+ * kfree(). In case of a memory allocation error, the function returns %ENOMEM.
+ *
+ * Must not be called from atomic context.
+ */
+char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset)
+{
+	const char *label;
+	char *cpy;
+
+	label = gpiochip_is_requested(gc, offset);
+	if (!label)
+		return NULL;
+
+	cpy = kstrdup(label, GFP_KERNEL);
+	if (!cpy)
+		return ERR_PTR(-ENOMEM);
+
+	return cpy;
+}
+EXPORT_SYMBOL_GPL(gpiochip_dup_line_label);
+
 /**
  * gpiochip_request_own_desc - Allow GPIO chip to request its own descriptor
  * @gc: GPIO chip
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 100c329dc986..9796a34e2fee 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -532,6 +532,7 @@ struct gpio_chip {
 };
 
 const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset);
+char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset);
 
 /**
  * for_each_requested_gpio_in_range - iterates over requested GPIOs in a given range
-- 
2.40.1

Re: [PATCH v2 01/10] gpiolib: provide gpiochip_dup_line_label()

[Andy Shevchenko]

On Thu, Nov 30, 2023 at 02:46:21PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> gpiochip_is_requested() not only has a misleading name but it returns
> a pointer to a string that is freed when the descriptor is released.
> 
> Provide a new helper meant to replace it, which returns a copy of the
> label string instead.

...

> + * Must not be called from atomic context.

Put the respective lockdep annotation.

...

> +	char *cpy;

So, why not naming it fully, i.e. "copy"?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 01/10] gpiolib: provide gpiochip_dup_line_label()

[Bartosz Golaszewski]

On Thu, Nov 30, 2023 at 5:27 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Nov 30, 2023 at 02:46:21PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > gpiochip_is_requested() not only has a misleading name but it returns
> > a pointer to a string that is freed when the descriptor is released.
> >
> > Provide a new helper meant to replace it, which returns a copy of the
> > label string instead.
>
> ...
>
> > + * Must not be called from atomic context.
>
> Put the respective lockdep annotation.
>
> ...
>
> > +     char *cpy;
>
> So, why not naming it fully, i.e. "copy"?
>

Ekhm... let me quote the BigPinguin :)

--

C is a Spartan language, and your naming conventions should follow suit.
Unlike Modula-2 and Pascal programmers, C programmers do not use cute
names like ThisVariableIsATemporaryCounter. A C programmer would call that
variable ``tmp``, which is much easier to write, and not the least more
difficult to understand.

--

Bart

> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v2 01/10] gpiolib: provide gpiochip_dup_line_label()

[Andy Shevchenko]

On Thu, Nov 30, 2023 at 06:48:06PM +0100, Bartosz Golaszewski wrote:
> On Thu, Nov 30, 2023 at 5:27 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Nov 30, 2023 at 02:46:21PM +0100, Bartosz Golaszewski wrote:

...

> > > +     char *cpy;
> >
> > So, why not naming it fully, i.e. "copy"?
> >
> 
> Ekhm... let me quote the BigPinguin :)
> 
> --
> 
> C is a Spartan language, and your naming conventions should follow suit.
> Unlike Modula-2 and Pascal programmers, C programmers do not use cute
> names like ThisVariableIsATemporaryCounter. A C programmer would call that
> variable ``tmp``, which is much easier to write, and not the least more
> difficult to understand.

In contrary the cpy is more difficult to understand.

`git grep -lw cpy` vs. `git grep -lw copy` suggests that my variant
is preferable (even excluding tools/ and Documentation/).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 01/10] gpiolib: provide gpiochip_dup_line_label()

[Bartosz Golaszewski]

On Thu, Nov 30, 2023 at 7:00 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Nov 30, 2023 at 06:48:06PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Nov 30, 2023 at 5:27 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Nov 30, 2023 at 02:46:21PM +0100, Bartosz Golaszewski wrote:
>
> ...
>
> > > > +     char *cpy;
> > >
> > > So, why not naming it fully, i.e. "copy"?
> > >
> >
> > Ekhm... let me quote the BigPinguin :)
> >
> > --
> >
> > C is a Spartan language, and your naming conventions should follow suit.
> > Unlike Modula-2 and Pascal programmers, C programmers do not use cute
> > names like ThisVariableIsATemporaryCounter. A C programmer would call that
> > variable ``tmp``, which is much easier to write, and not the least more
> > difficult to understand.
>
> In contrary the cpy is more difficult to understand.
>
> `git grep -lw cpy` vs. `git grep -lw copy` suggests that my variant
> is preferable (even excluding tools/ and Documentation/).
>

You are a thorough reviewer but man, are you also a bikeshedder. :)

Whatever, let's make it 'copy'.

Bart

> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v2 01/10] gpiolib: provide gpiochip_dup_line_label()

[Bartosz Golaszewski]

On Thu, Nov 30, 2023 at 5:27 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Nov 30, 2023 at 02:46:21PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > gpiochip_is_requested() not only has a misleading name but it returns
> > a pointer to a string that is freed when the descriptor is released.
> >
> > Provide a new helper meant to replace it, which returns a copy of the
> > label string instead.
>
> ...
>
> > + * Must not be called from atomic context.
>
> Put the respective lockdep annotation.
>

What are you referring to?

Bart

> ...
>
> > +     char *cpy;
>
> So, why not naming it fully, i.e. "copy"?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

[PATCH v2 02/10] gpio: wm831x: use gpiochip_dup_line_label()

[Bartosz Golaszewski]

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

Use the new gpiochip_dup_line_label() helper to safely retrieve the
descriptor label.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpio-wm831x.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-wm831x.c b/drivers/gpio/gpio-wm831x.c
index 7eaf8a28638c..f7d5120ff8f1 100644
--- a/drivers/gpio/gpio-wm831x.c
+++ b/drivers/gpio/gpio-wm831x.c
@@ -8,6 +8,7 @@
  *
  */
 
+#include <linux/cleanup.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/module.h>
@@ -160,18 +161,21 @@ static void wm831x_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	for (i = 0; i < chip->ngpio; i++) {
 		int gpio = i + chip->base;
 		int reg;
-		const char *label, *pull, *powerdomain;
+		const char *pull, *powerdomain;
 
 		/* We report the GPIO even if it's not requested since
 		 * we're also reporting things like alternate
 		 * functions which apply even when the GPIO is not in
 		 * use as a GPIO.
 		 */
-		label = gpiochip_is_requested(chip, i);
-		if (!label)
-			label = "Unrequested";
+		char *label __free(kfree) = gpiochip_dup_line_label(chip, i);
+		if (IS_ERR(label)) {
+			dev_err(wm831x->dev, "Failed to duplicate label\n");
+			continue;
+		}
 
-		seq_printf(s, " gpio-%-3d (%-20.20s) ", gpio, label);
+		seq_printf(s, " gpio-%-3d (%-20.20s) ",
+			   gpio, label ?: "Unrequested");
 
 		reg = wm831x_reg_read(wm831x, WM831X_GPIO1_CONTROL + i);
 		if (reg < 0) {
-- 
2.40.1

[PATCH v2 03/10] gpio: wm8994: use gpiochip_dup_line_label()

[Bartosz Golaszewski]

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

Use the new gpiochip_dup_line_label() helper to safely retrieve the
descriptor label.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpio-wm8994.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-wm8994.c b/drivers/gpio/gpio-wm8994.c
index f4a474cef32d..bf05c9b5882b 100644
--- a/drivers/gpio/gpio-wm8994.c
+++ b/drivers/gpio/gpio-wm8994.c
@@ -8,6 +8,7 @@
  *
  */
 
+#include <linux/cleanup.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/module.h>
@@ -193,18 +194,20 @@ static void wm8994_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	for (i = 0; i < chip->ngpio; i++) {
 		int gpio = i + chip->base;
 		int reg;
-		const char *label;
 
 		/* We report the GPIO even if it's not requested since
 		 * we're also reporting things like alternate
 		 * functions which apply even when the GPIO is not in
 		 * use as a GPIO.
 		 */
-		label = gpiochip_is_requested(chip, i);
-		if (!label)
-			label = "Unrequested";
+		char *label __free(kfree) = gpiochip_dup_line_label(chip, i);
+		if (IS_ERR(label)) {
+			dev_err(wm8994->dev, "Failed to duplicate label\n");
+			continue;
+		}
 
-		seq_printf(s, " gpio-%-3d (%-20.20s) ", gpio, label);
+		seq_printf(s, " gpio-%-3d (%-20.20s) ", gpio,
+			   label ?: "Unrequested");
 
 		reg = wm8994_reg_read(wm8994, WM8994_GPIO_1 + i);
 		if (reg < 0) {
-- 
2.40.1

Re: [PATCH v2 03/10] gpio: wm8994: use gpiochip_dup_line_label()

[Andy Shevchenko]

On Thu, Nov 30, 2023 at 02:46:23PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Use the new gpiochip_dup_line_label() helper to safely retrieve the
> descriptor label.

...

> -		seq_printf(s, " gpio-%-3d (%-20.20s) ", gpio, label);
> +		seq_printf(s, " gpio-%-3d (%-20.20s) ", gpio,
> +			   label ?: "Unrequested");

Haha, see previous comment (in previous email).

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 04/10] gpio: stmpe: use gpiochip_dup_line_label()

[Bartosz Golaszewski]

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

Use the new gpiochip_dup_line_label() helper to safely retrieve the
descriptor label.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpio-stmpe.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index 27cc4da53565..6c5ee81d71b3 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -5,6 +5,7 @@
  * Author: Rabin Vincent <rabin.vincent@stericsson.com> for ST-Ericsson
  */
 
+#include <linux/cleanup.h>
 #include <linux/init.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
@@ -255,7 +256,6 @@ static void stmpe_dbg_show_one(struct seq_file *s,
 {
 	struct stmpe_gpio *stmpe_gpio = gpiochip_get_data(gc);
 	struct stmpe *stmpe = stmpe_gpio->stmpe;
-	const char *label = gpiochip_is_requested(gc, offset);
 	bool val = !!stmpe_gpio_get(gc, offset);
 	u8 bank = offset / 8;
 	u8 dir_reg = stmpe->regs[STMPE_IDX_GPDR_LSB + bank];
@@ -263,6 +263,10 @@ static void stmpe_dbg_show_one(struct seq_file *s,
 	int ret;
 	u8 dir;
 
+	char *label __free(kfree) = gpiochip_dup_line_label(gc, offset);
+	if (IS_ERR(label))
+		return;
+
 	ret = stmpe_reg_read(stmpe, dir_reg);
 	if (ret < 0)
 		return;
-- 
2.40.1

[PATCH v2 05/10] pinctrl: abx500: use gpiochip_dup_line_label()

[Bartosz Golaszewski]

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

Use the new gpiochip_dup_line_label() helper to safely retrieve the
descriptor label.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pinctrl/nomadik/pinctrl-abx500.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/nomadik/pinctrl-abx500.c b/drivers/pinctrl/nomadik/pinctrl-abx500.c
index d3c32d809bac..80e3ac333136 100644
--- a/drivers/pinctrl/nomadik/pinctrl-abx500.c
+++ b/drivers/pinctrl/nomadik/pinctrl-abx500.c
@@ -6,7 +6,9 @@
  *
  * Driver allows to use AxB5xx unused pins to be used as GPIO
  */
+
 #include <linux/bitops.h>
+#include <linux/cleanup.h>
 #include <linux/err.h>
 #include <linux/gpio/driver.h>
 #include <linux/init.h>
@@ -453,12 +455,11 @@ static void abx500_gpio_dbg_show_one(struct seq_file *s,
 				     unsigned offset, unsigned gpio)
 {
 	struct abx500_pinctrl *pct = pinctrl_dev_get_drvdata(pctldev);
-	const char *label = gpiochip_is_requested(chip, offset - 1);
 	u8 gpio_offset = offset - 1;
 	int mode = -1;
 	bool is_out;
 	bool pd;
-	int ret;
+	int ret = -ENOMEM;
 
 	const char *modes[] = {
 		[ABX500_DEFAULT]	= "default",
@@ -474,6 +475,10 @@ static void abx500_gpio_dbg_show_one(struct seq_file *s,
 		[ABX500_GPIO_PULL_UP]		= "pull up",
 	};
 
+	char *label __free(kfree) = gpiochip_dup_line_label(chip, offset - 1);
+	if (IS_ERR(label))
+		goto out;
+
 	ret = abx500_gpio_get_bit(chip, AB8500_GPIO_DIR1_REG,
 			gpio_offset, &is_out);
 	if (ret < 0)
-- 
2.40.1

[PATCH v2 06/10] pinctrl: nomadik: use gpiochip_dup_line_label()

[Bartosz Golaszewski]

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

Use the new gpiochip_dup_line_label() helper to safely retrieve the
descriptor label.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pinctrl/nomadik/pinctrl-nomadik.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/nomadik/pinctrl-nomadik.c b/drivers/pinctrl/nomadik/pinctrl-nomadik.c
index 863732287b1e..7911353ac97d 100644
--- a/drivers/pinctrl/nomadik/pinctrl-nomadik.c
+++ b/drivers/pinctrl/nomadik/pinctrl-nomadik.c
@@ -8,6 +8,7 @@
  * Copyright (C) 2011-2013 Linus Walleij <linus.walleij@linaro.org>
  */
 #include <linux/bitops.h>
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/err.h>
@@ -917,7 +918,6 @@ static void nmk_gpio_dbg_show_one(struct seq_file *s,
 	struct pinctrl_dev *pctldev, struct gpio_chip *chip,
 	unsigned offset, unsigned gpio)
 {
-	const char *label = gpiochip_is_requested(chip, offset);
 	struct nmk_gpio_chip *nmk_chip = gpiochip_get_data(chip);
 	int mode;
 	bool is_out;
@@ -934,6 +934,10 @@ static void nmk_gpio_dbg_show_one(struct seq_file *s,
 		[NMK_GPIO_ALT_C+4]	= "altC4",
 	};
 
+	char *label = gpiochip_dup_line_label(chip, offset);
+	if (IS_ERR(label))
+		return;
+
 	clk_enable(nmk_chip->clk);
 	is_out = !!(readl(nmk_chip->addr + NMK_GPIO_DIR) & BIT(offset));
 	pull = !(readl(nmk_chip->addr + NMK_GPIO_PDIS) & BIT(offset));
-- 
2.40.1

[PATCH v2 07/10] pinctrl: baytrail: use gpiochip_dup_line_label()

[Bartosz Golaszewski]

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

Use the new gpiochip_dup_line_label() helper to safely retrieve the
descriptor label.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pinctrl/intel/pinctrl-baytrail.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index 3cd0798ee631..3c8c02043481 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -9,6 +9,7 @@
 #include <linux/acpi.h>
 #include <linux/array_size.h>
 #include <linux/bitops.h>
+#include <linux/cleanup.h>
 #include <linux/gpio/driver.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
@@ -1173,7 +1174,6 @@ static void byt_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 		const char *pull_str = NULL;
 		const char *pull = NULL;
 		unsigned long flags;
-		const char *label;
 		unsigned int pin;
 
 		pin = vg->soc->pins[i].number;
@@ -1200,9 +1200,10 @@ static void byt_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 			seq_printf(s, "Pin %i: can't retrieve community\n", pin);
 			continue;
 		}
-		label = gpiochip_is_requested(chip, i);
-		if (!label)
-			label = "Unrequested";
+
+		char *label __free(kfree) = gpiochip_dup_line_label(chip, i);
+		if (IS_ERR(label))
+			continue;
 
 		switch (conf0 & BYT_PULL_ASSIGN_MASK) {
 		case BYT_PULL_ASSIGN_UP:
@@ -1231,7 +1232,7 @@ static void byt_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 		seq_printf(s,
 			   " gpio-%-3d (%-20.20s) %s %s %s pad-%-3d offset:0x%03x mux:%d %s%s%s",
 			   pin,
-			   label,
+			   label ?: "Unrequested",
 			   val & BYT_INPUT_EN ? "  " : "in",
 			   val & BYT_OUTPUT_EN ? "   " : "out",
 			   str_hi_lo(val & BYT_LEVEL),
-- 
2.40.1

Re: [PATCH v2 07/10] pinctrl: baytrail: use gpiochip_dup_line_label()

[Andy Shevchenko]

On Thu, Nov 30, 2023 at 02:46:27PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Use the new gpiochip_dup_line_label() helper to safely retrieve the
> descriptor label.

...

>  		seq_printf(s,
>  			   " gpio-%-3d (%-20.20s) %s %s %s pad-%-3d offset:0x%03x mux:%d %s%s%s",
>  			   pin,
> -			   label,
> +			   label ?: "Unrequested",

This already fourth (?) duplication among drivers.
Perhaps you want a helper:
gpiochip_dup_line_label_fallback() // naming is up to you
which will return the same for everybody and we don't need to hunt for
the different meaning of "Unrequested".

Also the word "Unrequested" is a bit doubtful as it can be a label, right?
Something with special characters / spaces / etc would suit better?
In any case it might require to add a warning (?) to the GPIO lib core
when label gets assigned if it clashes with the "reserved" word.

>  			   val & BYT_INPUT_EN ? "  " : "in",
>  			   val & BYT_OUTPUT_EN ? "   " : "out",
>  			   str_hi_lo(val & BYT_LEVEL),

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 07/10] pinctrl: baytrail: use gpiochip_dup_line_label()

[Bartosz Golaszewski]

On Thu, Nov 30, 2023 at 5:36 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Nov 30, 2023 at 02:46:27PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Use the new gpiochip_dup_line_label() helper to safely retrieve the
> > descriptor label.
>
> ...
>
> >               seq_printf(s,
> >                          " gpio-%-3d (%-20.20s) %s %s %s pad-%-3d offset:0x%03x mux:%d %s%s%s",
> >                          pin,
> > -                        label,
> > +                        label ?: "Unrequested",
>
> This already fourth (?) duplication among drivers.
> Perhaps you want a helper:
> gpiochip_dup_line_label_fallback() // naming is up to you
> which will return the same for everybody and we don't need to hunt for
> the different meaning of "Unrequested".
>

IMO the overhead here is very small in return for better readability
(IOW: `label ?: "Unrequested"` is more readable than some function
named `gpiochip_dup_line_label_fallback()`). Given the string is in
.rodata anyway, I wouldn't be surprised if adding a helper resulted in
bigger code.

> Also the word "Unrequested" is a bit doubtful as it can be a label, right?
> Something with special characters / spaces / etc would suit better?
> In any case it might require to add a warning (?) to the GPIO lib core
> when label gets assigned if it clashes with the "reserved" word.
>

Agreed but this is a functional change in debugfs output. I know
debugfs is not considered stable but I didn't write it, I don't know
who's using it and I prefer to leave it be.

Bart

> >                          val & BYT_INPUT_EN ? "  " : "in",
> >                          val & BYT_OUTPUT_EN ? "   " : "out",
> >                          str_hi_lo(val & BYT_LEVEL),
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

[PATCH v2 08/10] pinctrl: sppctl: use gpiochip_dup_line_label()

[Bartosz Golaszewski]

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

Use the new gpiochip_dup_line_label() helper to safely retrieve the
descriptor label.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pinctrl/sunplus/sppctl.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/sunplus/sppctl.c b/drivers/pinctrl/sunplus/sppctl.c
index bb5ef391dbe4..ae156f779a16 100644
--- a/drivers/pinctrl/sunplus/sppctl.c
+++ b/drivers/pinctrl/sunplus/sppctl.c
@@ -4,6 +4,7 @@
  * Copyright (C) Sunplus Tech / Tibbo Tech.
  */
 
+#include <linux/cleanup.h>
 #include <linux/bitfield.h>
 #include <linux/device.h>
 #include <linux/err.h>
@@ -500,16 +501,15 @@ static int sppctl_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
 
 static void sppctl_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 {
-	const char *label;
 	int i;
 
 	for (i = 0; i < chip->ngpio; i++) {
-		label = gpiochip_is_requested(chip, i);
-		if (!label)
-			label = "";
+		char *label __free(kfree) = gpiochip_dup_line_label(chip, i);
+		if (IS_ERR(label))
+			continue;
 
 		seq_printf(s, " gpio-%03d (%-16.16s | %-16.16s)", i + chip->base,
-			   chip->names[i], label);
+			   chip->names[i], label ?: "");
 		seq_printf(s, " %c", sppctl_gpio_get_direction(chip, i) ? 'I' : 'O');
 		seq_printf(s, ":%d", sppctl_gpio_get(chip, i));
 		seq_printf(s, " %s", sppctl_first_get(chip, i) ? "gpi" : "mux");
-- 
2.40.1

Re: [PATCH v2 08/10] pinctrl: sppctl: use gpiochip_dup_line_label()

[Andy Shevchenko]

On Thu, Nov 30, 2023 at 02:46:28PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Use the new gpiochip_dup_line_label() helper to safely retrieve the
> descriptor label.

...

>  	for (i = 0; i < chip->ngpio; i++) {
> -		label = gpiochip_is_requested(chip, i);
> -		if (!label)
> -			label = "";
> +		char *label __free(kfree) = gpiochip_dup_line_label(chip, i);
> +		if (IS_ERR(label))
> +			continue;
>  
>  		seq_printf(s, " gpio-%03d (%-16.16s | %-16.16s)", i + chip->base,
> -			   chip->names[i], label);
> +			   chip->names[i], label ?: "");

So, as it's non-ABI change, we still can use "reserved" word here as well
("Unrequested" or whatever.)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 08/10] pinctrl: sppctl: use gpiochip_dup_line_label()

[Bartosz Golaszewski]

On Thu, Nov 30, 2023 at 5:37 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Nov 30, 2023 at 02:46:28PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Use the new gpiochip_dup_line_label() helper to safely retrieve the
> > descriptor label.
>
> ...
>
> >       for (i = 0; i < chip->ngpio; i++) {
> > -             label = gpiochip_is_requested(chip, i);
> > -             if (!label)
> > -                     label = "";
> > +             char *label __free(kfree) = gpiochip_dup_line_label(chip, i);
> > +             if (IS_ERR(label))
> > +                     continue;
> >
> >               seq_printf(s, " gpio-%03d (%-16.16s | %-16.16s)", i + chip->base,
> > -                        chip->names[i], label);
> > +                        chip->names[i], label ?: "");
>
> So, as it's non-ABI change, we still can use "reserved" word here as well
> ("Unrequested" or whatever.)
>

See my other comment regarding the changes in output.

Bart

> --
> With Best Regards,
> Andy Shevchenko
>
>

[PATCH v2 09/10] gpiolib: use gpiochip_dup_line_label() in for_each helpers

[Bartosz Golaszewski]

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

Rework for_each_requested_gpio_in_range() to use the new helper to
retrieve a dynamically allocated copy of the descriptor label and free
it at the end of each iteration. We need to leverage the CLASS()'
destructor to make sure that the label is freed even when breaking out
of the loop.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 include/linux/gpio/driver.h | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 9796a34e2fee..b1ed501e9ee0 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -534,17 +534,36 @@ struct gpio_chip {
 const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset);
 char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset);
 
+
+struct _gpiochip_for_each_data {
+	const char **label;
+	int *i;
+};
+
+DEFINE_CLASS(_gpiochip_for_each_data,
+	     struct _gpiochip_for_each_data,
+	     if (*_T.label) kfree(*_T.label),
+	     ({ struct _gpiochip_for_each_data _data = { label, i };
+	        *_data.i = 0;
+		_data; }),
+	     const char **label, int *i)
+
 /**
  * for_each_requested_gpio_in_range - iterates over requested GPIOs in a given range
- * @chip:	the chip to query
- * @i:		loop variable
- * @base:	first GPIO in the range
- * @size:	amount of GPIOs to check starting from @base
- * @label:	label of current GPIO
+ * @_chip:	the chip to query
+ * @_i:		loop variable
+ * @_base:	first GPIO in the range
+ * @_size:	amount of GPIOs to check starting from @base
+ * @_label:	label of current GPIO
  */
-#define for_each_requested_gpio_in_range(chip, i, base, size, label)			\
-	for (i = 0; i < size; i++)							\
-		if ((label = gpiochip_is_requested(chip, base + i)) == NULL) {} else
+#define for_each_requested_gpio_in_range(_chip, _i, _base, _size, _label)		\
+	for (CLASS(_gpiochip_for_each_data, _data)(&_label, &_i);			\
+	     *_data.i < _size;								\
+	     (*_data.i)++, kfree(*(_data.label)), *_data.label = NULL)			\
+		if ((*_data.label =							\
+			gpiochip_dup_line_label(_chip, _base + *_data.i)) == NULL) {}	\
+		else if (IS_ERR(*_data.label)) {}					\
+		else
 
 /* Iterates over all requested GPIO of the given @chip */
 #define for_each_requested_gpio(chip, i, label)						\
-- 
2.40.1

Re: [PATCH v2 09/10] gpiolib: use gpiochip_dup_line_label() in for_each helpers

[Andy Shevchenko]

On Thu, Nov 30, 2023 at 02:46:29PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Rework for_each_requested_gpio_in_range() to use the new helper to
> retrieve a dynamically allocated copy of the descriptor label and free
> it at the end of each iteration. We need to leverage the CLASS()'
> destructor to make sure that the label is freed even when breaking out
> of the loop.

...

>  const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset);
>  char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset);
>  
> +

One blank line is enough.

> +struct _gpiochip_for_each_data {
> +	const char **label;
> +	int *i;

Why is this a signed?

> +};

...

> +DEFINE_CLASS(_gpiochip_for_each_data,
> +	     struct _gpiochip_for_each_data,
> +	     if (*_T.label) kfree(*_T.label),
> +	     ({ struct _gpiochip_for_each_data _data = { label, i };
> +	        *_data.i = 0;
> +		_data; }),

To me indentation of ({}) is quite weird. Where is this style being used
instead of more readable

	({
	   ...
	})

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 09/10] gpiolib: use gpiochip_dup_line_label() in for_each helpers

[Bartosz Golaszewski]

On Thu, Nov 30, 2023 at 5:40 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Nov 30, 2023 at 02:46:29PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Rework for_each_requested_gpio_in_range() to use the new helper to
> > retrieve a dynamically allocated copy of the descriptor label and free
> > it at the end of each iteration. We need to leverage the CLASS()'
> > destructor to make sure that the label is freed even when breaking out
> > of the loop.
>
> ...
>
> >  const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset);
> >  char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset);
> >
> > +
>
> One blank line is enough.
>
> > +struct _gpiochip_for_each_data {
> > +     const char **label;
> > +     int *i;
>
> Why is this a signed?
>

Some users use signed, others use unsigned. It doesn't matter as we
can't overflow it with the limit on the lines we have.

Bart

> > +};
>
> ...
>
> > +DEFINE_CLASS(_gpiochip_for_each_data,
> > +          struct _gpiochip_for_each_data,
> > +          if (*_T.label) kfree(*_T.label),
> > +          ({ struct _gpiochip_for_each_data _data = { label, i };
> > +             *_data.i = 0;
> > +             _data; }),
>
> To me indentation of ({}) is quite weird. Where is this style being used
> instead of more readable
>

There are no guidelines for this type of C abuse AFAIK. The macro may
be ugly but at least it hides the details from users which look nice
instead.

Bart

>         ({
>            ...
>         })
>
> ?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v2 09/10] gpiolib: use gpiochip_dup_line_label() in for_each helpers

[Andy Shevchenko]

On Thu, Nov 30, 2023 at 06:42:37PM +0100, Bartosz Golaszewski wrote:
> On Thu, Nov 30, 2023 at 5:40 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Nov 30, 2023 at 02:46:29PM +0100, Bartosz Golaszewski wrote:

...

> > >  const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset);
> > >  char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset);
> > >
> > > +
> >
> > One blank line is enough.
> >
> > > +struct _gpiochip_for_each_data {
> > > +     const char **label;
> > > +     int *i;
> >
> > Why is this a signed?
> 
> Some users use signed, others use unsigned. It doesn't matter as we
> can't overflow it with the limit on the lines we have.

What's the problem to make it unsigned and be done with that for good?

> > > +};

...

> > > +DEFINE_CLASS(_gpiochip_for_each_data,
> > > +          struct _gpiochip_for_each_data,
> > > +          if (*_T.label) kfree(*_T.label),
> > > +          ({ struct _gpiochip_for_each_data _data = { label, i };
> > > +             *_data.i = 0;
> > > +             _data; }),
> >
> > To me indentation of ({}) is quite weird. Where is this style being used
> > instead of more readable
> 
> There are no guidelines for this type of C abuse AFAIK. The macro may
> be ugly but at least it hides the details from users which look nice
> instead.

If we can make it more readable for free, why not doing that way?

> >         ({
> >            ...
> >         })
> >
> > ?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 10/10] gpiolib: remove gpiochip_is_requested()

[Bartosz Golaszewski]

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

We have no external users of gpiochip_is_requested(). Let's remove it
and replace its internal calls with direct testing of the REQUESTED flag.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c      | 46 ++++++++++---------------------------
 include/linux/gpio/driver.h |  1 -
 2 files changed, 12 insertions(+), 35 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 8e932e6a6a8d..3070a4f7bbb1 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1085,7 +1085,7 @@ void gpiochip_remove(struct gpio_chip *gc)
 
 	spin_lock_irqsave(&gpio_lock, flags);
 	for (i = 0; i < gdev->ngpio; i++) {
-		if (gpiochip_is_requested(gc, i))
+		if (test_bit(FLAG_REQUESTED, &gdev->descs[i].flags))
 			break;
 	}
 	spin_unlock_irqrestore(&gpio_lock, flags);
@@ -2373,33 +2373,6 @@ void gpiod_free(struct gpio_desc *desc)
 	gpio_device_put(desc->gdev);
 }
 
-/**
- * gpiochip_is_requested - return string iff signal was requested
- * @gc: controller managing the signal
- * @offset: of signal within controller's 0..(ngpio - 1) range
- *
- * Returns NULL if the GPIO is not currently requested, else a string.
- * The string returned is the label passed to gpio_request(); if none has been
- * passed it is a meaningless, non-NULL constant.
- *
- * This function is for use by GPIO controller drivers.  The label can
- * help with diagnostics, and knowing that the signal is used as a GPIO
- * can help avoid accidentally multiplexing it to another controller.
- */
-const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset)
-{
-	struct gpio_desc *desc;
-
-	desc = gpiochip_get_desc(gc, offset);
-	if (IS_ERR(desc))
-		return NULL;
-
-	if (test_bit(FLAG_REQUESTED, &desc->flags) == 0)
-		return NULL;
-	return desc->label;
-}
-EXPORT_SYMBOL_GPL(gpiochip_is_requested);
-
 /**
  * gpiochip_dup_line_label - Get a copy of the consumer label.
  * @gc: GPIO chip controlling this line.
@@ -2414,16 +2387,21 @@ EXPORT_SYMBOL_GPL(gpiochip_is_requested);
  */
 char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset)
 {
-	const char *label;
+	struct gpio_desc *desc;
 	char *cpy;
 
-	label = gpiochip_is_requested(gc, offset);
-	if (!label)
+	desc = gpiochip_get_desc(gc, offset);
+	if (IS_ERR(desc))
 		return NULL;
 
-	cpy = kstrdup(label, GFP_KERNEL);
-	if (!cpy)
-		return ERR_PTR(-ENOMEM);
+	scoped_guard(spinlock_irqsave, &gpio_lock) {
+		if (!test_bit(FLAG_REQUESTED, &desc->flags))
+			return NULL;
+
+		cpy = kstrdup(desc->label, GFP_KERNEL);
+		if (!cpy)
+			return ERR_PTR(-ENOMEM);
+	}
 
 	return cpy;
 }
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index b1ed501e9ee0..9a44016789c8 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -531,7 +531,6 @@ struct gpio_chip {
 #endif /* CONFIG_OF_GPIO */
 };
 
-const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset);
 char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset);
 
 
-- 
2.40.1

Re: [PATCH v2 10/10] gpiolib: remove gpiochip_is_requested()

[Andy Shevchenko]

On Thu, Nov 30, 2023 at 02:46:30PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We have no external users of gpiochip_is_requested(). Let's remove it
> and replace its internal calls with direct testing of the REQUESTED flag.

...

> -	cpy = kstrdup(label, GFP_KERNEL);
> -	if (!cpy)
> -		return ERR_PTR(-ENOMEM);
> +	scoped_guard(spinlock_irqsave, &gpio_lock) {
> +		if (!test_bit(FLAG_REQUESTED, &desc->flags))
> +			return NULL;

> +		cpy = kstrdup(desc->label, GFP_KERNEL);
> +		if (!cpy)
> +			return ERR_PTR(-ENOMEM);

You just introduced these lines earlier in the series, and here you moved
them again. With guard() instead it may be kept in a better shape.

> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 10/10] gpiolib: remove gpiochip_is_requested()

[Bartosz Golaszewski]

On Thu, Nov 30, 2023 at 5:46 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Nov 30, 2023 at 02:46:30PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > We have no external users of gpiochip_is_requested(). Let's remove it
> > and replace its internal calls with direct testing of the REQUESTED flag.
>
> ...
>
> > -     cpy = kstrdup(label, GFP_KERNEL);
> > -     if (!cpy)
> > -             return ERR_PTR(-ENOMEM);
> > +     scoped_guard(spinlock_irqsave, &gpio_lock) {
> > +             if (!test_bit(FLAG_REQUESTED, &desc->flags))
> > +                     return NULL;
>
> > +             cpy = kstrdup(desc->label, GFP_KERNEL);
> > +             if (!cpy)
> > +                     return ERR_PTR(-ENOMEM);
>
> You just introduced these lines earlier in the series, and here you moved
> them again. With guard() instead it may be kept in a better shape.
>

I wanted to limit the critical section to a minimum hence scoped
variant. And this will go away as soon as we have a desc lock so it's
temporary anyway. What matters to me is how the code looks when
sending it to Torvalds. On the off chance that we don't get the
locking rework merged in time for v6.8, I want this to at least be
under the existing lock.

Bart

> > +     }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v2 10/10] gpiolib: remove gpiochip_is_requested()

[Andy Shevchenko]

On Thu, Nov 30, 2023 at 06:46:20PM +0100, Bartosz Golaszewski wrote:
> On Thu, Nov 30, 2023 at 5:46 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Nov 30, 2023 at 02:46:30PM +0100, Bartosz Golaszewski wrote:

...

> > > -     cpy = kstrdup(label, GFP_KERNEL);
> > > -     if (!cpy)
> > > -             return ERR_PTR(-ENOMEM);
> > > +     scoped_guard(spinlock_irqsave, &gpio_lock) {
> > > +             if (!test_bit(FLAG_REQUESTED, &desc->flags))
> > > +                     return NULL;
> >
> > > +             cpy = kstrdup(desc->label, GFP_KERNEL);
> > > +             if (!cpy)
> > > +                     return ERR_PTR(-ENOMEM);
> >
> > You just introduced these lines earlier in the series, and here you moved
> > them again. With guard() instead it may be kept in a better shape.
> >
> 
> I wanted to limit the critical section to a minimum hence scoped
> variant. And this will go away as soon as we have a desc lock so it's
> temporary anyway. What matters to me is how the code looks when
> sending it to Torvalds. On the off chance that we don't get the
> locking rework merged in time for v6.8, I want this to at least be
> under the existing lock.

guard() here is equally scoped, no? And what's wrong with that when gets
to Torvalds? He accepted your guard() cases last time IIRC.

> > > +     }

-- 
With Best Regards,
Andy Shevchenko