[PATCH v2 0/4] dtc unit-address and character set checks

[PATCH v2 0/4] dtc unit-address and character set checks

[Rob Herring]

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

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

[PATCH v2 1/4] checks: Add Warning for stricter property name character checking

[Rob Herring]

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 <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
v2:
- Disable check by default

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

diff --git a/checks.c b/checks.c
index 3d18e45374c8..22ef4748d7be 100644
--- a/checks.c
+++ b/checks.c
@@ -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);
+	}
+}
+CHECK(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,
--
2.10.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

[PATCH v2 2/4] checks: Add Warning for stricter node name character checking

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

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
v2:
- Disable check by default

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

diff --git a/checks.c b/checks.c
index 22ef4748d7be..67237ffe594e 100644
--- a/checks.c
+++ b/checks.c
@@ -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);
+}
+CHECK(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,

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

[PATCH v2 3/4] checks: Warn on node name unit-addresses with '0x' or leading 0s

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

diff --git a/checks.c b/checks.c
index 67237ffe594e..16d17d20caec 100644
--- a/checks.c
+++ b/checks.c
@@ -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,
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index ed489dbdd269..0f5c3db79b80 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -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
diff --git a/tests/unit-addr-leading-0s.dts b/tests/unit-addr-leading-0s.dts
new file mode 100644
index 000000000000..7c8e2cebbc84
--- /dev/null
+++ b/tests/unit-addr-leading-0s.dts
@@ -0,0 +1,10 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	node@001 {
+		reg = <1 0>;
+	};
+};
diff --git a/tests/unit-addr-leading-0x.dts b/tests/unit-addr-leading-0x.dts
new file mode 100644
index 000000000000..7ed7254e8dc2
--- /dev/null
+++ b/tests/unit-addr-leading-0x.dts
@@ -0,0 +1,10 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	node@0x1 {
+		reg = <1 0>;
+	};
+};
-- 
2.10.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

[PATCH v2 4/4] checks: Add bus checks for PCI buses

[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 <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
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(+)

diff --git a/checks.c b/checks.c
index 16d17d20caec..9ebb148f947a 100644
--- a/checks.c
+++ b/checks.c
@@ -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);
 
+static const struct bus_type pci_bus = {
+	.type = PCI_BUS_TYPE,
+};
+
+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 = &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);
+
+	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);
+
+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))
+		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 +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,
 
diff --git a/dtc.h b/dtc.h
index c6f125c68ba8..c538ef4afafb 100644
--- a/dtc.h
+++ b/dtc.h
@@ -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) \
-- 
2.10.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 v2 3/4] checks: Warn on node name unit-addresses with '0x' or leading 0s

[David Gibson]

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

On Fri, Feb 10, 2017 at 10:47:16AM -0600, Rob Herring wrote:
> 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 <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
> 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.

> 
>  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
> 
> diff --git a/checks.c b/checks.c
> index 67237ffe594e..16d17d20caec 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -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,
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index ed489dbdd269..0f5c3db79b80 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -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
> diff --git a/tests/unit-addr-leading-0s.dts b/tests/unit-addr-leading-0s.dts
> new file mode 100644
> index 000000000000..7c8e2cebbc84
> --- /dev/null
> +++ b/tests/unit-addr-leading-0s.dts
> @@ -0,0 +1,10 @@
> +/dts-v1/;
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	node@001 {
> +		reg = <1 0>;
> +	};
> +};
> diff --git a/tests/unit-addr-leading-0x.dts b/tests/unit-addr-leading-0x.dts
> new file mode 100644
> index 000000000000..7ed7254e8dc2
> --- /dev/null
> +++ b/tests/unit-addr-leading-0x.dts
> @@ -0,0 +1,10 @@
> +/dts-v1/;
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	node@0x1 {
> +		reg = <1 0>;
> +	};
> +};

-- 
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: 819 bytes --]

Re: [PATCH v2 4/4] checks: Add bus checks for PCI buses

[David Gibson]

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

On Fri, Feb 10, 2017 at 10:47:17AM -0600, Rob Herring wrote:
> 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 <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
> 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(+)
> 
> diff --git a/checks.c b/checks.c
> index 16d17d20caec..9ebb148f947a 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -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);
>  
> +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.

> +};
> +
> +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 = &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.

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

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

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.

> +
> +	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,
>  
> diff --git a/dtc.h b/dtc.h
> index c6f125c68ba8..c538ef4afafb 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -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) \

-- 
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: 819 bytes --]

Re: [PATCH v2 0/4] dtc unit-address and character set checks

[David Gibson]

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

On Fri, Feb 10, 2017 at 10:47:13AM -0600, Rob Herring wrote:
> 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.

I'm still a touch uncomfortable about the tests in 1 & 2, but since
they're off by default, I've merged them.

3 & 4 have further comments.

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

-- 
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: 819 bytes --]

Re: [PATCH v2 3/4] checks: Warn on node name unit-addresses with '0x' or leading 0s

[Rob Herring]

On Sun, Feb 12, 2017 at 10:52 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Fri, Feb 10, 2017 at 10:47:16AM -0600, Rob Herring wrote:
>> 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 <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> ---
>> 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.

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.

Rob

Re: [PATCH v2 4/4] checks: Add bus checks for PCI buses

[Rob Herring]

On Sun, Feb 12, 2017 at 11:03 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Fri, Feb 10, 2017 at 10:47:17AM -0600, Rob Herring wrote:
>> 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 <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> ---
>> 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(+)
>>
>> diff --git a/checks.c b/checks.c
>> index 16d17d20caec..9ebb148f947a 100644
>> --- a/checks.c
>> +++ b/checks.c
>> @@ -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);
>>
>> +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.

>> +};
>> +
>> +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 = &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.
>
>> +
>> +     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);
>> +
>> +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.
>
>> +             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);

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

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

Rob

Re: [PATCH v2 3/4] checks: Warn on node name unit-addresses with '0x' or leading 0s

[David Gibson]

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

On Mon, Feb 13, 2017 at 02:11:57PM -0600, Rob Herring wrote:
> On Sun, Feb 12, 2017 at 10:52 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Fri, Feb 10, 2017 at 10:47:16AM -0600, Rob Herring wrote:
> >> 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 <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >> ---
> >> 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.
> 
> 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.

Right.. and I'm saying why do that, when you're so close to having 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.

Ah, ok, that makes things a bit more problematic.

Alright, I can live with this test if you turn it off for anything
that has a specific bus type identified.  i.e. specific bus types
should always have unit address checks which supersede this quick and
dirty one.


-- 
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: 819 bytes --]

Re: [PATCH v2 4/4] checks: Add bus checks for PCI buses

[David Gibson]

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

On Mon, Feb 13, 2017 at 02:49:45PM -0600, Rob Herring wrote:
> On Sun, Feb 12, 2017 at 11:03 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Fri, Feb 10, 2017 at 10:47:17AM -0600, Rob Herring wrote:
> >> 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 <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >> ---
> >> 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(+)
> >>
> >> diff --git a/checks.c b/checks.c
> >> index 16d17d20caec..9ebb148f947a 100644
> >> --- a/checks.c
> >> +++ b/checks.c
> >> @@ -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);
> >>
> >> +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.
> 
> >> +};
> >> +
> >> +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 = &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.
> >
> >> +
> >> +     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);
> >> +
> >> +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.
> >
> >> +             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);
> 
> BTW, I just noticed these 2 checks I can drop. They can never be true
> since I'm masking the values.

Ah, good point.  It looks like ther should be more to check though -
dev and func are 8 bits, and you don't allow anything in the top 8
bits, but there's no checking of the remaining 16 bits.  Or the other
2 address cells for that matter.

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

Ah, good point.  Alright leave it as is for now.  When we get more bus
types we can see if it makes sense to generalize something.

-- 
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: 819 bytes --]