[PATCH] gpiolib: extend the critical sections of lookup tables

[PATCH] gpiolib: extend the critical sections of lookup tables

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

There are two places in the code where we retrieve a lookup table using
gpiod_find_lookup_table() (which protects the table list with the lookup
table lock) and then use it after the lock is released.

We need to keep the lookup table mutex locked the entire time we're using
the tables. Remove the locking from gpiod_find_lookup_table() and use
guards to protect the code actually using the table objects.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

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;
 
-- 
2.39.2

Re: [PATCH] gpiolib: extend the critical sections of lookup tables

[Linus Walleij]

On Wed, Sep 20, 2023 at 10:56 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> There are two places in the code where we retrieve a lookup table using
> gpiod_find_lookup_table() (which protects the table list with the lookup
> table lock) and then use it after the lock is released.
>
> We need to keep the lookup table mutex locked the entire time we're using
> the tables. Remove the locking from gpiod_find_lookup_table() and use
> guards to protect the code actually using the table objects.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

I convinced myself that this change is correct, good find!

> @@ -3822,8 +3822,6 @@ static struct gpiod_lookup_table *gpiod_find_lookup_table(struct device *dev)

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?

Either way:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH] gpiolib: extend the critical sections of lookup tables

[Bartosz Golaszewski]

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:
>
>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>
>> There are two places in the code where we retrieve a lookup table using
>> gpiod_find_lookup_table() (which protects the table list with the lookup
>> table lock) and then use it after the lock is released.
>>
>> We need to keep the lookup table mutex locked the entire time we're using
>> the tables. Remove the locking from gpiod_find_lookup_table() and use
>> guards to protect the code actually using the table objects.
>>
>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> I convinced myself that this change is correct, good find!
>
>> @@ -3822,8 +3822,6 @@ static struct gpiod_lookup_table *gpiod_find_lookup_table(struct device *dev)
>
> 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?

Bart

> Either way:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Yours,
> Linus Walleij
>

Re: [PATCH] gpiolib: extend the critical sections of lookup tables

[Linus Walleij]

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.

Yours,
Linus Walleij

Re: [PATCH] gpiolib: extend the critical sections of lookup tables

[Bartosz Golaszewski]

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

Re: [PATCH] gpiolib: extend the critical sections of lookup tables

[Bartosz Golaszewski]

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

Re: [PATCH] gpiolib: extend the critical sections of lookup tables

[Bartosz Golaszewski]

On Mon, Sep 25, 2023 at 4:24 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> 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

If there are no objections, I'd like to queue it this week.

Bart

Re: [PATCH] gpiolib: extend the critical sections of lookup tables

[Linus Walleij]

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