[PATCH v2] checks: add graph binding checks

[PATCH v2] checks: add graph binding checks

[Rob Herring]

Add checks for DT graph bindings. These checks check node names,
unit-addresses and link connections on ports, port, and endpoint nodes.

The graph nodes are matched by finding nodes named 'endpoint' or with a
'remote-endpoint' property. We can't match on 'ports' or 'port' nodes
because those names are used for non-graph nodes. While the graph nodes
aren't really buses, using the bus pointer to tag matched nodes is
convenient.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
v2:
- add test case
- fix unused variable error

 checks.c            | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/bad-graph.dts |  24 +++++++++
 tests/run_tests.sh  |   3 ++
 3 files changed, 166 insertions(+)
 create mode 100644 tests/bad-graph.dts

diff --git a/checks.c b/checks.c
index 1cded3658491..9fce9116ce11 100644
--- a/checks.c
+++ b/checks.c
@@ -1370,6 +1370,143 @@ static void check_interrupts_property(struct check *c,
 }
 WARNING(interrupts_property, check_interrupts_property, &phandle_references);
 
+static const struct bus_type graph_port_bus = {
+	.name = "graph-port",
+};
+
+static const struct bus_type graph_ports_bus = {
+	.name = "graph-ports",
+};
+
+static void check_graph_nodes(struct check *c, struct dt_info *dti,
+			      struct node *node)
+{
+	struct node *child;
+
+	for_each_child(node, child) {
+		if (!(strprefixeq(child->name, child->basenamelen, "endpoint") ||
+		      get_property(child, "remote-endpoint")))
+			continue;
+
+		node->bus = &graph_port_bus;
+	}
+
+	/* The parent of 'port' nodes can be either 'ports' or a device */
+	if (node->bus && !node->parent->bus &&
+	    (streq(node->parent->name, "ports") || get_property(node, "reg")))
+		node->parent->bus = &graph_ports_bus;
+}
+WARNING(graph_nodes, check_graph_nodes, NULL);
+
+static void check_graph_child_address(struct check *c, struct dt_info *dti,
+				      struct node *node)
+{
+	int cnt = 0;
+	struct node *child;
+
+	if (node->bus != &graph_ports_bus && node->bus != &graph_port_bus)
+		return;
+
+	for_each_child(node, child)
+		cnt++;
+
+	if (cnt == 1 && node->addr_cells != -1)
+		FAIL(c, dti, "graph node '%s' has single child node, unit address is not necessary",
+		     node->fullpath);
+}
+WARNING(graph_child_address, check_graph_child_address, NULL, &graph_nodes);
+
+static void check_graph_reg(struct check *c, struct dt_info *dti,
+			    struct node *node)
+{
+	char unit_addr[9];
+	const char *unitname = get_unitname(node);
+	struct property *prop;
+
+	prop = get_property(node, "reg");
+	if (!prop || !unitname)
+		return;
+
+	if (!(prop->val.val && prop->val.len == sizeof(cell_t))) {
+		FAIL(c, dti, "graph node '%s' malformed 'reg' property", node->fullpath);
+		return;
+	}
+
+	snprintf(unit_addr, sizeof(unit_addr), "%x", propval_cell(prop));
+	if (!streq(unitname, unit_addr))
+		FAIL(c, dti, "graph node '%s' unit address error, expected \"%s\"",
+		     node->fullpath, unit_addr);
+
+	if (node->parent->addr_cells != 1)
+		FAIL(c, dti, "'#address-cells' is %d, must be 1 in graph node '%s'",
+		     node->parent->addr_cells, node->fullpath);
+	if (node->parent->size_cells != 0)
+		FAIL(c, dti, "'#size-cells' is %d, must be 0 in graph node '%s'",
+		     node->parent->size_cells, node->fullpath);
+}
+
+static void check_graph_port(struct check *c, struct dt_info *dti,
+			     struct node *node)
+{
+	if (node->bus != &graph_port_bus)
+		return;
+
+	if (!strprefixeq(node->name, node->basenamelen, "port"))
+		FAIL(c, dti, "graph port node '%s' name should be 'port'",
+		     node->fullpath);
+
+	check_graph_reg(c, dti, node);
+}
+WARNING(graph_port, check_graph_port, NULL, &graph_nodes);
+
+static struct node *get_remote_endpoint(struct check *c, struct dt_info *dti,
+					struct node *endpoint)
+{
+	int phandle;
+	struct node *node;
+	struct property *prop;
+
+	prop = get_property(endpoint, "remote-endpoint");
+	if (!prop)
+		return NULL;
+
+	phandle = propval_cell(prop);
+	/* Give up if this is an overlay with external references */
+	if (phandle == 0 || phandle == -1)
+		return NULL;
+
+	node = get_node_by_phandle(dti->dt, phandle);
+	if (!node)
+		FAIL(c, dti, "graph endpoint node '%s' 'remote-endpoint' phandle is not valid",
+		     endpoint->fullpath);
+
+	return node;
+}
+
+static void check_graph_endpoint(struct check *c, struct dt_info *dti,
+				 struct node *node)
+{
+	struct node *remote_node;
+
+	if (!node->parent || node->parent->bus != &graph_port_bus)
+		return;
+
+	if (!strprefixeq(node->name, node->basenamelen, "endpoint"))
+		FAIL(c, dti, "graph endpont node '%s' name should be 'endpoint'",
+		     node->fullpath);
+
+	check_graph_reg(c, dti, node);
+
+	remote_node = get_remote_endpoint(c, dti, node);
+	if (!remote_node)
+		return;
+
+	if (get_remote_endpoint(c, dti, remote_node) != node)
+		FAIL(c, dti, "graph endpoint node '%s' connection to '%s' is not bidirectional",
+		     node->fullpath, remote_node->fullpath);
+}
+WARNING(graph_endpoint, check_graph_endpoint, NULL, &graph_nodes);
+
 static struct check *check_table[] = {
 	&duplicate_node_names, &duplicate_property_names,
 	&node_name_chars, &node_name_format, &property_name_chars,
@@ -1429,6 +1566,8 @@ static struct check *check_table[] = {
 
 	&alias_paths,
 
+	&graph_nodes, &graph_child_address, &graph_port, &graph_endpoint,
+
 	&always_fail,
 };
 
diff --git a/tests/bad-graph.dts b/tests/bad-graph.dts
new file mode 100644
index 000000000000..522da0edbbc8
--- /dev/null
+++ b/tests/bad-graph.dts
@@ -0,0 +1,24 @@
+/dts-v1/;
+
+/ {
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		bad_endpoint: port-a@0 {
+			reg = <0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			endpoint@d0 {
+				reg = <0>;
+				remote-endpoint = <0xdeadbeef>;
+			};
+
+		};
+
+		port@1 {
+			reg = <0>;
+		};
+	};
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 61c95f1fc899..87915ef4c9f9 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -561,6 +561,9 @@ dtc_tests () {
     check_tests unit-addr-leading-0s.dts unit_address_format
     check_tests bad-phandle-cells.dts interrupts_extended_property
     check_tests bad-gpio.dts gpios_property
+    check_tests bad-graph.dts graph_child_address
+    check_tests bad-graph.dts graph_port
+    check_tests bad-graph.dts graph_endpoint
     run_sh_test dtc-checkfails.sh deprecated_gpio_property -- -Wdeprecated_gpio_property -I dts -O dtb bad-gpio.dts
     check_tests bad-interrupt-cells.dts interrupts_property
     run_sh_test dtc-checkfails.sh node_name_chars -- -I dtb -O dtb bad_node_char.dtb
-- 
2.14.1

Re: [PATCH v2] checks: add graph binding checks

[David Gibson]

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

On Thu, Dec 14, 2017 at 09:18:53PM -0600, Rob Herring wrote:
> Add checks for DT graph bindings. These checks check node names,
> unit-addresses and link connections on ports, port, and endpoint nodes.
> 
> The graph nodes are matched by finding nodes named 'endpoint' or with a
> 'remote-endpoint' property. We can't match on 'ports' or 'port' nodes
> because those names are used for non-graph nodes. While the graph nodes
> aren't really buses, using the bus pointer to tag matched nodes is
> convenient.
> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Could you give me a reference to somewhere these graph nodes are
described?  It' a type I wasn't even vaguely aware of up until now.

> ---
> v2:
> - add test case
> - fix unused variable error
> 
>  checks.c            | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/bad-graph.dts |  24 +++++++++
>  tests/run_tests.sh  |   3 ++
>  3 files changed, 166 insertions(+)
>  create mode 100644 tests/bad-graph.dts
> 
> diff --git a/checks.c b/checks.c
> index 1cded3658491..9fce9116ce11 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -1370,6 +1370,143 @@ static void check_interrupts_property(struct check *c,
>  }
>  WARNING(interrupts_property, check_interrupts_property, &phandle_references);
>  
> +static const struct bus_type graph_port_bus = {
> +	.name = "graph-port",
> +};
> +
> +static const struct bus_type graph_ports_bus = {
> +	.name = "graph-ports",
> +};
> +
> +static void check_graph_nodes(struct check *c, struct dt_info *dti,
> +			      struct node *node)
> +{
> +	struct node *child;
> +
> +	for_each_child(node, child) {
> +		if (!(strprefixeq(child->name, child->basenamelen, "endpoint") ||
> +		      get_property(child, "remote-endpoint")))
> +			continue;
> +
> +		node->bus = &graph_port_bus;
> +	}
> +
> +	/* The parent of 'port' nodes can be either 'ports' or a device */
> +	if (node->bus && !node->parent->bus &&
> +	    (streq(node->parent->name, "ports") || get_property(node, "reg")))
> +		node->parent->bus = &graph_ports_bus;
> +}
> +WARNING(graph_nodes, check_graph_nodes, NULL);
> +
> +static void check_graph_child_address(struct check *c, struct dt_info *dti,
> +				      struct node *node)
> +{
> +	int cnt = 0;
> +	struct node *child;
> +
> +	if (node->bus != &graph_ports_bus && node->bus != &graph_port_bus)
> +		return;
> +
> +	for_each_child(node, child)
> +		cnt++;
> +
> +	if (cnt == 1 && node->addr_cells != -1)
> +		FAIL(c, dti, "graph node '%s' has single child node, unit address is not necessary",
> +		     node->fullpath);
> +}
> +WARNING(graph_child_address, check_graph_child_address, NULL, &graph_nodes);
> +
> +static void check_graph_reg(struct check *c, struct dt_info *dti,
> +			    struct node *node)
> +{
> +	char unit_addr[9];
> +	const char *unitname = get_unitname(node);
> +	struct property *prop;
> +
> +	prop = get_property(node, "reg");
> +	if (!prop || !unitname)
> +		return;
> +
> +	if (!(prop->val.val && prop->val.len == sizeof(cell_t))) {
> +		FAIL(c, dti, "graph node '%s' malformed 'reg' property", node->fullpath);
> +		return;
> +	}
> +
> +	snprintf(unit_addr, sizeof(unit_addr), "%x", propval_cell(prop));
> +	if (!streq(unitname, unit_addr))
> +		FAIL(c, dti, "graph node '%s' unit address error, expected \"%s\"",
> +		     node->fullpath, unit_addr);
> +
> +	if (node->parent->addr_cells != 1)
> +		FAIL(c, dti, "'#address-cells' is %d, must be 1 in graph node '%s'",
> +		     node->parent->addr_cells, node->fullpath);
> +	if (node->parent->size_cells != 0)
> +		FAIL(c, dti, "'#size-cells' is %d, must be 0 in graph node '%s'",
> +		     node->parent->size_cells, node->fullpath);
> +}
> +
> +static void check_graph_port(struct check *c, struct dt_info *dti,
> +			     struct node *node)
> +{
> +	if (node->bus != &graph_port_bus)
> +		return;
> +
> +	if (!strprefixeq(node->name, node->basenamelen, "port"))
> +		FAIL(c, dti, "graph port node '%s' name should be 'port'",
> +		     node->fullpath);
> +
> +	check_graph_reg(c, dti, node);
> +}
> +WARNING(graph_port, check_graph_port, NULL, &graph_nodes);
> +
> +static struct node *get_remote_endpoint(struct check *c, struct dt_info *dti,
> +					struct node *endpoint)
> +{
> +	int phandle;
> +	struct node *node;
> +	struct property *prop;
> +
> +	prop = get_property(endpoint, "remote-endpoint");
> +	if (!prop)
> +		return NULL;
> +
> +	phandle = propval_cell(prop);
> +	/* Give up if this is an overlay with external references */
> +	if (phandle == 0 || phandle == -1)
> +		return NULL;
> +
> +	node = get_node_by_phandle(dti->dt, phandle);
> +	if (!node)
> +		FAIL(c, dti, "graph endpoint node '%s' 'remote-endpoint' phandle is not valid",
> +		     endpoint->fullpath);
> +
> +	return node;
> +}
> +
> +static void check_graph_endpoint(struct check *c, struct dt_info *dti,
> +				 struct node *node)
> +{
> +	struct node *remote_node;
> +
> +	if (!node->parent || node->parent->bus != &graph_port_bus)
> +		return;
> +
> +	if (!strprefixeq(node->name, node->basenamelen, "endpoint"))
> +		FAIL(c, dti, "graph endpont node '%s' name should be 'endpoint'",
> +		     node->fullpath);
> +
> +	check_graph_reg(c, dti, node);
> +
> +	remote_node = get_remote_endpoint(c, dti, node);
> +	if (!remote_node)
> +		return;
> +
> +	if (get_remote_endpoint(c, dti, remote_node) != node)
> +		FAIL(c, dti, "graph endpoint node '%s' connection to '%s' is not bidirectional",
> +		     node->fullpath, remote_node->fullpath);
> +}
> +WARNING(graph_endpoint, check_graph_endpoint, NULL, &graph_nodes);
> +
>  static struct check *check_table[] = {
>  	&duplicate_node_names, &duplicate_property_names,
>  	&node_name_chars, &node_name_format, &property_name_chars,
> @@ -1429,6 +1566,8 @@ static struct check *check_table[] = {
>  
>  	&alias_paths,
>  
> +	&graph_nodes, &graph_child_address, &graph_port, &graph_endpoint,
> +
>  	&always_fail,
>  };
>  
> diff --git a/tests/bad-graph.dts b/tests/bad-graph.dts
> new file mode 100644
> index 000000000000..522da0edbbc8
> --- /dev/null
> +++ b/tests/bad-graph.dts
> @@ -0,0 +1,24 @@
> +/dts-v1/;
> +
> +/ {
> +	ports {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		bad_endpoint: port-a@0 {
> +			reg = <0>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			endpoint@d0 {
> +				reg = <0>;
> +				remote-endpoint = <0xdeadbeef>;
> +			};
> +
> +		};
> +
> +		port@1 {
> +			reg = <0>;
> +		};
> +	};
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 61c95f1fc899..87915ef4c9f9 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -561,6 +561,9 @@ dtc_tests () {
>      check_tests unit-addr-leading-0s.dts unit_address_format
>      check_tests bad-phandle-cells.dts interrupts_extended_property
>      check_tests bad-gpio.dts gpios_property
> +    check_tests bad-graph.dts graph_child_address
> +    check_tests bad-graph.dts graph_port
> +    check_tests bad-graph.dts graph_endpoint
>      run_sh_test dtc-checkfails.sh deprecated_gpio_property -- -Wdeprecated_gpio_property -I dts -O dtb bad-gpio.dts
>      check_tests bad-interrupt-cells.dts interrupts_property
>      run_sh_test dtc-checkfails.sh node_name_chars -- -I dtb -O dtb bad_node_char.dtb

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

Re: [PATCH v2] checks: add graph binding checks

[Rob Herring]

On Mon, Dec 18, 2017 at 11:04 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Thu, Dec 14, 2017 at 09:18:53PM -0600, Rob Herring wrote:
>> Add checks for DT graph bindings. These checks check node names,
>> unit-addresses and link connections on ports, port, and endpoint nodes.
>>
>> The graph nodes are matched by finding nodes named 'endpoint' or with a
>> 'remote-endpoint' property. We can't match on 'ports' or 'port' nodes
>> because those names are used for non-graph nodes. While the graph nodes
>> aren't really buses, using the bus pointer to tag matched nodes is
>> convenient.
>>
>> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>
> Could you give me a reference to somewhere these graph nodes are
> described?  It' a type I wasn't even vaguely aware of up until now.

It is newish:

https://www.kernel.org/doc/Documentation/devicetree/bindings/graph.txt

Rob

Re: [PATCH v2] checks: add graph binding checks

[Rob Herring]

On Tue, Dec 19, 2017 at 12:36 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Dec 18, 2017 at 11:04 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> On Thu, Dec 14, 2017 at 09:18:53PM -0600, Rob Herring wrote:
>>> Add checks for DT graph bindings. These checks check node names,
>>> unit-addresses and link connections on ports, port, and endpoint nodes.
>>>
>>> The graph nodes are matched by finding nodes named 'endpoint' or with a
>>> 'remote-endpoint' property. We can't match on 'ports' or 'port' nodes
>>> because those names are used for non-graph nodes. While the graph nodes
>>> aren't really buses, using the bus pointer to tag matched nodes is
>>> convenient.
>>>
>>> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>
>> Could you give me a reference to somewhere these graph nodes are
>> described?  It' a type I wasn't even vaguely aware of up until now.
>
> It is newish:
>
> https://www.kernel.org/doc/Documentation/devicetree/bindings/graph.txt

Any comments on this?

Rob

Re: [PATCH v2] checks: add graph binding checks

[David Gibson]

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

On Mon, Jan 29, 2018 at 08:49:51AM -0600, Rob Herring wrote:
> On Tue, Dec 19, 2017 at 12:36 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Mon, Dec 18, 2017 at 11:04 PM, David Gibson
> > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >> On Thu, Dec 14, 2017 at 09:18:53PM -0600, Rob Herring wrote:
> >>> Add checks for DT graph bindings. These checks check node names,
> >>> unit-addresses and link connections on ports, port, and endpoint nodes.
> >>>
> >>> The graph nodes are matched by finding nodes named 'endpoint' or with a
> >>> 'remote-endpoint' property. We can't match on 'ports' or 'port' nodes
> >>> because those names are used for non-graph nodes. While the graph nodes
> >>> aren't really buses, using the bus pointer to tag matched nodes is
> >>> convenient.
> >>>
> >>> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >>
> >> Could you give me a reference to somewhere these graph nodes are
> >> described?  It' a type I wasn't even vaguely aware of up until now.
> >
> > It is newish:
> >
> > https://www.kernel.org/doc/Documentation/devicetree/bindings/graph.txt
> 
> Any comments on this?

Sorry, I've been sidetracked by all sorts of other things.  It's still
on my list.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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