Hello Patrick Rudolph,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/32874
to review the following change.
Change subject: fit: Check all compat strings for highest match ......................................................................
fit: Check all compat strings for highest match
The compat string matching code was mostly copied from depthcharge. One of the few differences is that we now store the list of compat strings we're willing to match in a list rather than an array. Since our lists insert at the front, that means the strings are now ordered lowest to highest (not highest to lowest like in depthcharge).
We did rewrite the compat_rank matching code to accomodate for that... however, what we didn't do is remove the break-statement in the loop that matches all compat strings. When we search the lowest priority first, we can't abort the search as soon as we found a match -- we have to keep looking because we might find a higher priority match later.
This patch fixes the issue so that my Kevin can actually match for google,kevin-rev5 (and doesn't just jump at the first best google,kevin match).
Change-Id: Ibe3d84bbce6de3cd49c746a667ae1ccfdc843105 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/lib/fit.c 1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/32874/1
diff --git a/src/lib/fit.c b/src/lib/fit.c index 1b02c6a..0d3cb2d 100644 --- a/src/lib/fit.c +++ b/src/lib/fit.c @@ -463,7 +463,6 @@ config->compat_rank = i; config->compat_string = compat_node->compat_string; - break; } i++; }
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32874 )
Change subject: fit: Check all compat strings for highest match ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32874/5/src/lib/fit.c File src/lib/fit.c:
https://review.coreboot.org/#/c/32874/5/src/lib/fit.c@461 PS5, Line 461: if (pos >= 0) { should we compare pos with config->compat_pos? how do we make sure the pos after look is the highest?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32874 )
Change subject: fit: Check all compat strings for highest match ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32874/5/src/lib/fit.c File src/lib/fit.c:
https://review.coreboot.org/#/c/32874/5/src/lib/fit.c@461 PS5, Line 461: if (pos >= 0) {
should we compare pos with config->compat_pos? how do we make sure the pos after look is the highest […]
'pos' isn't actually used for ranking, it's literally just the index of the selected substring in the FDT property (used only to print the "(match)" behind the selected string in line 554). compat_rank is the only thing used for ranking.
This code is essentially finding the highest-ranked match by matching strings in order of rank (implicitly, because compat_strings is ordered lowest to highest rank), so that by the time the loop is done it will reflect the highest one.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32874 )
Change subject: fit: Check all compat strings for highest match ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32874 )
Change subject: fit: Check all compat strings for highest match ......................................................................
fit: Check all compat strings for highest match
The compat string matching code was mostly copied from depthcharge. One of the few differences is that we now store the list of compat strings we're willing to match in a list rather than an array. Since our lists insert at the front, that means the strings are now ordered lowest to highest (not highest to lowest like in depthcharge).
We did rewrite the compat_rank matching code to accomodate for that... however, what we didn't do is remove the break-statement in the loop that matches all compat strings. When we search the lowest priority first, we can't abort the search as soon as we found a match -- we have to keep looking because we might find a higher priority match later.
This patch fixes the issue so that my Kevin can actually match for google,kevin-rev5 (and doesn't just jump at the first best google,kevin match).
Change-Id: Ibe3d84bbce6de3cd49c746a667ae1ccfdc843105 Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/32874 Reviewed-by: Hung-Te Lin hungte@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/lib/fit.c 1 file changed, 0 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Hung-Te Lin: Looks good to me, approved
diff --git a/src/lib/fit.c b/src/lib/fit.c index 6ac0a89..37dfc8d 100644 --- a/src/lib/fit.c +++ b/src/lib/fit.c @@ -463,7 +463,6 @@ config->compat_rank = i; config->compat_string = compat_node->compat_string; - break; } i++; }
-
Hung-Te Lin (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)