dtc unit-address and character set checks

While '?', '.', '+', '*', and '_' are considered valid characters their use is discouraged in recommended practices. '#' is also only recommended to be used at the beginning of property names.

Testing this found one typo error with '.' used instead of ','. The rest of the warnings were all from underscores.

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

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

checks.c +35 -0

index 3d18e45374c8..a0d4a9d968d7 100644
@@ -239,6 +239,7 @@ ERROR(duplicate_property_names, check_duplicate_property_names, NULL);
 #define UPPERCASE	"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
 #define DIGITS		"0123456789"
 #define PROPNODECHARS	LOWERCASE UPPERCASE DIGITS ",._+*#?-"
+#define PROPNODECHARSSTRICT	LOWERCASE UPPERCASE DIGITS ",-"
 
 static void check_node_name_chars(struct check *c, struct dt_info *dti,
 				  struct node *node)

@@ -299,6 +300,38 @@ static void check_property_name_chars(struct check *c, struct dt_info *dti,
 }
 ERROR(property_name_chars, check_property_name_chars, PROPNODECHARS);
 
+static void check_property_name_chars_strict(struct check *c,
+					     struct dt_info *dti,
+					     struct node *node)
+{
+	struct property *prop;
+
+	for_each_property(node, prop) {
+		const char *name = prop->name;
+		int n = strspn(name, c->data);
+
+		if (n == strlen(prop->name))
+			continue;
+
+		/* Certain names are whitelisted */
+		if (strcmp(name, "device_type") == 0)
+			continue;
+
+		/*
+		 * # is only allowed at the beginning of property names not counting
+		 * the vendor prefix.
+		 */
+		if (name[n] == '#' && ((n == 0) || (name[n-1] == ','))) {
+			name += n + 1;
+			n = strspn(name, c->data);
+		}
+		if (n < strlen(name))
+			FAIL(c, "Character '%c' not recommended in property name \"%s\", node %s",
+			     name[n], prop->name, node->fullpath);
+	}
+}
+WARNING(property_name_chars_strict, check_property_name_chars_strict, PROPNODECHARSSTRICT);
+
 #define DESCLABEL_FMT	"%s%s%s%s%s"
 #define DESCLABEL_ARGS(node,prop,mark)		\
 	((mark) ? "value of " : ""),		\

@@ -703,6 +736,8 @@ 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,
 
+	&property_name_chars_strict,
+
 	&addr_size_cells, &reg_format, &ranges_format,
 
 	&unit_address_vs_reg,

While '#', '?', '.', '+', '*', and '_' are considered valid characters, their use is discouraged in recommended practices.

Testing this found a few cases of '.'. The majority of the warnings were all from underscores.

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

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

checks.c +12 -0

index a0d4a9d968d7..0c78d69316bc 100644
@@ -252,6 +252,17 @@ static void check_node_name_chars(struct check *c, struct dt_info *dti,
 }
 ERROR(node_name_chars, check_node_name_chars, PROPNODECHARS "@");
 
+static void check_node_name_chars_strict(struct check *c, struct dt_info *dti,
+					 struct node *node)
+{
+	int n = strspn(node->name, c->data);
+
+	if (n < node->basenamelen)
+		FAIL(c, "Character '%c' not recommended in node %s",
+		     node->name[n], node->fullpath);
+}
+WARNING(node_name_chars_strict, check_node_name_chars_strict, PROPNODECHARSSTRICT);
+
 static void check_node_name_format(struct check *c, struct dt_info *dti,
 				   struct node *node)
 {

@@ -737,6 +748,7 @@ static struct check *check_table[] = {
 	&device_type_is_string, &model_is_string, &status_is_string,
 
 	&property_name_chars_strict,
+	&node_name_chars_strict,
 
 	&addr_size_cells, &reg_format, &ranges_format,
 

Quoted 5 lines · Rob Herring

While '#', '?', '.', '+', '*', and '_' are considered valid characters, their use is discouraged in recommended practices.

Testing this found a few cases of '.'. The majority of the warnings were all from underscores.

Hmm. The Opal firmware on POWER8 machines uses both '.' and '#' in node names in some places. So I'm not terribly convinced this is a good idea.

Node name unit-addresses should never begin with 0x or leading 0s regardless of whether they have a bus specific address (i.e. one with commas) or not. Add warnings to check for these cases.

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

---
 checks.c                       | 10 ++++++++++
 tests/run_tests.sh             |  2 ++
 tests/unit-addr-leading-0s.dts | 10 ++++++++++
 tests/unit-addr-leading-0x.dts | 10 ++++++++++
 4 files changed, 32 insertions(+)
 create mode 100644 tests/unit-addr-leading-0s.dts
 create mode 100644 tests/unit-addr-leading-0x.dts

checks.c +10 -0

index 0c78d69316bc..8e310d69ca3d 100644
@@ -288,6 +288,16 @@ static void check_unit_address_vs_reg(struct check *c, struct dt_info *dti,
 		if (!unitname[0])
 			FAIL(c, "Node %s has a reg or ranges property, but no unit name",
 			    node->fullpath);
+
+		if (!strncmp(unitname, "0x", 2)) {
+			FAIL(c, "Node %s unit name should not have leading \"0x\"",
+			    node->fullpath);
+			/* skip over 0x for next test */
+			unitname += 2;
+		}
+		if (unitname[0] == '0' && isxdigit(unitname[1]))
+			FAIL(c, "Node %s unit name should not have leading 0s",
+			    node->fullpath);
 	} else {
 		if (unitname[0])
 			FAIL(c, "Node %s has a unit name, but no reg property",

tests/run_tests.sh +2 -0

index 157dbaea7600..2c06666a9e23 100755
@@ -539,6 +539,8 @@ dtc_tests () {
     check_tests obsolete-chosen-interrupt-controller.dts obsolete_chosen_interrupt_controller
     check_tests reg-without-unit-addr.dts unit_address_vs_reg
     check_tests unit-addr-without-reg.dts unit_address_vs_reg
+    check_tests unit-addr-leading-0x.dts unit_address_vs_reg
+    check_tests unit-addr-leading-0s.dts unit_address_vs_reg
     run_sh_test dtc-checkfails.sh node_name_chars -- -I dtb -O dtb bad_node_char.dtb
     run_sh_test dtc-checkfails.sh node_name_format -- -I dtb -O dtb bad_node_format.dtb
     run_sh_test dtc-checkfails.sh prop_name_chars -- -I dtb -O dtb bad_prop_char.dtb

tests/unit-addr-leading-0s.dts new file +10 -0

new file mode 100644
index 000000000000..7c8e2cebbc84
@@ -0,0 +1,10 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	node@001 {
+		reg = <1 0>;
+	};
+};

tests/unit-addr-leading-0x.dts new file +10 -0

new file mode 100644
index 000000000000..7ed7254e8dc2
@@ -0,0 +1,10 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	node@0x1 {
+		reg = <1 0>;
+	};
+};

In preparation to support bus specific checks, add the necessary infrastructure to determine and set the bus type for nodes.

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

---
 checks.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 dtc.h    | 11 +++++++++++
 2 files changed, 62 insertions(+)

checks.c +0 -0

index 8e310d69ca3d..5ef63a6a4317 100644
@@ -587,6 +587,53 @@ static void fixup_path_references(struct check *c, struct dt_info *dti,
 }
 ERROR(path_references, fixup_path_references, NULL, &duplicate_node_names);
 

checks.c +12 -0

+		if (!(*bus)->bridge_is_type(node))
+			continue;
+
+		node->bus_type = *bus;
+		break;
+	}
+}
+ERROR(bus_type, fixup_bus_type, NULL);
+
+static void check_bus_bridge(struct check *c, struct dt_info *dti,
+			     struct node *node)
+{

checks.c +4 -0

 /*
  * Semantic checks
  */

@@ -753,6 +800,7 @@ static struct check *check_table[] = {
 
 	&explicit_phandles,
 	&phandle_references, &path_references,
+	&bus_type,
 
 	&address_cells_is_cell, &size_cells_is_cell, &interrupt_cells_is_cell,
 	&device_type_is_string, &model_is_string, &status_is_string,

@@ -764,6 +812,9 @@ static struct check *check_table[] = {
 
 	&unit_address_vs_reg,
 
+	&bus_bridge,
+	&bus_device,
+
 	&avoid_default_addr_size,
 	&obsolete_chosen_interrupt_controller,
 

dtc.h +11 -0

index c6f125c68ba8..f27397cd7622 100644
@@ -136,6 +136,16 @@ struct label {
 	struct label *next;
 };
 
+struct check;
+struct node;
+struct dt_info;
+
+struct bus_type {
+        bool (*bridge_is_type)(struct node *node);
+        void (*check_bridge)(struct check *c, struct dt_info *dti, struct node *node);
+        void (*check_device)(struct check *c, struct dt_info *dti, struct node *node);
+};
+
 struct property {
 	bool deleted;
 	char *name;

@@ -162,6 +172,7 @@ struct node {
 	int addr_cells, size_cells;
 
 	struct label *labels;
+	struct bus_type *bus_type;
 };
 
 #define for_each_label_withdel(l0, l) \

Quoted 8 lines · Rob Herring

In preparation to support bus specific checks, add the necessary infrastructure to determine and set the bus type for nodes.

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

---
 checks.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 dtc.h    | 11 +++++++++++
 2 files changed, 62 insertions(+)

Review

Quoted 13 lines

(quoted diff) +13 -0

+static void check_bus_bridge(struct check *c, struct dt_info *dti,
+			     struct node *node)
+{
+	struct bus_type *bt;
+
+	if (!node->bus_type)
+		return;
+
+	bt = node->bus_type;
+	if (bt->check_bridge)
+		bt->check_bridge(c, dti, node);
+}
+WARNING(bus_bridge, check_bus_bridge, NULL);

Hrm. So, we have a double multiplex here, and I'm not sure that it's necessary. First the table of checks themselves, then the table of bus types. Could we eliminate that simply by having each bus type implement a check function which tests for that bus type then, performans any checks for the bridge then sets the bus_type field.

It would mean that if there are multiple bus-specific things we want to check about the bridge, we could put them into separate checks. That might be clearer in some cases, and it means our existing messages can show something informative via the check name which failed, rather than just saying "(bus_bridge)" for anything wrong with any bus bridge.

We could still use the bus_type field for "semi-generic" checks like validating unit names that are the same in outline between different buses, but differ in details.

Quoted 9 lines · David Gibson

Quoted 8 lines · Rob Herring

In preparation to support bus specific checks, add the necessary infrastructure to determine and set the bus type for nodes.

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

---
 checks.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 dtc.h    | 11 +++++++++++
 2 files changed, 62 insertions(+)

[...]

Quoted 27 lines

Review

Quoted 13 lines

(quoted diff) +13 -0

+static void check_bus_bridge(struct check *c, struct dt_info *dti,
+			     struct node *node)
+{
+	struct bus_type *bt;
+
+	if (!node->bus_type)
+		return;
+
+	bt = node->bus_type;
+	if (bt->check_bridge)
+		bt->check_bridge(c, dti, node);
+}
+WARNING(bus_bridge, check_bus_bridge, NULL);

Hrm. So, we have a double multiplex here, and I'm not sure that it's necessary. First the table of checks themselves, then the table of bus types. Could we eliminate that simply by having each bus type implement a check function which tests for that bus type then, performans any checks for the bridge then sets the bus_type field.

It would mean that if there are multiple bus-specific things we want to check about the bridge, we could put them into separate checks. That might be clearer in some cases, and it means our existing messages can show something informative via the check name which failed, rather than just saying "(bus_bridge)" for anything wrong with any bus bridge.

Quoted 3 lines

We could still use the bus_type field for "semi-generic" checks like validating unit names that are the same in outline between different buses, but differ in details.

Something like this what you have in mind? If we have additional checks to add, we can just use the existing prerq functionality.

8<--------------------------------------------------------------------------

Quoted 1 line

From 5cea821e6078f84eaae5af317651a4a696d1f0f7 Mon Sep 17 00:00:00 2001

From: Rob Herring <[email protected]>
Date: Wed, 1 Feb 2017 15:43:32 -0600
Subject: [PATCH v2] checks: Add bus checks for PCI buses

Add PCI bridge and device node checks. We identify PCI bridges with 'device_type = "pci"' as only PCI bridges should set that property. For bridges, check that ranges is present and #address-cells and

For devices, the primary check is the reg property and the unit address. Device unit addresses are in the form DD or DD,F where DD is the device 0-0x1f and F is the function 0-7.

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

---
 checks.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 dtc.h    |  4 +++-
 2 files changed, 77 insertions(+), 1 deletion(-)

checks.c +74 -0

index 225ace39c698..b727c49d457c 100644
@@ -702,6 +702,77 @@ static void check_ranges_format(struct check *c, struct dt_info *dti,
 }
 WARNING(ranges_format, check_ranges_format, NULL, &addr_size_cells);
 
+static void check_pci_bridge(struct check *c, struct dt_info *dti, struct node *node)
+{
+	struct property *prop;
+
+	prop = get_property(node, "device_type");
+	if (!prop || strcmp(prop->val.val, "pci"))
+		return;
+
+	node->bus_type = PCI_BUS_TYPE;
+
+	prop = get_property(node, "ranges");
+	if (!prop) {
+		FAIL(c, "Node %s missing ranges for PCI bridge (or not a bridge)",
+			     node->fullpath);
+		return;
+	}
+
+	if (node_addr_cells(node) != 3)
+		FAIL(c, "Node %s incorrect #address-cells for PCI bridge",
+			     node->fullpath);
+	if (node_size_cells(node) != 2)
+		FAIL(c, "Node %s incorrect #size-cells for PCI bridge",
+			     node->fullpath);
+}
+WARNING(pci_bridge, check_pci_bridge, NULL, &device_type_is_string,&addr_size_cells);
+
+static void check_pci_device(struct check *c, struct dt_info *dti, struct node *node)
+{
+	struct property *prop;
+	const char *unitname = get_unitname(node);
+	char unit_addr[5];
+	unsigned int dev, func, reg;
+
+	if (!node->parent || (node->parent->bus_type != PCI_BUS_TYPE))
+		return;
+
+	prop = get_property(node, "reg");
+	if (!prop)
+		return;
+
+	reg = fdt32_to_cpu(*((cell_t *)prop->val.val));
+
+	dev = (reg & 0xf800) >> 11;
+	func = (reg & 0x700) >> 8;
+
+	if (reg & 0xff000000)
+		FAIL(c, "Node %s PCI reg address is not configuration space",
+			     node->fullpath);
+
+	if (dev > 0x1f)
+		FAIL(c, "Node %s PCI device number out of range",
+			     node->fullpath);
+	if (func > 7)
+		FAIL(c, "Node %s PCI function number out of range",
+		     node->fullpath);
+
+	if (func == 0) {
+		snprintf(unit_addr, sizeof(unit_addr), "%x", dev);
+		if (!strcmp(unitname, unit_addr))
+			return;
+	}
+
+	snprintf(unit_addr, sizeof(unit_addr), "%x,%x", dev, func);
+	if (!strcmp(unitname, unit_addr))
+		return;
+
+	FAIL(c, "Node %s PCI unit address format error, expected \"%s\"",
+	     node->fullpath, unit_addr);
+}
+WARNING(pci_device, check_pci_device, NULL, &reg_format);
+
 /*
  * Style checks
  */

@@ -775,6 +846,9 @@ static struct check *check_table[] = {
 	&unit_address_vs_reg,
 	&unit_address_format,
 
+	&pci_bridge,
+	&pci_device,
+
 	&avoid_default_addr_size,
 	&obsolete_chosen_interrupt_controller,
 

dtc.h +3 -1

index c6f125c68ba8..03c249c65101 100644
@@ -146,6 +146,8 @@ struct property {
 	struct label *labels;
 };
 
+#define PCI_BUS_TYPE	1
+
 struct node {
 	bool deleted;
 	char *name;

@@ -159,7 +161,7 @@ struct node {
 	int basenamelen;
 
 	cell_t phandle;
-	int addr_cells, size_cells;
+	int addr_cells, size_cells, bus_type;
 
 	struct label *labels;
 };

checks.c +2 -0

@@ -587,7 +592,88 @@ static void fixup_path_references(struct check *c, struct dt_info *dti,
 }
 ERROR(path_references, fixup_path_references, NULL, &duplicate_node_names);
 
+static bool is_pci_bridge(struct node *node)
+{

(quoted diff) +2 -0

+}
+

+ &pci_bus_type,
NULL
};

Quoting Rob Herring (2017-01-24 09:45:34)

Quoted 6 lines

checks.c +2 -0

@@ -587,7 +592,88 @@ static void fixup_path_references(struct check *c, struct dt_info *dti,
 }
 ERROR(path_references, fixup_path_references, NULL, &duplicate_node_names);
 
+static bool is_pci_bridge(struct node *node)
+{

checks.c static void fixup_path_references(struct check *c, struct dt_info *dti, · line 606

Quoted 10 lines

checks.c +10 -0

+       struct property *prop;
+
+       prop = get_property(node, "device_type");
+       if (!prop)
+               return false;
+
+       if (strcmp(prop->val.val, "pci") == 0)
+               return true;
+
+       return false;

This could be simplified?

        return strcmp(prop->val.val, "pci") == 0;

Quoted 3 lines

(quoted diff) +2 -0

+}
+

[...]

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+struct bus_type pci_bus_type = {

static? const?

checks.c static void fixup_path_references(struct check *c, struct dt_info *dti, · line 675

Quoted 6 lines

checks.c +5 -0

+        .bridge_is_type = is_pci_bridge,
+        .check_bridge = pci_check_bridge,
+        .check_device = pci_check_device,
+};
+
 struct bus_type *bus_types[] = {

static? const?