[PATCH] checks: stop allowing nr-gpio, only allow nr-gpios

[PATCH] checks: stop allowing nr-gpio, only allow nr-gpios

[Ilya Lipnitskiy]

There are no instances of nr-gpio in the Linux kernel tree, only
"[<vendor>,]nr-gpios", so make the check stricter to not allow nr-gpio.

Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 checks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/checks.c b/checks.c
index c4207720ce8c..2800b5786b03 100644
--- a/checks.c
+++ b/checks.c
@@ -1494,7 +1494,7 @@ static bool prop_is_gpio(struct property *prop)
 	 * *-gpios and *-gpio can appear in property names,
 	 * so skip over any false matches (only one known ATM)
 	 */
-	if (strstr(prop->name, "nr-gpio"))
+	if (strstr(prop->name, "nr-gpios"))
 		return false;
 
 	str = strrchr(prop->name, '-');
-- 
2.31.1

[PATCH v2] checks: tigthen up nr-gpios prop exception

[Ilya Lipnitskiy]

There are no instances of nr-gpio in the Linux kernel tree, only
"[<vendor>,]nr-gpios", so make the check stricter to not allow nr-gpio.

Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 checks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/checks.c b/checks.c
index c4207720ce8c..2800b5786b03 100644
--- a/checks.c
+++ b/checks.c
@@ -1494,7 +1494,7 @@ static bool prop_is_gpio(struct property *prop)
 	 * *-gpios and *-gpio can appear in property names,
 	 * so skip over any false matches (only one known ATM)
 	 */
-	if (strstr(prop->name, "nr-gpio"))
+	if (strstr(prop->name, "nr-gpios"))
 		return false;
 
 	str = strrchr(prop->name, '-');
-- 
2.31.1

[PATCH v3] checks: tigthen up nr-gpios prop exception

[Ilya Lipnitskiy]

There are no instances of nr-gpio in the Linux kernel tree, only
"[<vendor>,]nr-gpios", so make the check stricter.

nr-gpios without a "vendor," prefix is also invalid, according to the DT
spec[0], and there are no DT files in the Linux kernel tree with
non-vendor nr-gpios. There are some drivers, but they are not DT spec
compliant, so don't suppress the check for them.

[0]:
Link: https://github.com/devicetree-org/dt-schema/blob/cb53a16a1eb3e2169ce170c071e47940845ec26e/schemas/gpio/gpio-consumer.yaml#L20

Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/checks.c b/checks.c
index c4207720ce8c..7e9d73af02a3 100644
--- a/checks.c
+++ b/checks.c
@@ -1494,7 +1494,7 @@ static bool prop_is_gpio(struct property *prop)
 	 * *-gpios and *-gpio can appear in property names,
 	 * so skip over any false matches (only one known ATM)
 	 */
-	if (strstr(prop->name, "nr-gpio"))
+	if (strstr(prop->name, ",nr-gpios"))
 		return false;
 
 	str = strrchr(prop->name, '-');
-- 
2.31.1

Re: [PATCH v3] checks: tigthen up nr-gpios prop exception

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1740 bytes --]

On Mon, Apr 05, 2021 at 04:28:55PM -0700, Ilya Lipnitskiy wrote:
> There are no instances of nr-gpio in the Linux kernel tree, only
> "[<vendor>,]nr-gpios", so make the check stricter.
> 
> nr-gpios without a "vendor," prefix is also invalid, according to the DT
> spec[0], and there are no DT files in the Linux kernel tree with
> non-vendor nr-gpios. There are some drivers, but they are not DT spec
> compliant, so don't suppress the check for them.
> 
> [0]:
> Link: https://github.com/devicetree-org/dt-schema/blob/cb53a16a1eb3e2169ce170c071e47940845ec26e/schemas/gpio/gpio-consumer.yaml#L20
> 
> Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Applied, since it looks like it definitely improves the current
situation.

The use of strstr() at all seems kind of bogus to me.  Shouldn't we
explicitly be checking that ",nr-gpios" forms the *end* of the
property name?

> ---
>  checks.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/checks.c b/checks.c
> index c4207720ce8c..7e9d73af02a3 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -1494,7 +1494,7 @@ static bool prop_is_gpio(struct property *prop)
>  	 * *-gpios and *-gpio can appear in property names,
>  	 * so skip over any false matches (only one known ATM)
>  	 */
> -	if (strstr(prop->name, "nr-gpio"))
> +	if (strstr(prop->name, ",nr-gpios"))
>  		return false;
>  
>  	str = strrchr(prop->name, '-');

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3] checks: tigthen up nr-gpios prop exception

[Ilya Lipnitskiy]

On Wed, Apr 7, 2021 at 7:59 PM David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Mon, Apr 05, 2021 at 04:28:55PM -0700, Ilya Lipnitskiy wrote:
> > There are no instances of nr-gpio in the Linux kernel tree, only
> > "[<vendor>,]nr-gpios", so make the check stricter.
> >
> > nr-gpios without a "vendor," prefix is also invalid, according to the DT
> > spec[0], and there are no DT files in the Linux kernel tree with
> > non-vendor nr-gpios. There are some drivers, but they are not DT spec
> > compliant, so don't suppress the check for them.
> >
> > [0]:
> > Link: https://github.com/devicetree-org/dt-schema/blob/cb53a16a1eb3e2169ce170c071e47940845ec26e/schemas/gpio/gpio-consumer.yaml#L20
> >
> > Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>
> Applied, since it looks like it definitely improves the current
> situation.
>
> The use of strstr() at all seems kind of bogus to me.  Shouldn't we
> explicitly be checking that ",nr-gpios" forms the *end* of the
> property name?
Yeah, I noticed that too. We could replace strstr with something
similar to strcmp_suffix in drivers/of/property.c:

static int strcmp_suffix(const char *str, const char *suffix)
{
        unsigned int len, suffix_len;

        len = strlen(str);
        suffix_len = strlen(suffix);
        if (len <= suffix_len)
                return -1;
        return strcmp(str + len - suffix_len, suffix);
}

Ilya

Re: [PATCH v3] checks: tigthen up nr-gpios prop exception

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2050 bytes --]

On Sat, Apr 10, 2021 at 11:20:54AM -0700, Ilya Lipnitskiy wrote:
> On Wed, Apr 7, 2021 at 7:59 PM David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Mon, Apr 05, 2021 at 04:28:55PM -0700, Ilya Lipnitskiy wrote:
> > > There are no instances of nr-gpio in the Linux kernel tree, only
> > > "[<vendor>,]nr-gpios", so make the check stricter.
> > >
> > > nr-gpios without a "vendor," prefix is also invalid, according to the DT
> > > spec[0], and there are no DT files in the Linux kernel tree with
> > > non-vendor nr-gpios. There are some drivers, but they are not DT spec
> > > compliant, so don't suppress the check for them.
> > >
> > > [0]:
> > > Link: https://github.com/devicetree-org/dt-schema/blob/cb53a16a1eb3e2169ce170c071e47940845ec26e/schemas/gpio/gpio-consumer.yaml#L20
> > >
> > > Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >
> > Applied, since it looks like it definitely improves the current
> > situation.
> >
> > The use of strstr() at all seems kind of bogus to me.  Shouldn't we
> > explicitly be checking that ",nr-gpios" forms the *end* of the
> > property name?
> Yeah, I noticed that too. We could replace strstr with something
> similar to strcmp_suffix in drivers/of/property.c:
> 
> static int strcmp_suffix(const char *str, const char *suffix)
> {
>         unsigned int len, suffix_len;
> 
>         len = strlen(str);
>         suffix_len = strlen(suffix);
>         if (len <= suffix_len)
>                 return -1;
>         return strcmp(str + len - suffix_len, suffix);
> }

Right.  If you add something like this, you should reverse the sense
and call it 'strends' to match 'strstarts' which we already define in
dtc.h.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]