[PATCH v2 0/3] dtc: checks for phandle with arg properties

[PATCH v2 0/3] dtc: checks for phandle with arg properties

[Rob Herring]

David,

Here's a new version of checks for phandle with arg style properties. This
checks interrupt and gpio bindings and other common bindings which follow 
the same phandle plus args pattern.

This generates ~200 warnings building the ARM dts files in the kernel.

Rob

Rob Herring (3):
  checks: add phandle with arg property checks
  checks: add gpio binding properties check
  checks: add interrupts property check

 checks.c                      | 253 ++++++++++++++++++++++++++++++++++++++++++
 dtc.h                         |   1 +
 livetree.c                    |   6 +
 tests/bad-gpio.dts            |  13 +++
 tests/bad-interrupt-cells.dts |  12 ++
 tests/bad-phandle-cells.dts   |  11 ++
 tests/run_tests.sh            |   4 +
 7 files changed, 300 insertions(+)
 create mode 100644 tests/bad-gpio.dts
 create mode 100644 tests/bad-interrupt-cells.dts
 create mode 100644 tests/bad-phandle-cells.dts

-- 
2.11.0

[PATCH v2 1/3] checks: add phandle with arg property checks

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

diff --git a/checks.c b/checks.c
index afabf64337d5..548d7e118c42 100644
--- a/checks.c
+++ b/checks.c
@@ -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);
 
+struct provider {
+	const char *prop_name;
+	const char *cell_name;
+	bool optional;
+};
+
+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) {
+			cellsize = 0;
+			continue;
+		}
+
+		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;
+		}
+
+		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);
+		}
+	}
+}
+
+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,
 };
 
diff --git a/dtc.h b/dtc.h
index 409db76c94b7..3c0532a7c3ab 100644
--- a/dtc.h
+++ b/dtc.h
@@ -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,
diff --git a/livetree.c b/livetree.c
index aecd27875fdd..c815176ec241 100644
--- a/livetree.c
+++ b/livetree.c
@@ -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)
 {
diff --git a/tests/bad-phandle-cells.dts b/tests/bad-phandle-cells.dts
new file mode 100644
index 000000000000..7f7c6a25fd25
--- /dev/null
+++ b/tests/bad-phandle-cells.dts
@@ -0,0 +1,11 @@
+/dts-v1/;
+
+/ {
+	intc: interrupt-controller {
+		#interrupt-cells = <3>;
+	};
+
+	node {
+		interrupts-extended = <&intc>;
+	};
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 3bc5b41ce76d..7cbc6971130a 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -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
-- 
2.11.0

[PATCH v2 2/3] checks: add gpio binding properties check

[Rob Herring]

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

diff --git a/checks.c b/checks.c
index 548d7e118c42..3315a4646497 100644
--- a/checks.c
+++ b/checks.c
@@ -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,
 };
 
diff --git a/tests/bad-gpio.dts b/tests/bad-gpio.dts
new file mode 100644
index 000000000000..6b77be447b82
--- /dev/null
+++ b/tests/bad-gpio.dts
@@ -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>;
+	};
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 7cbc6971130a..2a29fa4ee451 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -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
-- 
2.11.0

[PATCH v2 3/3] checks: add interrupts property check

[Rob Herring]

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

diff --git a/checks.c b/checks.c
index 3315a4646497..6b505e9c49d8 100644
--- a/checks.c
+++ b/checks.c
@@ -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,
 };
diff --git a/tests/bad-interrupt-cells.dts b/tests/bad-interrupt-cells.dts
new file mode 100644
index 000000000000..39fc78fdc11d
--- /dev/null
+++ b/tests/bad-interrupt-cells.dts
@@ -0,0 +1,12 @@
+/dts-v1/;
+
+/ {
+	interrupt-parent = <&intc>;
+	intc: interrupt-controller {
+		#interrupt-cells = <3>;
+	};
+
+	node {
+		interrupts = <1>;
+	};
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 2a29fa4ee451..cc35205f5768 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -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
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] checks: add phandle with arg property checks

[David Gibson]

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

On Tue, Aug 22, 2017 at 06:02:06PM -0500, Rob Herring wrote:
> 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 <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
> 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
> 
> diff --git a/checks.c b/checks.c
> index afabf64337d5..548d7e118c42 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -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);
>  
> +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.

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

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

> +	}
> +}
> +
> +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,
>  };
>  
> diff --git a/dtc.h b/dtc.h
> index 409db76c94b7..3c0532a7c3ab 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -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,
> diff --git a/livetree.c b/livetree.c
> index aecd27875fdd..c815176ec241 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -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)
>  {
> diff --git a/tests/bad-phandle-cells.dts b/tests/bad-phandle-cells.dts
> new file mode 100644
> index 000000000000..7f7c6a25fd25
> --- /dev/null
> +++ b/tests/bad-phandle-cells.dts
> @@ -0,0 +1,11 @@
> +/dts-v1/;
> +
> +/ {
> +	intc: interrupt-controller {
> +		#interrupt-cells = <3>;
> +	};
> +
> +	node {
> +		interrupts-extended = <&intc>;
> +	};
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 3bc5b41ce76d..7cbc6971130a 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -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

-- 
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 2/3] checks: add gpio binding properties check

[David Gibson]

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

On Tue, Aug 22, 2017 at 06:02:07PM -0500, Rob Herring wrote:
> 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 <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

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

Not merging right now, just in case it needs any tweaks to apply on
top of a revised patch #1.

> ---
> 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
> 
> diff --git a/checks.c b/checks.c
> index 548d7e118c42..3315a4646497 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -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,
>  };
>  
> diff --git a/tests/bad-gpio.dts b/tests/bad-gpio.dts
> new file mode 100644
> index 000000000000..6b77be447b82
> --- /dev/null
> +++ b/tests/bad-gpio.dts
> @@ -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>;
> +	};
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 7cbc6971130a..2a29fa4ee451 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -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

-- 
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 3/3] checks: add interrupts property check

[David Gibson]

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

On Tue, Aug 22, 2017 at 06:02:08PM -0500, Rob Herring wrote:
> 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 <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

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

> ---
> 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
> 
> diff --git a/checks.c b/checks.c
> index 3315a4646497..6b505e9c49d8 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -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,
>  };
> diff --git a/tests/bad-interrupt-cells.dts b/tests/bad-interrupt-cells.dts
> new file mode 100644
> index 000000000000..39fc78fdc11d
> --- /dev/null
> +++ b/tests/bad-interrupt-cells.dts
> @@ -0,0 +1,12 @@
> +/dts-v1/;
> +
> +/ {
> +	interrupt-parent = <&intc>;
> +	intc: interrupt-controller {
> +		#interrupt-cells = <3>;
> +	};
> +
> +	node {
> +		interrupts = <1>;
> +	};
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 2a29fa4ee451..cc35205f5768 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -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

-- 
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 1/3] checks: add phandle with arg property checks

[Rob Herring]

On Wed, Aug 23, 2017 at 9:03 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Tue, Aug 22, 2017 at 06:02:06PM -0500, Rob Herring wrote:
>> 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 <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> ---
>> 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
>>
>> diff --git a/checks.c b/checks.c
>> index afabf64337d5..548d7e118c42 100644
>> --- a/checks.c
>> +++ b/checks.c
>> @@ -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);
>>
>> +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:

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

Well hidden, isn't it. :)

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


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

Rob

Re: [PATCH v2 1/3] checks: add phandle with arg property checks

[Rob Herring]

On Thu, Aug 24, 2017 at 12:23 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Aug 23, 2017 at 9:03 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> On Tue, Aug 22, 2017 at 06:02:06PM -0500, Rob Herring wrote:
>>> 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:

[...]

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

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);
}
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] checks: add phandle with arg property checks

[David Gibson]

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

On Thu, Aug 24, 2017 at 12:23:16PM -0500, Rob Herring wrote:
> On Wed, Aug 23, 2017 at 9:03 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Tue, Aug 22, 2017 at 06:02:06PM -0500, Rob Herring wrote:
> >> 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 <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >> ---
> >> 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
> >>
> >> diff --git a/checks.c b/checks.c
> >> index afabf64337d5..548d7e118c42 100644
> >> --- a/checks.c
> >> +++ b/checks.c
> >> @@ -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);
> >>
> >> +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:
> 
> >> +WARNING_PROPERTY_PHANDLE_CELLS(msi_parent, "msi-parent", "#msi-cells", true);
> 
> Well hidden, isn't it. :)

Little bit, yes.  Objection withdrawn.

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

Ah ok.  A comment to that effect would be helpful.

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

Ok.

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

-- 
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 1/3] checks: add phandle with arg property checks

[Rob Herring]

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

On Thu, Aug 24, 2017 at 7:49 PM, David Gibson <david@gibson.dropbear.id.au>
wrote:
> On Thu, Aug 24, 2017 at 12:23:16PM -0500, Rob Herring wrote:
>> On Wed, Aug 23, 2017 at 9:03 PM, David Gibson
>> <david@gibson.dropbear.id.au> wrote:
>> > On Tue, Aug 22, 2017 at 06:02:06PM -0500, Rob Herring wrote:
>> >> 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:
>> >>

[...]

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

If #whatever-cells is wrong, we can't tell immediately. Say it is 1 in the
above case, but really should be 2. Then on the 2nd loop iteration, we
think the value "2" is a phandle which will actually succeed in the lookup.
The failure we get is #whatever-cells couldn't be found (or maybe it is
found by luck, who knows) in the node pointed to by phandle 2. There's a
case of this in the kernel that I hit which took me a minute to realize
what was going on. The point is that if the property value has errors, we
may go into the weeds and the error reported may not be that precise.

Rob

[-- Attachment #2: Type: text/html, Size: 2224 bytes --]

Re: [PATCH v2 1/3] checks: add phandle with arg property checks

[David Gibson]

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

On Thu, Aug 24, 2017 at 02:19:27PM -0500, Rob Herring wrote:
> On Thu, Aug 24, 2017 at 12:23 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Wed, Aug 23, 2017 at 9:03 PM, David Gibson
> > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >> On Tue, Aug 22, 2017 at 06:02:06PM -0500, Rob Herring wrote:
> >>> 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:
> 
> [...]
> 
> >>> +             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.
> 
> 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);

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.

-- 
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 1/3] checks: add phandle with arg property checks

[Rob Herring]

On Fri, Aug 25, 2017 at 8:17 AM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Thu, Aug 24, 2017 at 02:19:27PM -0500, Rob Herring wrote:
> > On Thu, Aug 24, 2017 at 12:23 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > > On Wed, Aug 23, 2017 at 9:03 PM, David Gibson
> > > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > >> On Tue, Aug 22, 2017 at 06:02:06PM -0500, Rob Herring wrote:
> > >>> 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:
> >
> > [...]
> >
> > >>> +             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.
> >
> > 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);
>
> 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.

Rob

Re: [PATCH v2 1/3] checks: add phandle with arg property checks

[David Gibson]

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

On Fri, Aug 25, 2017 at 10:27:09AM -0500, Rob Herring wrote:
> On Fri, Aug 25, 2017 at 8:17 AM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Thu, Aug 24, 2017 at 02:19:27PM -0500, Rob Herring wrote:
> > > On Thu, Aug 24, 2017 at 12:23 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > > > On Wed, Aug 23, 2017 at 9:03 PM, David Gibson
> > > > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > >> On Tue, Aug 22, 2017 at 06:02:06PM -0500, Rob Herring wrote:
> > > >>> 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:
> > >
> > > [...]
> > >
> > > >>> +             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.
> > >
> > > 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);
> >
> > 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.

Simply s/valid phandle/phandle reference/ would be sufficient.

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