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

Message ID 20170822230208.20987-2-robh@kernel.org
State New
Headers show
Series
  • dtc: checks for phandle with arg properties
Related show

Commit Message

Rob Herring Aug. 22, 2017, 11:02 p.m.
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@kernel.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

-- 
2.11.0

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

Comments

David Gibson Aug. 24, 2017, 2:03 a.m. | #1
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@kernel.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
Rob Herring Aug. 24, 2017, 5:23 p.m. | #2
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:

>>

>>       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@kernel.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Aug. 24, 2017, 7:19 p.m. | #3
On Thu, Aug 24, 2017 at 12:23 PM, Rob Herring <robh@kernel.org> 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:


[...]

>>> +             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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Gibson Aug. 25, 2017, 12:49 a.m. | #4
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:

> >>

> >>       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@kernel.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
David Gibson Aug. 25, 2017, 1:17 p.m. | #5
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@kernel.org> 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:

> 

> [...]

> 

> >>> +             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
Rob Herring Aug. 25, 2017, 3:27 p.m. | #6
On Fri, Aug 25, 2017 at 8:17 AM, David Gibson
<david@gibson.dropbear.id.au> 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@kernel.org> 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:

> >

> > [...]

> >

> > >>> +             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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Gibson Aug. 29, 2017, 7:26 a.m. | #7
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@gibson.dropbear.id.au> 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@kernel.org> 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:

> > >

> > > [...]

> > >

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

Patch

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