Nico Huber 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/dd079a64_cad8dfd7 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.
Neither did I factor in the addition of a new chipset with a low ISL value or one without an ISL field. I'm not sure if the list of things I didn't factor in is finite. Hence, I'm not taking any of it into account. The purpose of this patch was not to predict the future.
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.
Well, beside the "It does not enforce anything" part, you are right. You just put it in different words.
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.
You are assuming that there is a fix. We can't predict the future. We can make educated guesses, though. For instance, when I wrote this patch, I would not have guessed that a server platform will land on this ICCRIBA == 0 branch. Trying to predict such details can lead to walking in circles patch wise. For instance, I assume that it's easier to follow the code when it's more condensed, like after this patch. Before the entire patch train it was a forest of if's and nobody knew where to put anything. We should avoid that this happens again.
Anyway, I already agreed that you should try to go ahead days ago. So I don't understand why you have to keep arguing and wasting everybody's time.
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.
Only one thing I have to ask you: As the purpose of this patch was to normalize, please do so with explicit if's for all chipsets then. warn_peculiar_desc() probably shouldn't have to check anything then.