Attention is currently required from: Felix Singer, Nico Huber, Angel Pons, Anastasia Klimchuk, Alexander Goncharov.
Edward O'Callaghan 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/686087c0_4eeccae5 PS4, Line 13:
Folks, can we please retry this review? We (Angel) agree with Nico's concerns regarding technical as […]
These are great questions Angel. I also got told by aklm you would be keen to catch up over chat (this is a great idea) and I would be extremely excited to do that to help clear up understanding! We could summarise after and post here and/or list too so there is visibility for everyone for any conclusions. I'll jump into IRC today and we can perhaps organise some time that is most suitable for you, please no stress whatever suits you.
Let me try to answer the above questions best I can here and if there is more ambiguity left we can further deep dive over chat.
The life-time of a flash ctx is actually between the entry-point of a programmer init function and it's correspond shutdown function as a more precise definition than that of a successful probe of a flash chip. Actually the probing of a flash chip life-time is within the life-time of a programmers duties and that is post init and before shutdown, thus within the life-time of a flash ctx. While it may sound like a programmer_delay is coupled to the life-time of a programmer on the surface, this is actually misdirection because of how the identifiers happen to be named however a deeper inspection of the control flow graph reveals this is not the true case of how things actually work in terms of how the computer is doing its thing.
I will try with a diagram and I hope it renders: ``` +=[=========]=[=======]=+ {programmer life-times} |---+111111+---+22222+--+ {flash ctx 1 & 2 life-times} |-----**---------**-----+ {two probe cycles for-each flash ctx master 1 & 2}. ```
The `probe_flash()` function is revealing https://github.com/flashrom/flashrom/blob/master/flashrom.c#L775 where in this function we take a register_master entry {a collection of each different master type which can be thought of a given master at a time for a given dispatch} that takes this master, couples it to the flash ctx and dispatches the chip probe function for-each chip in the database. The probe function you will notice consumes a flash context and this is the same flash context that is coupled to the register master `flash->mst = mst;` within the context. The oddity here is the lack of coupling between `probe_flash()` and `programmer_init()` however that actually slightly an orthogonal issue to the problem of delay and is more a issue of a incomplete realisation of `const struct flashrom_programmer *flashprog` in the libflashrom problem space.
Zooming in this is a lot to take in but zooming out helps further to reveal the simpler structure at play here. The quintessential issue is that `programmer_delay()` is actually in truth `flashctx_delay()` and actually the indirection isn't even needed in the leaf drivers themselves - drivers have two cases, either the default `internal_delay()` or a custom version in the case of `ch341a && serprog`. Each driver can just directly call its own custom delay if it so needs to or in almost every case (modulo those two) just call the sensible default of `internal_delay()`. The core flashrom logic (say, spi25 helpers and so on where `programmer_delay()` is called) does indeed require some indirection to either dispatch a custom driver dependent delay or fall back to the default of `internal_delay()`. However, you will notice all these functions work with the `flash ctx` state machine and that it contains all required state for them to operate correctly due to the above life-time's I outlined above.
This is all perhaps quite a bit to digest in one go however I am more than happy to help explain or go over anything here. Hopefully the above helps answer some of the questions, keen to hear what you think and sorry for all my typos and bad ascii art :)