[PATCH] OF: base: match each node compatible against all given matches first

[PATCH] OF: base: match each node compatible against all given matches first

[Sebastian Hesselbarth]

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@gmail.com>
---
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: devicetree at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/of/base.c |   47 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 15 deletions(-)

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

[PATCH] OF: base: match each node compatible against all given matches first

[Thierry Reding]

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@gmail.com>
> ---
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: devicetree at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-kernel at vger.kernel.org
> ---
>  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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131202/d3a41a2a/attachment.sig>

[PATCH] OF: base: match each node compatible against all given matches first

[Rob Herring]

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 
>> <sebastian.hesselbarth@gmail.com> --- Cc: Grant Likely 
>> <grant.likely@linaro.org> Cc: Rob Herring 
>> <rob.herring@calxeda.com> Cc: Benjamin Herrenschmidt 
>> <benh@kernel.crashing.org> Cc: Russell King 
>> <linux@arm.linux.org.uk> Cc: devicetree at vger.kernel.org Cc: 
>> linux-arm-kernel at lists.infradead.org Cc: 
>> linux-kernel at vger.kernel.org --- 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.

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));


>> 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 at lists.infradead.org 
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] OF: base: match each node compatible against all given matches 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

[PATCH v2] OF: base: match each node compatible against all given matches first

[Sebastian Hesselbarth]

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@gmail.com>
---
Changelog:
v1->v2:
- Allow checks against nodes with no compatible (Reported by Rob Herring)
- Add some comments

Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Meelis Roos <mroos@linux.ee>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Scott Wood <scottwood@freescale.com>
Cc: devicetree at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/of/base.c |   53 +++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index f807d0e..8d007d8 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -731,24 +731,42 @@ 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);
+	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));
+
 	return NULL;
 }
 
@@ -757,7 +775,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.2.5

[PATCH v2] OF: base: match each node compatible against all given matches first

[Meelis Roos]

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

I think I am on the CC: list because of a CPU detection problem report 
on sparc64 (183912d352a242a276a7877852f107459a13aff9 (of: move 
of_get_cpu_node implementation to DT core library) caused trouble and 
was reverted). So while your V2 patch does not cause any visible harm on 
the same Sun E3500, my gut feeling is that an additional patch would be 
needed to actually test it (a patch like 
183912d352a242a276a7877852f107459a13aff9).

Is this correct or am I missing something?

> 
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Changelog:
> v1->v2:
> - Allow checks against nodes with no compatible (Reported by Rob Herring)
> - Add some comments
> 
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Meelis Roos <mroos@linux.ee>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: Scott Wood <scottwood@freescale.com>
> Cc: devicetree at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-kernel at vger.kernel.org
> ---
>  drivers/of/base.c |   53 +++++++++++++++++++++++++++++++++++++----------------
>  1 files changed, 37 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index f807d0e..8d007d8 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -731,24 +731,42 @@ 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);
> +	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));
> +
>  	return NULL;
>  }
>  
> @@ -757,7 +775,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)
> 

-- 
Meelis Roos (mroos at ut.ee)      http://www.cs.ut.ee/~mroos/

[PATCH v2] OF: base: match each node compatible against all given matches first

[Sebastian Hesselbarth]

On 12/03/2013 09:14 PM, Meelis Roos 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.
>
> I think I am on the CC: list because of a CPU detection problem report
> on sparc64 (183912d352a242a276a7877852f107459a13aff9 (of: move
> of_get_cpu_node implementation to DT core library) caused trouble and

The reason you are on Cc is that Thierry added you on last patch
version. I cannot see how above commit should be related with this
one, but maybe Thierry can comment on it.

> was reverted). So while your V2 patch does not cause any visible harm on
> the same Sun E3500, my gut feeling is that an additional patch would be
> needed to actually test it (a patch like
> 183912d352a242a276a7877852f107459a13aff9).

This patch deals with matching a node with more than one compatible
string on a (unordered) list of matches. Although not related to your
issue, it is good to hear that it causes no harm on DT-mature archs :)

I tested it with ARM and l2x0 cache controllers, where the specific
of_device_id (marvell,tauros3-cache) is sorted after the generic
one (arm,pl310-cache). The corresponding node's property is
compatible = "marvell,tauros3-cache", "arm,pl310-cache".

Without this patch, of_match_node always hits the first match that
equals _any_ of the above compatible strings. With this patch, it
hits the matches _in order_ of the compatible strings.

> Is this correct or am I missing something?

Thierry?

>> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> ---
>> Changelog:
>> v1->v2:
>> - Allow checks against nodes with no compatible (Reported by Rob Herring)
>> - Add some comments
>>
>> Cc: Grant Likely <grant.likely@linaro.org>
>> Cc: Rob Herring <rob.herring@calxeda.com>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Russell King <linux@arm.linux.org.uk>
>> Cc: Thierry Reding <thierry.reding@gmail.com>
>> Cc: Meelis Roos <mroos@linux.ee>
>> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
>> Cc: Scott Wood <scottwood@freescale.com>
>> Cc: devicetree at vger.kernel.org
>> Cc: linux-arm-kernel at lists.infradead.org
>> Cc: linux-kernel at vger.kernel.org
>> ---
>>   drivers/of/base.c |   53 +++++++++++++++++++++++++++++++++++++----------------
>>   1 files changed, 37 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index f807d0e..8d007d8 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -731,24 +731,42 @@ 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);
>> +	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));
>> +
>>   	return NULL;
>>   }
>>
>> @@ -757,7 +775,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)
>>
>

[PATCH v2] OF: base: match each node compatible against all given matches first

[Thierry Reding]

On Tue, Dec 03, 2013 at 11:55:07PM +0100, Sebastian Hesselbarth wrote:
> On 12/03/2013 09:14 PM, Meelis Roos 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.
> >
> >I think I am on the CC: list because of a CPU detection problem report
> >on sparc64 (183912d352a242a276a7877852f107459a13aff9 (of: move
> >of_get_cpu_node implementation to DT core library) caused trouble and
> 
> The reason you are on Cc is that Thierry added you on last patch
> version. I cannot see how above commit should be related with this
> one, but maybe Thierry can comment on it.
> 
> >was reverted). So while your V2 patch does not cause any visible harm on
> >the same Sun E3500, my gut feeling is that an additional patch would be
> >needed to actually test it (a patch like
> >183912d352a242a276a7877852f107459a13aff9).
> 
> This patch deals with matching a node with more than one compatible
> string on a (unordered) list of matches. Although not related to your
> issue, it is good to hear that it causes no harm on DT-mature archs :)
> 
> I tested it with ARM and l2x0 cache controllers, where the specific
> of_device_id (marvell,tauros3-cache) is sorted after the generic
> one (arm,pl310-cache). The corresponding node's property is
> compatible = "marvell,tauros3-cache", "arm,pl310-cache".
> 
> Without this patch, of_match_node always hits the first match that
> equals _any_ of the above compatible strings. With this patch, it
> hits the matches _in order_ of the compatible strings.
> 
> >Is this correct or am I missing something?
> 
> Thierry?

I added Meelis on Cc because he found a regression with my original
proposal (107a84e61cdd "of: match by compatible property first"). That
got later reverted in commit bc51b0c22ceb (Revert "of: match by
compatible property first"). Here's the commit message for reference:

commit bc51b0c22ceb
Author: Linus Torvalds <torvalds@linux-foundation.org>
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@linux.ee>
    Requested-by: Rob Herring <rob.herring@calxeda.com>
    Cc: Thierry Reding <thierry.reding@avionic-design.de>
    Cc: Grant Likely <grant.likely@secretlab.ca>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

So if Meelis still has access to the Sun Fire V100 and Sun Netra X1
machines that regressed last time around, it'd be great to get this
patch tested on them to verify that it indeed fixes the problem and
doesn't regress.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131204/0c5d7f94/attachment.sig>

[PATCH v2] OF: base: match each node compatible against all given matches first

[Meelis Roos]

> I added Meelis on Cc because he found a regression with my original
> proposal (107a84e61cdd "of: match by compatible property first"). That
> got later reverted in commit bc51b0c22ceb (Revert "of: match by
> compatible property first"). Here's the commit message for reference:
> 
> commit bc51b0c22ceb
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> 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@linux.ee>
>     Requested-by: Rob Herring <rob.herring@calxeda.com>
>     Cc: Thierry Reding <thierry.reding@avionic-design.de>
>     Cc: Grant Likely <grant.likely@secretlab.ca>
>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> So if Meelis still has access to the Sun Fire V100 and Sun Netra X1
> machines that regressed last time around, it'd be great to get this
> patch tested on them to verify that it indeed fixes the problem and
> doesn't regress.

OK, I had forgotten about that.

Tested successfully on the same Sun Fire V100 and Sun Netra X1, on top 
of 3.13-rc2. The sunsu console is detected fine and I see no problem for 
now.

-- 
Meelis Roos (mroos at linux.ee)

[PATCH v2] OF: base: match each node compatible against all given matches first

[Thierry Reding]

On Wed, Dec 04, 2013 at 03:08:39PM +0200, Meelis Roos wrote:
> > I added Meelis on Cc because he found a regression with my original
> > proposal (107a84e61cdd "of: match by compatible property first"). That
> > got later reverted in commit bc51b0c22ceb (Revert "of: match by
> > compatible property first"). Here's the commit message for reference:
> > 
> > commit bc51b0c22ceb
> > Author: Linus Torvalds <torvalds@linux-foundation.org>
> > 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@linux.ee>
> >     Requested-by: Rob Herring <rob.herring@calxeda.com>
> >     Cc: Thierry Reding <thierry.reding@avionic-design.de>
> >     Cc: Grant Likely <grant.likely@secretlab.ca>
> >     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > 
> > So if Meelis still has access to the Sun Fire V100 and Sun Netra X1
> > machines that regressed last time around, it'd be great to get this
> > patch tested on them to verify that it indeed fixes the problem and
> > doesn't regress.
> 
> OK, I had forgotten about that.

Yeah, I had always meant to come back to it, but that never quite
happened, so I'm all the more glad that Sebastian took over.

> Tested successfully on the same Sun Fire V100 and Sun Netra X1, on top 
> of 3.13-rc2. The sunsu console is detected fine and I see no problem for 
> now.

Excellent. Thanks for testing!

Rob, I guess this should be reasonable safe then to take for a spin in
linux-next?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131204/910f6bdf/attachment.sig>

[PATCH v2] OF: base: match each node compatible against all given matches first

[Rob Herring]

On Tue, Dec 3, 2013 at 7:52 AM, Sebastian Hesselbarth
<sebastian.hesselbarth@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 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@gmail.com>
> ---
> Changelog:
> v1->v2:
> - Allow checks against nodes with no compatible (Reported by Rob Herring)
> - Add some comments
>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Meelis Roos <mroos@linux.ee>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: Scott Wood <scottwood@freescale.com>
> Cc: devicetree at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-kernel at vger.kernel.org
> ---
>  drivers/of/base.c |   53 +++++++++++++++++++++++++++++++++++++----------------
>  1 files changed, 37 insertions(+), 16 deletions(-)

Applied.

Rob