[PATCH 0/5] Another batch of dtc checks

[PATCH 0/5] Another batch of dtc checks

[Rob Herring]

Here's another batch of dtc checks. The first 3 patches are pretty
straightforward. The last 2 patches are a bit more subjective.

For aliases, I would like to see them restricted to a documented list of
names. There are some platforms I've seen that seem to just define
aliases for many devices and most are not needed. The main use of
aliases in the Linux kernel is to provide fixed device numbers to
userspace, but this is an anti-goal of in the kernel.

Rob

Rob Herring (5):
  checks: add a string checks for label, bootargs and stdout-path
  checks: add string list check
  checks: add string list check for *-names properties
  checks: check for #{size,address}-cells without child nodes
  checks: add aliases node checks

 checks.c                   | 123 +++++++++++++++++++++++++++++++++++++++++++++
 tests/bad-string-props.dts |  11 ++++
 tests/run_tests.sh         |   2 +-
 3 files changed, 135 insertions(+), 1 deletion(-)

-- 
2.14.1

[PATCH 1/5] checks: add a string checks for label, bootargs and stdout-path

[Rob Herring]

Add more string property checks for label, bootargs, and stdout-path.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c                   | 4 ++++
 tests/bad-string-props.dts | 3 +++
 tests/run_tests.sh         | 2 +-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/checks.c b/checks.c
index f5bf5f97a3ad..a4a9d37ca19b 100644
--- a/checks.c
+++ b/checks.c
@@ -586,6 +586,9 @@ WARNING_IF_NOT_CELL(interrupt_cells_is_cell, "#interrupt-cells");
 WARNING_IF_NOT_STRING(device_type_is_string, "device_type");
 WARNING_IF_NOT_STRING(model_is_string, "model");
 WARNING_IF_NOT_STRING(status_is_string, "status");
+WARNING_IF_NOT_STRING(label_is_string, "label");
+WARNING_IF_NOT_STRING(bootargs_is_string, "bootargs");
+WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path");
 
 static void fixup_addr_size_cells(struct check *c, struct dt_info *dti,
 				  struct node *node)
@@ -1236,6 +1239,7 @@ static struct check *check_table[] = {
 
 	&address_cells_is_cell, &size_cells_is_cell, &interrupt_cells_is_cell,
 	&device_type_is_string, &model_is_string, &status_is_string,
+	&label_is_string, &bootargs_is_string, &stdout_path_is_string,
 
 	&property_name_chars_strict,
 	&node_name_chars_strict,
diff --git a/tests/bad-string-props.dts b/tests/bad-string-props.dts
index 396f82069cf7..9b5a7a1736ee 100644
--- a/tests/bad-string-props.dts
+++ b/tests/bad-string-props.dts
@@ -4,4 +4,7 @@
 	device_type = <0xdeadbeef>;
 	model = <0xdeadbeef>;
 	status = <0xdeadbeef>;
+	bootargs = <0xdeadbeef>;
+	stdout-path = <0xdeadbeef>;
+	label = <0xdeadbeef>;
 };
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 850bc165e757..c610aaeb053e 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -546,7 +546,7 @@ dtc_tests () {
     check_tests bad-name-property.dts name_properties
 
     check_tests bad-ncells.dts address_cells_is_cell size_cells_is_cell interrupt_cells_is_cell
-    check_tests bad-string-props.dts device_type_is_string model_is_string status_is_string
+    check_tests bad-string-props.dts device_type_is_string model_is_string status_is_string bootargs_is_string stdout_path_is_string label_is_string
     check_tests bad-reg-ranges.dts reg_format ranges_format
     check_tests bad-empty-ranges.dts ranges_format
     check_tests reg-ranges-root.dts reg_format ranges_format
-- 
2.14.1

[PATCH 2/5] checks: add string list check

[Rob Herring]

Add a check for string list properties with compatible being the first
check.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c                   | 34 ++++++++++++++++++++++++++++++++++
 tests/bad-string-props.dts |  8 ++++++++
 tests/run_tests.sh         |  2 +-
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/checks.c b/checks.c
index a4a9d37ca19b..4e23f29486bb 100644
--- a/checks.c
+++ b/checks.c
@@ -179,6 +179,36 @@ static void check_is_string(struct check *c, struct dt_info *dti,
 #define ERROR_IF_NOT_STRING(nm, propname) \
 	ERROR(nm, check_is_string, (propname))
 
+static void check_is_string_list(struct check *c, struct dt_info *dti,
+				 struct node *node)
+{
+	int rem, l;
+	struct property *prop;
+	char *propname = c->data;
+	char *str;
+
+	prop = get_property(node, propname);
+	if (!prop)
+		return; /* Not present, assumed ok */
+
+	str = prop->val.val;
+	rem = prop->val.len;
+	while (rem > 0) {
+		l = strnlen(str, rem);
+		if (l == rem) {
+			FAIL(c, dti, "\"%s\" property in %s is not a string list",
+			     propname, node->fullpath);
+			break;
+		}
+		rem -= l + 1;
+		str += l + 1;
+	}
+}
+#define WARNING_IF_NOT_STRING_LIST(nm, propname) \
+	WARNING(nm, check_is_string_list, (propname))
+#define ERROR_IF_NOT_STRING_LIST(nm, propname) \
+	ERROR(nm, check_is_string_list, (propname))
+
 static void check_is_cell(struct check *c, struct dt_info *dti,
 			  struct node *node)
 {
@@ -590,6 +620,8 @@ WARNING_IF_NOT_STRING(label_is_string, "label");
 WARNING_IF_NOT_STRING(bootargs_is_string, "bootargs");
 WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path");
 
+WARNING_IF_NOT_STRING_LIST(compatible_is_string_list, "compatible");
+
 static void fixup_addr_size_cells(struct check *c, struct dt_info *dti,
 				  struct node *node)
 {
@@ -1241,6 +1273,8 @@ 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,
+
 	&property_name_chars_strict,
 	&node_name_chars_strict,
 
diff --git a/tests/bad-string-props.dts b/tests/bad-string-props.dts
index 9b5a7a1736ee..5e226ce0c736 100644
--- a/tests/bad-string-props.dts
+++ b/tests/bad-string-props.dts
@@ -7,4 +7,12 @@
 	bootargs = <0xdeadbeef>;
 	stdout-path = <0xdeadbeef>;
 	label = <0xdeadbeef>;
+
+	node1 {
+		compatible = <0xdeadbeef>;
+	};
+
+	node2 {
+		compatible = "good", <0xdeadbeef>;
+	};
 };
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index c610aaeb053e..f492cf78ae84 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -546,7 +546,7 @@ dtc_tests () {
     check_tests bad-name-property.dts name_properties
 
     check_tests bad-ncells.dts address_cells_is_cell size_cells_is_cell interrupt_cells_is_cell
-    check_tests bad-string-props.dts device_type_is_string model_is_string status_is_string bootargs_is_string stdout_path_is_string label_is_string
+    check_tests bad-string-props.dts device_type_is_string model_is_string status_is_string bootargs_is_string stdout_path_is_string label_is_string compatible_is_string_list
     check_tests bad-reg-ranges.dts reg_format ranges_format
     check_tests bad-empty-ranges.dts ranges_format
     check_tests reg-ranges-root.dts reg_format ranges_format
-- 
2.14.1

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

[Rob Herring]

Add a string list check for common properties ending in "-names" such as
reg-names or interrupt-names.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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"))
+			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,
-- 
2.14.1

[PATCH 4/5] checks: check for #{size,address}-cells without child nodes

[Rob Herring]

nodes with a 'reg' property nor a ranges property.

An exception may be an overlay that adds nodes, but this case would need

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/checks.c b/checks.c
index 346b0256f9cb..a785b81bea07 100644
--- a/checks.c
+++ b/checks.c
@@ -982,6 +982,31 @@ static void check_avoid_default_addr_size(struct check *c, struct dt_info *dti,
 WARNING(avoid_default_addr_size, check_avoid_default_addr_size, NULL,
 	&addr_size_cells);
 
+static void check_avoid_unecessary_addr_size(struct check *c, struct dt_info *dti,
+					     struct node *node)
+{
+	struct property *prop;
+	struct node *child;
+	bool has_reg = false;
+
+	if (!node->parent || node->addr_cells < 0 || node->size_cells < 0)
+		return;
+
+	if (get_property(node, "ranges") || !node->children)
+		return;
+
+	for_each_child(node, child) {
+		prop = get_property(child, "reg");
+		if (prop)
+			has_reg = true;
+	}
+
+	if (!has_reg)
+		FAIL(c, dti, "unnecessary #address-cells/#size-cells without \"ranges\" or child \"reg\" property in %s",
+		     node->fullpath);
+}
+WARNING(avoid_unecessary_addr_size, check_avoid_unecessary_addr_size, NULL, &avoid_default_addr_size);
+
 static void check_obsolete_chosen_interrupt_controller(struct check *c,
 						       struct dt_info *dti,
 						       struct node *node)
@@ -1306,6 +1331,7 @@ static struct check *check_table[] = {
 	&simple_bus_reg,
 
 	&avoid_default_addr_size,
+	&avoid_unecessary_addr_size,
 	&obsolete_chosen_interrupt_controller,
 
 	&clocks_property,
-- 
2.14.1

[PATCH 5/5] checks: add aliases node checks

[Rob Herring]

Add checks for aliases node that properties are a valid path and that the
alias property names are a known, standard name.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/checks.c b/checks.c
index a785b81bea07..2c5b6c2eacb3 100644
--- a/checks.c
+++ b/checks.c
@@ -637,6 +637,48 @@ static void check_names_is_string_list(struct check *c, struct dt_info *dti,
 }
 WARNING(names_is_string_list, check_names_is_string_list, NULL);
 
+static void check_alias_paths(struct check *c, struct dt_info *dti,
+				    struct node *node)
+{
+	struct property *prop;
+
+	if (!streq(node->name, "aliases"))
+		return;
+
+	for_each_property(node, prop) {
+		if (!prop->val.val || !get_node_by_path(dti->dt, prop->val.val))
+			FAIL(c, dti, "aliases property '%s' is not a valid node (%s)",
+			     prop->name, prop->val.val);
+	}
+}
+WARNING(alias_paths, check_alias_paths, NULL);
+
+static void check_known_aliases(struct check *c, struct dt_info *dti,
+				    struct node *node)
+{
+	int i;
+	struct property *prop;
+	static char *aliases_strings[] = {
+		"ethernet", "gpio", "i2c", "rtc", "serial", "spi"
+	};
+
+	if (!streq(node->name, "aliases"))
+		return;
+
+	for_each_property(node, prop) {
+		for (i = 0; i < ARRAY_SIZE(aliases_strings); i++) {
+			if (strstarts(prop->name, aliases_strings[i]))
+				break;
+		}
+
+		if (i == ARRAY_SIZE(aliases_strings)) {
+			FAIL(c, dti, "unknown alias name %s", prop->name);
+			continue;
+		}
+	}
+}
+WARNING(known_aliases, check_known_aliases, NULL);
+
 static void fixup_addr_size_cells(struct check *c, struct dt_info *dti,
 				  struct node *node)
 {
@@ -1355,6 +1397,8 @@ static struct check *check_table[] = {
 	&gpios_property,
 	&interrupts_property,
 
+	&known_aliases,	&alias_paths,
+
 	&always_fail,
 };
 
-- 
2.14.1

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

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

[Andre Przywara]

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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rob Herring]

On Fri, Nov 17, 2017 at 9:12 AM, Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> 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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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

Re: [PATCH 1/5] checks: add a string checks for label, bootargs and stdout-path

[David Gibson]

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

On Fri, Nov 17, 2017 at 08:45:11AM -0600, Rob Herring wrote:
> Add more string property checks for label, bootargs, and stdout-path.

Where does 'label' appear?  I'm not immediately recalling it as a
property with global meaning.

bootargs and stdout-path are from /chosen of course.  I have some
mixed feelings about whether it's reasonable to check it's a string
everywhere, rather than specifically just in /chosen.

> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  checks.c                   | 4 ++++
>  tests/bad-string-props.dts | 3 +++
>  tests/run_tests.sh         | 2 +-
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/checks.c b/checks.c
> index f5bf5f97a3ad..a4a9d37ca19b 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -586,6 +586,9 @@ WARNING_IF_NOT_CELL(interrupt_cells_is_cell, "#interrupt-cells");
>  WARNING_IF_NOT_STRING(device_type_is_string, "device_type");
>  WARNING_IF_NOT_STRING(model_is_string, "model");
>  WARNING_IF_NOT_STRING(status_is_string, "status");
> +WARNING_IF_NOT_STRING(label_is_string, "label");
> +WARNING_IF_NOT_STRING(bootargs_is_string, "bootargs");
> +WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path");
>  
>  static void fixup_addr_size_cells(struct check *c, struct dt_info *dti,
>  				  struct node *node)
> @@ -1236,6 +1239,7 @@ static struct check *check_table[] = {
>  
>  	&address_cells_is_cell, &size_cells_is_cell, &interrupt_cells_is_cell,
>  	&device_type_is_string, &model_is_string, &status_is_string,
> +	&label_is_string, &bootargs_is_string, &stdout_path_is_string,
>  
>  	&property_name_chars_strict,
>  	&node_name_chars_strict,
> diff --git a/tests/bad-string-props.dts b/tests/bad-string-props.dts
> index 396f82069cf7..9b5a7a1736ee 100644
> --- a/tests/bad-string-props.dts
> +++ b/tests/bad-string-props.dts
> @@ -4,4 +4,7 @@
>  	device_type = <0xdeadbeef>;
>  	model = <0xdeadbeef>;
>  	status = <0xdeadbeef>;
> +	bootargs = <0xdeadbeef>;
> +	stdout-path = <0xdeadbeef>;
> +	label = <0xdeadbeef>;
>  };
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 850bc165e757..c610aaeb053e 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -546,7 +546,7 @@ dtc_tests () {
>      check_tests bad-name-property.dts name_properties
>  
>      check_tests bad-ncells.dts address_cells_is_cell size_cells_is_cell interrupt_cells_is_cell
> -    check_tests bad-string-props.dts device_type_is_string model_is_string status_is_string
> +    check_tests bad-string-props.dts device_type_is_string model_is_string status_is_string bootargs_is_string stdout_path_is_string label_is_string
>      check_tests bad-reg-ranges.dts reg_format ranges_format
>      check_tests bad-empty-ranges.dts ranges_format
>      check_tests reg-ranges-root.dts reg_format ranges_format

-- 
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 2/5] checks: add string list check

[David Gibson]

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

On Fri, Nov 17, 2017 at 08:45:12AM -0600, Rob Herring wrote:
> Add a check for string list properties with compatible being the first
> check.
> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Not applying right now, while resolving queries on 1/5.

> ---
>  checks.c                   | 34 ++++++++++++++++++++++++++++++++++
>  tests/bad-string-props.dts |  8 ++++++++
>  tests/run_tests.sh         |  2 +-
>  3 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/checks.c b/checks.c
> index a4a9d37ca19b..4e23f29486bb 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -179,6 +179,36 @@ static void check_is_string(struct check *c, struct dt_info *dti,
>  #define ERROR_IF_NOT_STRING(nm, propname) \
>  	ERROR(nm, check_is_string, (propname))
>  
> +static void check_is_string_list(struct check *c, struct dt_info *dti,
> +				 struct node *node)
> +{
> +	int rem, l;
> +	struct property *prop;
> +	char *propname = c->data;
> +	char *str;
> +
> +	prop = get_property(node, propname);
> +	if (!prop)
> +		return; /* Not present, assumed ok */
> +
> +	str = prop->val.val;
> +	rem = prop->val.len;
> +	while (rem > 0) {
> +		l = strnlen(str, rem);
> +		if (l == rem) {
> +			FAIL(c, dti, "\"%s\" property in %s is not a string list",
> +			     propname, node->fullpath);
> +			break;
> +		}
> +		rem -= l + 1;
> +		str += l + 1;
> +	}
> +}
> +#define WARNING_IF_NOT_STRING_LIST(nm, propname) \
> +	WARNING(nm, check_is_string_list, (propname))
> +#define ERROR_IF_NOT_STRING_LIST(nm, propname) \
> +	ERROR(nm, check_is_string_list, (propname))
> +
>  static void check_is_cell(struct check *c, struct dt_info *dti,
>  			  struct node *node)
>  {
> @@ -590,6 +620,8 @@ WARNING_IF_NOT_STRING(label_is_string, "label");
>  WARNING_IF_NOT_STRING(bootargs_is_string, "bootargs");
>  WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path");
>  
> +WARNING_IF_NOT_STRING_LIST(compatible_is_string_list, "compatible");
> +
>  static void fixup_addr_size_cells(struct check *c, struct dt_info *dti,
>  				  struct node *node)
>  {
> @@ -1241,6 +1273,8 @@ 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,
> +
>  	&property_name_chars_strict,
>  	&node_name_chars_strict,
>  
> diff --git a/tests/bad-string-props.dts b/tests/bad-string-props.dts
> index 9b5a7a1736ee..5e226ce0c736 100644
> --- a/tests/bad-string-props.dts
> +++ b/tests/bad-string-props.dts
> @@ -7,4 +7,12 @@
>  	bootargs = <0xdeadbeef>;
>  	stdout-path = <0xdeadbeef>;
>  	label = <0xdeadbeef>;
> +
> +	node1 {
> +		compatible = <0xdeadbeef>;
> +	};
> +
> +	node2 {
> +		compatible = "good", <0xdeadbeef>;
> +	};
>  };
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index c610aaeb053e..f492cf78ae84 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -546,7 +546,7 @@ dtc_tests () {
>      check_tests bad-name-property.dts name_properties
>  
>      check_tests bad-ncells.dts address_cells_is_cell size_cells_is_cell interrupt_cells_is_cell
> -    check_tests bad-string-props.dts device_type_is_string model_is_string status_is_string bootargs_is_string stdout_path_is_string label_is_string
> +    check_tests bad-string-props.dts device_type_is_string model_is_string status_is_string bootargs_is_string stdout_path_is_string label_is_string compatible_is_string_list
>      check_tests bad-reg-ranges.dts reg_format ranges_format
>      check_tests bad-empty-ranges.dts ranges_format
>      check_tests reg-ranges-root.dts reg_format ranges_format

-- 
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 3/5] checks: add string list check for *-names properties

[David Gibson]

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

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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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

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

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

[David Gibson]

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

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-5wv7dgnIgG8@public.gmane.org> 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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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

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

Re: [PATCH 4/5] checks: check for #{size,address}-cells without child nodes

[David Gibson]

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

On Fri, Nov 17, 2017 at 08:45:14AM -0600, Rob Herring wrote:
> nodes with a 'reg' property nor a ranges property.
> 
> An exception may be an overlay that adds nodes, but this case would
> need

Sentence doesn't seem finished..

In any case, I'm not sure this is a good idea.  It's not uncommon to
have bus bridge nodes that ought to have a well defined #address and
#size cells, but just don't happen to have any plugged devices yet.
An overlay that adds nodes is one possibility, but a bus where the
children can be probed is another.

The check for 'ranges' will get some of those cases, but a bus bridge
which doesn't directly map the child address space into the parents
(e.g. indirect access) is perfectly legitimate still.

> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  checks.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/checks.c b/checks.c
> index 346b0256f9cb..a785b81bea07 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -982,6 +982,31 @@ static void check_avoid_default_addr_size(struct check *c, struct dt_info *dti,
>  WARNING(avoid_default_addr_size, check_avoid_default_addr_size, NULL,
>  	&addr_size_cells);
>  
> +static void check_avoid_unecessary_addr_size(struct check *c, struct dt_info *dti,
> +					     struct node *node)
> +{
> +	struct property *prop;
> +	struct node *child;
> +	bool has_reg = false;
> +
> +	if (!node->parent || node->addr_cells < 0 || node->size_cells < 0)
> +		return;
> +
> +	if (get_property(node, "ranges") || !node->children)
> +		return;
> +
> +	for_each_child(node, child) {
> +		prop = get_property(child, "reg");
> +		if (prop)
> +			has_reg = true;
> +	}
> +
> +	if (!has_reg)
> +		FAIL(c, dti, "unnecessary #address-cells/#size-cells without \"ranges\" or child \"reg\" property in %s",
> +		     node->fullpath);
> +}
> +WARNING(avoid_unecessary_addr_size, check_avoid_unecessary_addr_size, NULL, &avoid_default_addr_size);
> +
>  static void check_obsolete_chosen_interrupt_controller(struct check *c,
>  						       struct dt_info *dti,
>  						       struct node *node)
> @@ -1306,6 +1331,7 @@ static struct check *check_table[] = {
>  	&simple_bus_reg,
>  
>  	&avoid_default_addr_size,
> +	&avoid_unecessary_addr_size,
>  	&obsolete_chosen_interrupt_controller,
>  
>  	&clocks_property,

-- 
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 5/5] checks: add aliases node checks

[David Gibson]

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

On Fri, Nov 17, 2017 at 08:45:15AM -0600, Rob Herring wrote:
> Add checks for aliases node that properties are a valid path and that the
> alias property names are a known, standard name.
> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

The check for valid paths is definitely good.  I'm not convinced by
the restrictive list of alias names though.

It's true that in trees originating from dts files, aliases don't have
a lot of purpose, but in trees from real OF that's not so.  It's
pretty reasonable for those to define arbitrary aliases for user
convenience - and the names here aren't even the common ones for that
purpose: those are "net", "disk" and "cd" so you can do things like
"boot disk" from the user interface without having to enter a hideous
path giving PCI and SCSI addresses.

> ---
>  checks.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/checks.c b/checks.c
> index a785b81bea07..2c5b6c2eacb3 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -637,6 +637,48 @@ static void check_names_is_string_list(struct check *c, struct dt_info *dti,
>  }
>  WARNING(names_is_string_list, check_names_is_string_list, NULL);
>  
> +static void check_alias_paths(struct check *c, struct dt_info *dti,
> +				    struct node *node)
> +{
> +	struct property *prop;
> +
> +	if (!streq(node->name, "aliases"))
> +		return;
> +
> +	for_each_property(node, prop) {
> +		if (!prop->val.val || !get_node_by_path(dti->dt, prop->val.val))
> +			FAIL(c, dti, "aliases property '%s' is not a valid node (%s)",
> +			     prop->name, prop->val.val);
> +	}
> +}
> +WARNING(alias_paths, check_alias_paths, NULL);
> +
> +static void check_known_aliases(struct check *c, struct dt_info *dti,
> +				    struct node *node)
> +{
> +	int i;
> +	struct property *prop;
> +	static char *aliases_strings[] = {
> +		"ethernet", "gpio", "i2c", "rtc", "serial", "spi"
> +	};
> +
> +	if (!streq(node->name, "aliases"))
> +		return;
> +
> +	for_each_property(node, prop) {
> +		for (i = 0; i < ARRAY_SIZE(aliases_strings); i++) {
> +			if (strstarts(prop->name, aliases_strings[i]))
> +				break;
> +		}
> +
> +		if (i == ARRAY_SIZE(aliases_strings)) {
> +			FAIL(c, dti, "unknown alias name %s", prop->name);
> +			continue;
> +		}
> +	}
> +}
> +WARNING(known_aliases, check_known_aliases, NULL);
> +
>  static void fixup_addr_size_cells(struct check *c, struct dt_info *dti,
>  				  struct node *node)
>  {
> @@ -1355,6 +1397,8 @@ static struct check *check_table[] = {
>  	&gpios_property,
>  	&interrupts_property,
>  
> +	&known_aliases,	&alias_paths,
> +
>  	&always_fail,
>  };
>  

-- 
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 4/5] checks: check for #{size,address}-cells without child nodes

[Rob Herring]

On Sun, Nov 19, 2017 at 6:14 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Fri, Nov 17, 2017 at 08:45:14AM -0600, Rob Herring wrote:
>> nodes with a 'reg' property nor a ranges property.
>>
>> An exception may be an overlay that adds nodes, but this case would
>> need
>
> Sentence doesn't seem finished..

It was there until I rebased. Since the line started with #, git dropped it...

"#{size,address}-cells in the overlay to properly compile already."

> In any case, I'm not sure this is a good idea.  It's not uncommon to
> have bus bridge nodes that ought to have a well defined #address and
> #size cells, but just don't happen to have any plugged devices yet.
> An overlay that adds nodes is one possibility, but a bus where the
> children can be probed is another.

Wouldn't an overlay without #{size,address}-cells have warnings from
avoid_default_addr_size?

As long as there are no child nodes, the check is not run. So we're
limited to false positives if we have a mixture of nodes with and
without unit addresses and only the nodes without unit addresses are
populated. I have seen this with PCI hosts with an interrupt
controller child node.

In general, I'm struggling with how to have tests that check for
things that we generally want to avoid, but we still allow exceptions.
Some of these may be things we want to avoid in new bindings, but
wouldn't fix for existing cases. Another example is things that
were/are valid for OF, but not FDT.

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

Re: [PATCH 1/5] checks: add a string checks for label, bootargs and stdout-path

[Rob Herring]

On Sun, Nov 19, 2017 at 6:09 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Fri, Nov 17, 2017 at 08:45:11AM -0600, Rob Herring wrote:
>> Add more string property checks for label, bootargs, and stdout-path.
>
> Where does 'label' appear?  I'm not immediately recalling it as a
> property with global meaning.

It's documented in DT spec/ePAPR. It can be anywhere. It's the
property for the human readable identifiers such as the label on
ethernet ports, serial ports, LEDs, etc.

> bootargs and stdout-path are from /chosen of course.  I have some
> mixed feelings about whether it's reasonable to check it's a string
> everywhere, rather than specifically just in /chosen.

We don't really want to see the same property names with different
meanings. I think checking the location is a separate check. You've
generally suggested splitting up tests rather than have all in one
tests. Not sure if we should check known properties only appear in
chosen or that chosen only has known properties (assuming we can
enumerate that list. Maybe we allow things like "linux,*").

Rob

Re: [PATCH 1/5] checks: add a string checks for label, bootargs and stdout-path

[David Gibson]

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

On Mon, Nov 20, 2017 at 11:26:49AM -0600, Rob Herring wrote:
> On Sun, Nov 19, 2017 at 6:09 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Fri, Nov 17, 2017 at 08:45:11AM -0600, Rob Herring wrote:
> >> Add more string property checks for label, bootargs, and stdout-path.
> >
> > Where does 'label' appear?  I'm not immediately recalling it as a
> > property with global meaning.
> 
> It's documented in DT spec/ePAPR. It can be anywhere. It's the
> property for the human readable identifiers such as the label on
> ethernet ports, serial ports, LEDs, etc.

Ah, ok.  I'd completely forgotten about that one.  Ok, that's a fine
check then.

> > bootargs and stdout-path are from /chosen of course.  I have some
> > mixed feelings about whether it's reasonable to check it's a string
> > everywhere, rather than specifically just in /chosen.
> 
> We don't really want to see the same property names with different
> meanings.

Hm.  As a rule of thumb, yes, but I don't know that it's super
important for properties that aren't very common.  Even then /chosen
is really special.

> I think checking the location is a separate check. You've
> generally suggested splitting up tests rather than have all in one
> tests. Not sure if we should check known properties only appear in
> chosen or that chosen only has known properties (assuming we can
> enumerate that list. Maybe we allow things like "linux,*").

I think for now a better approach would be to add one or more checks
specifically to validate the /chosen node.

-- 
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 4/5] checks: check for #{size,address}-cells without child nodes

[David Gibson]

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

On Mon, Nov 20, 2017 at 10:22:53AM -0600, Rob Herring wrote:
> On Sun, Nov 19, 2017 at 6:14 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Fri, Nov 17, 2017 at 08:45:14AM -0600, Rob Herring wrote:
> >> nodes with a 'reg' property nor a ranges property.
> >>
> >> An exception may be an overlay that adds nodes, but this case would
> >> need
> >
> > Sentence doesn't seem finished..
> 
> It was there until I rebased. Since the line started with #, git dropped it...
> 
> "#{size,address}-cells in the overlay to properly compile already."

Ah, right.

> > In any case, I'm not sure this is a good idea.  It's not uncommon to
> > have bus bridge nodes that ought to have a well defined #address and
> > #size cells, but just don't happen to have any plugged devices yet.
> > An overlay that adds nodes is one possibility, but a bus where the
> > children can be probed is another.
> 
> Wouldn't an overlay without #{size,address}-cells have warnings from
> avoid_default_addr_size?

Well, yes.  The checks are pretty much designed for whole trees and
don't work well on overlays at the moment.  That's a rather more
substantial change to fix.

> As long as there are no child nodes, the check is not run.

Ah!  Sorry, I missed that.  Objection withdrawn in that case.

> So we're
> limited to false positives if we have a mixture of nodes with and
> without unit addresses and only the nodes without unit addresses are
> populated. I have seen this with PCI hosts with an interrupt
> controller child node.

Sounds like an incorrect representation if the intrrupt controller
doesn't have a PCI config space presence.

> In general, I'm struggling with how to have tests that check for
> things that we generally want to avoid, but we still allow exceptions.
> Some of these may be things we want to avoid in new bindings, but
> wouldn't fix for existing cases. Another example is things that
> were/are valid for OF, but not FDT.

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