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

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

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

On Mon, Aug 14, 2017 at 04:48:06PM -0500, Rob Herring wrote:

Yay! Someone actually implementing checks.

> 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. Add a checks for properties
> following this pattern.

I think this description would be easier to follow if you led with an example.

> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Looks pretty good, though I have some suggestions.

> ---
>  checks.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 117 insertions(+)
> 
> diff --git a/checks.c b/checks.c
> index afabf64337d5..c0450e118043 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -956,6 +956,120 @@ 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 marker *m = prop->val.markers;
> +
> +	if (!m) {
> +		FAIL(c, dti, "Missing phandles in %s:%s",
> +		     node->fullpath, prop->name);
> +		return;
> +	}
> +	for_each_marker_of_type(m, REF_PHANDLE) {

So going through the markers I think is not the best approach.
That'll work if the source contains a reference here, which it usually
will, but there are some circumstances where it could contain a "raw"
phandle value (the most obvious being when you're decompiling an
existing dtb).

But I don't think you really need to.  Instead you should be able to
read the actual value, look it up with get_node_by_phandle().  You can
make this check dependent on fixup_phandle_references to make sure
it's executed after the references are resolved.

That should also let you more strictly verify the property in the
client node, including checking for misaligned entries of extraneous bits.

> +		int cellsize;
> +		struct node *root = dti->dt;
> +		struct node *provider_node;
> +		struct property *cellprop;
> +
> +		provider_node = get_node_by_ref(root, m->ref);
> +		if (!provider_node) {
> +			FAIL(c, dti, "Could not get provider for %s:%s",
> +			     node->fullpath, prop->name);
> +			break;
> +		}

AFAIK the "provider" terminology isn't standardized somewhere.  It's a
reasonable term internally, but if can rephrase to it in the displayed
messages that would be a bonus.

> +		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 %s in provider %s for %s",
> +			     provider->cell_name,
> +		     	     provider_node->fullpath,
> +	     		     node->fullpath);
> +			break;
> +		}
> +
> +		if (prop->val.len < ((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 const struct provider providers[] = {
> +	{ "clocks", "#clock-cells" },
> +	{ "phys", "#phy-cells" },
> +	{ "interrupts-extended", "#interrupt-cells" },
> +	{ "mboxes", "#mbox-cells" },
> +	{ "pwms", "#pwm-cells" },
> +	{ "dmas", "#dma-cells" },
> +	{ "resets", "#reset-cells" },
> +	{ "hwlocks", "#hwlock-cells" },
> +	{ "power-domains", "#power-domain-cells" },
> +	{ "io-channels", "#io-channel-cells" },
> +	{ "iommus", "#iommu-cells" },
> +	{ "mux-controls", "#mux-control-cells" },
> +	{ "cooling-device", "#cooling-cells" },
> +	{ "thermal-sensors", "#thermal-sensor-cells" },
> +	{ "sound-dais", "#sound-dai-cells" },
> +	{ "msi-parent", "#msi-cells", true },
> +	{ NULL },
> +};

How hard would it be to use macros to make each of these providers
different check structures.  I think the output would be a bit more
useful if errors in different types of these things was reported as a
different check failure rather than one big generic one.

It does mean listing everything in the check_table which is a pain.  I
would really like to change things so that a single macro can both
declare the check and add it to the master list, but I haven't thought
of a portable way to do that so far.

If you do need the providers array, ARRAY_SIZE() is preferred over a
terminator NULL.

> +static void check_provider_cells_property(struct check *c,
> +					  struct dt_info *dti,
> +				          struct node *node)
> +{
> +	int i;
> +
> +	for (i = 0; providers[i].prop_name; i++) {
> +		struct property *prop = get_property(node, providers[i].prop_name);
> +		if (!prop)
> +			continue;
> +		check_property_phandle_args(c, dti, node, prop, &providers[i]);
> +	}
> +}
> +WARNING(provider_cells_property, check_provider_cells_property, NULL);
> +
> +static void check_gpio_cells_property(struct check *c,
> +					  struct dt_info *dti,
> +				          struct node *node)
> +{
> +	struct property *prop;
> +
> +	for_each_property(node, prop) {
> +		char *str;
> +		struct provider provider;
> +
> +		/* Skip over false matches */
> +		if (strstr(prop->name, "nr-gpio"))
> +			continue;
> +
> +		str = strrchr(prop->name, '-');
> +		if (!str || !(streq(str, "-gpio") || streq(str, "-gpios")))
> +			continue;
> +
> +		provider.prop_name = prop->name;
> +		provider.cell_name = "#gpio-cells";
> +		provider.optional = false;
> +		check_property_phandle_args(c, dti, node, prop, &provider);
> +	}
> +
> +}
> +WARNING(gpio_cells_property, check_gpio_cells_property, NULL);

Since the gpio stuff is a bit tweaker, it would be good to go into a
separate patch.  Then the commit message can explain what the logic
here is for (I'm having a little trouble following it).

>  static struct check *check_table[] = {
>  	&duplicate_node_names, &duplicate_property_names,
>  	&node_name_chars, &node_name_format, &property_name_chars,
> @@ -987,6 +1101,9 @@ static struct check *check_table[] = {
>  	&avoid_default_addr_size,
>  	&obsolete_chosen_interrupt_controller,
>  
> +	&provider_cells_property,
> +	&gpio_cells_property,
> +
>  	&always_fail,
>  };
>  

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