[PATCH v4 0/4] Bugfix for of_match_node ordering

[PATCH v4 0/4] Bugfix for of_match_node ordering

[Grant Likely]

Hi all,

I've taken Kevin's latest rework and done even more rework on it. :-) I
didn't quite like how it was looking so I rolled his scoring code
directly into __of_device_is_compatible() so that the function always
returns a score in a way that is still compatible with the existing
users (ie. non-zero == successful match). This eliminates the need for a
separate pscore argument and it also let me roll the type and name
checks into the same function. I'm a lot happier with it overall and it
makes for a slightly smaller number of lines of code changed. Please
take a look and give it a spin. This is basically a bug fix so I'll need
to push it out to Linus in the near future.

Acks and Tested-bys would be particularly appreciated.

Thanks,
g.

Kevin Hao (2):
	Revert "of: search the best compatible match first in __of_match_node()"
	of: reimplement the matching method for __of_match_node()

Grant Likely (2):
	of: Move testcase FDT data into drivers/of
	of: Add self test for of_match_node()

[PATCH v4 1/4] Revert "of: search the best compatible match first in __of_match_node()"

[Grant Likely]

From: Kevin Hao <haokexin@gmail.com>

This reverts commit 06b29e76a74b2373e6f8b5a7938b3630b9ae98b2.
As pointed out by Grant Likely, we should also take the type and name
into account when searching the best compatible match. That means the
match with compatible, type and name should be better than the match
just with the same compatible string. So revert this and we will
implement another method to find the best match entry.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
Signed-off-by: Grant Likely <grant.likely@linaro.org>
---
 drivers/of/base.c | 43 +------------------------------------------
 1 file changed, 1 insertion(+), 42 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 10b51106c854..ba195fbce4c6 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -730,49 +730,13 @@ 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])
@@ -796,12 +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. 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.
+ *	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.3.2

[PATCH v4 2/4] of: reimplement the matching method for __of_match_node()

[Grant Likely]

From: Kevin Hao <haokexin@gmail.com>

In the current implementation of __of_match_node(), it will compare
each given match entry against all the 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, we define a following priority order for the match, and
then scan all the entries to find the best match.
  1. specific compatible && type && name
  2. specific compatible && type
  3. specific compatible && name
  4. specific compatible
  5. general compatible && type && name
  6. general compatible && type
  7. general compatible && name
  8. general compatible
  9. type && name
  10. type
  11. name

v4: Short-circuit failure cases instead of mucking with score, and
    remove extra __of_device_is_compatible() wrapper stub.
    Move scoring logic directly into __of_device_is_compatible()

v3: Also need to bail out when there does have a compatible member in match
    entry, but it doesn't match with the device node's compatible.

v2: Fix the bug such as we get the same score for the following two match
    entries with the empty node 'name2 { };'
	struct of_device_id matches[] = {
		{.name = "name2", },
		{.name = "name2", .type = "type1", },
		{}
	};

Signed-off-by: Kevin Hao <haokexin@gmail.com>
[grant.likely: added v4 changes]
Signed-off-by: Grant Likely <grant.likely@linaro.org>
---
 drivers/of/base.c | 108 +++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 74 insertions(+), 34 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index ba195fbce4c6..30ffc291d0a0 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -342,27 +342,73 @@ struct device_node *of_get_cpu_node(int cpu, unsigned int *thread)
 }
 EXPORT_SYMBOL(of_get_cpu_node);
 
-/** Checks if the given "compat" string matches one of the strings in
- * the device's "compatible" property
+/**
+ * __of_device_is_compatible() - Check if the node matches given constraints
+ * @device: pointer to node
+ * @compat: required compatible string, NULL or "" for any match
+ * @type: required device_type value, NULL or "" for any match
+ * @name: required node name, NULL or "" for any match
+ *
+ * Checks if the given @compat, @type and @name strings match the
+ * properties of the given @device. A constraints can be skipped by
+ * passing NULL or an empty string as the constraint.
+ *
+ * Returns 0 for no match, and a positive integer on match. The return
+ * value is a relative score with larger values indicating better
+ * matches. The score is weighted for the most specific compatible value
+ * to get the highest score. Matching type is next, followed by matching
+ * name. Practically speaking, this results in the following priority
+ * order for matches:
+ *
+ * 1. specific compatible && type && name
+ * 2. specific compatible && type
+ * 3. specific compatible && name
+ * 4. specific compatible
+ * 5. general compatible && type && name
+ * 6. general compatible && type
+ * 7. general compatible && name
+ * 8. general compatible
+ * 9. type && name
+ * 10. type
+ * 11. name
  */
 static int __of_device_is_compatible(const struct device_node *device,
-				     const char *compat)
+				     const char *compat, const char *type, const char *name)
 {
 	const char* cp;
-	int cplen, l;
+	int cplen, l, index, score = 0;
 
-	cp = __of_get_property(device, "compatible", &cplen);
-	if (cp == NULL)
-		return 0;
-	while (cplen > 0) {
-		if (of_compat_cmp(cp, compat, strlen(compat)) == 0)
-			return 1;
-		l = strlen(cp) + 1;
-		cp += l;
-		cplen -= l;
+	/* Compatible match has highest priority */
+	if (compat && compat[0]) {
+		cp = __of_get_property(device, "compatible", &cplen);
+		if (!cp)
+			return 0;
+		for (index = 0; cplen > 0; index++, cp += l, cplen -= l) {
+			l = strlen(cp) + 1;
+			if (of_compat_cmp(cp, compat, strlen(compat)) == 0) {
+				score = INT_MAX/2 - (index << 2);
+				break;
+			}
+		}
+		if (!score)
+			return 0;
 	}
 
-	return 0;
+	/* Matching type is better than matching name */
+	if (type && type[0]) {
+		if (!device->type || of_node_cmp(type, device->type))
+			return 0;
+		score += 2;
+	}
+
+	/* Matching name is a bit better than not */
+	if (name && name[0]) {
+		if (!device->name || of_node_cmp(name, device->name))
+			return 0;
+		score++;
+	}
+
+	return score;
 }
 
 /** Checks if the given "compat" string matches one of the strings in
@@ -375,7 +421,7 @@ int of_device_is_compatible(const struct device_node *device,
 	int res;
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
-	res = __of_device_is_compatible(device, compat);
+	res = __of_device_is_compatible(device, compat, NULL, NULL);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return res;
 }
@@ -681,10 +727,7 @@ struct device_node *of_find_compatible_node(struct device_node *from,
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	np = from ? from->allnext : of_allnodes;
 	for (; np; np = np->allnext) {
-		if (type
-		    && !(np->type && (of_node_cmp(np->type, type) == 0)))
-			continue;
-		if (__of_device_is_compatible(np, compatible) &&
+		if (__of_device_is_compatible(np, compatible, type, NULL) &&
 		    of_node_get(np))
 			break;
 	}
@@ -734,25 +777,22 @@ 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 *best_match = NULL;
+	int score, best_score = 0;
+
 	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++;
+	for (; matches->name[0] || matches->type[0] || matches->compatible[0]; matches++) {
+		score = __of_device_is_compatible(node, matches->compatible,
+						  matches->type, matches->name);
+		if (score > best_score) {
+			best_match = matches;
+			best_score = score;
+		}
 	}
-	return NULL;
+
+	return best_match;
 }
 
 /**
-- 
1.8.3.2

Re: [PATCH v4 2/4] of: reimplement the matching method for __of_match_node()

[Rob Herring]

On Wed, Feb 19, 2014 at 8:14 AM, Grant Likely <grant.likely@linaro.org> wrote:
> From: Kevin Hao <haokexin@gmail.com>
>
> In the current implementation of __of_match_node(), it will compare
> each given match entry against all the 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, we define a following priority order for the match, and
> then scan all the entries to find the best match.
>   1. specific compatible && type && name
>   2. specific compatible && type
>   3. specific compatible && name
>   4. specific compatible
>   5. general compatible && type && name
>   6. general compatible && type
>   7. general compatible && name
>   8. general compatible
>   9. type && name
>   10. type
>   11. name

While I agree this should be the right order, I worry that this may be
changing the matching in some case as all previous attempts have done.

>
> v4: Short-circuit failure cases instead of mucking with score, and
>     remove extra __of_device_is_compatible() wrapper stub.
>     Move scoring logic directly into __of_device_is_compatible()
>
> v3: Also need to bail out when there does have a compatible member in match
>     entry, but it doesn't match with the device node's compatible.
>
> v2: Fix the bug such as we get the same score for the following two match
>     entries with the empty node 'name2 { };'
>         struct of_device_id matches[] = {
>                 {.name = "name2", },
>                 {.name = "name2", .type = "type1", },
>                 {}
>         };
>
> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> [grant.likely: added v4 changes]
> Signed-off-by: Grant Likely <grant.likely@linaro.org>
> ---
>  drivers/of/base.c | 108 +++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 74 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index ba195fbce4c6..30ffc291d0a0 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -342,27 +342,73 @@ struct device_node *of_get_cpu_node(int cpu, unsigned int *thread)
>  }
>  EXPORT_SYMBOL(of_get_cpu_node);
>
> -/** Checks if the given "compat" string matches one of the strings in
> - * the device's "compatible" property
> +/**
> + * __of_device_is_compatible() - Check if the node matches given constraints
> + * @device: pointer to node
> + * @compat: required compatible string, NULL or "" for any match
> + * @type: required device_type value, NULL or "" for any match
> + * @name: required node name, NULL or "" for any match
> + *
> + * Checks if the given @compat, @type and @name strings match the
> + * properties of the given @device. A constraints can be skipped by
> + * passing NULL or an empty string as the constraint.
> + *
> + * Returns 0 for no match, and a positive integer on match. The return
> + * value is a relative score with larger values indicating better
> + * matches. The score is weighted for the most specific compatible value
> + * to get the highest score. Matching type is next, followed by matching
> + * name. Practically speaking, this results in the following priority
> + * order for matches:
> + *
> + * 1. specific compatible && type && name
> + * 2. specific compatible && type
> + * 3. specific compatible && name
> + * 4. specific compatible
> + * 5. general compatible && type && name
> + * 6. general compatible && type
> + * 7. general compatible && name
> + * 8. general compatible
> + * 9. type && name
> + * 10. type
> + * 11. name
>   */
>  static int __of_device_is_compatible(const struct device_node *device,
> -                                    const char *compat)
> +                                    const char *compat, const char *type, const char *name)
>  {
>         const char* cp;
> -       int cplen, l;
> +       int cplen, l, index, score = 0;
>
> -       cp = __of_get_property(device, "compatible", &cplen);
> -       if (cp == NULL)
> -               return 0;
> -       while (cplen > 0) {
> -               if (of_compat_cmp(cp, compat, strlen(compat)) == 0)
> -                       return 1;
> -               l = strlen(cp) + 1;
> -               cp += l;
> -               cplen -= l;
> +       /* Compatible match has highest priority */
> +       if (compat && compat[0]) {
> +               cp = __of_get_property(device, "compatible", &cplen);
> +               if (!cp)
> +                       return 0;
> +               for (index = 0; cplen > 0; index++, cp += l, cplen -= l) {
> +                       l = strlen(cp) + 1;

This could use of_property_for_each_string.

> +                       if (of_compat_cmp(cp, compat, strlen(compat)) == 0) {
> +                               score = INT_MAX/2 - (index << 2);
> +                               break;
> +                       }
> +               }
> +               if (!score)
> +                       return 0;
>         }
>
> -       return 0;
> +       /* Matching type is better than matching name */
> +       if (type && type[0]) {
> +               if (!device->type || of_node_cmp(type, device->type))
> +                       return 0;
> +               score += 2;
> +       }
> +
> +       /* Matching name is a bit better than not */
> +       if (name && name[0]) {
> +               if (!device->name || of_node_cmp(name, device->name))
> +                       return 0;
> +               score++;
> +       }
> +
> +       return score;
>  }
>
>  /** Checks if the given "compat" string matches one of the strings in
> @@ -375,7 +421,7 @@ int of_device_is_compatible(const struct device_node *device,
>         int res;
>
>         raw_spin_lock_irqsave(&devtree_lock, flags);
> -       res = __of_device_is_compatible(device, compat);
> +       res = __of_device_is_compatible(device, compat, NULL, NULL);
>         raw_spin_unlock_irqrestore(&devtree_lock, flags);
>         return res;
>  }
> @@ -681,10 +727,7 @@ struct device_node *of_find_compatible_node(struct device_node *from,
>         raw_spin_lock_irqsave(&devtree_lock, flags);
>         np = from ? from->allnext : of_allnodes;
>         for (; np; np = np->allnext) {
> -               if (type
> -                   && !(np->type && (of_node_cmp(np->type, type) == 0)))
> -                       continue;
> -               if (__of_device_is_compatible(np, compatible) &&
> +               if (__of_device_is_compatible(np, compatible, type, NULL) &&
>                     of_node_get(np))
>                         break;
>         }
> @@ -734,25 +777,22 @@ 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 *best_match = NULL;
> +       int score, best_score = 0;
> +
>         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++;
> +       for (; matches->name[0] || matches->type[0] || matches->compatible[0]; matches++) {
> +               score = __of_device_is_compatible(node, matches->compatible,
> +                                                 matches->type, matches->name);
> +               if (score > best_score) {
> +                       best_match = matches;
> +                       best_score = score;
> +               }
>         }
> -       return NULL;
> +
> +       return best_match;
>  }
>
>  /**
> --
> 1.8.3.2
>

Re: [PATCH v4 2/4] of: reimplement the matching method for __of_match_node()

[Grant Likely]

On Wed, 19 Feb 2014 08:54:57 -0600, Rob Herring <robherring2@gmail.com> wrote:
> On Wed, Feb 19, 2014 at 8:14 AM, Grant Likely <grant.likely@linaro.org> wrote:
> > From: Kevin Hao <haokexin@gmail.com>
> >
> > In the current implementation of __of_match_node(), it will compare
> > each given match entry against all the 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, we define a following priority order for the match, and
> > then scan all the entries to find the best match.
> >   1. specific compatible && type && name
> >   2. specific compatible && type
> >   3. specific compatible && name
> >   4. specific compatible
> >   5. general compatible && type && name
> >   6. general compatible && type
> >   7. general compatible && name
> >   8. general compatible
> >   9. type && name
> >   10. type
> >   11. name
> 
> While I agree this should be the right order, I worry that this may be
> changing the matching in some case as all previous attempts have done.

True, but they should be few and far between, and they can be
fixed up with bugfixes in drivers. This time we actually have test cases
that show the behaviour is correct. The previous versions failed the
current test cases (I checked).

> > +       /* Compatible match has highest priority */
> > +       if (compat && compat[0]) {
> > +               cp = __of_get_property(device, "compatible", &cplen);
> > +               if (!cp)
> > +                       return 0;
> > +               for (index = 0; cplen > 0; index++, cp += l, cplen -= l) {
> > +                       l = strlen(cp) + 1;
> 
> This could use of_property_for_each_string.

Good point. Changed.

g.

[PATCH v4 3/4] of: Move testcase FDT data into drivers/of

[Grant Likely]

The testcase data is usable by any platform. This patch moves it into
the drivers/of directory so it can be included by any architecture.

Using the test cases requires manually adding #include <testcases.dtsi>
to the end of the boards .dtsi file and enabling CONFIG_OF_SELFTEST. Not
pretty though. A useful project would be to make the testcase code
easier to execute.

Signed-off-by: Grant Likely <grant.likely@linaro.org>
---
 arch/arm/boot/dts/testcases/tests-interrupts.dtsi | 58 -----------------------
 arch/arm/boot/dts/testcases/tests-phandle.dtsi    | 39 ---------------
 arch/arm/boot/dts/testcases/tests.dtsi            |  2 -
 arch/arm/boot/dts/versatile-pb.dts                |  4 +-
 drivers/of/testcase-data/testcases.dtsi           |  2 +
 drivers/of/testcase-data/tests-interrupts.dtsi    | 58 +++++++++++++++++++++++
 drivers/of/testcase-data/tests-phandle.dtsi       | 39 +++++++++++++++
 scripts/Makefile.lib                              |  1 +
 8 files changed, 102 insertions(+), 101 deletions(-)
 delete mode 100644 arch/arm/boot/dts/testcases/tests-interrupts.dtsi
 delete mode 100644 arch/arm/boot/dts/testcases/tests-phandle.dtsi
 delete mode 100644 arch/arm/boot/dts/testcases/tests.dtsi
 create mode 100644 drivers/of/testcase-data/testcases.dtsi
 create mode 100644 drivers/of/testcase-data/tests-interrupts.dtsi
 create mode 100644 drivers/of/testcase-data/tests-phandle.dtsi

diff --git a/arch/arm/boot/dts/testcases/tests-interrupts.dtsi b/arch/arm/boot/dts/testcases/tests-interrupts.dtsi
deleted file mode 100644
index c843720bd3e5..000000000000
--- a/arch/arm/boot/dts/testcases/tests-interrupts.dtsi
+++ /dev/null
@@ -1,58 +0,0 @@
-
-/ {
-	testcase-data {
-		interrupts {
-			#address-cells = <1>;
-			#size-cells = <1>;
-			test_intc0: intc0 {
-				interrupt-controller;
-				#interrupt-cells = <1>;
-			};
-
-			test_intc1: intc1 {
-				interrupt-controller;
-				#interrupt-cells = <3>;
-			};
-
-			test_intc2: intc2 {
-				interrupt-controller;
-				#interrupt-cells = <2>;
-			};
-
-			test_intmap0: intmap0 {
-				#interrupt-cells = <1>;
-				#address-cells = <0>;
-				interrupt-map = <1 &test_intc0 9>,
-						<2 &test_intc1 10 11 12>,
-						<3 &test_intc2 13 14>,
-						<4 &test_intc2 15 16>;
-			};
-
-			test_intmap1: intmap1 {
-				#interrupt-cells = <2>;
-				interrupt-map = <0x5000 1 2 &test_intc0 15>;
-			};
-
-			interrupts0 {
-				interrupt-parent = <&test_intc0>;
-				interrupts = <1>, <2>, <3>, <4>;
-			};
-
-			interrupts1 {
-				interrupt-parent = <&test_intmap0>;
-				interrupts = <1>, <2>, <3>, <4>;
-			};
-
-			interrupts-extended0 {
-				reg = <0x5000 0x100>;
-				interrupts-extended = <&test_intc0 1>,
-						      <&test_intc1 2 3 4>,
-						      <&test_intc2 5 6>,
-						      <&test_intmap0 1>,
-						      <&test_intmap0 2>,
-						      <&test_intmap0 3>,
-						      <&test_intmap1 1 2>;
-			};
-		};
-	};
-};
diff --git a/arch/arm/boot/dts/testcases/tests-phandle.dtsi b/arch/arm/boot/dts/testcases/tests-phandle.dtsi
deleted file mode 100644
index 0007d3cd7dc2..000000000000
--- a/arch/arm/boot/dts/testcases/tests-phandle.dtsi
+++ /dev/null
@@ -1,39 +0,0 @@
-
-/ {
-	testcase-data {
-		phandle-tests {
-			provider0: provider0 {
-				#phandle-cells = <0>;
-			};
-
-			provider1: provider1 {
-				#phandle-cells = <1>;
-			};
-
-			provider2: provider2 {
-				#phandle-cells = <2>;
-			};
-
-			provider3: provider3 {
-				#phandle-cells = <3>;
-			};
-
-			consumer-a {
-				phandle-list =	<&provider1 1>,
-						<&provider2 2 0>,
-						<0>,
-						<&provider3 4 4 3>,
-						<&provider2 5 100>,
-						<&provider0>,
-						<&provider1 7>;
-				phandle-list-names = "first", "second", "third";
-
-				phandle-list-bad-phandle = <12345678 0 0>;
-				phandle-list-bad-args = <&provider2 1 0>,
-							<&provider3 0>;
-				empty-property;
-				unterminated-string = [40 41 42 43];
-			};
-		};
-	};
-};
diff --git a/arch/arm/boot/dts/testcases/tests.dtsi b/arch/arm/boot/dts/testcases/tests.dtsi
deleted file mode 100644
index 3f123ecc9dd7..000000000000
--- a/arch/arm/boot/dts/testcases/tests.dtsi
+++ /dev/null
@@ -1,2 +0,0 @@
-/include/ "tests-phandle.dtsi"
-/include/ "tests-interrupts.dtsi"
diff --git a/arch/arm/boot/dts/versatile-pb.dts b/arch/arm/boot/dts/versatile-pb.dts
index f43907c40c93..65f657711323 100644
--- a/arch/arm/boot/dts/versatile-pb.dts
+++ b/arch/arm/boot/dts/versatile-pb.dts
@@ -1,4 +1,4 @@
-/include/ "versatile-ab.dts"
+#include <versatile-ab.dts>
 
 / {
 	model = "ARM Versatile PB";
@@ -47,4 +47,4 @@
 	};
 };
 
-/include/ "testcases/tests.dtsi"
+#include <testcases.dtsi>
diff --git a/drivers/of/testcase-data/testcases.dtsi b/drivers/of/testcase-data/testcases.dtsi
new file mode 100644
index 000000000000..3cc2f55534ac
--- /dev/null
+++ b/drivers/of/testcase-data/testcases.dtsi
@@ -0,0 +1,2 @@
+#include "tests-phandle.dtsi"
+#include "tests-interrupts.dtsi"
diff --git a/drivers/of/testcase-data/tests-interrupts.dtsi b/drivers/of/testcase-data/tests-interrupts.dtsi
new file mode 100644
index 000000000000..c843720bd3e5
--- /dev/null
+++ b/drivers/of/testcase-data/tests-interrupts.dtsi
@@ -0,0 +1,58 @@
+
+/ {
+	testcase-data {
+		interrupts {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			test_intc0: intc0 {
+				interrupt-controller;
+				#interrupt-cells = <1>;
+			};
+
+			test_intc1: intc1 {
+				interrupt-controller;
+				#interrupt-cells = <3>;
+			};
+
+			test_intc2: intc2 {
+				interrupt-controller;
+				#interrupt-cells = <2>;
+			};
+
+			test_intmap0: intmap0 {
+				#interrupt-cells = <1>;
+				#address-cells = <0>;
+				interrupt-map = <1 &test_intc0 9>,
+						<2 &test_intc1 10 11 12>,
+						<3 &test_intc2 13 14>,
+						<4 &test_intc2 15 16>;
+			};
+
+			test_intmap1: intmap1 {
+				#interrupt-cells = <2>;
+				interrupt-map = <0x5000 1 2 &test_intc0 15>;
+			};
+
+			interrupts0 {
+				interrupt-parent = <&test_intc0>;
+				interrupts = <1>, <2>, <3>, <4>;
+			};
+
+			interrupts1 {
+				interrupt-parent = <&test_intmap0>;
+				interrupts = <1>, <2>, <3>, <4>;
+			};
+
+			interrupts-extended0 {
+				reg = <0x5000 0x100>;
+				interrupts-extended = <&test_intc0 1>,
+						      <&test_intc1 2 3 4>,
+						      <&test_intc2 5 6>,
+						      <&test_intmap0 1>,
+						      <&test_intmap0 2>,
+						      <&test_intmap0 3>,
+						      <&test_intmap1 1 2>;
+			};
+		};
+	};
+};
diff --git a/drivers/of/testcase-data/tests-phandle.dtsi b/drivers/of/testcase-data/tests-phandle.dtsi
new file mode 100644
index 000000000000..0007d3cd7dc2
--- /dev/null
+++ b/drivers/of/testcase-data/tests-phandle.dtsi
@@ -0,0 +1,39 @@
+
+/ {
+	testcase-data {
+		phandle-tests {
+			provider0: provider0 {
+				#phandle-cells = <0>;
+			};
+
+			provider1: provider1 {
+				#phandle-cells = <1>;
+			};
+
+			provider2: provider2 {
+				#phandle-cells = <2>;
+			};
+
+			provider3: provider3 {
+				#phandle-cells = <3>;
+			};
+
+			consumer-a {
+				phandle-list =	<&provider1 1>,
+						<&provider2 2 0>,
+						<0>,
+						<&provider3 4 4 3>,
+						<&provider2 5 100>,
+						<&provider0>,
+						<&provider1 7>;
+				phandle-list-names = "first", "second", "third";
+
+				phandle-list-bad-phandle = <12345678 0 0>;
+				phandle-list-bad-args = <&provider2 1 0>,
+							<&provider3 0>;
+				empty-property;
+				unterminated-string = [40 41 42 43];
+			};
+		};
+	};
+};
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 49392ecbef17..79c059e70860 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -152,6 +152,7 @@ ld_flags       = $(LDFLAGS) $(ldflags-y)
 dtc_cpp_flags  = -Wp,-MD,$(depfile).pre.tmp -nostdinc                    \
 		 -I$(srctree)/arch/$(SRCARCH)/boot/dts                   \
 		 -I$(srctree)/arch/$(SRCARCH)/boot/dts/include           \
+		 -I$(srctree)/drivers/of/testcase-data                   \
 		 -undef -D__DTS__
 
 # Finds the multi-part object the current object will be linked into
-- 
1.8.3.2

[PATCH v4 4/4] of: Add self test for of_match_node()

[Grant Likely]

Adds a selftest function for the of_match_node function. of_match_node
is supposed to handle precedence for the compatible property as well as
the name and device_type values. This patch adds some test case data and
a function that makes sure each test node matches against the correct
entry of an of_device_id table.

This code was written to verify the new of_match_node() implementation
that is an earlier part of this series.

Signed-off-by: Grant Likely <grant.likely@linaro.org>
Cc: Kevin Hao <haokexin@gmail.com>
---
 drivers/of/selftest.c                     | 67 +++++++++++++++++++++++++++++++
 drivers/of/testcase-data/testcases.dtsi   |  1 +
 drivers/of/testcase-data/tests-match.dtsi | 19 +++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 drivers/of/testcase-data/tests-match.dtsi

diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
index e21012bde639..6643d1920985 100644
--- a/drivers/of/selftest.c
+++ b/drivers/of/selftest.c
@@ -300,6 +300,72 @@ static void __init of_selftest_parse_interrupts_extended(void)
 	of_node_put(np);
 }
 
+static struct of_device_id match_node_table[] = {
+	{ .data = "A", .name = "name0", }, /* Name alone is lowest priority */
+	{ .data = "B", .type = "type1", }, /* followed by type alone */
+
+	{ .data = "Ca", .name = "name2", .type = "type1", }, /* followed by both together */
+	{ .data = "Cb", .name = "name2", }, /* Only match when type doesn't match */
+	{ .data = "Cc", .name = "name2", .type = "type2", },
+
+	{ .data = "E", .compatible = "compat3" },
+	{ .data = "G", .compatible = "compat2", },
+	{ .data = "H", .compatible = "compat2", .name = "name5", },
+	{ .data = "I", .compatible = "compat2", .type = "type1", },
+	{ .data = "J", .compatible = "compat2", .type = "type1", .name = "name8", },
+	{ .data = "K", .compatible = "compat2", .name = "name9", },
+	{}
+};
+
+static struct {
+	const char *path;
+	const char *data;
+} match_node_tests[] = {
+	{ .path = "/testcase-data/match-node/name0", .data = "A", },
+	{ .path = "/testcase-data/match-node/name1", .data = "B", },
+	{ .path = "/testcase-data/match-node/a/name2", .data = "Ca", },
+	{ .path = "/testcase-data/match-node/b/name2", .data = "Cb", },
+	{ .path = "/testcase-data/match-node/c/name2", .data = "Cc", },
+	{ .path = "/testcase-data/match-node/name3", .data = "E", },
+	{ .path = "/testcase-data/match-node/name4", .data = "G", },
+	{ .path = "/testcase-data/match-node/name5", .data = "H", },
+	{ .path = "/testcase-data/match-node/name6", .data = "G", },
+	{ .path = "/testcase-data/match-node/name7", .data = "I", },
+	{ .path = "/testcase-data/match-node/name8", .data = "J", },
+	{ .path = "/testcase-data/match-node/name9", .data = "K", },
+};
+
+static void __init of_selftest_match_node(void)
+{
+	struct device_node *np;
+	const struct of_device_id *match;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(match_node_tests); i++) {
+		np = of_find_node_by_path(match_node_tests[i].path);
+		if (!np) {
+			selftest(0, "missing testcase node %s\n",
+				match_node_tests[i].path);
+			continue;
+		}
+
+		match = of_match_node(match_node_table, np);
+		if (!match) {
+			selftest(0, "%s didn't match anything\n",
+				match_node_tests[i].path);
+			continue;
+		}
+
+		if (strcmp(match->data, match_node_tests[i].data) != 0) {
+			selftest(0, "%s got wrong match. expected %s, got %s\n",
+				match_node_tests[i].path, match_node_tests[i].data,
+				(const char *)match->data);
+			continue;
+		}
+		selftest(1, "passed");
+	}
+}
+
 static int __init of_selftest(void)
 {
 	struct device_node *np;
@@ -316,6 +382,7 @@ static int __init of_selftest(void)
 	of_selftest_property_match_string();
 	of_selftest_parse_interrupts();
 	of_selftest_parse_interrupts_extended();
+	of_selftest_match_node();
 	pr_info("end of selftest - %i passed, %i failed\n",
 		selftest_results.passed, selftest_results.failed);
 	return 0;
diff --git a/drivers/of/testcase-data/testcases.dtsi b/drivers/of/testcase-data/testcases.dtsi
index 3cc2f55534ac..3a5b75a8e4d7 100644
--- a/drivers/of/testcase-data/testcases.dtsi
+++ b/drivers/of/testcase-data/testcases.dtsi
@@ -1,2 +1,3 @@
 #include "tests-phandle.dtsi"
 #include "tests-interrupts.dtsi"
+#include "tests-match.dtsi"
diff --git a/drivers/of/testcase-data/tests-match.dtsi b/drivers/of/testcase-data/tests-match.dtsi
new file mode 100644
index 000000000000..c9e541129534
--- /dev/null
+++ b/drivers/of/testcase-data/tests-match.dtsi
@@ -0,0 +1,19 @@
+
+/ {
+	testcase-data {
+		match-node {
+			name0 { };
+			name1 { device_type = "type1"; };
+			a { name2 { device_type = "type1"; }; };
+			b { name2 { }; };
+			c { name2 { device_type = "type2"; }; };
+			name3 { compatible = "compat3"; };
+			name4 { compatible = "compat2", "compat3"; };
+			name5 { compatible = "compat2", "compat3"; };
+			name6 { compatible = "compat1", "compat2", "compat3"; };
+			name7 { compatible = "compat2"; device_type = "type1"; };
+			name8 { compatible = "compat2"; device_type = "type1"; };
+			name9 { compatible = "compat2"; };
+		};
+	};
+};
-- 
1.8.3.2

Re: [PATCH v4 0/4] Bugfix for of_match_node ordering

[Sachin Kamat]

Hi Grant,

I observe the following boot failure with today's (next-20140220) linux-next
tree on Exynos based boards with the default exynos_defconfig.

Uncompressing Linux... done, booting the kernel.
[    0.000000] Booting Linux on physical CPU 0x900
[    0.000000] Linux version 3.14.0-rc3-next-20140220 (sachin@linaro)
(gcc version 4.8.2 20130805 (prerelease) (crosstool-NG linaro-1.
13.1-4.8-2013.08 - Linaro GCC 2013.08) ) #1132 SMP PREEMPT Thu Feb 20
13:49:27 IST 2014
[    0.000000] CPU: ARMv7 Processor [412fc091] revision 1 (ARMv7), cr=10c5387d
[    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
instruction cache
[    0.000000] Machine model: Insignal Origen evaluation board based
on Exynos4210
[    0.000000] bootconsole [earlycon0] enabled
[    0.000000] Memory policy: Data cache writealloc
[    0.000000] CPU EXYNOS4210 (id 0x43210011)
[    0.000000] On node 0 totalpages: 258048
[    0.000000]   Normal zone: 1520 pages used for memmap
[    0.000000]   Normal zone: 0 pages reserved
[    0.000000]   Normal zone: 190464 pages, LIFO batch:31
[    0.000000]   HighMem zone: 528 pages used for memmap
[    0.000000]   HighMem zone: 67584 pages, LIFO batch:15
[    0.000000] BUG: spinlock recursion on CPU#0, swapper/0
[    0.000000]  lock: devtree_lock+0x0/0x10, .magic: dead4ead, .owner:
swapper/0, .owner_cpu: 0
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted
3.14.0-rc3-next-20140220 #1132
[    0.000000] [<c0013e9c>] (unwind_backtrace) from [<c0011250>]
(show_stack+0x10/0x14)
[    0.000000] [<c0011250>] (show_stack) from [<c0386740>]
(dump_stack+0x7c/0xbc)
[    0.000000] [<c0386740>] (dump_stack) from [<c005501c>]
(do_raw_spin_lock+0x188/0x18c)
[    0.000000] [<c005501c>] (do_raw_spin_lock) from [<c038b614>]
(_raw_spin_lock_irqsave+0x20/0x28)
[    0.000000] [<c038b614>] (_raw_spin_lock_irqsave) from [<c02de37c>]
(of_find_property+0x20/0x4c)
[    0.000000] [<c02de37c>] (of_find_property) from [<c02df110>]
(__of_device_is_compatible+0xb4/0x110)
[    0.000000] [<c02df110>] (__of_device_is_compatible) from
[<c02df22c>] (of_find_compatible_node+0x4c/0x7c)
[    0.000000] [<c02df22c>] (of_find_compatible_node) from
[<c04cedf4>] (exynos_firmware_init+0x18/0x7c)
[    0.000000] [<c04cedf4>] (exynos_firmware_init) from [<c04caef0>]
(setup_arch+0x860/0x898)
[    0.000000] [<c04caef0>] (setup_arch) from [<c04c7820>]
(start_kernel+0x80/0x3dc)
[    0.000000] [<c04c7820>] (start_kernel) from [<40008074>] (0x40008074)
[    0.000000] BUG: spinlock lockup suspected on CPU#0, swapper/0
[    0.000000]  lock: devtree_lock+0x0/0x10, .magic: dead4ead, .owner:
swapper/0, .owner_cpu: 0
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted
3.14.0-rc3-next-20140220 #1132
[    0.000000] [<c0013e9c>] (unwind_backtrace) from [<c0011250>]
(show_stack+0x10/0x14)
[    0.000000] [<c0011250>] (show_stack) from [<c0386740>]
(dump_stack+0x7c/0xbc)
[    0.000000] [<c0386740>] (dump_stack) from [<c0054fac>]
(do_raw_spin_lock+0x118/0x18c)
[    0.000000] [<c0054fac>] (do_raw_spin_lock) from [<c038b614>]
(_raw_spin_lock_irqsave+0x20/0x28)
[    0.000000] [<c038b614>] (_raw_spin_lock_irqsave) from [<c02de37c>]
(of_find_property+0x20/0x4c)
[    0.000000] [<c02de37c>] (of_find_property) from [<c02df110>]
(__of_device_is_compatible+0xb4/0x110)
[    0.000000] [<c02df110>] (__of_device_is_compatible) from
[<c02df22c>] (of_find_compatible_node+0x4c/0x7c)
[    0.000000] [<c02df22c>] (of_find_compatible_node) from
[<c04cedf4>] (exynos_firmware_init+0x18/0x7c)
[    0.000000] [<c04cedf4>] (exynos_firmware_init) from [<c04caef0>]
(setup_arch+0x860/0x898)
[    0.000000] [<c04caef0>] (setup_arch) from [<c04c7820>]
(start_kernel+0x80/0x3dc)
[    0.000000] [<c04c7820>] (start_kernel) from [<40008074>] (0x40008074)

Regards,
Sachin



On 19 February 2014 19:44, Grant Likely <grant.likely@linaro.org> wrote:
> Hi all,
>
> I've taken Kevin's latest rework and done even more rework on it. :-) I
> didn't quite like how it was looking so I rolled his scoring code
> directly into __of_device_is_compatible() so that the function always
> returns a score in a way that is still compatible with the existing
> users (ie. non-zero == successful match). This eliminates the need for a
> separate pscore argument and it also let me roll the type and name
> checks into the same function. I'm a lot happier with it overall and it
> makes for a slightly smaller number of lines of code changed. Please
> take a look and give it a spin. This is basically a bug fix so I'll need
> to push it out to Linus in the near future.
>
> Acks and Tested-bys would be particularly appreciated.
>
> Thanks,
> g.
>
> Kevin Hao (2):
>         Revert "of: search the best compatible match first in __of_match_node()"
>         of: reimplement the matching method for __of_match_node()
>
> Grant Likely (2):
>         of: Move testcase FDT data into drivers/of
>         of: Add self test for of_match_node()
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 0/4] Bugfix for of_match_node ordering

[Kevin Hao]

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

On Thu, Feb 20, 2014 at 02:09:08PM +0530, Sachin Kamat wrote:
> Hi Grant,
> 
> I observe the following boot failure with today's (next-20140220) linux-next
> tree on Exynos based boards with the default exynos_defconfig.

Does this help?

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 8a27fc907ab6..9cc893530b9a 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -381,12 +381,16 @@ static int __of_device_is_compatible(const struct device_node *device,
 
 	/* Compatible match has highest priority */
 	if (compat && compat[0]) {
-		of_property_for_each_string(device, "compatible", prop, cp) {
+		prop = __of_find_property(device, "compatible", NULL);
+		if (!prop)
+			return 0;
+
+		for (cp = of_prop_next_string(prop, NULL); cp;
+				cp = of_prop_next_string(prop, cp), index++) {
 			if (of_compat_cmp(cp, compat, strlen(compat)) == 0) {
 				score = INT_MAX/2 - (index << 2);
 				break;
 			}
-			index++;
 		}
 		if (!score)
 			return 0;


Thanks,
Kevin

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

Re: [PATCH v4 0/4] Bugfix for of_match_node ordering

[Sachin Kamat]

Hi Kevin,

On 20 February 2014 15:42, Kevin Hao <haokexin@gmail.com> wrote:
> On Thu, Feb 20, 2014 at 02:09:08PM +0530, Sachin Kamat wrote:
>> Hi Grant,
>>
>> I observe the following boot failure with today's (next-20140220) linux-next
>> tree on Exynos based boards with the default exynos_defconfig.
>
> Does this help?

The below patch works for me. Thanks for the fix.

>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 8a27fc907ab6..9cc893530b9a 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -381,12 +381,16 @@ static int __of_device_is_compatible(const struct device_node *device,
>
>         /* Compatible match has highest priority */
>         if (compat && compat[0]) {
> -               of_property_for_each_string(device, "compatible", prop, cp) {
> +               prop = __of_find_property(device, "compatible", NULL);
> +               if (!prop)
> +                       return 0;
> +
> +               for (cp = of_prop_next_string(prop, NULL); cp;
> +                               cp = of_prop_next_string(prop, cp), index++) {
>                         if (of_compat_cmp(cp, compat, strlen(compat)) == 0) {
>                                 score = INT_MAX/2 - (index << 2);
>                                 break;
>                         }
> -                       index++;
>                 }
>                 if (!score)
>                         return 0;
>
>
> Thanks,
> Kevin



-- 
With warm regards,
Sachin

Re: [PATCH v4 0/4] Bugfix for of_match_node ordering

[Kevin Hao]

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

On Thu, Feb 20, 2014 at 03:57:07PM +0530, Sachin Kamat wrote:
> Hi Kevin,
> 
> On 20 February 2014 15:42, Kevin Hao <haokexin@gmail.com> wrote:
> > On Thu, Feb 20, 2014 at 02:09:08PM +0530, Sachin Kamat wrote:
> >> Hi Grant,
> >>
> >> I observe the following boot failure with today's (next-20140220) linux-next
> >> tree on Exynos based boards with the default exynos_defconfig.
> >
> > Does this help?
> 
> The below patch works for me. Thanks for the fix.

Thanks Sachin.

Hi Grant,

I assume you would fold this change into the original patch. But if you want
a separate patch, please let me know.

Thanks,
Kevin

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

Re: [PATCH v4 0/4] Bugfix for of_match_node ordering

[Sachin Kamat]

On 20 February 2014 16:25, Kevin Hao <haokexin@gmail.com> wrote:
> On Thu, Feb 20, 2014 at 03:57:07PM +0530, Sachin Kamat wrote:
>> Hi Kevin,
>>
>> On 20 February 2014 15:42, Kevin Hao <haokexin@gmail.com> wrote:
>> > On Thu, Feb 20, 2014 at 02:09:08PM +0530, Sachin Kamat wrote:
>> >> Hi Grant,
>> >>
>> >> I observe the following boot failure with today's (next-20140220) linux-next
>> >> tree on Exynos based boards with the default exynos_defconfig.
>> >
>> > Does this help?
>>
>> The below patch works for me. Thanks for the fix.
>
> Thanks Sachin.
>
> Hi Grant,
>
> I assume you would fold this change into the original patch. But if you want
> a separate patch, please let me know.
>
> Thanks,
> Kevin

FWIW, On Exynos platform
Tested-by: Sachin Kamat <sachin.kamat@linaro.org>


-- 
With warm regards,
Sachin

Re: [PATCH v4 0/4] Bugfix for of_match_node ordering

[Grant Likely]

On Thu, 20 Feb 2014 18:12:40 +0800, Kevin Hao <haokexin@gmail.com> wrote:
> On Thu, Feb 20, 2014 at 02:09:08PM +0530, Sachin Kamat wrote:
> > Hi Grant,
> > 
> > I observe the following boot failure with today's (next-20140220) linux-next
> > tree on Exynos based boards with the default exynos_defconfig.
> 
> Does this help?

I've merged this in. I could get my unicore test platform to fail to
boot by turning on lock debugging. I'll leave that option on from now
one.

I've pushed this out to the following branch. Please test and report. It
will also be picked up by linux-next tomorrow.

git://git.secretlab.ca/git/linux devicetree/merge

> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 8a27fc907ab6..9cc893530b9a 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -381,12 +381,16 @@ static int __of_device_is_compatible(const struct device_node *device,
>  
>  	/* Compatible match has highest priority */
>  	if (compat && compat[0]) {
> -		of_property_for_each_string(device, "compatible", prop, cp) {
> +		prop = __of_find_property(device, "compatible", NULL);
> +		if (!prop)
> +			return 0;

The above 2 lines are unnecessary. of_prop_next_string() will return
NULL if prop is NULL.

g.

> +
> +		for (cp = of_prop_next_string(prop, NULL); cp;
> +				cp = of_prop_next_string(prop, cp), index++) {
>  			if (of_compat_cmp(cp, compat, strlen(compat)) == 0) {
>  				score = INT_MAX/2 - (index << 2);
>  				break;
>  			}
> -			index++;
>  		}
>  		if (!score)
>  			return 0;
> 
> 
> Thanks,
> Kevin

Re: [PATCH v4 0/4] Bugfix for of_match_node ordering

[Grant Likely]

On Thu, Feb 20, 2014 at 8:39 AM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> Hi Grant,
>
> I observe the following boot failure with today's (next-20140220) linux-next
> tree on Exynos based boards with the default exynos_defconfig.

Ugh, nested locking. that is not good. Kevin's patch looks correct and
I'll merge it in. I'm a little disturbed though that you're the only
one who has reported problems. Looking at what it does I would expect
pretty much every SMP platform for freak out about this, but I've
heard nothing from the powerpc guys.

I'll merge in the fix of course, but I'd like to know what I'm missing.

g.

>
> Uncompressing Linux... done, booting the kernel.
> [    0.000000] Booting Linux on physical CPU 0x900
> [    0.000000] Linux version 3.14.0-rc3-next-20140220 (sachin@linaro)
> (gcc version 4.8.2 20130805 (prerelease) (crosstool-NG linaro-1.
> 13.1-4.8-2013.08 - Linaro GCC 2013.08) ) #1132 SMP PREEMPT Thu Feb 20
> 13:49:27 IST 2014
> [    0.000000] CPU: ARMv7 Processor [412fc091] revision 1 (ARMv7), cr=10c5387d
> [    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
> instruction cache
> [    0.000000] Machine model: Insignal Origen evaluation board based
> on Exynos4210
> [    0.000000] bootconsole [earlycon0] enabled
> [    0.000000] Memory policy: Data cache writealloc
> [    0.000000] CPU EXYNOS4210 (id 0x43210011)
> [    0.000000] On node 0 totalpages: 258048
> [    0.000000]   Normal zone: 1520 pages used for memmap
> [    0.000000]   Normal zone: 0 pages reserved
> [    0.000000]   Normal zone: 190464 pages, LIFO batch:31
> [    0.000000]   HighMem zone: 528 pages used for memmap
> [    0.000000]   HighMem zone: 67584 pages, LIFO batch:15
> [    0.000000] BUG: spinlock recursion on CPU#0, swapper/0
> [    0.000000]  lock: devtree_lock+0x0/0x10, .magic: dead4ead, .owner:
> swapper/0, .owner_cpu: 0
> [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted
> 3.14.0-rc3-next-20140220 #1132
> [    0.000000] [<c0013e9c>] (unwind_backtrace) from [<c0011250>]
> (show_stack+0x10/0x14)
> [    0.000000] [<c0011250>] (show_stack) from [<c0386740>]
> (dump_stack+0x7c/0xbc)
> [    0.000000] [<c0386740>] (dump_stack) from [<c005501c>]
> (do_raw_spin_lock+0x188/0x18c)
> [    0.000000] [<c005501c>] (do_raw_spin_lock) from [<c038b614>]
> (_raw_spin_lock_irqsave+0x20/0x28)
> [    0.000000] [<c038b614>] (_raw_spin_lock_irqsave) from [<c02de37c>]
> (of_find_property+0x20/0x4c)
> [    0.000000] [<c02de37c>] (of_find_property) from [<c02df110>]
> (__of_device_is_compatible+0xb4/0x110)
> [    0.000000] [<c02df110>] (__of_device_is_compatible) from
> [<c02df22c>] (of_find_compatible_node+0x4c/0x7c)
> [    0.000000] [<c02df22c>] (of_find_compatible_node) from
> [<c04cedf4>] (exynos_firmware_init+0x18/0x7c)
> [    0.000000] [<c04cedf4>] (exynos_firmware_init) from [<c04caef0>]
> (setup_arch+0x860/0x898)
> [    0.000000] [<c04caef0>] (setup_arch) from [<c04c7820>]
> (start_kernel+0x80/0x3dc)
> [    0.000000] [<c04c7820>] (start_kernel) from [<40008074>] (0x40008074)
> [    0.000000] BUG: spinlock lockup suspected on CPU#0, swapper/0
> [    0.000000]  lock: devtree_lock+0x0/0x10, .magic: dead4ead, .owner:
> swapper/0, .owner_cpu: 0
> [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted
> 3.14.0-rc3-next-20140220 #1132
> [    0.000000] [<c0013e9c>] (unwind_backtrace) from [<c0011250>]
> (show_stack+0x10/0x14)
> [    0.000000] [<c0011250>] (show_stack) from [<c0386740>]
> (dump_stack+0x7c/0xbc)
> [    0.000000] [<c0386740>] (dump_stack) from [<c0054fac>]
> (do_raw_spin_lock+0x118/0x18c)
> [    0.000000] [<c0054fac>] (do_raw_spin_lock) from [<c038b614>]
> (_raw_spin_lock_irqsave+0x20/0x28)
> [    0.000000] [<c038b614>] (_raw_spin_lock_irqsave) from [<c02de37c>]
> (of_find_property+0x20/0x4c)
> [    0.000000] [<c02de37c>] (of_find_property) from [<c02df110>]
> (__of_device_is_compatible+0xb4/0x110)
> [    0.000000] [<c02df110>] (__of_device_is_compatible) from
> [<c02df22c>] (of_find_compatible_node+0x4c/0x7c)
> [    0.000000] [<c02df22c>] (of_find_compatible_node) from
> [<c04cedf4>] (exynos_firmware_init+0x18/0x7c)
> [    0.000000] [<c04cedf4>] (exynos_firmware_init) from [<c04caef0>]
> (setup_arch+0x860/0x898)
> [    0.000000] [<c04caef0>] (setup_arch) from [<c04c7820>]
> (start_kernel+0x80/0x3dc)
> [    0.000000] [<c04c7820>] (start_kernel) from [<40008074>] (0x40008074)
>
> Regards,
> Sachin
>
>
>
> On 19 February 2014 19:44, Grant Likely <grant.likely@linaro.org> wrote:
>> Hi all,
>>
>> I've taken Kevin's latest rework and done even more rework on it. :-) I
>> didn't quite like how it was looking so I rolled his scoring code
>> directly into __of_device_is_compatible() so that the function always
>> returns a score in a way that is still compatible with the existing
>> users (ie. non-zero == successful match). This eliminates the need for a
>> separate pscore argument and it also let me roll the type and name
>> checks into the same function. I'm a lot happier with it overall and it
>> makes for a slightly smaller number of lines of code changed. Please
>> take a look and give it a spin. This is basically a bug fix so I'll need
>> to push it out to Linus in the near future.
>>
>> Acks and Tested-bys would be particularly appreciated.
>>
>> Thanks,
>> g.
>>
>> Kevin Hao (2):
>>         Revert "of: search the best compatible match first in __of_match_node()"
>>         of: reimplement the matching method for __of_match_node()
>>
>> Grant Likely (2):
>>         of: Move testcase FDT data into drivers/of
>>         of: Add self test for of_match_node()
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 0/4] Bugfix for of_match_node ordering

[Sören Brinkmann]

Hi Grant,

On Thu, 2014-02-20 at 11:29AM +0000, Grant Likely wrote:
> On Thu, Feb 20, 2014 at 8:39 AM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> > Hi Grant,
> >
> > I observe the following boot failure with today's (next-20140220) linux-next
> > tree on Exynos based boards with the default exynos_defconfig.
> 
> Ugh, nested locking. that is not good. Kevin's patch looks correct and
> I'll merge it in. I'm a little disturbed though that you're the only
> one who has reported problems. Looking at what it does I would expect
> pretty much every SMP platform for freak out about this, but I've
> heard nothing from the powerpc guys.
> 
> I'll merge in the fix of course, but I'd like to know what I'm missing.

FWIW, I see a deadlock on Zynq (ARM) as well. The proposed patch
resolves it. Feel free to add my
Tested-by: Soren Brinkmann <soren.brinkmann@xilinx.com>

	Sören