* [PATCH 1/3] checks: Add interrupt provider test
[not found] ` <20200515141827.27957-1-andre.przywara-5wv7dgnIgG8@public.gmane.org>
@ 2020-05-15 14:18 ` Andre Przywara
[not found] ` <20200515141827.27957-2-andre.przywara-5wv7dgnIgG8@public.gmane.org>
2020-05-15 14:18 ` [PATCH 2/3] checks: Validate interrupt-map properties Andre Przywara
2020-05-15 14:18 ` [PATCH 3/3] RFC: checks: interrupt-map: Dump entries on error Andre Przywara
2 siblings, 1 reply; 6+ messages in thread
From: Andre Przywara @ 2020-05-15 14:18 UTC (permalink / raw)
To: David Gibson, Rob Herring; +Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA
An interrupt provider (an actual interrupt-controller node or an
interrupt nexus) should have both #address-cells and #interrupt-cells
properties explicitly defined.
Add an extra test for this. We check for the #interrupt-cells property
already, but this does not cover every controller so far, only those that
get referenced by an interrupts property in some node. Also we miss
interrupt nexus nodes.
A missing #address-cells property is less critical, but creates
ambiguities when used in interrupt-map properties, so warn about this as
well now.
This removes the now redundant warning in the existing interrupts test.
Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
checks.c | 25 ++++++++++++++++++++++++-
tests/bad-interrupt-controller.dts | 7 +++++++
tests/run_tests.sh | 1 +
3 files changed, 32 insertions(+), 1 deletion(-)
create mode 100644 tests/bad-interrupt-controller.dts
diff --git a/checks.c b/checks.c
index 4b3c486..a8213c0 100644
--- a/checks.c
+++ b/checks.c
@@ -1547,6 +1547,28 @@ static bool node_is_interrupt_provider(struct node *node)
return false;
}
+
+static void check_interrupt_provider(struct check *c,
+ struct dt_info *dti,
+ struct node *node)
+{
+ struct property *prop;
+
+ if (!node_is_interrupt_provider(node))
+ return;
+
+ prop = get_property(node, "#interrupt-cells");
+ 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);
+
static void check_interrupts_property(struct check *c,
struct dt_info *dti,
struct node *node)
@@ -1604,7 +1626,7 @@ static void check_interrupts_property(struct check *c,
prop = get_property(irq_node, "#interrupt-cells");
if (!prop) {
- FAIL(c, dti, irq_node, "Missing #interrupt-cells in interrupt-parent");
+ /* We warn about that already in another test. */
return;
}
@@ -1828,6 +1850,7 @@ static struct check *check_table[] = {
&deprecated_gpio_property,
&gpios_property,
&interrupts_property,
+ &interrupt_provider,
&alias_paths,
diff --git a/tests/bad-interrupt-controller.dts b/tests/bad-interrupt-controller.dts
new file mode 100644
index 0000000..62fa118
--- /dev/null
+++ b/tests/bad-interrupt-controller.dts
@@ -0,0 +1,7 @@
+/dts-v1/;
+
+/ {
+ intc: interrupt-controller {
+ interrupt-controller;
+ };
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index eccb85d..294585b 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -714,6 +714,7 @@ dtc_tests () {
check_tests "$SRCDIR/bad-graph.dts" graph_endpoint
run_sh_test "$SRCDIR/dtc-checkfails.sh" deprecated_gpio_property -- -Wdeprecated_gpio_property -I dts -O dtb "$SRCDIR/bad-gpio.dts"
check_tests "$SRCDIR/bad-interrupt-cells.dts" interrupts_property
+ check_tests "$SRCDIR/bad-interrupt-controller.dts" interrupt_provider
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.14.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/3] checks: Validate interrupt-map properties
[not found] ` <20200515141827.27957-1-andre.przywara-5wv7dgnIgG8@public.gmane.org>
2020-05-15 14:18 ` [PATCH 1/3] checks: Add interrupt provider test Andre Przywara
@ 2020-05-15 14:18 ` Andre Przywara
[not found] ` <20200515141827.27957-3-andre.przywara-5wv7dgnIgG8@public.gmane.org>
2020-05-15 14:18 ` [PATCH 3/3] RFC: checks: interrupt-map: Dump entries on error Andre Przywara
2 siblings, 1 reply; 6+ messages in thread
From: Andre Przywara @ 2020-05-15 14:18 UTC (permalink / raw)
To: David Gibson, Rob Herring; +Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA
The interrupt-map in an interrupt nexus is quite a tricky property: Each
entry contains five fields, the size of four of those depending on some
*-cells entries from two different nodes. This is even hard to validate
in a .dts file, especially when the associated interrupt controller is
described in a separate (included) file.
Add checks to validate those entries, by:
- Checking some basic properties of the interrupt nexus node.
- Checking that a map entry contains at least enough cells to point to
the associated interrupt controller.
- Checking that the phandle points to an actual interrupt controller.
- Checking that there are enough entries to describe an interrupt in
that interrupt controller's domain.
If each iteration passes and we exhaust exactly all the cells in the
interrupt-map property, the check passes.
Report errors on the way, and abort the check if that happens.
Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
checks.c | 88 +++++++++++++++++++++++++++++++++++++++++++++
tests/bad-interrupt-map.dts | 21 +++++++++++
tests/run_tests.sh | 1 +
3 files changed, 110 insertions(+)
create mode 100644 tests/bad-interrupt-map.dts
diff --git a/checks.c b/checks.c
index a8213c0..d01df01 100644
--- a/checks.c
+++ b/checks.c
@@ -1639,6 +1639,92 @@ static void check_interrupts_property(struct check *c,
}
WARNING(interrupts_property, check_interrupts_property, &phandle_references);
+static void check_interrupt_map(struct check *c, struct dt_info *dti,
+ struct node *node)
+{
+ struct property *map = get_property(node, "interrupt-map");
+ struct property *prop;
+ int i, cells, irq_cells;
+
+ /* We are only interested in interrupt nexus nodes. */
+ if (!map)
+ return;
+
+ if (map->val.len % sizeof(cell_t)) {
+ FAIL_PROP(c, dti, node, map,
+ "size (%d) is invalid, expected multiple of %zu",
+ map->val.len, sizeof(cell_t));
+ return;
+ }
+ cells = map->val.len / sizeof(cell_t);
+
+ prop = get_property(node, "#interrupt-cells");
+ if (!prop) {
+ /* We warn about that already in another test. */
+ return;
+ }
+ irq_cells = propval_cell(prop);
+
+ for (i = 0; i < cells;) {
+ int phandle_idx = i + node_addr_cells(node) + irq_cells;
+ cell_t intc_phandle, intc_irq_cells, intc_addr_cells;
+ struct node *intc = NULL;
+
+ if (phandle_idx + 1 >= cells) {
+ FAIL_PROP(c, dti, node, map,
+ "insufficient cells for interrupt-map entry");
+ return;
+ }
+ intc_phandle = propval_cell_n(map, phandle_idx);
+ /* Give up if this is an overlay with external references */
+ if ((intc_phandle == 0 || intc_phandle == -1) &&
+ dti->dtsflags & DTSF_PLUGIN)
+ break;
+ /* Avoid the assert in get_node_by_phandle() */
+ if (intc_phandle != 0)
+ intc = get_node_by_phandle(dti->dt, intc_phandle);
+ if (!intc) {
+ FAIL_PROP(c, dti, node, map,
+ "invalid phandle for interrupt-map entry");
+ return;
+ }
+
+ if (!node_is_interrupt_provider(intc)) {
+ FAIL_PROP(c,dti, node, map,
+ "interrupt-map phandle does not point to interrupt provider");
+ return;
+ }
+
+ prop = get_property(intc, "#address-cells");
+ if (!prop) {
+ /*
+ * Linux treats non-existing #address-cells in the
+ * interrupt parent as 0, and not 2, as the spec
+ * suggests. Deal with that here.
+ * We have a separate check for an explicit #a-c
+ * in an interrupt provider that warns already.
+ */
+ intc_addr_cells = 0;
+ } else
+ intc_addr_cells = propval_cell(prop);
+
+ prop = get_property(intc, "#interrupt-cells");
+ if (!prop) {
+ /* We warn about that already in another test. */
+ return;
+ }
+ intc_irq_cells = propval_cell(prop);
+
+ if (phandle_idx + intc_addr_cells + intc_irq_cells >= cells) {
+ FAIL_PROP(c, dti, node, map,
+ "insufficient cells for interrupt-map entry");
+ return;
+ }
+ i = phandle_idx + 1 + intc_addr_cells + intc_irq_cells;
+ }
+}
+WARNING(interrupt_map, check_interrupt_map, NULL);
+
static const struct bus_type graph_port_bus = {
.name = "graph-port",
};
@@ -1814,6 +1900,8 @@ static struct check *check_table[] = {
&pci_device_reg,
&pci_device_bus_num,
+ &interrupt_map,
+
&simple_bus_bridge,
&simple_bus_reg,
diff --git a/tests/bad-interrupt-map.dts b/tests/bad-interrupt-map.dts
new file mode 100644
index 0000000..cf9618f
--- /dev/null
+++ b/tests/bad-interrupt-map.dts
@@ -0,0 +1,21 @@
+/dts-v1/;
+
+/ {
+ intc: interrupt-controller {
+ interrupt-controller;
+ #address-cells = <2>;
+ #interrupt-cells = <3>;
+ };
+
+ nexus-node {
+ #address-cells = <1>;
+ #interrupt-cells = <1>;
+/*
+ * The cells after the phandle are the address in the interrupt controller's
+ * domain. This here encodes 0 cells , but the actual number is 2 above.
+ */
+ interrupt-map = <0 0 &intc 1 42 4>,
+ <0 1 &intc 1 43 4>,
+ <0 2 &intc 1 44 4>;
+ };
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 294585b..7c149d9 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -715,6 +715,7 @@ dtc_tests () {
run_sh_test "$SRCDIR/dtc-checkfails.sh" deprecated_gpio_property -- -Wdeprecated_gpio_property -I dts -O dtb "$SRCDIR/bad-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
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.14.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/3] RFC: checks: interrupt-map: Dump entries on error
[not found] ` <20200515141827.27957-1-andre.przywara-5wv7dgnIgG8@public.gmane.org>
2020-05-15 14:18 ` [PATCH 1/3] checks: Add interrupt provider test Andre Przywara
2020-05-15 14:18 ` [PATCH 2/3] checks: Validate interrupt-map properties Andre Przywara
@ 2020-05-15 14:18 ` Andre Przywara
2 siblings, 0 replies; 6+ messages in thread
From: Andre Przywara @ 2020-05-15 14:18 UTC (permalink / raw)
To: David Gibson, Rob Herring; +Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA
While the interrupt-map property is now validated, the current error
messages will probably leave people scratching their head on what the
actual problem is.
To help with the diagnosis, dump the map property formatted in a way
that corresponds to the actual interpretation from an IRQ consumer's
point of view. While this can't pinpoint the actual problem, it's much
easier to see what went wrong, especially when comparing it to the .dts
source.
As this will generate one line per mapped interrupt, it can be
potentially long on those larger maps.
A report would look like this:
Warning (interrupt_map): /soc/pci@1c00000:interrupt-map: invalid phandle for interrupt-map entry
< <0x0 0x0 0x0>, <0x1>, <&gic@17a00000>, <0x0>, <0x87 0x4 0x0> >,
< <0x0 0x0 0x2>, <0x1>, <&invalid>, >
Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
checks.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 79 insertions(+)
diff --git a/checks.c b/checks.c
index d01df01..d814e8d 100644
--- a/checks.c
+++ b/checks.c
@@ -1639,6 +1639,80 @@ static void check_interrupts_property(struct check *c,
}
WARNING(interrupts_property, check_interrupts_property, &phandle_references);
+static void dump_cell_group(struct property *prop, int index, int cells,
+ int nr_cells)
+{
+ int limit, i;
+
+ if (index + cells > nr_cells)
+ limit = nr_cells - index;
+ else
+ limit = cells;
+
+ fputs("<", stderr);
+ for (i = 0; i < limit; i++) {
+ cell_t v = propval_cell_n(prop, index + i);
+ fprintf(stderr, "%s0x%x", i == 0 ? "" : " ", v);
+ }
+ if (limit < cells)
+ fputs(", MISSING", stderr);
+ fputs(">", stderr);
+}
+
+static void dump_interrupt_map(struct node *root, struct node *nexus,
+ struct property *map, int irq_cells)
+{
+ int cells = map->val.len / 4;
+ struct property *prop;
+ int value;
+ int i;
+
+ for (i = 0; i < cells;) {
+ struct node *intc = NULL;
+
+ fprintf(stderr, "%s\t< ", i == 0 ? "" : ",\n");
+ dump_cell_group(map, i, node_addr_cells(nexus), cells);
+ fputs(", ", stderr);
+ dump_cell_group(map, i + node_addr_cells(nexus), irq_cells, cells);
+ fputs(", <&", stderr);
+ i += node_addr_cells(nexus) + irq_cells;
+ if (i >= cells)
+ break;
+ value = propval_cell_n(map, i);
+ if (value != 0)
+ intc = get_node_by_phandle(root, value);
+ fprintf(stderr, "%s>, ",
+ intc && intc->name ? intc->name : "invalid");
+ if (!intc) {
+ fprintf(stderr, ">\n");
+ return;
+ }
+ /*
+ * Linux treats non-existing #address-cells in the interrupt
+ * parent as 0 (and not 2, as the spec says).
+ * Deal with that, since many DTs rely on that.
+ */
+ prop = get_property(intc, "#address-cells");
+ if (prop)
+ value = propval_cell(prop);
+ else
+ value = 0;
+ dump_cell_group(map, i + 1, value, cells);
+ fputs(", ", stderr);
+ i += 1 + value;
+
+ prop = get_property(intc, "#interrupt-cells");
+ if (prop) {
+ value = propval_cell(prop);
+ dump_cell_group(map, i, value, cells);
+ } else
+ fputs("<?>", stderr);
+ fputs(" >", stderr);
+ i += value;
+ }
+ fputs("\n", stderr);
+}
+
static void check_interrupt_map(struct check *c, struct dt_info *dti,
struct node *node)
{
@@ -1673,6 +1747,7 @@ static void check_interrupt_map(struct check *c, struct dt_info *dti,
if (phandle_idx + 1 >= cells) {
FAIL_PROP(c, dti, node, map,
"insufficient cells for interrupt-map entry");
+ dump_interrupt_map(dti->dt, node, map, irq_cells);
return;
}
intc_phandle = propval_cell_n(map, phandle_idx);
@@ -1686,12 +1761,14 @@ static void check_interrupt_map(struct check *c, struct dt_info *dti,
if (!intc) {
FAIL_PROP(c, dti, node, map,
"invalid phandle for interrupt-map entry");
+ dump_interrupt_map(dti->dt, node, map, irq_cells);
return;
}
if (!node_is_interrupt_provider(intc)) {
FAIL_PROP(c,dti, node, map,
"interrupt-map phandle does not point to interrupt provider");
+ dump_interrupt_map(dti->dt, node, map, irq_cells);
return;
}
@@ -1711,6 +1788,7 @@ static void check_interrupt_map(struct check *c, struct dt_info *dti,
prop = get_property(intc, "#interrupt-cells");
if (!prop) {
/* We warn about that already in another test. */
+ dump_interrupt_map(dti->dt, node, map, irq_cells);
return;
}
intc_irq_cells = propval_cell(prop);
@@ -1718,6 +1796,7 @@ static void check_interrupt_map(struct check *c, struct dt_info *dti,
if (phandle_idx + intc_addr_cells + intc_irq_cells >= cells) {
FAIL_PROP(c, dti, node, map,
"insufficient cells for interrupt-map entry");
+ dump_interrupt_map(dti->dt, node, map, irq_cells);
return;
}
i = phandle_idx + 1 + intc_addr_cells + intc_irq_cells;
--
2.14.1
^ permalink raw reply related [flat|nested] 6+ messages in thread