dtc unit-address and character set checks

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

---
v2:
- Split into separate check from unit_address_vs_reg

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

checks.c +21 -0

index 67237ffe594e..16d17d20caec 100644
@@ -296,6 +296,26 @@ static void check_unit_address_vs_reg(struct check *c, struct dt_info *dti,
 }
 WARNING(unit_address_vs_reg, check_unit_address_vs_reg, NULL);
 
+static void check_unit_address_format(struct check *c, struct dt_info *dti,
+				      struct node *node)
+{
+	const char *unitname = get_unitname(node);
+
+	if (!unitname[0])
+		return;
+
+	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);
+}
+WARNING(unit_address_format, check_unit_address_format, NULL, &node_name_format);
+
 static void check_property_name_chars(struct check *c, struct dt_info *dti,
 				      struct node *node)
 {

@@ -753,6 +773,7 @@ static struct check *check_table[] = {
 	&addr_size_cells, &reg_format, &ranges_format,
 
 	&unit_address_vs_reg,
+	&unit_address_format,
 
 	&avoid_default_addr_size,
 	&obsolete_chosen_interrupt_controller,

tests/run_tests.sh +2 -0

index ed489dbdd269..0f5c3db79b80 100755
@@ -540,6 +540,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_format
+    check_tests unit-addr-leading-0s.dts unit_address_format
     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>;
+	};
+};

Quoted 8 lines · Rob Herring

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

---
v2:
- Split into separate check from unit_address_vs_reg

I'm still not thrilled with applying this test unconditionally - especially when 4/4 introduces pretty much exactly the infrastructure to do this better. If you add a unit name formatter function to the bus_type structure you really can then extend unit_address_vs_reg to verify them against each other, which will cover this as well more subtle mismatches.

Obviously it would only do it for known bus types - but adding a "simple-bus" type would cover a lot of the cases, and a few for i2c and spi would cover most of the rest.

Quoted 17 lines · David Gibson

Quoted 8 lines · Rob Herring

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

---
v2:
- Split into separate check from unit_address_vs_reg

I'm still not thrilled with applying this test unconditionally - especially when 4/4 introduces pretty much exactly the infrastructure to do this better. If you add a unit name formatter function to the bus_type structure you really can then extend unit_address_vs_reg to verify them against each other, which will cover this as well more subtle mismatches.

Quoted 3 lines

Obviously it would only do it for known bus types - but adding a "simple-bus" type would cover a lot of the cases, and a few for i2c and spi would cover most of the rest.

Sigh. Around in circles we go. As we discussed before, this is simple, but common problem check. It is not a complete check of the entire unit-address.

It only checks the first number (in case of ',') and doesn't validate the number itself. While in theory we can conceive of a unit-address that would be a problem for this check, we have not identified one. It's not like there's lots of unknown cases here. We know the buses that are out there for the most part. We can always blacklist any problematic cases or turn off the check by default if the need arises.

As far as bus types, I could easily support simple-bus, but there's no way to identify i2c and spi (and others like mdio) buses reliably. Maybe go off node names if we got those standardized, but I can't check standard node names for the same reason.

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

---
v2:
- Remove bus_type functions. Combine test for bus_type and bridge check into single check.
- Add a check that PCI bridge node name is pci or pcie.

 checks.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 dtc.h    |  7 ++++++
 2 files changed, 90 insertions(+)

checks.c +0 -0

index 16d17d20caec..9ebb148f947a 100644

checks.c +6 -0

+};
+
+static void check_pci_bridge(struct check *c, struct dt_info *dti, struct node *node)
+{
+	struct property *prop;
+

checks.c +15 -0

+
+	prop = get_property(node, "ranges");
+	if (!prop)
+		FAIL(c, "Node %s missing ranges for PCI bridge (or not a bridge)",
+			     node->fullpath);
+
+	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);

checks.c +21 -0

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

checks.c +9 -0

+
+	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 +855,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 +7 -0

index c6f125c68ba8..c538ef4afafb 100644
@@ -136,6 +136,12 @@ struct label {
 	struct label *next;
 };
 
+#define PCI_BUS_TYPE	1
+
+struct bus_type {
+	int type;
+};
+
 struct property {
 	bool deleted;
 	char *name;

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

Quoted 27 lines · Rob Herring

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

---
v2:
- Remove bus_type functions. Combine test for bus_type and bridge check into single check.
- Add a check that PCI bridge node name is pci or pcie.

 checks.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 dtc.h    |  7 ++++++
 2 files changed, 90 insertions(+)

checks.c +0 -0

index 16d17d20caec..9ebb148f947a 100644
@@ -702,6 +702,86 @@ static void check_ranges_format(struct check *c, struct dt_info *dti,
 }
 WARNING(ranges_format, check_ranges_format, NULL, &addr_size_cells);
 

checks.c static void check_ranges_format(struct check *c, struct dt_info *dti, · line 706

Quoted 2 lines

checks.c +2 -0

+static const struct bus_type pci_bus = {
+     .type = PCI_BUS_TYPE,

Since you can use the struct pointer itself as a handle on the bus type, I don't think there's any value to having the enum-style type value. What _would_ be useful is a human readable bus type name.

Quoted 6 lines

checks.c +6 -0

+};
+
+static void check_pci_bridge(struct check *c, struct dt_info *dti, struct node *node)
+{
+     struct property *prop;
+

checks.c static void check_ranges_format(struct check *c, struct dt_info *dti, · line 722

Quoted 10 lines

checks.c +10 -0

+     prop = get_property(node, "device_type");
+     if (!prop || strcmp(prop->val.val, "pci"))
+             return;
+
+     node->bus = &pci_bus;
+
+     if (strncmp(node->name, "pci", node->basenamelen) &&
+         strncmp(node->name, "pcie", node->basenamelen))
+             FAIL(c, "Node %s node name is not \"pci\" or \"pcie\"",
+                          node->fullpath);

Please use the strneq() macro - I frequently get confused about whether strcmp()/strncmp() comparisons need an ! or not for equality testing. streq() / strneq() help me remember.

Quoted 15 lines

checks.c +15 -0

+
+     prop = get_property(node, "ranges");
+     if (!prop)
+             FAIL(c, "Node %s missing ranges for PCI bridge (or not a bridge)",
+                          node->fullpath);
+
+     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);

checks.c static void check_ranges_format(struct check *c, struct dt_info *dti, · line 747

Quoted 10 lines

checks.c +10 -0

+
+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 ||
+         (node->parent->bus->type != PCI_BUS_TYPE))

You can just use node->parent->bus != &pci_bus here.

Quoted 13 lines

checks.c +13 -0

+             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",

checks.c static void check_ranges_format(struct check *c, struct dt_info *dti, · line 768

Quoted 8 lines

checks.c +8 -0

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

Quoted 6 lines

checks.c +6 -0

+
+     if (func == 0) {
+             snprintf(unit_addr, sizeof(unit_addr), "%x", dev);
+             if (!strcmp(unitname, unit_addr))
+                     return;
+     }

checks.c static void check_ranges_format(struct check *c, struct dt_info *dti, · line 778

Quoted 4 lines

checks.c +4 -0

+
+     snprintf(unit_addr, sizeof(unit_addr), "%x,%x", dev, func);
+     if (!strcmp(unitname, unit_addr))
+             return;

So as mentioned in my comments to 3/4, the test above, I would put back into unit_address_vs_reg, using a callback in the bus_type which formats a reg into the correct unit address.

Quoted 28 lines · David Gibson

Quoted 27 lines · Rob Herring

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

---
v2:
- Remove bus_type functions. Combine test for bus_type and bridge check into single check.
- Add a check that PCI bridge node name is pci or pcie.

 checks.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 dtc.h    |  7 ++++++
 2 files changed, 90 insertions(+)

checks.c +0 -0

index 16d17d20caec..9ebb148f947a 100644
@@ -702,6 +702,86 @@ static void check_ranges_format(struct check *c, struct dt_info *dti,
 }
 WARNING(ranges_format, check_ranges_format, NULL, &addr_size_cells);
 

Quoted 6 lines

checks.c static void check_ranges_format(struct check *c, struct dt_info *dti, · line 706

Quoted 2 lines

checks.c +2 -0

+static const struct bus_type pci_bus = {
+     .type = PCI_BUS_TYPE,

Since you can use the struct pointer itself as a handle on the bus type, I don't think there's any value to having the enum-style type value. What _would_ be useful is a human readable bus type name.

Okay.

Quoted 65 lines

Quoted 6 lines

checks.c +6 -0

+};
+
+static void check_pci_bridge(struct check *c, struct dt_info *dti, struct node *node)
+{
+     struct property *prop;
+

checks.c static void check_ranges_format(struct check *c, struct dt_info *dti, · line 722

Quoted 10 lines

checks.c +10 -0

+     prop = get_property(node, "device_type");
+     if (!prop || strcmp(prop->val.val, "pci"))
+             return;
+
+     node->bus = &pci_bus;
+
+     if (strncmp(node->name, "pci", node->basenamelen) &&
+         strncmp(node->name, "pcie", node->basenamelen))
+             FAIL(c, "Node %s node name is not \"pci\" or \"pcie\"",
+                          node->fullpath);

Please use the strneq() macro - I frequently get confused about whether strcmp()/strncmp() comparisons need an ! or not for equality testing. streq() / strneq() help me remember.

Quoted 15 lines

checks.c +15 -0

+
+     prop = get_property(node, "ranges");
+     if (!prop)
+             FAIL(c, "Node %s missing ranges for PCI bridge (or not a bridge)",
+                          node->fullpath);
+
+     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);

checks.c static void check_ranges_format(struct check *c, struct dt_info *dti, · line 747

Quoted 10 lines

checks.c +10 -0

+
+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 ||
+         (node->parent->bus->type != PCI_BUS_TYPE))

You can just use node->parent->bus != &pci_bus here.

Quoted 16 lines

checks.c +16 -0

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

Quoted 5 lines

checks.c static void check_ranges_format(struct check *c, struct dt_info *dti, · line 768

Quoted 5 lines

checks.c +5 -0

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

BTW, I just noticed these 2 checks I can drop. They can never be true since I'm masking the values.

Quoted 6 lines

Quoted 6 lines

checks.c +6 -0

+
+     if (func == 0) {
+             snprintf(unit_addr, sizeof(unit_addr), "%x", dev);
+             if (!strcmp(unitname, unit_addr))
+                     return;
+     }

Quoted 8 lines

checks.c static void check_ranges_format(struct check *c, struct dt_info *dti, · line 778

Quoted 4 lines

checks.c +4 -0

+
+     snprintf(unit_addr, sizeof(unit_addr), "%x,%x", dev, func);
+     if (!strcmp(unitname, unit_addr))
+             return;

So as mentioned in my comments to 3/4, the test above, I would put back into unit_address_vs_reg, using a callback in the bus_type which formats a reg into the correct unit address.

Humm, that doesn't really work. The unit address can be in 2 different forms when func# is 0. We can have either <dev> or <dev>,0.

This is a series of checks designed to check problems commonly found in binding reviews. The first 2 patches implement a stricter character set for property and node names. The 3rd patch checks unit address formatting. The 4th patch adds checks for PCI buses including more specific checks on unit address formatting.

In patches 1 and 2, I've turned the checks off by default. In patch 3, the leading '0x' or 0s check was moved to its own check. In patch 4, most of the bus struct was removed with only the type and the checks are called directly.

Rob

Rob Herring (4):
checks: Add Warning for stricter property name character checking
checks: Add Warning for stricter node name character checking
checks: Warn on node name unit-addresses with '0x' or leading 0s
checks: Add bus checks for PCI buses

 checks.c                       | 151 +++++++++++++++++++++++++++++++++++++++++
 dtc.h                          |   7 ++
 tests/run_tests.sh             |   2 +
 tests/unit-addr-leading-0s.dts |  10 +++
 tests/unit-addr-leading-0x.dts |  10 +++
 5 files changed, 180 insertions(+)
 create mode 100644 tests/unit-addr-leading-0s.dts
 create mode 100644 tests/unit-addr-leading-0x.dts