Attention is currently required from: Felix Singer, Edward O'Callaghan, Anastasia Klimchuk, Alexander Goncharov.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67393 )
Change subject: tree/: Move programmer_delay() out of programmer state machine ......................................................................
Patch Set 5:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/67393/comment/270b755a_917f02b5 PS4, Line 13: Sorry that this took too long. It's often hard for me emotionally when I get the feeling to be misunderstood or that it's futile to continue without starting to explain things from zero. When discussions start like this, people not understan- ding each other at one level, it often goes round and round to more basic levels before a common ground can be found.
I'll try to explain my thoughts about a single point and not discuss any technical details. IMO, this patch is the wrong place to discuss future design.
Next paragraph I could not parse, clearly programmer global problem got fixed in the context of programmer_delay() here. I guess this paragraph is perhaps speculating beyond the scope of this patch?
Of course it's speculating beyond the scope of this patch. This is quite necessary to make sure we don't walk in circles (patch wise). There will be a single patch in the future that will allow us to say re-entry in libflashrom is safe. It is this currently unknown, future patch that we have to work towards. How can we do that if we don't plan ahead?
In this instance, I saw the attempt to squeeze logic that currently resides in the programmer structures into master structures. To me it seemed that this was only done because the master structures already make it easier to avoid globals. But not because the logic clearly belongs to the masters. This avoids to reason about the programmer structures.
But what will happen in the future? We will most likely have to reason about the programmer structures anyway, when it comes to the huge `internal` programmer. If it turns out that we need to change the structures later, it's quite possible that we'll realize that moving the functions here wasn't necessary and probably change it back (a circle!). I'm not saying it will be like this, just that it is possible and we can't say what will be if we don't look ahead.
Now every single patch risks regressions (already proven for this patch), every single patch needs to be written, reviewed, every single patch can affect future work (e.g. set a precedent how to get rid of global state in similar cases). That means there is a lot of effort and burden, even if it only took 10~20 minutes to write a patch. If we risk that the work will be undone in the near future, that risks waste of time (and not only for the people involved in the review). I don't know how it is for you, I can only speak for myself, I have limited time.
There is a very simple way to avoid most of the need to look ahead:
Never start with the low-hanging fruits.
Starting with the easy programmer cases may feel nice and rewarding but is risky. For instance, if you had tackled the `internal` programmer first wrt. global state, there would be a very low risk to get any structures wrong; we wouldn't have this discussion.
I guess the whole thought can be summarized like this: Tackling the easiest programmer cases first and the hardest last is the wrong order. The hardest cases will require reasoning; doing them last postpones reasoning that may lead to changes that render the solutions for the easier ones incorrect.