[rfc, PATCH v1 1/1] gpiolib: Get rid of never false gpio_is_valid() calls

[rfc, PATCH v1 1/1] gpiolib: Get rid of never false gpio_is_valid() calls

[Andy Shevchenko]

In the cases when gpio_is_valid() is called with unsigned parameter
the result is always true in the GPIO library code, hence the check
for false won't ever be true. Get rid of such calls.

While at it, move GPIO device base to be unsigned to clearly show
it won't ever be negative. This requires a new definition for the
maximum GPIO number in the system.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-legacy.c | 10 +++++-----
 drivers/gpio/gpiolib-sysfs.c  |  2 +-
 drivers/gpio/gpiolib.c        | 19 +++++++++----------
 drivers/gpio/gpiolib.h        |  2 +-
 include/linux/gpio.h          |  6 ++++++
 5 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/gpio/gpiolib-legacy.c b/drivers/gpio/gpiolib-legacy.c
index b138682fec3d..3392e758d36f 100644
--- a/drivers/gpio/gpiolib-legacy.c
+++ b/drivers/gpio/gpiolib-legacy.c
@@ -28,10 +28,9 @@ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
 	struct gpio_desc *desc;
 	int err;
 
-	desc = gpio_to_desc(gpio);
-
 	/* Compatibility: assume unavailable "valid" GPIOs will appear later */
-	if (!desc && gpio_is_valid(gpio))
+	desc = gpio_to_desc(gpio);
+	if (!desc)
 		return -EPROBE_DEFER;
 
 	err = gpiod_request(desc, label);
@@ -63,10 +62,11 @@ EXPORT_SYMBOL_GPL(gpio_request_one);
  */
 int gpio_request(unsigned gpio, const char *label)
 {
-	struct gpio_desc *desc = gpio_to_desc(gpio);
+	struct gpio_desc *desc;
 
 	/* Compatibility: assume unavailable "valid" GPIOs will appear later */
-	if (!desc && gpio_is_valid(gpio))
+	desc = gpio_to_desc(gpio);
+	if (!desc)
 		return -EPROBE_DEFER;
 
 	return gpiod_request(desc, label);
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 67fc09a57f26..cc5674937dd2 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -412,7 +412,7 @@ static ssize_t base_show(struct device *dev,
 {
 	const struct gpio_device *gdev = dev_get_drvdata(dev);
 
-	return sysfs_emit(buf, "%d\n", gdev->base);
+	return sysfs_emit(buf, "%u\n", gdev->base);
 }
 static DEVICE_ATTR_RO(base);
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 351b9d0f5682..1e70306d2c75 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -150,9 +150,6 @@ struct gpio_desc *gpio_to_desc(unsigned gpio)
 		}
 	}
 
-	if (!gpio_is_valid(gpio))
-		pr_warn("invalid GPIO %d\n", gpio);
-
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(gpio_to_desc);
@@ -297,10 +294,10 @@ struct gpio_chip *gpio_device_get_chip(struct gpio_device *gdev)
 EXPORT_SYMBOL_GPL(gpio_device_get_chip);
 
 /* dynamic allocation of GPIOs, e.g. on a hotplugged device */
-static int gpiochip_find_base_unlocked(int ngpio)
+static int gpiochip_find_base_unlocked(u16 ngpio)
 {
+	unsigned int base = GPIO_DYNAMIC_BASE;
 	struct gpio_device *gdev;
-	int base = GPIO_DYNAMIC_BASE;
 
 	list_for_each_entry_srcu(gdev, &gpio_devices, list,
 				 lockdep_is_held(&gpio_devices_lock)) {
@@ -311,9 +308,11 @@ static int gpiochip_find_base_unlocked(int ngpio)
 		base = gdev->base + gdev->ngpio;
 		if (base < GPIO_DYNAMIC_BASE)
 			base = GPIO_DYNAMIC_BASE;
+		if (base > GPIO_DYNAMIC_MAX - ngpio)
+			break;
 	}
 
-	if (gpio_is_valid(base)) {
+	if (base <= GPIO_DYNAMIC_MAX - ngpio) {
 		pr_debug("%s: found new base at %d\n", __func__, base);
 		return base;
 	} else {
@@ -746,7 +745,7 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
 	if (ret)
 		goto err_remove_device;
 
-	dev_dbg(&gdev->dev, "registered GPIOs %d to %d on %s\n", gdev->base,
+	dev_dbg(&gdev->dev, "registered GPIOs %u to %u on %s\n", gdev->base,
 		gdev->base + gdev->ngpio - 1, gdev->label);
 
 	return 0;
@@ -4797,14 +4796,14 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev)
 			value = gpio_chip_get_value(gc, desc);
 			is_irq = test_bit(FLAG_USED_AS_IRQ, &desc->flags);
 			active_low = test_bit(FLAG_ACTIVE_LOW, &desc->flags);
-			seq_printf(s, " gpio-%-3d (%-20.20s|%-20.20s) %s %s %s%s\n",
+			seq_printf(s, " gpio-%-3u (%-20.20s|%-20.20s) %s %s %s%s\n",
 				   gpio, desc->name ?: "", gpiod_get_label(desc),
 				   is_out ? "out" : "in ",
 				   value >= 0 ? (value ? "hi" : "lo") : "?  ",
 				   is_irq ? "IRQ " : "",
 				   active_low ? "ACTIVE LOW" : "");
 		} else if (desc->name) {
-			seq_printf(s, " gpio-%-3d (%-20.20s)\n", gpio, desc->name);
+			seq_printf(s, " gpio-%-3u (%-20.20s)\n", gpio, desc->name);
 		}
 
 		gpio++;
@@ -4876,7 +4875,7 @@ static int gpiolib_seq_show(struct seq_file *s, void *v)
 		return 0;
 	}
 
-	seq_printf(s, "%s%s: GPIOs %d-%d", priv->newline ? "\n" : "",
+	seq_printf(s, "%s%s: GPIOs %u-%u", priv->newline ? "\n" : "",
 		   dev_name(&gdev->dev),
 		   gdev->base, gdev->base + gdev->ngpio - 1);
 	parent = gc->parent;
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index ada36aa0f81a..49715607c0db 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -61,7 +61,7 @@ struct gpio_device {
 	struct module		*owner;
 	struct gpio_chip __rcu	*chip;
 	struct gpio_desc	*descs;
-	int			base;
+	unsigned int		base;
 	u16			ngpio;
 	bool			can_sleep;
 	const char		*label;
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index 4aaedcf424ce..56ac7e7a2889 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -74,6 +74,12 @@ static inline bool gpio_is_valid(int number)
  * Until they are all fixed, leave 0-512 space for them.
  */
 #define GPIO_DYNAMIC_BASE	512
+/*
+ * Define the maximum of the possible GPIO in the global numberspace.
+ * While the GPIO base and numbers are positive, we limit it with signed
+ * maximum as a lot of code is using negative values for special cases.
+ */
+#define GPIO_DYNAMIC_MAX	INT_MAX
 
 /* Always use the library code for GPIO management calls,
  * or when sleeping may be involved.
-- 
2.43.0.rc1.1.gbec44491f096

Re: [rfc, PATCH v1 1/1] gpiolib: Get rid of never false gpio_is_valid() calls

[Linus Walleij]

On Wed, Feb 21, 2024 at 10:32 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> In the cases when gpio_is_valid() is called with unsigned parameter
> the result is always true in the GPIO library code, hence the check
> for false won't ever be true. Get rid of such calls.
>
> While at it, move GPIO device base to be unsigned to clearly show
> it won't ever be negative. This requires a new definition for the
> maximum GPIO number in the system.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Looks right to me:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I guess it's related to the patch where we dropped <asm/gpio.h>
and ARCH_NR_GPIOS because after that a lot if the semantics
were removed from this function, but it's not quite a fix more of
a cleanup.

Yours,
Linus Walleij

Re: [rfc, PATCH v1 1/1] gpiolib: Get rid of never false gpio_is_valid() calls

[Bartosz Golaszewski]

On Wed, Feb 21, 2024 at 10:32 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> In the cases when gpio_is_valid() is called with unsigned parameter
> the result is always true in the GPIO library code, hence the check
> for false won't ever be true. Get rid of such calls.
>
> While at it, move GPIO device base to be unsigned to clearly show
> it won't ever be negative. This requires a new definition for the
> maximum GPIO number in the system.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---

It looks like a risky change that late in the release cycle. I want to
avoid some CI problems at rc6. Please resend it once v6.9-rc1 is
tagged.

Bart

Re: [rfc, PATCH v1 1/1] gpiolib: Get rid of never false gpio_is_valid() calls

[Andy Shevchenko]

On Tue, Feb 27, 2024 at 02:06:05PM +0100, Bartosz Golaszewski wrote:
> On Wed, Feb 21, 2024 at 10:32 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > In the cases when gpio_is_valid() is called with unsigned parameter
> > the result is always true in the GPIO library code, hence the check
> > for false won't ever be true. Get rid of such calls.
> >
> > While at it, move GPIO device base to be unsigned to clearly show
> > it won't ever be negative. This requires a new definition for the
> > maximum GPIO number in the system.

> > ---
> 
> It looks like a risky change that late in the release cycle. I want to
> avoid some CI problems at rc6. Please resend it once v6.9-rc1 is
> tagged.

Not sure why resend, but I missed that somehow. Can you consider applying it?

-- 
With Best Regards,
Andy Shevchenko

Re: [rfc, PATCH v1 1/1] gpiolib: Get rid of never false gpio_is_valid() calls

[Bartosz Golaszewski]

On Wed, Apr 17, 2024 at 12:46 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Feb 27, 2024 at 02:06:05PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Feb 21, 2024 at 10:32 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > In the cases when gpio_is_valid() is called with unsigned parameter
> > > the result is always true in the GPIO library code, hence the check
> > > for false won't ever be true. Get rid of such calls.
> > >
> > > While at it, move GPIO device base to be unsigned to clearly show
> > > it won't ever be negative. This requires a new definition for the
> > > maximum GPIO number in the system.
>
> > > ---
> >
> > It looks like a risky change that late in the release cycle. I want to
> > avoid some CI problems at rc6. Please resend it once v6.9-rc1 is
> > tagged.
>
> Not sure why resend, but I missed that somehow. Can you consider applying it?
>

Applied, thanks!

Bart

Re: [rfc, PATCH v1 1/1] gpiolib: Get rid of never false gpio_is_valid() calls

[Andy Shevchenko]

On Wed, Apr 17, 2024 at 10:47:23PM +0200, Bartosz Golaszewski wrote:
> On Wed, Apr 17, 2024 at 12:46 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Feb 27, 2024 at 02:06:05PM +0100, Bartosz Golaszewski wrote:
> > > On Wed, Feb 21, 2024 at 10:32 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > In the cases when gpio_is_valid() is called with unsigned parameter
> > > > the result is always true in the GPIO library code, hence the check
> > > > for false won't ever be true. Get rid of such calls.
> > > >
> > > > While at it, move GPIO device base to be unsigned to clearly show
> > > > it won't ever be negative. This requires a new definition for the
> > > > maximum GPIO number in the system.
> >
> > > > ---
> > >
> > > It looks like a risky change that late in the release cycle. I want to
> > > avoid some CI problems at rc6. Please resend it once v6.9-rc1 is
> > > tagged.
> >
> > Not sure why resend, but I missed that somehow. Can you consider applying it?
> 
> Applied, thanks!

Thank you!

I have grepped the kernel sources for these use cases:

  $ git grep -n -C6 '= devm_gpio_request([^A-Z]'
  $ git grep -n -C6 '= gpio_request([^A-Z]'

to see how many users might not have checked the validness of the GPIO before
passing to gpio_request(). All what I found is something like ~10 drivers.

They are basically in the risk category of my change.

Another risky part that touches everybody is the base finding algo.
I spent quite a time before sending this patch and looked at it again
to see if there is any potential flaw, but found nothing. Hopefully
we will see no reports or many and sooner than later while it sits
in Linux Next.

TL;DR: the above is a note to be in archives just in case.

-- 
With Best Regards,
Andy Shevchenko