Re: [PATCH|RFC] of: let of_match_device() always return best match

[Rob Herring]

On 10/03/2013 01:51 PM, Marc Kleine-Budde wrote:
> The function of_match_device() should tell if a struct device matches an
> of_device_id list and return the specific entry of that table matches the
> device best.
> 
> The underlying __of_match_node() implements the wrong search algorithm. It
> iterates over the list of of_device_ids, comparing the first compatible with
> _all_ compatibles of the struct device, then the second compatible of
> of_device_id and so on.
> 
> This leads to a problem, if the device has more than one compatible that match
> the of_device_id list. The implemented search algorithm may find not the "best"
> match. As the compatible list in the device is sorted from most to least
> specific.
> 
> For example:
> 
> The imx28.dtsi gives this compatible string for its CAN core:
> 
>> 	compatible = "fsl,imx28-flexcan", "fsl,p1010-flexcan";
> 
> The flexcan driver defines:
> 
>> static const struct of_device_id flexcan_of_match[] = {
>> 	{ .compatible = "fsl,p1010-flexcan", .data = &fsl_p1010_devtype_data, },
>> 	{ .compatible = "fsl,imx28-flexcan", .data = &fsl_imx28_devtype_data, },
>> 	{ .compatible = "fsl,imx6q-flexcan", .data = &fsl_imx6q_devtype_data, },
>> 	{ /* sentinel */ },
>> };
> 
> The "p1010" was the first Freescale SoC with the flexcan core. But this SoC has
> a bug, so a workaround has to be enabled in the driver. The mx28 has this bug
> fixed, so we don't need this quite costly workaround.
> 
> The __of_match_node() will compare:
> 
>     from  of_device_id		from device
>     fsl,p1010-flexcan		fsl,imx28-flexcan
>     fsl,p1010-flexcan		fsl,p1010-flexcan	-> MATCH
> 
> The of_match_device() function as it currently is implemented will always
> return p1010 not the mx28.
> 
> This patch fixes the problem by exchanging outer and inner loop. The first
> compatible of a device is compared against all compatible from the of_device_id
> list, then the second device compatible and so on.

This has been an issue for some time. A fix has been attempted before
and reverted if you look at the git history:

commit bc51b0c22cebf5c311a6f1895fcca9f78efd0478
Author: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Date:   Tue Jul 10 12:49:32 2012 -0700

    Revert "of: match by compatible property first"

    This reverts commit 107a84e61cdd3406c842a0e4be7efffd3a05dba6.

    Meelis Roos reports a regression since 3.5-rc5 that stops Sun Fire V100
    and Sun Netra X1 sparc64 machines from booting, hanging after enabling
    serial console.  He bisected it to commit 107a84e61cdd.

    Rob Herring explains:
     "The problem is match combinations of compatible plus name and/or type
      fail to match correctly.  I have a fix for this, but given how late it
      is for 3.5 I think it is best to revert this for now.  There could be
      other cases that rely on the current although wrong behavior.  I will
      post an updated version for 3.6."

    Bisected-and-reported-by: Meelis Roos <mroos@xxxxxxxx>
    Requested-by: Rob Herring <rob.herring@xxxxxxxxxxx>
    Cc: Thierry Reding <thierry.reding@xxxxxxxxxxxxxxxxx>
    Cc: Grant Likely <grant.likely@xxxxxxxxxxxx>
    Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>

There was also a fix attempted for this and the discussion here:

http://www.mail-archive.com/linuxppc-dev@xxxxxxxxxxxxxxxx/msg60163.html

You patch would hit the same issues I believe.

Rob

> 
> Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> ---
>  drivers/of/base.c | 40 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 865d3f6..5bff2fe 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -349,6 +349,35 @@ static int __of_device_is_compatible(const struct device_node *device,
>  	return 0;
>  }
>  
> +/** Returns the specific of_device_id, if the it matches one of the
> + * strings in the device's "compatible" property
> + */
> +static
> +const struct of_device_id *__of_device_matches_compatible(const struct device_node *device,
> +							  const struct of_device_id *matches)
> +{
> +	const struct of_device_id *m;
> +	const char* cp;
> +	int cplen, l;
> +
> +	cp = __of_get_property(device, "compatible", &cplen);
> +	if (cp == NULL)
> +		return NULL;
> +	while (cplen > 0) {
> +		m = matches;
> +		while (m->compatible[0]) {
> +			if (of_compat_cmp(cp, m->compatible, strlen(m->compatible)) == 0)
> +				return m;
> +			m++;
> +		}
> +		l = strlen(cp) + 1;
> +		cp += l;
> +		cplen -= l;
> +	}
> +
> +	return NULL;
> +}
> +
>  /** Checks if the given "compat" string matches one of the strings in
>   * the device's "compatible" property
>   */
> @@ -717,22 +746,23 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
>  {
>  	if (!matches)
>  		return NULL;
> -
> -	while (matches->name[0] || matches->type[0] || matches->compatible[0]) {
> +	while (matches->name[0] || matches->type[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++;
>  	}
> +
> +	if (matches->compatible[0])
> +		return __of_device_matches_compatible(node, matches);
> +
>  	return NULL;
>  }
>  
> 

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