Another batch of dtc checks

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

Signed-off-by Rob Herring <[email protected]>

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

checks.c +4 -0

index f5bf5f97a3ad..a4a9d37ca19b 100644
@@ -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,

tests/bad-string-props.dts +3 -0

index 396f82069cf7..9b5a7a1736ee 100644
@@ -4,4 +4,7 @@
 	device_type = <0xdeadbeef>;
 	model = <0xdeadbeef>;
 	status = <0xdeadbeef>;
+	bootargs = <0xdeadbeef>;
+	stdout-path = <0xdeadbeef>;
+	label = <0xdeadbeef>;
 };

tests/run_tests.sh +1 -1

index 850bc165e757..c610aaeb053e 100755
@@ -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

Quoted 1 line · Rob Herring

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.

Quoted 5 lines · David Gibson

Quoted 1 line · Rob Herring

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.

Quoted 3 lines

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,*").

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

Signed-off-by Rob Herring <[email protected]>

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

checks.c +34 -0

index a4a9d37ca19b..4e23f29486bb 100644
@@ -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,
 

tests/bad-string-props.dts +8 -0

index 9b5a7a1736ee..5e226ce0c736 100644
@@ -7,4 +7,12 @@
 	bootargs = <0xdeadbeef>;
 	stdout-path = <0xdeadbeef>;
 	label = <0xdeadbeef>;
+
+	node1 {
+		compatible = <0xdeadbeef>;
+	};
+
+	node2 {
+		compatible = "good", <0xdeadbeef>;
+	};
 };

tests/run_tests.sh +1 -1

index c610aaeb053e..f492cf78ae84 100755
@@ -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

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

Signed-off-by Rob Herring <[email protected]>

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

checks.c +0 -0

index 4e23f29486bb..346b0256f9cb 100644

checks.c +8 -0

+			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)
 {

Hi,

Quoted 16 lines · 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 <[email protected]>

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

checks.c +0 -0

index 4e23f29486bb..346b0256f9cb 100644
@@ -622,6 +622,21 @@ WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path");
 
 WARNING_IF_NOT_STRING_LIST(compatible_is_string_list, "compatible");
 

checks.c WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path"); · line 631

Quoted 7 lines

checks.c +7 -0

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

Quoted 19 lines · Andre Przywara

Hi,

Quoted 16 lines · 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 <[email protected]>

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

checks.c +0 -0

index 4e23f29486bb..346b0256f9cb 100644
@@ -622,6 +622,21 @@ WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path");
 
 WARNING_IF_NOT_STRING_LIST(compatible_is_string_list, "compatible");
 

Quoted 11 lines

checks.c WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path"); · line 631

Quoted 7 lines

checks.c +7 -0

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

Hi,

Quoted 16 lines · 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 <[email protected]>

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

checks.c +0 -0

index 4e23f29486bb..346b0256f9cb 100644
@@ -622,6 +622,21 @@ WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path");
 
 WARNING_IF_NOT_STRING_LIST(compatible_is_string_list, "compatible");
 

checks.c WARNING_IF_NOT_STRING(stdout_path_is_string, "stdout-path"); · line 631

Quoted 7 lines

checks.c +7 -0

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

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 <[email protected]>

---
 checks.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

checks.c +26 -0

index 346b0256f9cb..a785b81bea07 100644
@@ -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,

Quoted 4 lines · 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

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.

Quoted 3 lines · David Gibson

Quoted 2 lines · Rob Herring

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

Quoted 4 lines

Quoted 2 lines

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

Quoted 5 lines

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.

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 <[email protected]>

---
 checks.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

checks.c +44 -0

index a785b81bea07..2c5b6c2eacb3 100644
@@ -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,
 };