Attention is currently required from: Nico Huber. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55651 )
Change subject: ich_descriptors: Normalize chipset detection ......................................................................
Patch Set 5:
(2 comments)
File ich_descriptors.c:
https://review.coreboot.org/c/flashrom/+/55651/comment/72a6b325_b5a1d982 PS5, Line 936: if (content->ISL <= 16) : return CHIPSET_5_SERIES_IBEX_PEAK;
Please give some number example, I don't follow.
Let's say somebody comes along and wants to add support for NEW_CHIPSET with an ISL value of 80. To do that in a manner consistent with the pattern of this code, they'll probably do something like: if (content->ISL <= 80) return NEW_CHIPSET;
With this patch (CB:55651) applied it's unclear if the intent of the default case is to positively detect Ibex Peak, or to simply return Ibex Peak as a reasonable default when nothing else is explicitly detected. In the former case, adding detection for NEW_CHIPSET breaks Ibex Peak detection. In the latter case, adding detection for NEW_CHIPSET makes [11,80] the range for NEW_CHIPSET (assuming FLMAP2 == 0, of course) and [81,255] for Ibex Peak, which seems totally fine since we don't care about ISL values > 80.
The original code made this completely unambiguous by using a control statement to enforce an allowable range for Ibex Peak detection.
https://review.coreboot.org/c/flashrom/+/55651/comment/5c1ebd23_2ba5ff10 PS5, Line 953: return CHIPSET_8_SERIES_LYNX_POINT; : warn_peculiar_desc(true, "Wildcat Point"); : return CHIPSET_9_SERIES_WILDCAT_POINT;
We don't know any difference in the descriptor between these two. So […]
Fair point - after a bit of searching it seems Wildcat Point is a refresh of Lynx Point. A developer with a Wildcat Point system might be confused by matching Lynx Point - perhaps a comment would be helpful in situations like this.
-
David Hendricks (Code Review)