[3/5] checks: add string list check for *-names properties

Message ID 20171117144515.10870-4-robh@kernel.org
State New
Headers show
Series
  • Another batch of dtc checks
Related show

Commit Message

Rob Herring Nov. 17, 2017, 2:45 p.m.
Add a string list check for common properties ending in "-names" such as
reg-names or interrupt-names.

Signed-off-by: Rob Herring <robh@kernel.org>

---
 checks.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Andre Przywara Nov. 17, 2017, 3:12 p.m. | #1
Hi,

On 17/11/17 14:45, Rob Herring wrote:
> Add a string list check for common properties ending in "-names" such as

> reg-names or interrupt-names.

> 

> Signed-off-by: Rob Herring <robh@kernel.org>

> ---

>  checks.c | 17 ++++++++++++++++-

>  1 file changed, 16 insertions(+), 1 deletion(-)

> 

> diff --git a/checks.c b/checks.c

> index 4e23f29486bb..346b0256f9cb 100644

> --- a/checks.c

> +++ b/checks.c

> @@ -622,6 +622,21 @@ WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path");

>  

>  WARNING_IF_NOT_STRING_LIST(compatible_is_string_list, "compatible");

>  

> +static void check_names_is_string_list(struct check *c, struct dt_info *dti,

> +				       struct node *node)

> +{

> +	struct property *prop;

> +

> +	for_each_property(node, prop) {

> +		if (!strstr(prop->name, "-names"))


But that would match "actually-names-dont-matter" as well, resulting in
a false positive? Should we check if the string appears at the *end* of
the property name?

Cheers,
Andre.

> +			continue;

> +

> +		c->data = prop->name;

> +		check_is_string_list(c, dti, node);

> +	}

> +}

> +WARNING(names_is_string_list, check_names_is_string_list, NULL);

> +

>  static void fixup_addr_size_cells(struct check *c, struct dt_info *dti,

>  				  struct node *node)

>  {

> @@ -1273,7 +1288,7 @@ static struct check *check_table[] = {

>  	&device_type_is_string, &model_is_string, &status_is_string,

>  	&label_is_string, &bootargs_is_string, &stdout_path_is_string,

>  

> -	&compatible_is_string_list,

> +	&compatible_is_string_list, &names_is_string_list,

>  

>  	&property_name_chars_strict,

>  	&node_name_chars_strict,

> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Nov. 17, 2017, 6:07 p.m. | #2
On Fri, Nov 17, 2017 at 9:12 AM, Andre Przywara <andre.przywara@arm.com> wrote:
> Hi,

>

> On 17/11/17 14:45, Rob Herring wrote:

>> Add a string list check for common properties ending in "-names" such as

>> reg-names or interrupt-names.

>>

>> Signed-off-by: Rob Herring <robh@kernel.org>

>> ---

>>  checks.c | 17 ++++++++++++++++-

>>  1 file changed, 16 insertions(+), 1 deletion(-)

>>

>> diff --git a/checks.c b/checks.c

>> index 4e23f29486bb..346b0256f9cb 100644

>> --- a/checks.c

>> +++ b/checks.c

>> @@ -622,6 +622,21 @@ WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path");

>>

>>  WARNING_IF_NOT_STRING_LIST(compatible_is_string_list, "compatible");

>>

>> +static void check_names_is_string_list(struct check *c, struct dt_info *dti,

>> +                                    struct node *node)

>> +{

>> +     struct property *prop;

>> +

>> +     for_each_property(node, prop) {

>> +             if (!strstr(prop->name, "-names"))

>

> But that would match "actually-names-dont-matter" as well, resulting in

> a false positive? Should we check if the string appears at the *end* of

> the property name?


Perhaps. IMO, once a word is used, it needs to be reserved for that
purpose. For example, the gpio hogs binding use of "gpios" with just
numbers and no phandle is bad because we have a mixture of types for a
given property name or suffix. So we should really enforce that
"-names" only appears as a suffix and use anywhere else is a warning.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Gibson Nov. 20, 2017, 12:10 a.m. | #3
On Fri, Nov 17, 2017 at 03:12:02PM +0000, Andre Przywara wrote:
> Hi,

> 

> On 17/11/17 14:45, Rob Herring wrote:

> > Add a string list check for common properties ending in "-names" such as

> > reg-names or interrupt-names.

> > 

> > Signed-off-by: Rob Herring <robh@kernel.org>

> > ---

> >  checks.c | 17 ++++++++++++++++-

> >  1 file changed, 16 insertions(+), 1 deletion(-)

> > 

> > diff --git a/checks.c b/checks.c

> > index 4e23f29486bb..346b0256f9cb 100644

> > --- a/checks.c

> > +++ b/checks.c

> > @@ -622,6 +622,21 @@ WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path");

> >  

> >  WARNING_IF_NOT_STRING_LIST(compatible_is_string_list, "compatible");

> >  

> > +static void check_names_is_string_list(struct check *c, struct dt_info *dti,

> > +				       struct node *node)

> > +{

> > +	struct property *prop;

> > +

> > +	for_each_property(node, prop) {

> > +		if (!strstr(prop->name, "-names"))

> 

> But that would match "actually-names-dont-matter" as well, resulting in

> a false positive? Should we check if the string appears at the *end* of

> the property name?


Yes, we should.

-- 
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
David Gibson Nov. 20, 2017, 12:11 a.m. | #4
On Fri, Nov 17, 2017 at 12:07:48PM -0600, Rob Herring wrote:
> On Fri, Nov 17, 2017 at 9:12 AM, Andre Przywara <andre.przywara@arm.com> wrote:

> > Hi,

> >

> > On 17/11/17 14:45, Rob Herring wrote:

> >> Add a string list check for common properties ending in "-names" such as

> >> reg-names or interrupt-names.

> >>

> >> Signed-off-by: Rob Herring <robh@kernel.org>

> >> ---

> >>  checks.c | 17 ++++++++++++++++-

> >>  1 file changed, 16 insertions(+), 1 deletion(-)

> >>

> >> diff --git a/checks.c b/checks.c

> >> index 4e23f29486bb..346b0256f9cb 100644

> >> --- a/checks.c

> >> +++ b/checks.c

> >> @@ -622,6 +622,21 @@ WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path");

> >>

> >>  WARNING_IF_NOT_STRING_LIST(compatible_is_string_list, "compatible");

> >>

> >> +static void check_names_is_string_list(struct check *c, struct dt_info *dti,

> >> +                                    struct node *node)

> >> +{

> >> +     struct property *prop;

> >> +

> >> +     for_each_property(node, prop) {

> >> +             if (!strstr(prop->name, "-names"))

> >

> > But that would match "actually-names-dont-matter" as well, resulting in

> > a false positive? Should we check if the string appears at the *end* of

> > the property name?

> 

> Perhaps. IMO, once a word is used, it needs to be reserved for that

> purpose. For example, the gpio hogs binding use of "gpios" with just

> numbers and no phandle is bad because we have a mixture of types for a

> given property name or suffix. So we should really enforce that

> "-names" only appears as a suffix and use anywhere else is a warning.


That sounds... overly restrictive to me.  The grabbing of a whole
bunch of gpios words is an example of poor - or at least nonstandard -
binding design IMO.

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

Patch

diff --git a/checks.c b/checks.c
index 4e23f29486bb..346b0256f9cb 100644
--- a/checks.c
+++ b/checks.c
@@ -622,6 +622,21 @@  WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path");
 
 WARNING_IF_NOT_STRING_LIST(compatible_is_string_list, "compatible");
 
+static void check_names_is_string_list(struct check *c, struct dt_info *dti,
+				       struct node *node)
+{
+	struct property *prop;
+
+	for_each_property(node, prop) {
+		if (!strstr(prop->name, "-names"))
+			continue;
+
+		c->data = prop->name;
+		check_is_string_list(c, dti, node);
+	}
+}
+WARNING(names_is_string_list, check_names_is_string_list, NULL);
+
 static void fixup_addr_size_cells(struct check *c, struct dt_info *dti,
 				  struct node *node)
 {
@@ -1273,7 +1288,7 @@  static struct check *check_table[] = {
 	&device_type_is_string, &model_is_string, &status_is_string,
 	&label_is_string, &bootargs_is_string, &stdout_path_is_string,
 
-	&compatible_is_string_list,
+	&compatible_is_string_list, &names_is_string_list,
 
 	&property_name_chars_strict,
 	&node_name_chars_strict,