Re: [PATCH] OF: base: match each node compatible against all givenmatches first

[Sebastian Hesselbarth]

On 12/02/13 15:00, Rob Herring wrote:
> On 12/02/2013 06:03 AM, Thierry Reding wrote:
> > On Thu, Nov 28, 2013 at 07:36:25PM +0100, Sebastian Hesselbarth
> > wrote:
> > > Currently, of_match_node compares each given match against all
> > > node's compatible strings with of_device_is_compatible.
> > >
> > > To achieve multiple compatible strings per node with ordering
> > > from specific to generic, this requires given matches to be
> > > ordered from specific to generic. For most of the drivers this
> > > is not true and also an alphabetical ordering is more sane
> > > there.
> > >
> > > Therefore, this patch modifies of_match_node to match each of
> > > the node's compatible strings against all given matches first,
> > > before checking the next compatible string. This implies that
> > > node's compatibles are ordered from specific to generic while
> > > given matches can be in any order.
> > >
> > > Signed-off-by: Sebastian Hesselbarth
[...]
> > That said, I think you might actually have nailed it with this
> > patch. From what I remember all earlier attempt failed because
> > they didn't match all compatible/name/type combinations properly.
> > I'm adding a few people on Cc who were involved with the other
> > patches, perhaps they can give your patch a spin and see if it
> > fixes things for them.
>
> I agree and the change here is simple enough I think the chances of a
> regression are low. But there is one issue...
>
> > Thierry
> >
> > [0]: https://lkml.org/lkml/2013/10/3/585
> >
> > > diff --git a/drivers/of/base.c b/drivers/of/base.c index
> > > 7d4c70f..183a9c7 100644 --- a/drivers/of/base.c +++
> > > b/drivers/of/base.c @@ -713,23 +713,37 @@ static const struct
> > > of_device_id *__of_match_node(const struct of_device_id *matches,
> > > const struct device_node *node) { +	const char *cp; + int cplen,
> > > l; + if (!matches) return NULL;
> > >
> > > -	while (matches->name[0] || matches->type[0] ||
> > > matches->compatible[0]) { -		int match = 1; -		if
> > > (matches->name[0]) -			match &= node->name -				&&
> > > !strcmp(matches->name, node->name); -		if (matches->type[0]) -
> > > match &= node->type -				&& !strcmp(matches->type, node->type);
> > > - if (matches->compatible[0]) -			match &=
> > > __of_device_is_compatible(node, -							   matches->compatible);
> > >   -		if (match) -			return matches; -		matches++; +	cp =
> > > __of_get_property(node, "compatible", &cplen); +	if (!cp) +
> > > return NULL;
>
> No compatible property and matching on name and/or type only is valid.
> Remove the if here and changing the following should work:
>
> > > + +	while (cplen > 0) {
>
> do {
>
> > > +		const struct of_device_id *m = matches; + +		while
> > > (m->name[0] || m->type[0] || m->compatible[0]) { +			int match =
> > > 1; +			if (m->name[0]) +				match &= node->name +					&&
> > > !strcmp(m->name, node->name); +			if (m->type[0]) +				match &=
> > > node->type + && !strcmp(m->type, node->type); +			if
> > > (m->compatible[0])
>
> if (cp && m->compatible[0])
>
> > > +				match &= !of_compat_cmp(cp, m->compatible, +
> > > strlen(m->compatible)); +			if (match) +				return m; +			m++; +
> > > } +		l = strlen(cp) + 1; +		cp += l; +		cplen -= l; }
>
>
> } while (cp && (cplen > 0));

Rob,

although quoting somehow got messed up, I see the point. The pointer
arith right above loop end condition needs to be skipped too if !cp.

I will rework the loop to allow name/type matching with cp == NULL
and resend in a day or two.

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/