[PATCH 1/3] checks: Add an interrupt-map check

[PATCH 1/3] checks: Add an interrupt-map check

[Rob Herring]

Add a check for parsing 'interrupt-map' properties. The check primarily
tests parsing 'interrupt-map' properties which depends on and the parent
interrupt controller (or another map) node.

Note that this does not require '#address-cells' in the interrupt-map
parent, but treats missing '#address-cells' as 0 which is how the Linux
kernel parses it. There's numerous cases that expect this behavior.

Cc: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c                           | 88 ++++++++++++++++++++++++++++++
 tests/bad-interrupt-map-mask.dts   | 20 +++++++
 tests/bad-interrupt-map-parent.dts | 17 ++++++
 tests/bad-interrupt-map.dts        | 19 +++++++
 tests/run_tests.sh                 |  3 +
 5 files changed, 147 insertions(+)
 create mode 100644 tests/bad-interrupt-map-mask.dts
 create mode 100644 tests/bad-interrupt-map-parent.dts
 create mode 100644 tests/bad-interrupt-map.dts

diff --git a/checks.c b/checks.c
index fb3fc9cda4b1..d4e7facd7f37 100644
--- a/checks.c
+++ b/checks.c
@@ -1589,6 +1589,93 @@ static void check_interrupt_provider(struct check *c,
 }
 WARNING(interrupt_provider, check_interrupt_provider, NULL);
 
+static void check_interrupt_map(struct check *c,
+				struct dt_info *dti,
+				struct node *node)
+{
+	struct node *root = dti->dt;
+	struct property *prop, *irq_map_prop;
+	size_t cellsize, cell, map_cells;
+
+	irq_map_prop = get_property(node, "interrupt-map");
+	if (!irq_map_prop)
+		return;
+
+	prop = get_property(node, "#interrupt-cells");
+	if (!prop)
+		/* Already checked in check_interrupt_provider */
+		return;
+	cellsize = propval_cell(prop);
+
+	prop = get_property(node, "#address-cells");
+	if (!prop) {
+		FAIL(c, dti, node,
+		     "Missing '#address-cells' in interrupt-map provider");
+		return;
+	}
+	cellsize += propval_cell(prop);
+
+	prop = get_property(node, "interrupt-map-mask");
+	if (prop && (prop->val.len != (cellsize * sizeof(cell_t))))
+		FAIL_PROP(c, dti, node, prop,
+			  "property size (%d) is invalid, expected %zu",
+			  prop->val.len, cellsize * sizeof(cell_t));
+
+	if (!is_multiple_of(irq_map_prop->val.len, sizeof(cell_t))) {
+		FAIL_PROP(c, dti, node, irq_map_prop,
+			  "property size (%d) is invalid, expected multiple of %zu",
+			  irq_map_prop->val.len, sizeof(cell_t));
+		return;
+	}
+
+	map_cells = irq_map_prop->val.len / sizeof(cell_t);
+	for (cell = 0; cell < map_cells; ) {
+		struct node *provider_node;
+		struct property *cellprop;
+		int phandle;
+		size_t parent_cellsize;
+
+		if ((cell + cellsize) >= map_cells) {
+			FAIL_PROP(c, dti, node, irq_map_prop,
+				  "property size (%d) too small, expected > %zu",
+				  irq_map_prop->val.len, (cell + cellsize) * sizeof(cell_t));
+			break;
+		}
+
+		phandle = propval_cell_n(irq_map_prop, cell + cellsize);
+
+		if ((phandle == 0) || (phandle == -1)) {
+			FAIL_PROP(c, dti, node, irq_map_prop,
+				  "Cell %zu is not a phandle(%d)",
+				  cell + cellsize, phandle);
+			break;
+		}
+		provider_node = get_node_by_phandle(root, phandle);
+		if (!provider_node) {
+			FAIL_PROP(c, dti, node, irq_map_prop,
+				  "Could not get phandle node for (cell %zu)",
+				  cell);
+			break;
+		}
+
+		cellprop = get_property(provider_node, "#interrupt-cells");
+		if (cellprop) {
+			parent_cellsize = propval_cell(cellprop);
+		} else {
+			FAIL(c, dti, node, "Missing property '#interrupt-cells' in node %s or bad phandle (referred from interrupt-map[%zu])",
+			     provider_node->fullpath, cell);
+			break;
+		}
+
+		cellprop = get_property(provider_node, "#address-cells");
+		if (cellprop)
+			parent_cellsize += propval_cell(cellprop);
+
+		cell += cellsize + 1 + parent_cellsize;
+	}
+}
+WARNING(interrupt_map, check_interrupt_map, NULL, &phandle_references);
+
 static void check_interrupts_property(struct check *c,
 				      struct dt_info *dti,
 				      struct node *node)
@@ -1887,6 +1974,7 @@ static struct check *check_table[] = {
 	&gpios_property,
 	&interrupts_property,
 	&interrupt_provider,
+	&interrupt_map,
 
 	&alias_paths,
 
diff --git a/tests/bad-interrupt-map-mask.dts b/tests/bad-interrupt-map-mask.dts
new file mode 100644
index 000000000000..10eaffd62310
--- /dev/null
+++ b/tests/bad-interrupt-map-mask.dts
@@ -0,0 +1,20 @@
+/dts-v1/;
+
+/ {
+	interrupt-parent = <&intc>;
+	intc: interrupt-controller {
+		#interrupt-cells = <3>;
+		interrupt-controller;
+	};
+
+	node {
+		#address-cells = <0>;
+		#interrupt-cells = <1>;
+		interrupt-map = <1 &intc 1 2 3>;
+		interrupt-map-mask = <0 0>;
+
+		child {
+			interrupts = <1>;
+		};
+	};
+};
diff --git a/tests/bad-interrupt-map-parent.dts b/tests/bad-interrupt-map-parent.dts
new file mode 100644
index 000000000000..fe88ce2fe76f
--- /dev/null
+++ b/tests/bad-interrupt-map-parent.dts
@@ -0,0 +1,17 @@
+/dts-v1/;
+
+/ {
+	interrupt-parent = <&intc>;
+	intc: interrupt-controller {
+	};
+
+	node {
+		#address-cells = <0>;
+		#interrupt-cells = <1>;
+		interrupt-map = <1 &intc 1 2 3>;
+
+		child {
+			interrupts = <1>;
+		};
+	};
+};
diff --git a/tests/bad-interrupt-map.dts b/tests/bad-interrupt-map.dts
new file mode 100644
index 000000000000..6df8f93ae00c
--- /dev/null
+++ b/tests/bad-interrupt-map.dts
@@ -0,0 +1,19 @@
+/dts-v1/;
+
+/ {
+	interrupt-parent = <&intc>;
+	intc: interrupt-controller {
+		#interrupt-cells = <3>;
+		interrupt-controller;
+	};
+
+	node {
+		/* Missing #address-cells = <0>; */
+		#interrupt-cells = <1>;
+		interrupt-map = <1 &intc 1 2 3>;
+
+		child {
+			interrupts = <1>;
+		};
+	};
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 0e270feb3e47..d100d5aaa21f 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -717,6 +717,9 @@ dtc_tests () {
     run_sh_test "$SRCDIR/dtc-checkfails.sh" -n deprecated_gpio_property -- -Wdeprecated_gpio_property -I dts -O dtb "$SRCDIR/good-gpio.dts"
     check_tests "$SRCDIR/bad-interrupt-cells.dts" interrupts_property
     check_tests "$SRCDIR/bad-interrupt-controller.dts" interrupt_provider
+    check_tests "$SRCDIR/bad-interrupt-map.dts" interrupt_map
+    check_tests "$SRCDIR/bad-interrupt-map-parent.dts" interrupt_map
+    check_tests "$SRCDIR/bad-interrupt-map-mask.dts" interrupt_map
     run_sh_test "$SRCDIR/dtc-checkfails.sh" node_name_chars -- -I dtb -O dtb bad_node_char.dtb
     run_sh_test "$SRCDIR/dtc-checkfails.sh" node_name_format -- -I dtb -O dtb bad_node_format.dtb
     run_sh_test "$SRCDIR/dtc-checkfails.sh" property_name_chars -- -I dtb -O dtb bad_prop_char.dtb
-- 
2.30.2

[PATCH 2/3] checks: Drop interrupt provider '#address-cells' check

[Rob Herring]

'#address-cells' is only needed when parsing 'interrupt-map' properties, so
remove it from the common interrupt-provider test.

Cc: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/checks.c b/checks.c
index d4e7facd7f37..2efa36377e80 100644
--- a/checks.c
+++ b/checks.c
@@ -1581,11 +1581,6 @@ static void check_interrupt_provider(struct check *c,
 	if (!prop)
 		FAIL(c, dti, node,
 		     "Missing #interrupt-cells in interrupt provider");
-
-	prop = get_property(node, "#address-cells");
-	if (!prop)
-		FAIL(c, dti, node,
-		     "Missing #address-cells in interrupt provider");
 }
 WARNING(interrupt_provider, check_interrupt_provider, NULL);
 
-- 
2.30.2

[PATCH 3/3] checks: Ensure '#interrupt-cells' only exists in interrupt providers

[Rob Herring]

The interrupt provider check currently checks if an interrupt provider
has #interrupt-cells, but not whether #interrupt-cells is present
outside of interrupt-providers. Rework the check to cover the latter
case.

Cc: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/checks.c b/checks.c
index 2efa36377e80..7f769e92ef71 100644
--- a/checks.c
+++ b/checks.c
@@ -1573,14 +1573,20 @@ static void check_interrupt_provider(struct check *c,
 				     struct node *node)
 {
 	struct property *prop;
+	bool irq_provider = node_is_interrupt_provider(node);
 
-	if (!node_is_interrupt_provider(node))
+	prop = get_property(node, "#interrupt-cells");
+	if (irq_provider && !prop) {
+		FAIL(c, dti, node,
+		     "Missing '#interrupt-cells' in interrupt-controller");
 		return;
+	}
 
-	prop = get_property(node, "#interrupt-cells");
-	if (!prop)
+	if (!irq_provider && prop) {
 		FAIL(c, dti, node,
-		     "Missing #interrupt-cells in interrupt provider");
+		     "'#interrupt-cells' found, but node is not an interrupt provider");
+		return;
+	}
 }
 WARNING(interrupt_provider, check_interrupt_provider, NULL);
 
-- 
2.30.2

Re: [PATCH 1/3] checks: Add an interrupt-map check

[David Gibson]

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

On Tue, Sep 28, 2021 at 04:23:33PM -0500, Rob Herring wrote:
> Add a check for parsing 'interrupt-map' properties. The check primarily
> tests parsing 'interrupt-map' properties which depends on and the parent
> interrupt controller (or another map) node.
> 
> Note that this does not require '#address-cells' in the interrupt-map
> parent, but treats missing '#address-cells' as 0 which is how the Linux
> kernel parses it. There's numerous cases that expect this behavior.
> 
> Cc: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  checks.c                           | 88 ++++++++++++++++++++++++++++++
>  tests/bad-interrupt-map-mask.dts   | 20 +++++++
>  tests/bad-interrupt-map-parent.dts | 17 ++++++
>  tests/bad-interrupt-map.dts        | 19 +++++++
>  tests/run_tests.sh                 |  3 +
>  5 files changed, 147 insertions(+)
>  create mode 100644 tests/bad-interrupt-map-mask.dts
>  create mode 100644 tests/bad-interrupt-map-parent.dts
>  create mode 100644 tests/bad-interrupt-map.dts
> 
> diff --git a/checks.c b/checks.c
> index fb3fc9cda4b1..d4e7facd7f37 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -1589,6 +1589,93 @@ static void check_interrupt_provider(struct check *c,
>  }
>  WARNING(interrupt_provider, check_interrupt_provider, NULL);
>  
> +static void check_interrupt_map(struct check *c,
> +				struct dt_info *dti,
> +				struct node *node)
> +{
> +	struct node *root = dti->dt;
> +	struct property *prop, *irq_map_prop;
> +	size_t cellsize, cell, map_cells;
> +
> +	irq_map_prop = get_property(node, "interrupt-map");
> +	if (!irq_map_prop)
> +		return;
> +
> +	prop = get_property(node, "#interrupt-cells");
> +	if (!prop)
> +		/* Already checked in check_interrupt_provider */

If you add check_interrupt_provider as a dependency, then you can omit
this check.  Of course, check_interrupt_provider itself should
probably have a dependency on the check that ensures than
len(#interrupt-cells) == 4.

> +		return;
> +	cellsize = propval_cell(prop);
> +
> +	prop = get_property(node, "#address-cells");
> +	if (!prop) {
> +		FAIL(c, dti, node,
> +		     "Missing '#address-cells' in interrupt-map provider");
> +		return;
> +	}
> +	cellsize += propval_cell(prop);
> +
> +	prop = get_property(node, "interrupt-map-mask");
> +	if (prop && (prop->val.len != (cellsize * sizeof(cell_t))))
> +		FAIL_PROP(c, dti, node, prop,
> +			  "property size (%d) is invalid, expected %zu",
> +			  prop->val.len, cellsize * sizeof(cell_t));
> +
> +	if (!is_multiple_of(irq_map_prop->val.len, sizeof(cell_t))) {
> +		FAIL_PROP(c, dti, node, irq_map_prop,
> +			  "property size (%d) is invalid, expected multiple of %zu",
> +			  irq_map_prop->val.len, sizeof(cell_t));
> +		return;
> +	}
> +
> +	map_cells = irq_map_prop->val.len / sizeof(cell_t);
> +	for (cell = 0; cell < map_cells; ) {
> +		struct node *provider_node;
> +		struct property *cellprop;
> +		int phandle;
> +		size_t parent_cellsize;
> +
> +		if ((cell + cellsize) >= map_cells) {
> +			FAIL_PROP(c, dti, node, irq_map_prop,
> +				  "property size (%d) too small, expected > %zu",
> +				  irq_map_prop->val.len, (cell + cellsize) * sizeof(cell_t));
> +			break;
> +		}
> +
> +		phandle = propval_cell_n(irq_map_prop, cell + cellsize);
> +
> +		if ((phandle == 0) || (phandle == -1)) {
> +			FAIL_PROP(c, dti, node, irq_map_prop,
> +				  "Cell %zu is not a phandle(%d)",
> +				  cell + cellsize, phandle);
> +			break;
> +		}
> +		provider_node = get_node_by_phandle(root, phandle);
> +		if (!provider_node) {
> +			FAIL_PROP(c, dti, node, irq_map_prop,
> +				  "Could not get phandle node for (cell %zu)",
> +				  cell);
> +			break;
> +		}
> +
> +		cellprop = get_property(provider_node, "#interrupt-cells");
> +		if (cellprop) {
> +			parent_cellsize = propval_cell(cellprop);
> +		} else {
> +			FAIL(c, dti, node, "Missing property '#interrupt-cells' in node %s or bad phandle (referred from interrupt-map[%zu])",
> +			     provider_node->fullpath, cell);
> +			break;
> +		}
> +
> +		cellprop = get_property(provider_node, "#address-cells");
> +		if (cellprop)
> +			parent_cellsize += propval_cell(cellprop);
> +
> +		cell += cellsize + 1 + parent_cellsize;
> +	}
> +}
> +WARNING(interrupt_map, check_interrupt_map, NULL, &phandle_references);
> +
>  static void check_interrupts_property(struct check *c,
>  				      struct dt_info *dti,
>  				      struct node *node)
> @@ -1887,6 +1974,7 @@ static struct check *check_table[] = {
>  	&gpios_property,
>  	&interrupts_property,
>  	&interrupt_provider,
> +	&interrupt_map,
>  
>  	&alias_paths,
>  
> diff --git a/tests/bad-interrupt-map-mask.dts b/tests/bad-interrupt-map-mask.dts
> new file mode 100644
> index 000000000000..10eaffd62310
> --- /dev/null
> +++ b/tests/bad-interrupt-map-mask.dts
> @@ -0,0 +1,20 @@
> +/dts-v1/;
> +
> +/ {
> +	interrupt-parent = <&intc>;
> +	intc: interrupt-controller {
> +		#interrupt-cells = <3>;
> +		interrupt-controller;
> +	};
> +
> +	node {
> +		#address-cells = <0>;
> +		#interrupt-cells = <1>;
> +		interrupt-map = <1 &intc 1 2 3>;
> +		interrupt-map-mask = <0 0>;
> +
> +		child {
> +			interrupts = <1>;
> +		};
> +	};
> +};
> diff --git a/tests/bad-interrupt-map-parent.dts b/tests/bad-interrupt-map-parent.dts
> new file mode 100644
> index 000000000000..fe88ce2fe76f
> --- /dev/null
> +++ b/tests/bad-interrupt-map-parent.dts
> @@ -0,0 +1,17 @@
> +/dts-v1/;
> +
> +/ {
> +	interrupt-parent = <&intc>;
> +	intc: interrupt-controller {
> +	};
> +
> +	node {
> +		#address-cells = <0>;
> +		#interrupt-cells = <1>;
> +		interrupt-map = <1 &intc 1 2 3>;

If you just want to verify the check on the bad parent, shouldn't you
have an interrupt-map-mask here?

> +
> +		child {
> +			interrupts = <1>;
> +		};
> +	};
> +};
> diff --git a/tests/bad-interrupt-map.dts b/tests/bad-interrupt-map.dts
> new file mode 100644
> index 000000000000..6df8f93ae00c
> --- /dev/null
> +++ b/tests/bad-interrupt-map.dts
> @@ -0,0 +1,19 @@
> +/dts-v1/;
> +
> +/ {
> +	interrupt-parent = <&intc>;
> +	intc: interrupt-controller {
> +		#interrupt-cells = <3>;
> +		interrupt-controller;
> +	};
> +
> +	node {
> +		/* Missing #address-cells = <0>; */
> +		#interrupt-cells = <1>;
> +		interrupt-map = <1 &intc 1 2 3>;

Likewise here, since missing interrupt-map-mask doesn't seem to be the
fault you're checking for.

> +
> +		child {
> +			interrupts = <1>;
> +		};
> +	};
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 0e270feb3e47..d100d5aaa21f 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -717,6 +717,9 @@ dtc_tests () {
>      run_sh_test "$SRCDIR/dtc-checkfails.sh" -n deprecated_gpio_property -- -Wdeprecated_gpio_property -I dts -O dtb "$SRCDIR/good-gpio.dts"
>      check_tests "$SRCDIR/bad-interrupt-cells.dts" interrupts_property
>      check_tests "$SRCDIR/bad-interrupt-controller.dts" interrupt_provider
> +    check_tests "$SRCDIR/bad-interrupt-map.dts" interrupt_map
> +    check_tests "$SRCDIR/bad-interrupt-map-parent.dts" interrupt_map
> +    check_tests "$SRCDIR/bad-interrupt-map-mask.dts" interrupt_map
>      run_sh_test "$SRCDIR/dtc-checkfails.sh" node_name_chars -- -I dtb -O dtb bad_node_char.dtb
>      run_sh_test "$SRCDIR/dtc-checkfails.sh" node_name_format -- -I dtb -O dtb bad_node_format.dtb
>      run_sh_test "$SRCDIR/dtc-checkfails.sh" property_name_chars -- -I dtb -O dtb bad_prop_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 2/3] checks: Drop interrupt provider '#address-cells' check

[David Gibson]

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

On Tue, Sep 28, 2021 at 04:23:34PM -0500, Rob Herring wrote:
> '#address-cells' is only needed when parsing 'interrupt-map' properties, so
> remove it from the common interrupt-provider test.
> 
> Cc: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

(in the context of the previous patch)

> ---
>  checks.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/checks.c b/checks.c
> index d4e7facd7f37..2efa36377e80 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -1581,11 +1581,6 @@ static void check_interrupt_provider(struct check *c,
>  	if (!prop)
>  		FAIL(c, dti, node,
>  		     "Missing #interrupt-cells in interrupt provider");
> -
> -	prop = get_property(node, "#address-cells");
> -	if (!prop)
> -		FAIL(c, dti, node,
> -		     "Missing #address-cells in interrupt provider");
>  }
>  WARNING(interrupt_provider, check_interrupt_provider, NULL);
>  

-- 
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 3/3] checks: Ensure '#interrupt-cells' only exists in interrupt providers

[David Gibson]

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

On Tue, Sep 28, 2021 at 04:23:35PM -0500, Rob Herring wrote:
> The interrupt provider check currently checks if an interrupt provider
> has #interrupt-cells, but not whether #interrupt-cells is present
> outside of interrupt-providers. Rework the check to cover the latter
> case.
> 
> Cc: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>


> ---
>  checks.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/checks.c b/checks.c
> index 2efa36377e80..7f769e92ef71 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -1573,14 +1573,20 @@ static void check_interrupt_provider(struct check *c,
>  				     struct node *node)
>  {
>  	struct property *prop;
> +	bool irq_provider = node_is_interrupt_provider(node);
>  
> -	if (!node_is_interrupt_provider(node))
> +	prop = get_property(node, "#interrupt-cells");
> +	if (irq_provider && !prop) {
> +		FAIL(c, dti, node,
> +		     "Missing '#interrupt-cells' in interrupt-controller");
>  		return;
> +	}
>  
> -	prop = get_property(node, "#interrupt-cells");
> -	if (!prop)
> +	if (!irq_provider && prop) {
>  		FAIL(c, dti, node,
> -		     "Missing #interrupt-cells in interrupt provider");
> +		     "'#interrupt-cells' found, but node is not an interrupt provider");
> +		return;
> +	}
>  }
>  WARNING(interrupt_provider, check_interrupt_provider, NULL);
>  

-- 
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 1/3] checks: Add an interrupt-map check

[Rob Herring]

On Sun, Oct 10, 2021 at 11:58 PM David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Tue, Sep 28, 2021 at 04:23:33PM -0500, Rob Herring wrote:
> > Add a check for parsing 'interrupt-map' properties. The check primarily
> > tests parsing 'interrupt-map' properties which depends on and the parent
> > interrupt controller (or another map) node.
> >
> > Note that this does not require '#address-cells' in the interrupt-map
> > parent, but treats missing '#address-cells' as 0 which is how the Linux
> > kernel parses it. There's numerous cases that expect this behavior.
> >
> > Cc: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> > Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > ---
> >  checks.c                           | 88 ++++++++++++++++++++++++++++++
> >  tests/bad-interrupt-map-mask.dts   | 20 +++++++
> >  tests/bad-interrupt-map-parent.dts | 17 ++++++
> >  tests/bad-interrupt-map.dts        | 19 +++++++
> >  tests/run_tests.sh                 |  3 +
> >  5 files changed, 147 insertions(+)
> >  create mode 100644 tests/bad-interrupt-map-mask.dts
> >  create mode 100644 tests/bad-interrupt-map-parent.dts
> >  create mode 100644 tests/bad-interrupt-map.dts
> >
> > diff --git a/checks.c b/checks.c
> > index fb3fc9cda4b1..d4e7facd7f37 100644
> > --- a/checks.c
> > +++ b/checks.c
> > @@ -1589,6 +1589,93 @@ static void check_interrupt_provider(struct check *c,
> >  }
> >  WARNING(interrupt_provider, check_interrupt_provider, NULL);
> >
> > +static void check_interrupt_map(struct check *c,
> > +                             struct dt_info *dti,
> > +                             struct node *node)
> > +{
> > +     struct node *root = dti->dt;
> > +     struct property *prop, *irq_map_prop;
> > +     size_t cellsize, cell, map_cells;
> > +
> > +     irq_map_prop = get_property(node, "interrupt-map");
> > +     if (!irq_map_prop)
> > +             return;
> > +
> > +     prop = get_property(node, "#interrupt-cells");
> > +     if (!prop)
> > +             /* Already checked in check_interrupt_provider */
>
> If you add check_interrupt_provider as a dependency, then you can omit
> this check.  Of course, check_interrupt_provider itself should
> probably have a dependency on the check that ensures than
> len(#interrupt-cells) == 4.

Okay.

> > +             return;
> > +     cellsize = propval_cell(prop);
> > +
> > +     prop = get_property(node, "#address-cells");
> > +     if (!prop) {
> > +             FAIL(c, dti, node,
> > +                  "Missing '#address-cells' in interrupt-map provider");
> > +             return;
> > +     }
> > +     cellsize += propval_cell(prop);
> > +
> > +     prop = get_property(node, "interrupt-map-mask");
> > +     if (prop && (prop->val.len != (cellsize * sizeof(cell_t))))
> > +             FAIL_PROP(c, dti, node, prop,
> > +                       "property size (%d) is invalid, expected %zu",
> > +                       prop->val.len, cellsize * sizeof(cell_t));
> > +
> > +     if (!is_multiple_of(irq_map_prop->val.len, sizeof(cell_t))) {
> > +             FAIL_PROP(c, dti, node, irq_map_prop,
> > +                       "property size (%d) is invalid, expected multiple of %zu",
> > +                       irq_map_prop->val.len, sizeof(cell_t));
> > +             return;
> > +     }
> > +
> > +     map_cells = irq_map_prop->val.len / sizeof(cell_t);
> > +     for (cell = 0; cell < map_cells; ) {
> > +             struct node *provider_node;
> > +             struct property *cellprop;
> > +             int phandle;
> > +             size_t parent_cellsize;
> > +
> > +             if ((cell + cellsize) >= map_cells) {
> > +                     FAIL_PROP(c, dti, node, irq_map_prop,
> > +                               "property size (%d) too small, expected > %zu",
> > +                               irq_map_prop->val.len, (cell + cellsize) * sizeof(cell_t));
> > +                     break;
> > +             }
> > +
> > +             phandle = propval_cell_n(irq_map_prop, cell + cellsize);
> > +
> > +             if ((phandle == 0) || (phandle == -1)) {
> > +                     FAIL_PROP(c, dti, node, irq_map_prop,
> > +                               "Cell %zu is not a phandle(%d)",
> > +                               cell + cellsize, phandle);
> > +                     break;
> > +             }
> > +             provider_node = get_node_by_phandle(root, phandle);
> > +             if (!provider_node) {
> > +                     FAIL_PROP(c, dti, node, irq_map_prop,
> > +                               "Could not get phandle node for (cell %zu)",
> > +                               cell);
> > +                     break;
> > +             }
> > +
> > +             cellprop = get_property(provider_node, "#interrupt-cells");
> > +             if (cellprop) {
> > +                     parent_cellsize = propval_cell(cellprop);
> > +             } else {
> > +                     FAIL(c, dti, node, "Missing property '#interrupt-cells' in node %s or bad phandle (referred from interrupt-map[%zu])",
> > +                          provider_node->fullpath, cell);
> > +                     break;
> > +             }
> > +
> > +             cellprop = get_property(provider_node, "#address-cells");
> > +             if (cellprop)
> > +                     parent_cellsize += propval_cell(cellprop);
> > +
> > +             cell += cellsize + 1 + parent_cellsize;
> > +     }
> > +}
> > +WARNING(interrupt_map, check_interrupt_map, NULL, &phandle_references);
> > +
> >  static void check_interrupts_property(struct check *c,
> >                                     struct dt_info *dti,
> >                                     struct node *node)
> > @@ -1887,6 +1974,7 @@ static struct check *check_table[] = {
> >       &gpios_property,
> >       &interrupts_property,
> >       &interrupt_provider,
> > +     &interrupt_map,
> >
> >       &alias_paths,
> >
> > diff --git a/tests/bad-interrupt-map-mask.dts b/tests/bad-interrupt-map-mask.dts
> > new file mode 100644
> > index 000000000000..10eaffd62310
> > --- /dev/null
> > +++ b/tests/bad-interrupt-map-mask.dts
> > @@ -0,0 +1,20 @@
> > +/dts-v1/;
> > +
> > +/ {
> > +     interrupt-parent = <&intc>;
> > +     intc: interrupt-controller {
> > +             #interrupt-cells = <3>;
> > +             interrupt-controller;
> > +     };
> > +
> > +     node {
> > +             #address-cells = <0>;
> > +             #interrupt-cells = <1>;
> > +             interrupt-map = <1 &intc 1 2 3>;
> > +             interrupt-map-mask = <0 0>;
> > +
> > +             child {
> > +                     interrupts = <1>;
> > +             };
> > +     };
> > +};
> > diff --git a/tests/bad-interrupt-map-parent.dts b/tests/bad-interrupt-map-parent.dts
> > new file mode 100644
> > index 000000000000..fe88ce2fe76f
> > --- /dev/null
> > +++ b/tests/bad-interrupt-map-parent.dts
> > @@ -0,0 +1,17 @@
> > +/dts-v1/;
> > +
> > +/ {
> > +     interrupt-parent = <&intc>;
> > +     intc: interrupt-controller {
> > +     };
> > +
> > +     node {
> > +             #address-cells = <0>;
> > +             #interrupt-cells = <1>;
> > +             interrupt-map = <1 &intc 1 2 3>;
>
> If you just want to verify the check on the bad parent, shouldn't you
> have an interrupt-map-mask here?

No, a missing interrupt-map-mask is not an error. Only the wrong
number of cells is an error. It's treated as all 1s mask if not
present. So, in interest of having a minimal test, I omitted it.

Rob

Re: [PATCH 1/3] checks: Add an interrupt-map check

[David Gibson]

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

On Mon, Oct 11, 2021 at 11:56:19AM -0500, Rob Herring wrote:
> On Sun, Oct 10, 2021 at 11:58 PM David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Tue, Sep 28, 2021 at 04:23:33PM -0500, Rob Herring wrote:
[snip]
> > > diff --git a/tests/bad-interrupt-map-parent.dts b/tests/bad-interrupt-map-parent.dts
> > > new file mode 100644
> > > index 000000000000..fe88ce2fe76f
> > > --- /dev/null
> > > +++ b/tests/bad-interrupt-map-parent.dts
> > > @@ -0,0 +1,17 @@
> > > +/dts-v1/;
> > > +
> > > +/ {
> > > +     interrupt-parent = <&intc>;
> > > +     intc: interrupt-controller {
> > > +     };
> > > +
> > > +     node {
> > > +             #address-cells = <0>;
> > > +             #interrupt-cells = <1>;
> > > +             interrupt-map = <1 &intc 1 2 3>;
> >
> > If you just want to verify the check on the bad parent, shouldn't you
> > have an interrupt-map-mask here?
> 
> No, a missing interrupt-map-mask is not an error. Only the wrong
> number of cells is an error. It's treated as all 1s mask if not
> present. So, in interest of having a minimal test, I omitted it.

Ah, sorry, my mistake.

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