| Message ID | 20230303215814.24783-1-asmaa@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] gpio: mmio: handle "ngpios" properly in bgpio_init() | expand |
On Fri, Mar 3, 2023 at 11:58 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote: Thanks for an update! My comments below. > 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. ... > + 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). ... > +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? > + 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; > +} ... > pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__, > - 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.
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/202303050354.HH9DhJsr-lkp@intel.com/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 <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303050354.HH9DhJsr-lkp@intel.com/
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "gpiochip_get_ngpios" [drivers/gpio/gpio-generic.ko] undefined!
> > + 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. > > ... > > > +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? 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. > > > + 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; > > +} > > ... > > > pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__, > > - 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?
On Mon, Mar 6, 2023 at 8:47 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote: ... > > > + 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. 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); ... > > > + 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). > 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. > > > + gc->ngpio = ngpios; > > > + } ... > > > pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__, > > > - 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? 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).
diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c index d9dff3dc92ae..c9f9f4e36c89 100644 --- a/drivers/gpio/gpio-mmio.c +++ b/drivers/gpio/gpio-mmio.c @@ -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; diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 939c776b9488..17b63f52fda7 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -647,6 +647,28 @@ static void gpiochip_setup_devs(void) } } +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; + } + + 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; +} + int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, struct lock_class_key *lock_key, struct lock_class_key *request_key) @@ -655,7 +677,6 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, struct gpio_device *gdev; unsigned long flags; unsigned int i; - u32 ngpios = 0; int base = 0; int ret = 0; @@ -704,36 +725,9 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, else gdev->owner = THIS_MODULE; - /* - * Try the device properties if the driver didn't supply the number - * of GPIO lines. - */ - ngpios = gc->ngpio; - if (ngpios == 0) { - ret = device_property_read_u32(&gdev->dev, "ngpios", &ngpios); - if (ret == -ENODATA) - /* - * -ENODATA means that there is no property found and - * we want to issue the error message to the user. - * Besides that, we want to return different error code - * to state that supplied value is not valid. - */ - ngpios = 0; - else if (ret) - goto err_free_dev_name; - - gc->ngpio = ngpios; - } - - if (gc->ngpio == 0) { - chip_err(gc, "tried to insert a GPIO chip with zero lines\n"); - ret = -EINVAL; - goto err_free_dev_name; - } - - if (gc->ngpio > FASTPATH_NGPIO) - chip_warn(gc, "line cnt %u is greater than fast path cnt %u\n", - gc->ngpio, FASTPATH_NGPIO); + ret = gpiochip_get_ngpios(gc, &gdev->dev); + if (ret) + return ret; gdev->descs = kcalloc(gc->ngpio, sizeof(*gdev->descs), GFP_KERNEL); if (!gdev->descs) { @@ -903,7 +897,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, /* failures here can mean systems won't boot... */ if (ret != -EPROBE_DEFER) { pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__, - base, base + (int)ngpios - 1, + base, base + (int)gc->ngpio - 1, gc->label ? : "generic", ret); } return ret; diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index b3c2db6eba80..c38cbf1b753b 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -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
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 <asmaa@nvidia.com> --- drivers/gpio/gpio-mmio.c | 7 ++++- drivers/gpio/gpiolib.c | 58 ++++++++++++++++++---------------------- drivers/gpio/gpiolib.h | 1 + 3 files changed, 33 insertions(+), 33 deletions(-)