| Message ID | 20230920085639.152441-1-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: extend the critical sections of lookup tables | expand |
On Wed, Sep 20, 2023 at 2:51 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Wed, 20 Sep 2023 12:58:58 +0200, Linus Walleij <linus.walleij@linaro.org> said: > > On Wed, Sep 20, 2023 at 11:33 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > >> On Wed, 20 Sep 2023 11:12:58 +0200, Linus Walleij > >> <linus.walleij@linaro.org> said: > >> > On Wed, Sep 20, 2023 at 10:56 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > >> > Can we rename this function gpiod_find_lookup_table_locked() > >> > as per precedents in the kernel, to indicate that it needs to be > >> > called with a lock held? > >> > > >> > >> I think you mean gpiod_find_lookup_table_unlocked() as with this change it > >> will no longer take the lock? > > > > I think the pattern is the one I indicated: *_locked() means the function > > is to be called with the appropriate lock held, cf > > arch/arm64/kvm/hyp/nvhe/mm.c > > > > pkvm_create_mappings() takes a lock and then calls > > pkvm_create_mappings_locked() which even asserts that > > the lock is held. > > > > Ha! I always though the pattern is to call the functions that *DON'T* take > the lock _unlocked(). This is what I used in gpiolib-cdev.c or gpio-sim.c. > > I guess both conventions make sense in some way. > > Bart I don't think I will be doing it just now. We don't use this convention elsewhere in drivers/gpio/ and we'll have a lot of locking reworked soon anyway. We may get to it when that is done. Bart
On Wed, Sep 27, 2023 at 9:01 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> If there are no objections, I'd like to queue it this week.
Go ahead.
Kernel looks better after this patch than before => queue it
Yours,
Linus Walleij
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index edffa0d2acaa..7c27a1efc1b0 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -3822,8 +3822,6 @@ static struct gpiod_lookup_table *gpiod_find_lookup_table(struct device *dev) const char *dev_id = dev ? dev_name(dev) : NULL; struct gpiod_lookup_table *table; - mutex_lock(&gpio_lookup_lock); - list_for_each_entry(table, &gpio_lookup_list, list) { if (table->dev_id && dev_id) { /* @@ -3831,21 +3829,18 @@ static struct gpiod_lookup_table *gpiod_find_lookup_table(struct device *dev) * a match */ if (!strcmp(table->dev_id, dev_id)) - goto found; + return table; } else { /* * One of the pointers is NULL, so both must be to have * a match */ if (dev_id == table->dev_id) - goto found; + return table; } } - table = NULL; -found: - mutex_unlock(&gpio_lookup_lock); - return table; + return NULL; } static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id, @@ -3855,6 +3850,8 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id, struct gpiod_lookup_table *table; struct gpiod_lookup *p; + guard(mutex)(&gpio_lookup_lock); + table = gpiod_find_lookup_table(dev); if (!table) return desc; @@ -3920,15 +3917,18 @@ static int platform_gpio_count(struct device *dev, const char *con_id) struct gpiod_lookup *p; unsigned int count = 0; - table = gpiod_find_lookup_table(dev); - if (!table) - return -ENOENT; + scoped_guard(mutex, &gpio_lookup_lock) { + table = gpiod_find_lookup_table(dev); + if (!table) + return -ENOENT; - for (p = &table->table[0]; p->key; p++) { - if ((con_id && p->con_id && !strcmp(con_id, p->con_id)) || - (!con_id && !p->con_id)) - count++; + for (p = &table->table[0]; p->key; p++) { + if ((con_id && p->con_id && !strcmp(con_id, p->con_id)) || + (!con_id && !p->con_id)) + count++; + } } + if (!count) return -ENOENT;