| Message ID | 20230912100727.23197-8-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | gpiolib: work towards removing gpiochip_find() | expand |
On Tue, Sep 12, 2023 at 12:07:23PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Remove all remaining uses of find_chip_by_name() (and subsequently: > gpiochip_find()) from gpiolib.c and use the new > gpio_device_find_by_label() instead. ... > for (hog = &hogs[0]; hog->chip_label; hog++) { > + struct gpio_device *gdev __free(gpio_device_put) = NULL; In the loop?! How does it work when loop goes second iteration and so on? > list_add_tail(&hog->list, &gpio_machine_hogs); > > /* > * The chip may have been registered earlier, so check if it > * exists and, if so, try to hog the line now. > */ > - gc = find_chip_by_name(hog->chip_label); > - if (gc) > - gpiochip_machine_hog(gc, hog); > + gdev = gpio_device_find_by_label(hog->chip_label); > + if (gdev) > + gpiochip_machine_hog(gpio_device_get_chip(gdev), hog); So, do we expect the chip_label be different between hogs? Ah, seems so as it covers _all_ hogs in the system. > } Even if the __free() scope works fine, I think this algorithm should be revisited to make sure we have iterating only on hogs of the same chip. Hence, the hogs should be placed into tree structure with a label being the key in it. ... > + struct gpio_device *gdev __free(gpio_device_put) = NULL; > + gc = gpio_device_get_chip(gdev); Similar wish here, perhaps maple tree can be utilized in the future for both of them.
On Tue, Sep 12, 2023 at 12:07:23PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Remove all remaining uses of find_chip_by_name() (and subsequently: > gpiochip_find()) from gpiolib.c and use the new > gpio_device_find_by_label() instead. ... > for (p = &table->table[0]; p->key; p++) { > - struct gpio_chip *gc; > + struct gpio_device *gdev __free(gpio_device_put) = NULL; > + gc = gpio_device_get_chip(gdev); What the heck is this, btw? You have gdev NULL here. > /* idx must always match exactly */ > if (p->idx != idx) > @@ -4004,9 +3996,8 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id, > return ERR_PTR(-EPROBE_DEFER); > } > > - gc = find_chip_by_name(p->key); > - > - if (!gc) { > + gdev = gpio_device_find_by_label(p->key); > + if (!gdev) { ... > if (gc->ngpio <= p->chip_hwnum) { > dev_err(dev, > "requested GPIO %u (%u) is out of range [0..%u] for chip %s\n", > - idx, p->chip_hwnum, gc->ngpio - 1, > + idx, p->chip_hwnum, gdev->chip->ngpio - 1, In other patch you use wrapper to get gdev->chip, why not here? > gc->label); Is this gc is different to gdev->chip? > return ERR_PTR(-EINVAL); > } ... Sorry, but this patch seems to me as WIP. Please, revisit it, make sure all things are done consistently.
On Tue, Sep 12, 2023 at 1:08 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Sep 12, 2023 at 12:07:23PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Remove all remaining uses of find_chip_by_name() (and subsequently: > > gpiochip_find()) from gpiolib.c and use the new > > gpio_device_find_by_label() instead. > > ... > > > for (hog = &hogs[0]; hog->chip_label; hog++) { > > + struct gpio_device *gdev __free(gpio_device_put) = NULL; > > In the loop?! How does it work when loop goes second iteration and so on? > This works fine, the variable goes out of scope (reference is put) on every iteration. Bart > > list_add_tail(&hog->list, &gpio_machine_hogs); > > > > /* > > * The chip may have been registered earlier, so check if it > > * exists and, if so, try to hog the line now. > > */ > > - gc = find_chip_by_name(hog->chip_label); > > - if (gc) > > - gpiochip_machine_hog(gc, hog); > > + gdev = gpio_device_find_by_label(hog->chip_label); > > + if (gdev) > > + gpiochip_machine_hog(gpio_device_get_chip(gdev), hog); > > So, do we expect the chip_label be different between hogs? Ah, seems so > as it covers _all_ hogs in the system. > > > } > > Even if the __free() scope works fine, I think this algorithm should be > revisited to make sure we have iterating only on hogs of the same chip. > Hence, the hogs should be placed into tree structure with a label being > the key in it. > > ... > > > + struct gpio_device *gdev __free(gpio_device_put) = NULL; > > > + gc = gpio_device_get_chip(gdev); > > Similar wish here, perhaps maple tree can be utilized in the future for both of them. > > -- > With Best Regards, > Andy Shevchenko > >
On Tue, Sep 12, 2023 at 1:16 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Sep 12, 2023 at 12:07:23PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Remove all remaining uses of find_chip_by_name() (and subsequently: > > gpiochip_find()) from gpiolib.c and use the new > > gpio_device_find_by_label() instead. > > ... > > > for (p = &table->table[0]; p->key; p++) { > > - struct gpio_chip *gc; > > + struct gpio_device *gdev __free(gpio_device_put) = NULL; > > > + gc = gpio_device_get_chip(gdev); > > What the heck is this, btw? You have gdev NULL here. > Gah! Thanks. I relied on tests succeeding and no KASAN warnings, I need to go through this line-by-line again. Bart > > /* idx must always match exactly */ > > if (p->idx != idx) > > @@ -4004,9 +3996,8 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id, > > return ERR_PTR(-EPROBE_DEFER); > > } > > > > - gc = find_chip_by_name(p->key); > > - > > - if (!gc) { > > + gdev = gpio_device_find_by_label(p->key); > > + if (!gdev) { > > ... > > > if (gc->ngpio <= p->chip_hwnum) { > > dev_err(dev, > > "requested GPIO %u (%u) is out of range [0..%u] for chip %s\n", > > - idx, p->chip_hwnum, gc->ngpio - 1, > > + idx, p->chip_hwnum, gdev->chip->ngpio - 1, > > In other patch you use wrapper to get gdev->chip, why not here? > > > gc->label); > > Is this gc is different to gdev->chip? > > > return ERR_PTR(-EINVAL); > > } > > ... > > Sorry, but this patch seems to me as WIP. Please, revisit it, make sure all > things are done consistently. > > -- > With Best Regards, > Andy Shevchenko > >
On Tue, Sep 12, 2023 at 1:35 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Tue, Sep 12, 2023 at 1:16 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Tue, Sep 12, 2023 at 12:07:23PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > Remove all remaining uses of find_chip_by_name() (and subsequently: > > > gpiochip_find()) from gpiolib.c and use the new > > > gpio_device_find_by_label() instead. > > > > ... > > > > > for (p = &table->table[0]; p->key; p++) { > > > - struct gpio_chip *gc; > > > + struct gpio_device *gdev __free(gpio_device_put) = NULL; > > > > > + gc = gpio_device_get_chip(gdev); > > > > What the heck is this, btw? You have gdev NULL here. > > > > Gah! Thanks. I relied on tests succeeding and no KASAN warnings, I > need to go through this line-by-line again. > Fortunately, this was just an unused leftover. I fixed it for v3. Bart > Bart > > > > /* idx must always match exactly */ > > > if (p->idx != idx) > > > @@ -4004,9 +3996,8 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id, > > > return ERR_PTR(-EPROBE_DEFER); > > > } > > > > > > - gc = find_chip_by_name(p->key); > > > - > > > - if (!gc) { > > > + gdev = gpio_device_find_by_label(p->key); > > > + if (!gdev) { > > > > ... > > > > > if (gc->ngpio <= p->chip_hwnum) { > > > dev_err(dev, > > > "requested GPIO %u (%u) is out of range [0..%u] for chip %s\n", > > > - idx, p->chip_hwnum, gc->ngpio - 1, > > > + idx, p->chip_hwnum, gdev->chip->ngpio - 1, > > > > In other patch you use wrapper to get gdev->chip, why not here? > > > > > gc->label); > > > > Is this gc is different to gdev->chip? > > > > > return ERR_PTR(-EINVAL); > > > } > > > > ... > > > > Sorry, but this patch seems to me as WIP. Please, revisit it, make sure all > > things are done consistently. > > > > -- > > With Best Regards, > > Andy Shevchenko > > > >
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 224e0d330009..a10d1d663524 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1144,18 +1144,6 @@ struct gpio_device *gpio_device_find_by_label(const char *label) } EXPORT_SYMBOL_GPL(gpio_device_find_by_label); -static int gpiochip_match_name(struct gpio_chip *gc, void *data) -{ - const char *name = data; - - return !strcmp(gc->label, name); -} - -static struct gpio_chip *find_chip_by_name(const char *name) -{ - return gpiochip_find((void *)name, gpiochip_match_name); -} - /** * gpio_device_get() - Increase the reference count of this GPIO device * @gdev: GPIO device to increase the refcount for @@ -3907,21 +3895,22 @@ EXPORT_SYMBOL_GPL(gpiod_remove_lookup_table); */ void gpiod_add_hogs(struct gpiod_hog *hogs) { - struct gpio_chip *gc; struct gpiod_hog *hog; mutex_lock(&gpio_machine_hogs_mutex); for (hog = &hogs[0]; hog->chip_label; hog++) { + struct gpio_device *gdev __free(gpio_device_put) = NULL; + list_add_tail(&hog->list, &gpio_machine_hogs); /* * The chip may have been registered earlier, so check if it * exists and, if so, try to hog the line now. */ - gc = find_chip_by_name(hog->chip_label); - if (gc) - gpiochip_machine_hog(gc, hog); + gdev = gpio_device_find_by_label(hog->chip_label); + if (gdev) + gpiochip_machine_hog(gpio_device_get_chip(gdev), hog); } mutex_unlock(&gpio_machine_hogs_mutex); @@ -3976,13 +3965,16 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id, struct gpio_desc *desc = ERR_PTR(-ENOENT); struct gpiod_lookup_table *table; struct gpiod_lookup *p; + struct gpio_chip *gc; table = gpiod_find_lookup_table(dev); if (!table) return desc; for (p = &table->table[0]; p->key; p++) { - struct gpio_chip *gc; + struct gpio_device *gdev __free(gpio_device_put) = NULL; + + gc = gpio_device_get_chip(gdev); /* idx must always match exactly */ if (p->idx != idx) @@ -4004,9 +3996,8 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id, return ERR_PTR(-EPROBE_DEFER); } - gc = find_chip_by_name(p->key); - - if (!gc) { + gdev = gpio_device_find_by_label(p->key); + if (!gdev) { /* * As the lookup table indicates a chip with * p->key should exist, assume it may @@ -4022,7 +4013,7 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id, if (gc->ngpio <= p->chip_hwnum) { dev_err(dev, "requested GPIO %u (%u) is out of range [0..%u] for chip %s\n", - idx, p->chip_hwnum, gc->ngpio - 1, + idx, p->chip_hwnum, gdev->chip->ngpio - 1, gc->label); return ERR_PTR(-EINVAL); }