dtc: checks for phandle with arg properties

Many common bindings follow the same pattern of client properties containing a phandle and N arg cells where N is defined in the provider with a '#<specifier>-cells' property such as:

	intc0: interrupt-controller@0 {
		#interrupt-cells = <3>;
	};
	intc1: interrupt-controller@1 {
		#interrupt-cells = <2>;
	};

	node {
		interrupts-extended = <&intc0 1 2 3>, <&intc1 4 5>;
	};

Add checks for properties following this pattern.

Signed-off-by Rob Herring <[email protected]>

---
v2:
- Make each property a separate check
- Iterate over raw cells rather than markers
- Fix property length check for 2nd to Nth items
- Improve error messages. If cell sizes are wrong, the next iteration can get a bad (but valid) phandle.
- Add a test

 checks.c                    | 104 ++++++++++++++++++++++++++++++++++++++++++++
 dtc.h                       |   1 +
 livetree.c                  |   6 +++
 tests/bad-phandle-cells.dts |  11 +++++
 tests/run_tests.sh          |   1 +
 5 files changed, 123 insertions(+)
 create mode 100644 tests/bad-phandle-cells.dts

checks.c +0 -0

index afabf64337d5..548d7e118c42 100644

checks.c +10 -0

+};
+
+static void check_property_phandle_args(struct check *c,
+					  struct dt_info *dti,
+				          struct node *node,
+				          struct property *prop,
+				          const struct provider *provider)
+{
+	struct node *root = dti->dt;
+	int cell, cellsize = 0;

checks.c +16 -0

+		}
+
+		provider_node = get_node_by_phandle(root, phandle);
+		if (!provider_node) {
+			FAIL(c, dti, "Could not get phandle node for %s:%s(cell %d)",
+			     node->fullpath, prop->name, cell);
+			break;
+		}
+
+		cellprop = get_property(provider_node, provider->cell_name);
+		if (cellprop) {
+			cellsize = propval_cell(cellprop);
+		} else if (provider->optional) {
+			cellsize = 0;
+		} else {
+			FAIL(c, dti, "Missing property '%s' in node %s or bad phandle (referred from %s:%s[%d])",

checks.c +54 -0

+	}
+}
+
+static void check_provider_cells_property(struct check *c,
+					  struct dt_info *dti,
+				          struct node *node)
+{
+	struct provider *provider = c->data;
+	struct property *prop;
+
+	prop = get_property(node, provider->prop_name);
+	if (!prop)
+		return;
+
+	check_property_phandle_args(c, dti, node, prop, provider);
+}
+#define WARNING_PROPERTY_PHANDLE_CELLS(nm, propname, cells_name, ...) \
+	static struct provider nm##_provider = { (propname), (cells_name), __VA_ARGS__ }; \
+	WARNING(nm##_property, check_provider_cells_property, &nm##_provider, &phandle_references);
+
+WARNING_PROPERTY_PHANDLE_CELLS(clocks, "clocks", "#clock-cells");
+WARNING_PROPERTY_PHANDLE_CELLS(cooling_device, "cooling-device", "#cooling-cells");
+WARNING_PROPERTY_PHANDLE_CELLS(dmas, "dmas", "#dma-cells");
+WARNING_PROPERTY_PHANDLE_CELLS(hwlocks, "hwlocks", "#hwlock-cells");
+WARNING_PROPERTY_PHANDLE_CELLS(interrupts_extended, "interrupts-extended", "#interrupt-cells");
+WARNING_PROPERTY_PHANDLE_CELLS(io_channels, "io-channels", "#io-channel-cells");
+WARNING_PROPERTY_PHANDLE_CELLS(iommus, "iommus", "#iommu-cells");
+WARNING_PROPERTY_PHANDLE_CELLS(mboxes, "mboxes", "#mbox-cells");
+WARNING_PROPERTY_PHANDLE_CELLS(msi_parent, "msi-parent", "#msi-cells", true);
+WARNING_PROPERTY_PHANDLE_CELLS(mux_controls, "mux-controls", "#mux-control-cells");
+WARNING_PROPERTY_PHANDLE_CELLS(phys, "phys", "#phy-cells");
+WARNING_PROPERTY_PHANDLE_CELLS(power_domains, "power-domains", "#power-domain-cells");
+WARNING_PROPERTY_PHANDLE_CELLS(pwms, "pwms", "#pwm-cells");
+WARNING_PROPERTY_PHANDLE_CELLS(resets, "resets", "#reset-cells");
+WARNING_PROPERTY_PHANDLE_CELLS(sound_dais, "sound-dais", "#sound-dai-cells");
+WARNING_PROPERTY_PHANDLE_CELLS(thermal_sensors, "thermal-sensors", "#thermal-sensor-cells");
+
 static struct check *check_table[] = {
 	&duplicate_node_names, &duplicate_property_names,
 	&node_name_chars, &node_name_format, &property_name_chars,

@@ -987,6 +1074,23 @@ static struct check *check_table[] = {
 	&avoid_default_addr_size,
 	&obsolete_chosen_interrupt_controller,
 
+	&clocks_property,
+	&cooling_device_property,
+	&dmas_property,
+	&hwlocks_property,
+	&interrupts_extended_property,
+	&io_channels_property,
+	&iommus_property,
+	&mboxes_property,
+	&msi_parent_property,
+	&mux_controls_property,
+	&phys_property,
+	&power_domains_property,
+	&pwms_property,
+	&resets_property,
+	&sound_dais_property,
+	&thermal_sensors_property,
+
 	&always_fail,
 };
 

dtc.h +1 -0

index 409db76c94b7..3c0532a7c3ab 100644
@@ -216,6 +216,7 @@ void append_to_property(struct node *node,
 const char *get_unitname(struct node *node);
 struct property *get_property(struct node *node, const char *propname);
 cell_t propval_cell(struct property *prop);
+cell_t propval_cell_n(struct property *prop, int n);
 struct property *get_property_by_label(struct node *tree, const char *label,
 				       struct node **node);
 struct marker *get_marker_label(struct node *tree, const char *label,

livetree.c +6 -0

index aecd27875fdd..c815176ec241 100644
@@ -396,6 +396,12 @@ cell_t propval_cell(struct property *prop)
 	return fdt32_to_cpu(*((fdt32_t *)prop->val.val));
 }
 
+cell_t propval_cell_n(struct property *prop, int n)
+{
+	assert(prop->val.len / sizeof(cell_t) >= n);
+	return fdt32_to_cpu(*((fdt32_t *)prop->val.val + n));
+}
+
 struct property *get_property_by_label(struct node *tree, const char *label,
 				       struct node **node)
 {

tests/bad-phandle-cells.dts new file +11 -0

new file mode 100644
index 000000000000..7f7c6a25fd25
@@ -0,0 +1,11 @@
+/dts-v1/;
+
+/ {
+	intc: interrupt-controller {
+		#interrupt-cells = <3>;
+	};
+
+	node {
+		interrupts-extended = <&intc>;
+	};
+};

tests/run_tests.sh +1 -0

index 3bc5b41ce76d..7cbc6971130a 100755
@@ -550,6 +550,7 @@ dtc_tests () {
     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
+    check_tests bad-phandle-cells.dts interrupts_extended_property
     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

The GPIO binding is different compared to other phandle plus args properties in that the property name has a variable, optional prefix. The format of the property name is [<name>-]gpio{s} where <name> can be any legal property string. Therefore, custom matching of property names is needed, but the common check_property_phandle_args() function can still be used.

It's possible that there are property names matching which are not GPIO binding specifiers. There's only been one case found in testing which is "[<vendor>,]nr-gpio{s}". This property has been blacklisted and the same should be done to any others we find. This check will prevent getting any more of these, too.

Signed-off-by Rob Herring <[email protected]>

---
v2:
- New patch with GPIO checks split to separate patch
- Add check for deprecated [<name>-]gpio form
- Rework property name matching to include "gpio" and "gpios"
- Skip the "gpios" property in gpio hogs binding
- Improve the blacklisting comment
- Add a test

 checks.c           | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/bad-gpio.dts | 13 ++++++++++
 tests/run_tests.sh |  2 ++
 3 files changed, 88 insertions(+)
 create mode 100644 tests/bad-gpio.dts

checks.c +73 -0

index 548d7e118c42..3315a4646497 100644
@@ -1043,6 +1043,76 @@ WARNING_PROPERTY_PHANDLE_CELLS(resets, "resets", "#reset-cells");
 WARNING_PROPERTY_PHANDLE_CELLS(sound_dais, "sound-dais", "#sound-dai-cells");
 WARNING_PROPERTY_PHANDLE_CELLS(thermal_sensors, "thermal-sensors", "#thermal-sensor-cells");
 
+static bool prop_is_gpio(struct property *prop)
+{
+	char *str;
+
+	/*
+	 * *-gpios and *-gpio can appear in property names,
+	 * so skip over any false matches (only one known ATM)
+	 */
+	if (strstr(prop->name, "nr-gpio"))
+		return false;
+
+	str = strrchr(prop->name, '-');
+	if (str)
+		str++;
+	else
+		str = prop->name;
+	if (!(streq(str, "gpios") || streq(str, "gpio")))
+		return false;
+
+	return true;
+}
+
+static void check_gpios_property(struct check *c,
+					  struct dt_info *dti,
+				          struct node *node)
+{
+	struct property *prop;
+
+	/* Skip GPIO hog nodes which have 'gpios' property */
+	if (get_property(node, "gpio-hog"))
+		return;
+
+	for_each_property(node, prop) {
+		struct provider provider;
+
+		if (!prop_is_gpio(prop))
+			continue;
+
+		provider.prop_name = prop->name;
+		provider.cell_name = "#gpio-cells";
+		provider.optional = false;
+		check_property_phandle_args(c, dti, node, prop, &provider);
+	}
+
+}
+WARNING(gpios_property, check_gpios_property, NULL, &phandle_references);
+
+static void check_deprecated_gpio_property(struct check *c,
+					   struct dt_info *dti,
+				           struct node *node)
+{
+	struct property *prop;
+
+	for_each_property(node, prop) {
+		char *str;
+
+		if (!prop_is_gpio(prop))
+			continue;
+
+		str = strstr(prop->name, "gpio");
+		if (!streq(str, "gpio"))
+			continue;
+
+		FAIL(c, dti, "'[*-]gpio' is deprecated, use '[*-]gpios' instead for %s:%s",
+		     node->fullpath, prop->name);
+	}
+
+}
+CHECK(deprecated_gpio_property, check_deprecated_gpio_property, NULL);
+
 static struct check *check_table[] = {
 	&duplicate_node_names, &duplicate_property_names,
 	&node_name_chars, &node_name_format, &property_name_chars,

@@ -1091,6 +1161,9 @@ static struct check *check_table[] = {
 	&sound_dais_property,
 	&thermal_sensors_property,
 
+	&deprecated_gpio_property,
+	&gpios_property,
+
 	&always_fail,
 };
 

tests/bad-gpio.dts new file +13 -0

new file mode 100644
index 000000000000..6b77be447b82
@@ -0,0 +1,13 @@
+/dts-v1/;
+
+/ {
+	gpio: gpio-controller {
+		#gpio-cells = <3>;
+	};
+
+	node {
+		nr-gpios = <1>;
+		foo-gpios = <&gpio>;
+		bar-gpio = <&gpio 1 2 3>;
+	};
+};

tests/run_tests.sh +2 -0

index 7cbc6971130a..2a29fa4ee451 100755
@@ -551,6 +551,8 @@ dtc_tests () {
     check_tests unit-addr-leading-0x.dts unit_address_format
     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
+    run_sh_test dtc-checkfails.sh deprecated_gpio_property -- -Wdeprecated_gpio_property -I dts -O dtb bad-gpio.dts
     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

Add a check for nodes with interrupts property that they have a valid parent, the parent has #interrupt-cells property, and the size is a valid multiple of #interrupt-cells.

This may not handle every possible case and doesn't deal with translation thru interrupt-map properties, but should be enough for modern dts files.

Signed-off-by Rob Herring <[email protected]>

---
v2:
- Add a test
- Check for interrupt-map when looking for interrupt parent.
- Check that explicit interrupt-parent node has an interrupt-controller or interrupt-map property.

 checks.c                      | 76 +++++++++++++++++++++++++++++++++++++++++++
 tests/bad-interrupt-cells.dts | 12 +++++++
 tests/run_tests.sh            |  1 +
 3 files changed, 89 insertions(+)
 create mode 100644 tests/bad-interrupt-cells.dts

checks.c +76 -0

index 3315a4646497..6b505e9c49d8 100644
@@ -1113,6 +1113,81 @@ static void check_deprecated_gpio_property(struct check *c,
 }
 CHECK(deprecated_gpio_property, check_deprecated_gpio_property, NULL);
 
+static bool node_is_interrupt_provider(struct node *node)
+{
+	struct property *prop;
+
+	prop = get_property(node, "interrupt-controller");
+	if (prop)
+		return true;
+
+	prop = get_property(node, "interrupt-map");
+	if (prop)
+		return true;
+
+	return false;
+}
+static void check_interrupts_property(struct check *c,
+				      struct dt_info *dti,
+				      struct node *node)
+{
+	struct node *root = dti->dt;
+	struct node *irq_node = NULL, *parent = node;
+	struct property *irq_prop, *prop = NULL;
+	int irq_cells, phandle;
+
+	irq_prop = get_property(node, "interrupts");
+	if (!irq_prop)
+		return;
+
+	while (parent && !prop) {
+		if (parent != node && node_is_interrupt_provider(parent)) {
+			irq_node = parent;
+			break;
+		}
+
+		prop = get_property(parent, "interrupt-parent");
+		if (prop) {
+			phandle = propval_cell(prop);
+			irq_node = get_node_by_phandle(root, phandle);
+			if (!irq_node) {
+				FAIL(c, dti, "Bad interrupt-parent phandle for %s",
+				     node->fullpath);
+				return;
+			}
+			if (!node_is_interrupt_provider(irq_node))
+				FAIL(c, dti,
+				     "Missing interrupt-controller or interrupt-map property in %s",
+				     irq_node->fullpath);
+
+			break;
+		}
+
+		parent = parent->parent;
+	}
+
+	if (!irq_node) {
+		FAIL(c, dti, "Missing interrupt-parent for %s", node->fullpath);
+		return;
+	}
+
+	prop = get_property(irq_node, "#interrupt-cells");
+	if (!prop) {
+		FAIL(c, dti, "Missing #interrupt-cells in interrupt-parent %s",
+		     irq_node->fullpath);
+		return;
+	}
+
+	irq_cells = propval_cell(prop);
+	if (irq_prop->val.len % (irq_cells * sizeof(cell_t))) {
+		FAIL(c, dti,
+		     "interrupts size is (%d), expected multiple of %d in %s",
+		     irq_prop->val.len, (int)(irq_cells * sizeof(cell_t)),
+		     node->fullpath);
+	}
+}
+WARNING(interrupts_property, check_interrupts_property, &phandle_references);
+
 static struct check *check_table[] = {
 	&duplicate_node_names, &duplicate_property_names,
 	&node_name_chars, &node_name_format, &property_name_chars,

@@ -1163,6 +1238,7 @@ static struct check *check_table[] = {
 
 	&deprecated_gpio_property,
 	&gpios_property,
+	&interrupts_property,
 
 	&always_fail,
 };

tests/bad-interrupt-cells.dts new file +12 -0

new file mode 100644
index 000000000000..39fc78fdc11d
@@ -0,0 +1,12 @@
+/dts-v1/;
+
+/ {
+	interrupt-parent = <&intc>;
+	intc: interrupt-controller {
+		#interrupt-cells = <3>;
+	};
+
+	node {
+		interrupts = <1>;
+	};
+};

tests/run_tests.sh +1 -0

index 2a29fa4ee451..cc35205f5768 100755
@@ -553,6 +553,7 @@ dtc_tests () {
     check_tests bad-phandle-cells.dts interrupts_extended_property
     check_tests bad-gpio.dts gpios_property
     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
     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

Quoted 43 lines · Rob Herring

Many common bindings follow the same pattern of client properties containing a phandle and N arg cells where N is defined in the provider with a '#<specifier>-cells' property such as:

      intc0: interrupt-controller@0 {
              #interrupt-cells = <3>;
      };
      intc1: interrupt-controller@1 {
              #interrupt-cells = <2>;
      };

      node {
              interrupts-extended = <&intc0 1 2 3>, <&intc1 4 5>;
      };

Add checks for properties following this pattern.

Signed-off-by Rob Herring <[email protected]>

---
v2:
- Make each property a separate check
- Iterate over raw cells rather than markers
- Fix property length check for 2nd to Nth items
- Improve error messages. If cell sizes are wrong, the next iteration can get a bad (but valid) phandle.
- Add a test

 checks.c                    | 104 ++++++++++++++++++++++++++++++++++++++++++++
 dtc.h                       |   1 +
 livetree.c                  |   6 +++
 tests/bad-phandle-cells.dts |  11 +++++
 tests/run_tests.sh          |   1 +
 5 files changed, 123 insertions(+)
 create mode 100644 tests/bad-phandle-cells.dts

checks.c +0 -0

index afabf64337d5..548d7e118c42 100644
@@ -956,6 +956,93 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,
 WARNING(obsolete_chosen_interrupt_controller,
      check_obsolete_chosen_interrupt_controller, NULL);
 

checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 962

Quoted 4 lines

checks.c +4 -0

+struct provider {
+     const char *prop_name;
+     const char *cell_name;
+     bool optional;

AFAICT you don't actually use this optional flag, even in the followup patches; it's always false.

checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 1037

Quoted 1 line

checks.c +1 -0

+WARNING_PROPERTY_PHANDLE_CELLS(msi_parent, "msi-parent", "#msi-cells", true);

Quoted 19 lines

checks.c +18 -0

+};
+
+static void check_property_phandle_args(struct check *c,
+                                       struct dt_info *dti,
+                                       struct node *node,
+                                       struct property *prop,
+                                       const struct provider *provider)
+{
+     struct node *root = dti->dt;
+     int cell, cellsize = 0;
+
+     for (cell = 0; cell < prop->val.len / sizeof(cell_t); cell += cellsize + 1) {
+             struct node *provider_node;
+             struct property *cellprop;
+             int phandle;
+
+             phandle = propval_cell_n(prop, cell);
+             if (phandle == 0 || phandle == -1) {

checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 982

Quoted 2 lines

checks.c +2 -0

+                     cellsize = 0;
+                     continue;

I'm not clear what case this is handling. If the property has an invalid (or unresolved) phandle value, shouldn't that be a FAIL? As it is we interpret the next cell as a phandle, and since we couldn't resolve the first phandle, we can't be at all sure that it really is a phandle.

Quoted 22 lines

checks.c +22 -0

+             }
+
+             provider_node = get_node_by_phandle(root, phandle);
+             if (!provider_node) {
+                     FAIL(c, dti, "Could not get phandle node for %s:%s(cell %d)",
+                          node->fullpath, prop->name, cell);
+                     break;
+             }
+
+             cellprop = get_property(provider_node, provider->cell_name);
+             if (cellprop) {
+                     cellsize = propval_cell(cellprop);
+             } else if (provider->optional) {
+                     cellsize = 0;
+             } else {
+                     FAIL(c, dti, "Missing property '%s' in node %s or bad phandle (referred from %s:%s[%d])",
+                          provider->cell_name,
+                          provider_node->fullpath,
+                          node->fullpath, prop->name, cell);
+                     break;
+             }
+

checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 1008

Quoted 4 lines

checks.c +4 -0

+             if (prop->val.len < ((cell + cellsize + 1) * sizeof(cell_t))) {
+                     FAIL(c, dti, "%s property size (%d) too small for cell size %d in %s",
+                          prop->name, prop->val.len, cellsize, node->fullpath);
+             }

How will this cope if the property is really badly formed - e.g. a 3 byte property, so you can't even grab a whole first phandle? I think it will trip the assert() in propval_cell_n() which isn't great.

Quoted 4 lines · David Gibson

Quoted 3 lines · Rob Herring

Many common bindings follow the same pattern of client properties containing a phandle and N arg cells where N is defined in the provider with a '#<specifier>-cells' property such as:

Quoted 8 lines

checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 1008

Quoted 4 lines

checks.c +4 -0

+             if (prop->val.len < ((cell + cellsize + 1) * sizeof(cell_t))) {
+                     FAIL(c, dti, "%s property size (%d) too small for cell size %d in %s",
+                          prop->name, prop->val.len, cellsize, node->fullpath);
+             }

How will this cope if the property is really badly formed - e.g. a 3 byte property, so you can't even grab a whole first phandle? I think it will trip the assert() in propval_cell_n() which isn't great.

At least for your example, we'd exit the loop (cell < 3/4). But I need to a check for that because it would be silent currently. I'll add a check that the size is a multiple of 4 and greater than 0.

However, the check here is not perfect because we could have "<&phandle1 1 2>" when really it should be "<&phandle1 &phandle2 &phandle3>". We don't fail until we're checking the 2nd phandle. That's why I added the "or bad phandle" and the cell # in the message above. In the opposite case, we'd be silent. One thing that could be done is double check things against the markers if they are present.

Quoted 44 lines · David Gibson

Quoted 43 lines · Rob Herring

Many common bindings follow the same pattern of client properties containing a phandle and N arg cells where N is defined in the provider with a '#<specifier>-cells' property such as:

      intc0: interrupt-controller@0 {
              #interrupt-cells = <3>;
      };
      intc1: interrupt-controller@1 {
              #interrupt-cells = <2>;
      };

      node {
              interrupts-extended = <&intc0 1 2 3>, <&intc1 4 5>;
      };

Add checks for properties following this pattern.

Signed-off-by Rob Herring <[email protected]>

---
v2:
- Make each property a separate check
- Iterate over raw cells rather than markers
- Fix property length check for 2nd to Nth items
- Improve error messages. If cell sizes are wrong, the next iteration can get a bad (but valid) phandle.
- Add a test

 checks.c                    | 104 ++++++++++++++++++++++++++++++++++++++++++++
 dtc.h                       |   1 +
 livetree.c                  |   6 +++
 tests/bad-phandle-cells.dts |  11 +++++
 tests/run_tests.sh          |   1 +
 5 files changed, 123 insertions(+)
 create mode 100644 tests/bad-phandle-cells.dts

checks.c +0 -0

index afabf64337d5..548d7e118c42 100644
@@ -956,6 +956,93 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,
 WARNING(obsolete_chosen_interrupt_controller,
      check_obsolete_chosen_interrupt_controller, NULL);
 

Quoted 7 lines

checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 962

Quoted 4 lines

checks.c +4 -0

+struct provider {
+     const char *prop_name;
+     const char *cell_name;
+     bool optional;

AFAICT you don't actually use this optional flag, even in the followup patches; it's always false.

Yes, it is. Here:

Quoted 1 line

checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 1037

Quoted 1 line

checks.c +1 -0

+WARNING_PROPERTY_PHANDLE_CELLS(msi_parent, "msi-parent", "#msi-cells", true);

Well hidden, isn't it. :)

Quoted 20 lines

Quoted 20 lines

checks.c +18 -0

+};
+
+static void check_property_phandle_args(struct check *c,
+                                       struct dt_info *dti,
+                                       struct node *node,
+                                       struct property *prop,
+                                       const struct provider *provider)
+{
+     struct node *root = dti->dt;
+     int cell, cellsize = 0;
+
+     for (cell = 0; cell < prop->val.len / sizeof(cell_t); cell += cellsize + 1) {
+             struct node *provider_node;
+             struct property *cellprop;
+             int phandle;
+
+             phandle = propval_cell_n(prop, cell);
+             if (phandle == 0 || phandle == -1) {

Quoted 8 lines

checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 982

Quoted 2 lines

checks.c +2 -0

+                     cellsize = 0;
+                     continue;

I'm not clear what case this is handling. If the property has an invalid (or unresolved) phandle value, shouldn't that be a FAIL? As it is we interpret the next cell as a phandle, and since we couldn't resolve the first phandle, we can't be at all sure that it really is a phandle.

It is valid to have a "blank" phandle when you have optional entries, but need to preserve the indexes of the entries. Say an array of gpio lines and some may not be hooked up. Not widely used, but it does exist in kernel dts files.

Quoted 24 lines

Quoted 24 lines

checks.c +22 -0

+             }
+
+             provider_node = get_node_by_phandle(root, phandle);
+             if (!provider_node) {
+                     FAIL(c, dti, "Could not get phandle node for %s:%s(cell %d)",
+                          node->fullpath, prop->name, cell);
+                     break;
+             }
+
+             cellprop = get_property(provider_node, provider->cell_name);
+             if (cellprop) {
+                     cellsize = propval_cell(cellprop);
+             } else if (provider->optional) {
+                     cellsize = 0;
+             } else {
+                     FAIL(c, dti, "Missing property '%s' in node %s or bad phandle (referred from %s:%s[%d])",
+                          provider->cell_name,
+                          provider_node->fullpath,
+                          node->fullpath, prop->name, cell);
+                     break;
+             }
+

Quoted 8 lines

checks.c static void check_obsolete_chosen_interrupt_controller(struct check *c, · line 1008

Quoted 4 lines

checks.c +4 -0

+             if (prop->val.len < ((cell + cellsize + 1) * sizeof(cell_t))) {
+                     FAIL(c, dti, "%s property size (%d) too small for cell size %d in %s",
+                          prop->name, prop->val.len, cellsize, node->fullpath);
+             }

How will this cope if the property is really badly formed - e.g. a 3 byte property, so you can't even grab a whole first phandle? I think it will trip the assert() in propval_cell_n() which isn't great.

At least for your example, we'd exit the loop (cell < 3/4). But I need to a check for that because it would be silent currently. I'll add a check that the size is a multiple of 4 and greater than 0.

However, the check here is not perfect because we could have "<&phandle1 1 2>" when really it should be "<&phandle1 &phandle2 &phandle3>". We don't fail until we're checking the 2nd phandle. That's why I added the "or bad phandle" and the cell # in the message above. In the opposite case, we'd be silent. One thing that could be done is double check things against the markers if they are present.

Quoted 5 lines · Rob Herring

Quoted 3 lines · David Gibson

Quoted 2 lines · Rob Herring

Many common bindings follow the same pattern of client properties containing a phandle and N arg cells where N is defined in the

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

with a '#<specifier>-cells' property such as:

Quoted 6 lines

However, the check here is not perfect because we could have "<&phandle1 1 2>" when really it should be "<&phandle1 &phandle2 &phandle3>". We don't fail until we're checking the 2nd phandle. That's why I added the "or bad phandle" and the cell # in the message above. In the opposite case, we'd be silent. One thing that could be done is double check things against the markers if they are present.

Uh.. I don't really understand what you're getting at here. We should be able to determine which of these cases it should be by the #whatever-cells at &phandle1.

Quoted 6 lines · Rob Herring

Quoted 4 lines · David Gibson

Quoted 3 lines · Rob Herring

Many common bindings follow the same pattern of client properties containing a phandle and N arg cells where N is defined in the provider with a '#<specifier>-cells' property such as:

[...]

Quoted 13 lines

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+             if (prop->val.len < ((cell + cellsize + 1) * sizeof(cell_t))) {
+                     FAIL(c, dti, "%s property size (%d) too small for cell size %d in %s",
+                          prop->name, prop->val.len, cellsize, node->fullpath);
+             }

How will this cope if the property is really badly formed - e.g. a 3 byte property, so you can't even grab a whole first phandle? I think it will trip the assert() in propval_cell_n() which isn't great.

At least for your example, we'd exit the loop (cell < 3/4). But I need to a check for that because it would be silent currently. I'll add a check that the size is a multiple of 4 and greater than 0.

Quoted 6 lines

However, the check here is not perfect because we could have "<&phandle1 1 2>" when really it should be "<&phandle1 &phandle2 &phandle3>". We don't fail until we're checking the 2nd phandle. That's why I added the "or bad phandle" and the cell # in the message above. In the opposite case, we'd be silent. One thing that could be done is double check things against the markers if they are present.

Here's what that looks like:

/* If we have markers, verify the current cell is a phandle */ if (prop->val.markers) {
struct marker *m = prop->val.markers;

  for_each_marker_of_type(m, REF_PHANDLE) {
    if (m->offset == (cell * sizeof(cell_t)))
      break;
  }
  if (!m)
  FAIL(c, dti, "Property '%s', cell %d is not a valid phandle in %s",
    prop->name, cell, node->fullpath);

Quoted 35 lines · Rob Herring

Quoted 6 lines · Rob Herring

Quoted 4 lines · David Gibson

Quoted 3 lines · Rob Herring

Many common bindings follow the same pattern of client properties containing a phandle and N arg cells where N is defined in the provider with a '#<specifier>-cells' property such as:

[...]

Quoted 13 lines

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+             if (prop->val.len < ((cell + cellsize + 1) * sizeof(cell_t))) {
+                     FAIL(c, dti, "%s property size (%d) too small for cell size %d in %s",
+                          prop->name, prop->val.len, cellsize, node->fullpath);
+             }

How will this cope if the property is really badly formed - e.g. a 3 byte property, so you can't even grab a whole first phandle? I think it will trip the assert() in propval_cell_n() which isn't great.

At least for your example, we'd exit the loop (cell < 3/4). But I need to a check for that because it would be silent currently. I'll add a check that the size is a multiple of 4 and greater than 0.

Quoted 6 lines

However, the check here is not perfect because we could have "<&phandle1 1 2>" when really it should be "<&phandle1 &phandle2 &phandle3>". We don't fail until we're checking the 2nd phandle. That's why I added the "or bad phandle" and the cell # in the message above. In the opposite case, we'd be silent. One thing that could be done is double check things against the markers if they are present.

Here's what that looks like:

/* If we have markers, verify the current cell is a phandle */ if (prop->val.markers) {
struct marker *m = prop->val.markers;

Quoted 7 lines

  for_each_marker_of_type(m, REF_PHANDLE) {
    if (m->offset == (cell * sizeof(cell_t)))
      break;
  }
  if (!m)
  FAIL(c, dti, "Property '%s', cell %d is not a valid phandle in %s",
    prop->name, cell, node->fullpath);

The logic seems sound, but I don't like the message. An integer literal is no less a phandle than a reference, just usually not the best way of entering one.

Quoted 45 lines · David Gibson

Quoted 35 lines · Rob Herring

Quoted 6 lines · Rob Herring

Quoted 4 lines · David Gibson

Quoted 3 lines · Rob Herring

Many common bindings follow the same pattern of client properties containing a phandle and N arg cells where N is defined in the provider with a '#<specifier>-cells' property such as:

[...]

Quoted 13 lines

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+             if (prop->val.len < ((cell + cellsize + 1) * sizeof(cell_t))) {
+                     FAIL(c, dti, "%s property size (%d) too small for cell size %d in %s",
+                          prop->name, prop->val.len, cellsize, node->fullpath);
+             }

How will this cope if the property is really badly formed - e.g. a 3 byte property, so you can't even grab a whole first phandle? I think it will trip the assert() in propval_cell_n() which isn't great.

At least for your example, we'd exit the loop (cell < 3/4). But I need to a check for that because it would be silent currently. I'll add a check that the size is a multiple of 4 and greater than 0.

Quoted 6 lines

However, the check here is not perfect because we could have "<&phandle1 1 2>" when really it should be "<&phandle1 &phandle2 &phandle3>". We don't fail until we're checking the 2nd phandle. That's why I added the "or bad phandle" and the cell # in the message above. In the opposite case, we'd be silent. One thing that could be done is double check things against the markers if they are present.

Here's what that looks like:

/* If we have markers, verify the current cell is a phandle */ if (prop->val.markers) {
struct marker *m = prop->val.markers;

Quoted 7 lines

  for_each_marker_of_type(m, REF_PHANDLE) {
    if (m->offset == (cell * sizeof(cell_t)))
      break;
  }
  if (!m)
  FAIL(c, dti, "Property '%s', cell %d is not a valid phandle in %s",
    prop->name, cell, node->fullpath);

Quoted 3 lines

The logic seems sound, but I don't like the message. An integer literal is no less a phandle than a reference, just usually not the best way of entering one.

Then what do you propose? There's not really any way I can distinguish a mixture. If #whatever-cells was wrong and I'm pointing to an integer literal that's not a phandle, it looks no different than if #whatever-cells is correct and I'm pointing to an integer literal that is a phandle.