gpio: mmio: handle "ngpios" properly in bgpio_init()

PATCH Superseded Asmaa Mnebhi 3 participants 5 messages Mar 3, 2023 – Mar 6, 2023 GPIO

Share & tools

Asmaa Mnebhi

author

2023-03-03 22:16 UTC #0 lore raw

PATCH v3 1/1 gpio: mmio: handle "ngpios" properly in bgpio_init()

bgpio_init() uses "sz" argument to populate ngpio, which is not accurate. Instead, read the "ngpios" property from the DT and if it doesn't exist, use the "sz" argument. With this change, drivers no longer need to overwrite the ngpio variable after calling bgpio_init.

Signed-off-by Asmaa Mnebhi <[email protected]>

---
 drivers/gpio/gpio-mmio.c |  7 ++++-
 drivers/gpio/gpiolib.c   | 58 ++++++++++++++++++----------------------
 drivers/gpio/gpiolib.h   |  1 +
 3 files changed, 33 insertions(+), 33 deletions(-)

drivers/gpio/gpio-mmio.c +6 -1

index d9dff3dc92ae..c9f9f4e36c89 100644
@@ -60,6 +60,8 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
 #include <linux/of.h>
 #include <linux/of_device.h>
 
+#include "gpiolib.h"
+
 static void bgpio_write8(void __iomem *reg, unsigned long data)
 {
 	writeb(data, reg);

@@ -614,10 +616,13 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev,
 	gc->parent = dev;
 	gc->label = dev_name(dev);
 	gc->base = -1;
-	gc->ngpio = gc->bgpio_bits;
 	gc->request = bgpio_request;
 	gc->be_bits = !!(flags & BGPIOF_BIG_ENDIAN);
 
+	ret = gpiochip_get_ngpios(gc, dev);
+	if (ret)
+		gc->ngpio = gc->bgpio_bits;
+
 	ret = bgpio_setup_io(gc, dat, set, clr, flags);
 	if (ret)
 		return ret;

drivers/gpio/gpiolib.c +26 ~~-32~~

82 lines not on this page · raw message

drivers/gpio/gpiolib.h +1 -0

index b3c2db6eba80..c38cbf1b753b 100644
@@ -207,6 +207,7 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce);
 int gpiod_hog(struct gpio_desc *desc, const char *name,
 		unsigned long lflags, enum gpiod_flags dflags);
+int gpiochip_get_ngpios(struct gpio_chip *gc, struct device *dev);
 
 /*
  * Return the GPIO number of the passed descriptor relative to its chip

Signature · 1 line

2.30.1

Andy Shevchenko

2023-03-03 22:38 UTC #1 lore raw

On Fri, Mar 3, 2023 at 11:58 PM Asmaa Mnebhi <[email protected]> wrote:

Thanks for an update!
My comments below.

Quoted 4 lines

bgpio_init() uses "sz" argument to populate ngpio, which is not accurate. Instead, read the "ngpios" property from the DT and if it doesn't exist, use the "sz" argument. With this change, drivers no longer need to overwrite the ngpio variable after calling bgpio_init.

You missed adding the () to the function name here.

...

First of all there should be two patches:
1) splitting out the new helper;
2) using it in the mmio driver.

...

drivers/gpio/gpio-mmio.c int bgpio_init(struct gpio_chip *gc, struct device *dev, · line 624

Quoted 3 lines

drivers/gpio/gpio-mmio.c +3 -0

+       ret = gpiochip_get_ngpios(gc, dev);
+       if (ret)
+               gc->ngpio = gc->bgpio_bits;

But this doesn't update bgpio_bits in the success case. Can you explain why it's not a problem (should be at least in the code as a comment).

...

drivers/gpio/gpiolib.c static void gpiochip_setup_devs(void) · line 660

Quoted 12 lines

drivers/gpio/gpiolib.c +11 -0

+int gpiochip_get_ngpios(struct gpio_chip *gc, struct device *dev)
+{
+       u32 ngpios = gc->ngpio;
+       int ret;
+
+       if (ngpios == 0) {
 
+               ret = device_property_read_u32(dev, "ngpios", &ngpios);
+               if (ret) {
+                       chip_err(gc, "Failed to get ngpios property\n");
+                       return -EINVAL;
+               }

This is not an equivalent to what was in the GPIO library. Why is it so?

Quoted 9 lines

drivers/gpio/gpiolib.c +9 -0

+               gc->ngpio = ngpios;
+       }
+
+       if (gc->ngpio > FASTPATH_NGPIO)
+               chip_warn(gc, "line cnt %u is greater than fast path cnt %u\n",
+                       gc->ngpio, FASTPATH_NGPIO);
+
+       return 0;
+}

...

drivers/gpio/gpiolib.c int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, · line 901

Quoted 4 lines

                pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__,

drivers/gpio/gpiolib.c +1 -1

-                      base, base + (int)ngpios - 1,
+                      base, base + (int)gc->ngpio - 1,
                       gc->label ? : "generic", ret);

AFAIU this will give a different result to what was previous in one of the error cases.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Asmaa Mnebhi

author

2023-03-06 18:47 UTC #2 lore raw

Quoted 6 lines

drivers/gpio/gpio-mmio.c int bgpio_init(struct gpio_chip *gc, struct device *dev, · line 624
Quoted 3 lines
drivers/gpio/gpio-mmio.c +3 -0
+       ret = gpiochip_get_ngpios(gc, dev);
+       if (ret)
+               gc->ngpio = gc->bgpio_bits;
But this doesn't update bgpio_bits in the success case. Can you explain why it's not a problem (should be at least in the code as a comment).

In the success rate, the bgpio_bits would also be equal to "sz * 8" anyways.
The argument " unsigned long sz" passed in bgpio_init is specifically for this purpose. That tells the gpio library the gpio register access size. if (!is_power_of_2(sz))

                 return -EINVAL;

gc->bgpio_bits = sz * 8;

If in the success case, we make it dependent on the ngpio value, we would need to round it up anyways to the closest (power of 2 && multiple of 8) which is the same as "sz * 8" I will add a comment in the code in my next patch.

Quoted 9 lines

...

Quoted 5 lines

drivers/gpio/gpiolib.c +5 -0

+int gpiochip_get_ngpios(struct gpio_chip *gc, struct device *dev) {
+       u32 ngpios = gc->ngpio;
+       int ret;
+
+       if (ngpios == 0) {

Quoted 7 lines

drivers/gpio/gpiolib.c static void gpiochip_setup_devs(void) · line 660
Quoted 5 lines
drivers/gpio/gpiolib.c +5 -0
+               ret = device_property_read_u32(dev, "ngpios", &ngpios);
+               if (ret) {
+                       chip_err(gc, "Failed to get ngpios property\n");
+                       return -EINVAL;
+               }
This is not an equivalent to what was in the GPIO library. Why is it so?

Sure. I will keep it the same in my next patch.
The reason I didn’t is because I noticed that the final result of the logic is the same i.e. " goto err_free_dev_name"
"if(ret == -ENODATA)" is handled separately is to add an informative message: chip_err(gc, "tried to insert a GPIO chip with zero lines\n"); and return ret = -EINVAL.

Quoted 13 lines

Quoted 10 lines

drivers/gpio/gpiolib.c +9 -0

+               gc->ngpio = ngpios;
+       }
+
+       if (gc->ngpio > FASTPATH_NGPIO)
+               chip_warn(gc, "line cnt %u is greater than fast path cnt %u\n",
+                       gc->ngpio, FASTPATH_NGPIO);
+
+       return 0;
+}

...

Quoted 7 lines

drivers/gpio/gpiolib.c int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, · line 901
Quoted 4 lines
                pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__,
drivers/gpio/gpiolib.c +1 -1
-                      base, base + (int)ngpios - 1,
+                      base, base + (int)gc->ngpio - 1,
                       gc->label ? : "generic", ret);
AFAIU this will give a different result to what was previous in one of the error cases.

this one provides the "local" gpio pin id i.e. 0->31 for example. chip_warn(gc, "line cnt %u is greater than fast path cnt %u\n", gc->ngpio, FASTPATH_NGPIO);

while this one provides the "global" gpio pin id. when bgpio_init sets the base : gc->base = -1; and gpiochip_add_data_with_key applies this logic:
pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__, base, base + (int)gc->ngpio - 1,

base = gc->base;
         if (base < 0) {
                 base = gpiochip_find_base(gc->ngpio);

Then the base would be = GPIO_DYNAMIC_BASE

Apologies if I misunderstood your question?

Andy Shevchenko

2023-03-06 20:16 UTC #3 lore raw

On Mon, Mar 6, 2023 at 8:47 PM Asmaa Mnebhi <[email protected]> wrote:

...

Quoted 7 lines

Quoted 6 lines
drivers/gpio/gpio-mmio.c int bgpio_init(struct gpio_chip *gc, struct device *dev, · line 624
Quoted 3 lines
drivers/gpio/gpio-mmio.c +3 -0
+       ret = gpiochip_get_ngpios(gc, dev);
+       if (ret)
+               gc->ngpio = gc->bgpio_bits;
But this doesn't update bgpio_bits in the success case. Can you explain why it's not a problem (should be at least in the code as a comment).

Quoted 8 lines

In the success rate, the bgpio_bits would also be equal to "sz * 8" anyways.
The argument " unsigned long sz" passed in bgpio_init is specifically for this purpose. That tells the gpio library the gpio register access size. if (!is_power_of_2(sz))
                 return -EINVAL;
gc->bgpio_bits = sz * 8;
If in the success case, we make it dependent on the ngpio value, we would need to round it up anyways to the closest (power of 2 && multiple of 8) which is the same as "sz * 8" I will add a comment in the code in my next patch.

I believe we should use only a single source for what we need. If we rely on ngpios, the bgpio_bits should be recalculated based on it. The expression doing this is very simple. Something like round_up(ngpios, 8);

...

Quoted 9 lines

Quoted 7 lines
drivers/gpio/gpiolib.c static void gpiochip_setup_devs(void) · line 660
Quoted 5 lines
drivers/gpio/gpiolib.c +5 -0
+               ret = device_property_read_u32(dev, "ngpios", &ngpios);
+               if (ret) {
+                       chip_err(gc, "Failed to get ngpios property\n");
+                       return -EINVAL;
+               }
This is not an equivalent to what was in the GPIO library. Why is it so?
Sure. I will keep it the same in my next patch.

No, you should take care about error codes properly. Now you are shadowing anything to -EINVAL. With this you must keep the comment in the code untouched (moved, but untouched).

Quoted 2 lines

The reason I didn’t is because I noticed that the final result of the logic is the same i.e. " goto err_free_dev_name"
"if(ret == -ENODATA)" is handled separately is to add an informative message: chip_err(gc, "tried to insert a GPIO chip with zero lines\n"); and return ret = -EINVAL.

Yes, but you missed one out of three cases.

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
(quoted diff) +2 -0
+               gc->ngpio = ngpios;
+       }

...

Quoted 11 lines

Quoted 7 lines
drivers/gpio/gpiolib.c int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, · line 901
Quoted 4 lines
                pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__,
drivers/gpio/gpiolib.c +1 -1
-                      base, base + (int)ngpios - 1,
+                      base, base + (int)gc->ngpio - 1,
                       gc->label ? : "generic", ret);
AFAIU this will give a different result to what was previous in one of the error cases.
this one provides the "local" gpio pin id i.e. 0->31 for example. chip_warn(gc, "line cnt %u is greater than fast path cnt %u\n", gc->ngpio, FASTPATH_NGPIO);

Quoted 8 lines

while this one provides the "global" gpio pin id. when bgpio_init sets the base : gc->base = -1; and gpiochip_add_data_with_key applies this logic:
pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__, base, base + (int)gc->ngpio - 1,
base = gc->base;
         if (base < 0) {
                 base = gpiochip_find_base(gc->ngpio);
Then the base would be = GPIO_DYNAMIC_BASE
Apologies if I misunderstood your question?

I'm talking about your change. It behaves differently in case of different errors and contents of gc->ngpio and the ngpios local variable.

...

Please, check again carefully all possible branches and cases (there are few of them, not just a couple).

Signature · 2 lines

With Best Regards,
Andy Shevchenko

kernel test robot

build bot

2023-03-04 19:52 UTC #4 lore raw

In reply to Asmaa Mnebhi

I love your patch! Yet something to improve:

Hi Asmaa,

I love your patch! Yet something to improve:

[auto build test ERROR on brgl/gpio/for-next]
[also build test ERROR on linus/master v6.2 next-20230303] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Asmaa-Mnebhi/gpio-mmio-handle-ngpios-properly-in-bgpio_init/20230304-061731
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link:    https://lore.kernel.org/r/20230303215814.24783-1-asmaa%40nvidia.com
patch subject: [PATCH v3] gpio: mmio: handle "ngpios" properly in bgpio_init()
config: x86_64-randconfig-c002 (https://download.01.org/0day-ci/archive/20230305/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/3a5c9651e99e80b10d5b205d9860c131bf90b3bc
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Asmaa-Mnebhi/gpio-mmio-handle-ngpios-properly-in-bgpio_init/20230304-061731
        git checkout 3a5c9651e99e80b10d5b205d9860c131bf90b3bc
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "gpiochip_get_ngpios" [drivers/gpio/gpio-generic.ko] undefined!

Signature · 2 lines

0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests