[PATCH] gpio-uclass: fix gpio lookup by label

[PATCH] gpio-uclass: fix gpio lookup by label

[Rasmus Villemoes]

Matching anything that just happens to have the sought-for label as a
prefix is wrong. For example, if the board designer has designated 10
lines for debug purposes, named "debug1" through "debug10", and we are
looking up "debug1", if debug10 happens to be met first during the
iteration we'd wrongly return that.

In theory, this can break existing users that could rely on this
quirk, but OTOH keeping the current broken semantics can cause a lot
of grief for people hitting this in the future and not understanding
why they don't find the line they expect. Considering how few in-tree
defconfigs currently set DM_GPIO_LOOKUP_LABEL (ignoring sandbox, only
four "real" boards), let's fix it before the use becomes more
widespread.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/gpio/gpio-uclass.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
index 0ed32b7217..01342202fa 100644
--- a/drivers/gpio/gpio-uclass.c
+++ b/drivers/gpio/gpio-uclass.c
@@ -91,15 +91,13 @@ static int gpio_to_device(unsigned int gpio, struct gpio_desc *desc)
 static int dm_gpio_lookup_label(const char *name,
 				struct gpio_dev_priv *uc_priv, ulong *offset)
 {
-	int len;
 	int i;
 
 	*offset = -1;
-	len = strlen(name);
 	for (i = 0; i < uc_priv->gpio_count; i++) {
 		if (!uc_priv->name[i])
 			continue;
-		if (!strncmp(name, uc_priv->name[i], len)) {
+		if (!strcmp(name, uc_priv->name[i])) {
 			*offset = i;
 			return 0;
 		}
-- 
2.37.2

Re: [PATCH] gpio-uclass: fix gpio lookup by label

[Simon Glass]

On Mon, 3 Oct 2022 at 03:03, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Matching anything that just happens to have the sought-for label as a
> prefix is wrong. For example, if the board designer has designated 10
> lines for debug purposes, named "debug1" through "debug10", and we are
> looking up "debug1", if debug10 happens to be met first during the
> iteration we'd wrongly return that.
>
> In theory, this can break existing users that could rely on this
> quirk, but OTOH keeping the current broken semantics can cause a lot
> of grief for people hitting this in the future and not understanding
> why they don't find the line they expect. Considering how few in-tree
> defconfigs currently set DM_GPIO_LOOKUP_LABEL (ignoring sandbox, only
> four "real" boards), let's fix it before the use becomes more
> widespread.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/gpio/gpio-uclass.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

It seems like we need a one-sided strncmp():

int strncmp(const char *match, const char *str, int max_len_of_str)

Re: [PATCH] gpio-uclass: fix gpio lookup by label

[Rasmus Villemoes]

On 04/10/2022 01.49, Simon Glass wrote:
> On Mon, 3 Oct 2022 at 03:03, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> Matching anything that just happens to have the sought-for label as a
>> prefix is wrong. For example, if the board designer has designated 10
>> lines for debug purposes, named "debug1" through "debug10", and we are
>> looking up "debug1", if debug10 happens to be met first during the
>> iteration we'd wrongly return that.
>>
>> In theory, this can break existing users that could rely on this
>> quirk, but OTOH keeping the current broken semantics can cause a lot
>> of grief for people hitting this in the future and not understanding
>> why they don't find the line they expect. Considering how few in-tree
>> defconfigs currently set DM_GPIO_LOOKUP_LABEL (ignoring sandbox, only
>> four "real" boards), let's fix it before the use becomes more
>> widespread.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>  drivers/gpio/gpio-uclass.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> It seems like we need a one-sided strncmp():
> 
> int strncmp(const char *match, const char *str, int max_len_of_str)

Eh, could you spell out what the semantics of that one would be? I can't
think of anything that would differ from actual strncmp().

  for (i = 0; i < m; ++i) {
    if (a[i] != b[i]) return a[i] - b[i];
    if (!a[i]) break;
  }
  return 0;

And why would we want anything other than actual string equality here?

Oh, and now that I look at lib/string.c, both the generic strcmp() and
strncmp() are broken. They correctly distinguish equality/inequality (up
to the given bound), but they don't compare strings correctly, for
several reasons. Sigh. Patch coming later.

Rasmus

Re: [PATCH] gpio-uclass: fix gpio lookup by label

[Simon Glass]

Hi Rasmus,

On Tue, 4 Oct 2022 at 07:43, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 04/10/2022 01.49, Simon Glass wrote:
> > On Mon, 3 Oct 2022 at 03:03, Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> >>
> >> Matching anything that just happens to have the sought-for label as a
> >> prefix is wrong. For example, if the board designer has designated 10
> >> lines for debug purposes, named "debug1" through "debug10", and we are
> >> looking up "debug1", if debug10 happens to be met first during the
> >> iteration we'd wrongly return that.
> >>
> >> In theory, this can break existing users that could rely on this
> >> quirk, but OTOH keeping the current broken semantics can cause a lot
> >> of grief for people hitting this in the future and not understanding
> >> why they don't find the line they expect. Considering how few in-tree
> >> defconfigs currently set DM_GPIO_LOOKUP_LABEL (ignoring sandbox, only
> >> four "real" boards), let's fix it before the use becomes more
> >> widespread.
> >>
> >> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> >> ---
> >>  drivers/gpio/gpio-uclass.c | 4 +---
> >>  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > It seems like we need a one-sided strncmp():
> >
> > int strncmp(const char *match, const char *str, int max_len_of_str)
>
> Eh, could you spell out what the semantics of that one would be? I can't
> think of anything that would differ from actual strncmp().
>
>   for (i = 0; i < m; ++i) {
>     if (a[i] != b[i]) return a[i] - b[i];
>     if (!a[i]) break;
>   }
>   return 0;
>
> And why would we want anything other than actual string equality here?

Your patch is fine which is why I added the tag.

>
> Oh, and now that I look at lib/string.c, both the generic strcmp() and
> strncmp() are broken. They correctly distinguish equality/inequality (up
> to the given bound), but they don't compare strings correctly, for
> several reasons. Sigh. Patch coming later.

OK.

Re the one-sided strncmp(), I mean that the second string has to be at
least as long as the first, but can be longer.

Regards,
Simon

Re: [PATCH] gpio-uclass: fix gpio lookup by label

[Heiko Schocher]

Hello Rasmus,

On 03.10.22 11:02, Rasmus Villemoes wrote:
> Matching anything that just happens to have the sought-for label as a
> prefix is wrong. For example, if the board designer has designated 10
> lines for debug purposes, named "debug1" through "debug10", and we are
> looking up "debug1", if debug10 happens to be met first during the
> iteration we'd wrongly return that.
> 
> In theory, this can break existing users that could rely on this
> quirk, but OTOH keeping the current broken semantics can cause a lot
> of grief for people hitting this in the future and not understanding
> why they don't find the line they expect. Considering how few in-tree
> defconfigs currently set DM_GPIO_LOOKUP_LABEL (ignoring sandbox, only
> four "real" boards), let's fix it before the use becomes more
> widespread.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/gpio/gpio-uclass.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

Re: [PATCH] gpio-uclass: fix gpio lookup by label

[Simon Glass]

Hello Rasmus,

On 03.10.22 11:02, Rasmus Villemoes wrote:
> Matching anything that just happens to have the sought-for label as a
> prefix is wrong. For example, if the board designer has designated 10
> lines for debug purposes, named "debug1" through "debug10", and we are
> looking up "debug1", if debug10 happens to be met first during the
> iteration we'd wrongly return that.
>
> In theory, this can break existing users that could rely on this
> quirk, but OTOH keeping the current broken semantics can cause a lot
> of grief for people hitting this in the future and not understanding
> why they don't find the line they expect. Considering how few in-tree
> defconfigs currently set DM_GPIO_LOOKUP_LABEL (ignoring sandbox, only
> four "real" boards), let's fix it before the use becomes more
> widespread.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/gpio/gpio-uclass.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

Applied to u-boot-dm, thanks!