Attention is currently required from: Bill XIE, Nikolai Artemiev.
Anastasia Klimchuk has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84253?usp=email )
Change subject: ichspi: Restore Sector erase opcode after send command ......................................................................
Patch Set 1:
(3 comments)
Patchset:
PS1: Bill, thank you so much for the patch! Especially, that you provided all the details of your debugging, and logs in the ticket, it is so much useful! I think you found a real issue.
I spent some time looking through everything, and I want to find the right solution (and add some more people to the patch and discuss). I understand that you made the patch and it unblocks your work, so there is no urgency from your side? It might be another week or two to discuss.
Also, I added comments, but they are not necessarily for you (hope this doesn't sound too odd :) ).
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/84253/comment/33d3cb94_84ad1e1e?usp... : PS1, Line 402: static OPCODE POSSIBLE_OPCODES[] = { What is the meaning of this array ? It is called POSSIBLE_OPCODES , but for example in the logs of the linked bug, during probing, opcodes are reprogrammed to 0x83, 0x5A, 0x15, 0xAB - and none of these is in the POSSIBLE_OPCODES .
https://review.coreboot.org/c/flashrom/+/84253/comment/d10bdf75_2b8c52ab?usp... : PS1, Line 664: int oppos = 2; // use original JEDEC_BE_D8 offset I have questions in my head
1) Why hardcoded offset 2 is chosen to reprogram?
I understand that something has to be reprogrammed. But why specifically index 2 was chosen?
There is "reprogramming on the fly", but there is no "restore reprogramming", so probably the idea was that if opcode is discovered missing, it would be reprogrammed back.
However, I think erase opcodes are not good candidates for reprogramming, because if one of them is missing everything still works, erase works with what's remaining available opcodes. Which breaks the idea of erase optimisations that we have now (but original code goes back to 2010, maybe it wasn't relevant at that time).
If I were to pick one of the opcodes in default configuration, to be used for reprogramming, I would pick offset 4, JEDEC_REMS "Read Electronic Manufacturer Signature". I don't think it's used often, also if it's missing it will be discovered missing and reprogrammed back.
Can we change offset 2 to 4 ?
2) This line was written in Oct 2010 (https://github.com/flashrom/flashrom/commit/738e252112271f63c8ad4c9a135cfe17...) , but since then the offset of JEDEC_BE_D8 has changed, see patch https://review.coreboot.org/c/flashrom/+/34689 (Aug 2019) After this patch , offset 2 is no longer JEDEC_BE_D8, but now it is JEDEC_SE. However, reprogram_opcode_on_the_fly still uses hard-coded offset 2 for re-programming. Is this fine?
3) And of course I am also wondering, how is that no one noticed this before? Is this perhaps most often the whole chip is erased?
-
Anastasia Klimchuk (Code Review)