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:
(1 comment)
File ich_descriptors.c:
https://review.coreboot.org/c/flashrom/+/55651/comment/d1f17932_93b6f27e PS5, Line 936: if (content->ISL <= 16) : return CHIPSET_5_SERIES_IBEX_PEAK;
I see the same side effects and conclude that for these numbers this change does not add assumptions.
You didn't factor in the addition of a new chipset with a large ISL value. That was the whole point.
I'm going to assume that what you call "the default case" is the return statement on line 943 (after this change). In line 942, we check `content->ISL != 16` always before the default case. So the information you are missing is still there, it's just not in the current state of the code inside an if statement.
The check you are referring to is for a warning that just tells us that the descriptor was not recognized. It does not enforce anything, unlike the original code which put Ibex Peak detection in an if-statement to enforce proper behavior when Ibex Peak is detected while making the intentions for default behavior obvious.
NB. The purpose of the code is to make flashrom work, not to provide a manual for future development.
Code needs to be maintainable. If it's not obvious how to do future development with this code then that is a problem that we should definitely fix.
Unless there is an actual bug that is fixed with this patch we should revert it. The original code is more explicit in each of its codepaths which makes it easier for future developers to avoid problems.