| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20140219124813.8FD66C4088D@trevor.secretlab.ca>
Date: Wed, 19 Feb 2014 12:48:13 +0000
From: Grant Likely <grant.likely@...aro.org>
To: Kevin Hao <haokexin@...il.com>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Cc: Kevin Hao <haokexin@...il.com>, Rob Herring <robh+dt@...nel.org>,
Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
Subject: Re: [PATCH v3 2/4] of: reimplement the matching method for __of_match_node()
On Wed, 19 Feb 2014 16:15:45 +0800, Kevin Hao <haokexin@...il.com> wrote:
> 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
>
> This is based on some pseudo-codes provided by Grant Likely.
>
> Signed-off-by: Kevin Hao <haokexin@...il.com>
> [grant.likely: Changed multiplier to 4 which makes more sense]
> Signed-off-by: Grant Likely <grant.likely@...aro.org>
> ---
> 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:
> name2 { }
>
> struct of_device_id matches[] = {
> {.name = "name2", },
> {.name = "name2", .type = "type1", },
> {}
> };
>
> drivers/of/base.c | 96 ++++++++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 77 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index ba195fbce4c6..8f79f006d86f 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -342,21 +342,28 @@ 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
> +/*
> + * Compare with the __of_device_is_compatible, this will return a score for the
> + * matching strings. The smaller value indicates the match for the more specific
> + * compatible string.
> */
> -static int __of_device_is_compatible(const struct device_node *device,
> - const char *compat)
> +static int __of_device_is_compatible_score(const struct device_node *device,
> + const char *compat, int *pscore)
> {
> const char* cp;
> int cplen, l;
> + int 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)
> + score++;
> + if (of_compat_cmp(cp, compat, strlen(compat)) == 0) {
> + if (pscore)
> + *pscore = score;
> return 1;
> + }
> l = strlen(cp) + 1;
> cp += l;
> cplen -= l;
> @@ -368,6 +375,15 @@ static int __of_device_is_compatible(const struct device_node *device,
> /** Checks if the given "compat" string matches one of the strings in
> * the device's "compatible" property
> */
> +static int __of_device_is_compatible(const struct device_node *device,
> + const char *compat)
> +{
> + return __of_device_is_compatible_score(device, compat, NULL);
> +}
> +
> +/** Checks if the given "compat" string matches one of the strings in
> + * the device's "compatible" property
> + */
> int of_device_is_compatible(const struct device_node *device,
> const char *compat)
> {
> @@ -734,25 +750,55 @@ 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 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;
> + int score = 0;
> +
> + /*
> + * Matching compatible is better than matching type and name,
> + * and the specific compatible is better than the general.
> + */
> + if (matches->compatible[0]) {
> + if (__of_device_is_compatible_score(node,
> + matches->compatible, &score))
> + score = INT_MAX - 4 * score;
> + else
> + score = INT_MIN;
> + }
> +
> + /*
> + * Matching type is better than matching name, but matching
> + * both is even better than that.
> + */
> + if (matches->type[0]) {
> + if (node->type && !strcmp(matches->type, node->type))
> + score += 2;
> + else
> + score = INT_MIN;
> + }
> +
> + /* Matching name is a bit better than not */
> + if (matches->name[0]) {
> + if (node->name && !strcmp(matches->name, node->name))
> + score++;
> + else
> + score = INT_MIN;
> + }
All that mucking about with setting the score to INT_MIN is pretty ugly.
I've reworked the patch to 'continue;' whenever one of the above matches
fail. That completely eliminates any possibility of accepting an entry
that has a failed match. Here's my diff. I'll merge this change into the
patch before committing. All of the test cases still pass with my
rework.
---
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 8f79f006d86f..fcb65d27d071 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -756,7 +756,7 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
if (!matches)
return NULL;
- while (matches->name[0] || matches->type[0] || matches->compatible[0]) {
+ for (; matches->name[0] || matches->type[0] || matches->compatible[0]; matches++) {
int score = 0;
/*
@@ -764,11 +764,10 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
* and the specific compatible is better than the general.
*/
if (matches->compatible[0]) {
- if (__of_device_is_compatible_score(node,
+ if (!__of_device_is_compatible_score(node,
matches->compatible, &score))
- score = INT_MAX - 4 * score;
- else
- score = INT_MIN;
+ continue;
+ score = INT_MAX - 4 * score;
}
/*
@@ -776,26 +775,22 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
* both is even better than that.
*/
if (matches->type[0]) {
- if (node->type && !strcmp(matches->type, node->type))
- score += 2;
- else
- score = INT_MIN;
+ if (!node->type || strcmp(matches->type, node->type))
+ continue;
+ score += 2;
}
/* Matching name is a bit better than not */
if (matches->name[0]) {
- if (node->name && !strcmp(matches->name, node->name))
- score++;
- else
- score = INT_MIN;
+ if (!node->name || strcmp(matches->name, node->name))
+ continue;
+ score++;
}
if (score > best_score) {
best_match = matches;
best_score = score;
}
-
- matches++;
}
return best_match;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists