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

From: Thierry Reding
Date: Mon Dec 02 2013 - 07:04:01 EST


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 <sebastian.hesselbarth@xxxxxxxxx>
> ---
> Cc: Grant Likely <grant.likely@xxxxxxxxxx>
> Cc: Rob Herring <rob.herring@xxxxxxxxxxx>
> Cc: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
> Cc: Russell King <linux@xxxxxxxxxxxxxxxx>
> Cc: devicetree@xxxxxxxxxxxxxxx
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
> drivers/of/base.c | 47 ++++++++++++++++++++++++++++++++---------------
> 1 file changed, 32 insertions(+), 15 deletions(-)

Hi Sebastian,

I think you're one in a long line of people attempting to fix this. I
tried myself over a year ago (commit 107a84e61cdd 'of: match by
compatible property first') but it caused a subtle regression late in
the release cycle and was reverted (commit bc51b0c22ceb 'Revert "of:
match by compatible property first").

Only recently there was another attempt [0] but it's pretty much
equivalent to what I did back then.

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.

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;
> +
> + while (cplen > 0) {
> + 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])
> + match &= !of_compat_cmp(cp, m->compatible,
> + strlen(m->compatible));
> + if (match)
> + return m;
> + m++;
> + }
> + l = strlen(cp) + 1;
> + cp += l;
> + cplen -= l;
> }
> return NULL;
> }
> @@ -739,7 +753,10 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
> * @matches: array of of device match structures to search in
> * @node: the of device structure to match against
> *
> - * Low level utility function used by device matching.
> + * Low level utility function used by device matching. Matching order
> + * is to compare each of the node's compatibles with all given matches
> + * first. This implies node's compatible is sorted from specific to
> + * generic while matches can be in any order.
> */
> const struct of_device_id *of_match_node(const struct of_device_id *matches,
> const struct device_node *node)
> --
> 1.7.10.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Attachment: pgp00000.pgp
Description: PGP signature