[PATCH 0/2] of: fix a regression when trying to find the best compatible match

[PATCH 0/2] of: fix a regression when trying to find the best compatible match

[Kevin Hao]

Hi,

This fix a regression when trying to find the best compatible match. You can
find the detail of the issue reported by Stephen Chivers at:
  https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-February/115278.html

I have made a patch to fix this previously.
  http://patchwork.ozlabs.org/patch/319624/

This is another respin with the implementation suggested by Rob Herring.

Kevin Hao (2):
  Revert "OF: base: match each node compatible against all given matches
    first"
  of: search the best compatible match first in __of_match_node()

 drivers/of/base.c | 88 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 54 insertions(+), 34 deletions(-)

-- 
1.8.5.3

[PATCH 1/2] Revert "OF: base: match each node compatible against all given matches first"

[Kevin Hao]

This reverts commit 105353145eafb3ea919f5cdeb652a9d8f270228e.
Stephen Chivers reported this is broken as we will get a match
entry '.type = "serial"' instead of the '.compatible = "ns16550"'
in the following scenario:
	serial0: serial@4500 {
		compatible = "fsl,ns16550", "ns16550";
	}

	struct of_device_id of_platform_serial_table[] = {
		{ .compatible = "ns8250",   .data = (void *)PORT_8250, },
		{ .compatible = "ns16450",  .data = (void *)PORT_16450, },
		{ .compatible = "ns16550a", .data = (void *)PORT_16550A, },
		{ .compatible = "ns16550",  .data = (void *)PORT_16550, },
		{ .compatible = "ns16750",  .data = (void *)PORT_16750, },
		{ .compatible = "ns16850",  .data = (void *)PORT_16850, },
		...
		{ .type = "serial",         .data = (void *)PORT_UNKNOWN, },
		{ /* end of list */ },
	};

So just revert this patch, we will use another implementation to find
the best compatible match in a follow-on patch.

Reported-by: Stephen N Chivers <schivers@csc.com.au>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 drivers/of/base.c | 53 ++++++++++++++++-------------------------------------
 1 file changed, 16 insertions(+), 37 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index ff85450d5683..ba195fbce4c6 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -734,42 +734,24 @@ 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;
 
-	cp = __of_get_property(node, "compatible", &cplen);
-	do {
-		const struct of_device_id *m = matches;
-
-		/* Check against matches with current compatible string */
-		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 &= cp
-					&& !of_compat_cmp(m->compatible, cp,
-							strlen(m->compatible));
-			if (match)
-				return m;
-			m++;
-		}
-
-		/* Get node's next compatible string */ 
-		if (cp) {
-			l = strlen(cp) + 1;
-			cp += l;
-			cplen -= l;
-		}
-	} while (cp && (cplen > 0));
-
+	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++;
+	}
 	return NULL;
 }
 
@@ -778,10 +760,7 @@ 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. 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.
+ *	Low level utility function used by device matching.
  */
 const struct of_device_id *of_match_node(const struct of_device_id *matches,
 					 const struct device_node *node)
-- 
1.8.5.3

[PATCH 2/2] of: search the best compatible match first in __of_match_node()

[Kevin Hao]

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 introduces a function to match each of the node's
compatible strings against all given compatible matches without type and
name 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. If we fail to find such a match
entry, then fall-back to the old method in order to keep compatibility.

Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 drivers/of/base.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index ba195fbce4c6..10b51106c854 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -730,13 +730,49 @@ out:
 }
 EXPORT_SYMBOL(of_find_node_with_property);
 
+static const struct of_device_id *
+of_match_compatible(const struct of_device_id *matches,
+			const struct device_node *node)
+{
+	const char *cp;
+	int cplen, l;
+	const struct of_device_id *m;
+
+	cp = __of_get_property(node, "compatible", &cplen);
+	while (cp && (cplen > 0)) {
+		m = matches;
+		while (m->name[0] || m->type[0] || m->compatible[0]) {
+			/* Only match for the entries without type and name */
+			if (m->name[0] || m->type[0] ||
+				of_compat_cmp(m->compatible, cp,
+					 strlen(m->compatible)))
+				m++;
+			else
+				return m;
+		}
+
+		/* Get node's next compatible string */
+		l = strlen(cp) + 1;
+		cp += l;
+		cplen -= l;
+	}
+
+	return NULL;
+}
+
 static
 const struct of_device_id *__of_match_node(const struct of_device_id *matches,
 					   const struct device_node *node)
 {
+	const struct of_device_id *m;
+
 	if (!matches)
 		return NULL;
 
+	m = of_match_compatible(matches, node);
+	if (m)
+		return m;
+
 	while (matches->name[0] || matches->type[0] || matches->compatible[0]) {
 		int match = 1;
 		if (matches->name[0])
@@ -760,7 +796,12 @@ 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. We have two ways
+ *	of matching:
+ *	- Try to find the best compatible match by comparing each compatible
+ *	  string of device node with all the given matches respectively.
+ *	- If the above method failed, then try to match the compatible by using
+ *	  __of_device_is_compatible() besides the match in type and name.
  */
 const struct of_device_id *of_match_node(const struct of_device_id *matches,
 					 const struct device_node *node)
-- 
1.8.5.3

Re: [PATCH 2/2] of: search the best compatible match first in __of_match_node()

[Rob Herring]

On Thu, Feb 13, 2014 at 11:22 PM, Kevin Hao <haokexin@gmail.com> 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 introduces a function to match each of the node's
> compatible strings against all given compatible matches without type and
> name 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. If we fail to find such a match
> entry, then fall-back to the old method in order to keep compatibility.
>
> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Signed-off-by: Kevin Hao <haokexin@gmail.com>

Looks good to me. I'll put this in next for a few days. I'd really
like to see some acks and tested-by's before sending to Linus.

We could be a bit more strict here and fallback to the old matching if
the match table has any entries with name or type. I don't think that
should be necessary though.

Rob

> ---
>  drivers/of/base.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index ba195fbce4c6..10b51106c854 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -730,13 +730,49 @@ out:
>  }
>  EXPORT_SYMBOL(of_find_node_with_property);
>
> +static const struct of_device_id *
> +of_match_compatible(const struct of_device_id *matches,
> +                       const struct device_node *node)
> +{
> +       const char *cp;
> +       int cplen, l;
> +       const struct of_device_id *m;
> +
> +       cp = __of_get_property(node, "compatible", &cplen);
> +       while (cp && (cplen > 0)) {
> +               m = matches;
> +               while (m->name[0] || m->type[0] || m->compatible[0]) {
> +                       /* Only match for the entries without type and name */
> +                       if (m->name[0] || m->type[0] ||
> +                               of_compat_cmp(m->compatible, cp,
> +                                        strlen(m->compatible)))
> +                               m++;
> +                       else
> +                               return m;
> +               }
> +
> +               /* Get node's next compatible string */
> +               l = strlen(cp) + 1;
> +               cp += l;
> +               cplen -= l;
> +       }
> +
> +       return NULL;
> +}
> +
>  static
>  const struct of_device_id *__of_match_node(const struct of_device_id *matches,
>                                            const struct device_node *node)
>  {
> +       const struct of_device_id *m;
> +
>         if (!matches)
>                 return NULL;
>
> +       m = of_match_compatible(matches, node);
> +       if (m)
> +               return m;
> +
>         while (matches->name[0] || matches->type[0] || matches->compatible[0]) {
>                 int match = 1;
>                 if (matches->name[0])
> @@ -760,7 +796,12 @@ 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. We have two ways
> + *     of matching:
> + *     - Try to find the best compatible match by comparing each compatible
> + *       string of device node with all the given matches respectively.
> + *     - If the above method failed, then try to match the compatible by using
> + *       __of_device_is_compatible() besides the match in type and name.
>   */
>  const struct of_device_id *of_match_node(const struct of_device_id *matches,
>                                          const struct device_node *node)
> --
> 1.8.5.3
>

Re: [PATCH 2/2] of: search the best compatible match first in __of_match_node()

[Kumar Gala]

On Feb 14, 2014, at 9:53 AM, Rob Herring <robherring2@gmail.com> wrote:

> On Thu, Feb 13, 2014 at 11:22 PM, Kevin Hao <haokexin@gmail.com> =
wrote:
>> Currently, of_match_node compares each given match against all node's
>> compatible strings with of_device_is_compatible.
>>=20
>> 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.
>>=20
>> Therefore, this patch introduces a function to match each of the =
node's
>> compatible strings against all given compatible matches without type =
and
>> name 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. If we fail to find such a match
>> entry, then fall-back to the old method in order to keep =
compatibility.
>>=20
>> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> Signed-off-by: Kevin Hao <haokexin@gmail.com>
>=20
> Looks good to me. I'll put this in next for a few days. I'd really
> like to see some acks and tested-by's before sending to Linus.
>=20
> We could be a bit more strict here and fallback to the old matching if
> the match table has any entries with name or type. I don't think that
> should be necessary though.
>=20
> Rob

Can you push the revert to Linus sooner, since currently a ton of boards =
wouldn=92t be working on the PPC side, so at least -rc3 has the =
possibility of working for them.

- k

>=20
>> ---
>> drivers/of/base.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 42 insertions(+), 1 deletion(-)
>>=20
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index ba195fbce4c6..10b51106c854 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -730,13 +730,49 @@ out:
>> }
>> EXPORT_SYMBOL(of_find_node_with_property);
>>=20
>> +static const struct of_device_id *
>> +of_match_compatible(const struct of_device_id *matches,
>> +                       const struct device_node *node)
>> +{
>> +       const char *cp;
>> +       int cplen, l;
>> +       const struct of_device_id *m;
>> +
>> +       cp =3D __of_get_property(node, "compatible", &cplen);
>> +       while (cp && (cplen > 0)) {
>> +               m =3D matches;
>> +               while (m->name[0] || m->type[0] || m->compatible[0]) =
{
>> +                       /* Only match for the entries without type =
and name */
>> +                       if (m->name[0] || m->type[0] ||
>> +                               of_compat_cmp(m->compatible, cp,
>> +                                        strlen(m->compatible)))
>> +                               m++;
>> +                       else
>> +                               return m;
>> +               }
>> +
>> +               /* Get node's next compatible string */
>> +               l =3D strlen(cp) + 1;
>> +               cp +=3D l;
>> +               cplen -=3D l;
>> +       }
>> +
>> +       return NULL;
>> +}
>> +
>> static
>> const struct of_device_id *__of_match_node(const struct of_device_id =
*matches,
>>                                           const struct device_node =
*node)
>> {
>> +       const struct of_device_id *m;
>> +
>>        if (!matches)
>>                return NULL;
>>=20
>> +       m =3D of_match_compatible(matches, node);
>> +       if (m)
>> +               return m;
>> +
>>        while (matches->name[0] || matches->type[0] || =
matches->compatible[0]) {
>>                int match =3D 1;
>>                if (matches->name[0])
>> @@ -760,7 +796,12 @@ 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. We have =
two ways
>> + *     of matching:
>> + *     - Try to find the best compatible match by comparing each =
compatible
>> + *       string of device node with all the given matches =
respectively.
>> + *     - If the above method failed, then try to match the =
compatible by using
>> + *       __of_device_is_compatible() besides the match in type and =
name.
>>  */
>> const struct of_device_id *of_match_node(const struct of_device_id =
*matches,
>>                                         const struct device_node =
*node)
>> --
>> 1.8.5.3

--=20
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, =
hosted by The Linux Foundation

Re: [PATCH 2/2] of: search the best compatible match first in __of_match_node()

[Kumar Gala]

On Feb 14, 2014, at 9:53 AM, Rob Herring <robherring2@gmail.com> wrote:

> On Thu, Feb 13, 2014 at 11:22 PM, Kevin Hao <haokexin@gmail.com> =
wrote:
>> Currently, of_match_node compares each given match against all node's
>> compatible strings with of_device_is_compatible.
>>=20
>> 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.
>>=20
>> Therefore, this patch introduces a function to match each of the =
node's
>> compatible strings against all given compatible matches without type =
and
>> name 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. If we fail to find such a match
>> entry, then fall-back to the old method in order to keep =
compatibility.
>>=20
>> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> Signed-off-by: Kevin Hao <haokexin@gmail.com>
>=20
> Looks good to me. I'll put this in next for a few days. I'd really
> like to see some acks and tested-by's before sending to Linus.
>=20
> We could be a bit more strict here and fallback to the old matching if
> the match table has any entries with name or type. I don't think that
> should be necessary though.
>=20
> Rob
>=20


Can you push the revert to Linus sooner, since currently a ton of boards =
wouldn=92t be working on the PPC side, so at least -rc3 has the =
possibility of working for them.

- k

Re: [PATCH 2/2] of: search the best compatible match first in __of_match_node()

[Stephen N Chivers]

Rob Herring <robherring2@gmail.com> wrote on 02/15/2014 02:53:40 AM:

> From: Rob Herring <robherring2@gmail.com>
> To: Kevin Hao <haokexin@gmail.com>
> Cc: "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, 
> linuxppc-dev <linuxppc-dev@lists.ozlabs.org>, Sebastian Hesselbarth 
> <sebastian.hesselbarth@gmail.com>, Stephen N Chivers 
> <schivers@csc.com.au>, Grant Likely <grant.likely@linaro.org>, Rob 
> Herring <robh+dt@kernel.org>, Kumar Gala <galak@codeaurora.org>
> Date: 02/15/2014 02:53 AM
> Subject: Re: [PATCH 2/2] of: search the best compatible match first 
> in __of_match_node()
> 
> On Thu, Feb 13, 2014 at 11:22 PM, Kevin Hao <haokexin@gmail.com> 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 introduces a function to match each of the 
node's
> > compatible strings against all given compatible matches without type 
and
> > name 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. If we fail to find such a match
> > entry, then fall-back to the old method in order to keep 
compatibility.
> >
> > Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> > Signed-off-by: Kevin Hao <haokexin@gmail.com>
> 
> Looks good to me. I'll put this in next for a few days. I'd really
> like to see some acks and tested-by's before sending to Linus.
> 
Tested-by: Stephen Chivers <schivers@csc.com>

I have tested the patch for the four PowerPC platforms
available to me.

They are:

MPC8349_MITXGP - Works.
MVME5100        - Works.
MVME4100 - Works.
SAM440EP - Works.

The MPC8349_MITXGP platform is present in Linux-3.13 and previous 
releases.
The MVME5100 is a "revived" platform that is in Linux-3.14-rc2.
The MVME4100 is a work in progress and is the 85xx platform that the
original failure report was for.
The SAM440EP is present in Linux-3.13 and previous releases.

The MPC8349_MITXGP is one of the 49 DTS files with the serial compatible:

compatible = "fsl,ns16550", "ns16550";

For the SAM440EP, the patch improves things from Linux-3.13. In that
release the same sort of problem as reported in:

"Linux-3.14-rc2: Order of serial node compatibles in DTS files."

occurs with slightly different symptoms:

of_serial ef600300.serial: Port found
of_serial ef600300.serial: Port found
of_serial ef600300.serial: Unknown serial port found, ignored
of_serial ef600400.serial: Port found
of_serial ef600400.serial: Port found
of_serial ef600400.serial: Unknown serial port found, ignored
of_serial ef600500.serial: Port found
of_serial ef600500.serial: Port found
of_serial ef600500.serial: Unknown serial port found, ignored
of_serial ef600600.serial: Port found
of_serial ef600600.serial: Port found
of_serial ef600600.serial: Unknown serial port found, ignored

The SAM440EP has a IBM/AMCC 440EP PowerPC CPU and so simply has
"ns16550" as its serial compatible.


> We could be a bit more strict here and fallback to the old matching if
> the match table has any entries with name or type. I don't think that
> should be necessary though.
> 
> Rob
Stephen Chivers,
CSC Australia Pty. Ltd.
> 
> > ---
> >  drivers/of/base.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 42 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index ba195fbce4c6..10b51106c854 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -730,13 +730,49 @@ out:
> >  }
> >  EXPORT_SYMBOL(of_find_node_with_property);
> >
> > +static const struct of_device_id *
> > +of_match_compatible(const struct of_device_id *matches,
> > +                       const struct device_node *node)
> > +{
> > +       const char *cp;
> > +       int cplen, l;
> > +       const struct of_device_id *m;
> > +
> > +       cp = __of_get_property(node, "compatible", &cplen);
> > +       while (cp && (cplen > 0)) {
> > +               m = matches;
> > +               while (m->name[0] || m->type[0] || m->compatible[0]) {
> > +                       /* Only match for the entries without type
> and name */
> > +                       if (m->name[0] || m->type[0] ||
> > +                               of_compat_cmp(m->compatible, cp,
> > +                                        strlen(m->compatible)))
> > +                               m++;
> > +                       else
> > +                               return m;
> > +               }
> > +
> > +               /* Get node's next compatible string */
> > +               l = strlen(cp) + 1;
> > +               cp += l;
> > +               cplen -= l;
> > +       }
> > +
> > +       return NULL;
> > +}
> > +
> >  static
> >  const struct of_device_id *__of_match_node(const struct 
> of_device_id *matches,
> >                                            const struct device_node 
*node)
> >  {
> > +       const struct of_device_id *m;
> > +
> >         if (!matches)
> >                 return NULL;
> >
> > +       m = of_match_compatible(matches, node);
> > +       if (m)
> > +               return m;
> > +
> >         while (matches->name[0] || matches->type[0] || 
> matches->compatible[0]) {
> >                 int match = 1;
> >                 if (matches->name[0])
> > @@ -760,7 +796,12 @@ 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. We have 
two ways
> > + *     of matching:
> > + *     - Try to find the best compatible match by comparing each 
compatible
> > + *       string of device node with all the given matches 
respectively.
> > + *     - If the above method failed, then try to match the 
> compatible by using
> > + *       __of_device_is_compatible() besides the match in type and 
name.
> >   */
> >  const struct of_device_id *of_match_node(const struct 
> of_device_id *matches,
> >                                          const struct device_node 
*node)
> > --
> > 1.8.5.3
> >

Re: [PATCH 2/2] of: search the best compatible match first in __of_match_node()

[Grant Likely]

On Fri, 14 Feb 2014 09:53:40 -0600, Rob Herring <robherring2@gmail.com> wrote:
> On Thu, Feb 13, 2014 at 11:22 PM, Kevin Hao <haokexin@gmail.com> 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 introduces a function to match each of the node's
> > compatible strings against all given compatible matches without type and
> > name 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. If we fail to find such a match
> > entry, then fall-back to the old method in order to keep compatibility.
> >
> > Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> > Signed-off-by: Kevin Hao <haokexin@gmail.com>
> 
> Looks good to me. I'll put this in next for a few days. I'd really
> like to see some acks and tested-by's before sending to Linus.

As I commented on the patch, I don't think the new solution is correct
either. I've made a suggestion on how to fix it, but in the mean time
the revert should be applied and sent to Linus.

g.

Re: [PATCH 2/2] of: search the best compatible match first in __of_match_node()

[Grant Likely]

On Fri, 14 Feb 2014 13:22:46 +0800, Kevin Hao <haokexin@gmail.com> 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 introduces a function to match each of the node's
> compatible strings against all given compatible matches without type and
> name 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. If we fail to find such a match
> entry, then fall-back to the old method in order to keep compatibility.
> 
> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> ---
>  drivers/of/base.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index ba195fbce4c6..10b51106c854 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -730,13 +730,49 @@ out:
>  }
>  EXPORT_SYMBOL(of_find_node_with_property);
>  
> +static const struct of_device_id *
> +of_match_compatible(const struct of_device_id *matches,
> +			const struct device_node *node)
> +{
> +	const char *cp;
> +	int cplen, l;
> +	const struct of_device_id *m;
> +
> +	cp = __of_get_property(node, "compatible", &cplen);
> +	while (cp && (cplen > 0)) {
> +		m = matches;
> +		while (m->name[0] || m->type[0] || m->compatible[0]) {
> +			/* Only match for the entries without type and name */
> +			if (m->name[0] || m->type[0] ||
> +				of_compat_cmp(m->compatible, cp,
> +					 strlen(m->compatible)))
> +				m++;

This seems wrong also. The compatible order should be checked for even
when m->name or m->type are set.  You actually need to score the entries
to do this properly. The pseudo-code should look like this:

uint best_score = ~0;
of_device_id *best_match = NULL;
for_each(matches) {
	uint score = ~0;
	for_each_compatible(index) {
		if (match->compatible == compatible[index])
			score = index * 10;
	}

	/* Matching name is a bit better than not */
	if (match->name == node->name)
		score--;

	/* Matching type is better than matching name */
	/* (but matching both is even better than that */
	if (match->type == node->type)
		score -= 2;

	if (score < best_score)
		best_match = match;
}
return best_match;

This is actually very similar to the original code. It is an easy
modification. This is very similar to how the of_fdt_is_compatible()
function works.

g.

Re: [PATCH 2/2] of: search the best compatible match first in __of_match_node()

[Kevin Hao]

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

On Mon, Feb 17, 2014 at 05:58:34PM +0000, Grant Likely wrote:
> This seems wrong also. The compatible order should be checked for even
> when m->name or m->type are set.  You actually need to score the entries
> to do this properly. The pseudo-code should look like this:
> 
> uint best_score = ~0;
> of_device_id *best_match = NULL;
> for_each(matches) {
> 	uint score = ~0;
> 	for_each_compatible(index) {
> 		if (match->compatible == compatible[index])
> 			score = index * 10;
> 	}
> 
> 	/* Matching name is a bit better than not */
> 	if (match->name == node->name)
> 		score--;
> 
> 	/* Matching type is better than matching name */
> 	/* (but matching both is even better than that */
> 	if (match->type == node->type)
> 		score -= 2;
> 
> 	if (score < best_score)
> 		best_match = match;
> }
> return best_match;
> 
> This is actually very similar to the original code. It is an easy
> modification. This is very similar to how the of_fdt_is_compatible()
> function works.

I like this idea and will make a new patch based on this.

Thanks,
Kevin
> 
> g.

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]