Bug #520
openFactor out verification from erase path
0%
Description
Currently, flashrom_image_erase differs from flashrom_image_write in how it performs its verification.
When writes are completed, verification occurs afterwards per the verify flags specified in the flashctx argument.
/* Verify only if we actually changed something. */
if (verify && !all_skipped) {
msg_cinfo("Verifying flash... ");
/* Work around chips which need some time to calm down. */
programmer_delay(flashctx, 1000*1000);
if (verify_all)
combine_image_by_layout(flashctx, newcontents, oldcontents);
ret = verify_by_layout(flashctx, verify_layout, curcontents, newcontents);
For the erase path, there is no post-operation verification. Instead, check_erased_range is called regardless of verify flags, performing verification even if the user doesn't request it and imposing a performance penalty.
Updated by Anastasia Klimchuk 3 months ago
Hello Vincent,
I looked into the code of legacy write/erase, and it does the same thing: verification after write respects flags, verification after erase is always done (i.e. ignores flags).
Which is not a great excuse if behaviour is wrong, so looking further,
The code that defines and documents the existing flag, which is -v also known as verify flash against <file> or the content provided on the standard input.
But this flag is only used to detect the "standalone" verify operation against the given file.
The flag which is used to detect verify_after_write is actually -n or dont_verify_it, which makes sense because it should verify by default. It is described as Skip the automatic verification of flash ROM contents after writing. which is consistent with the name.
So the name and description says it's for writing operation only, and the code is consistent.
I don't think that we can change the implementation of the existing flag which has been around for ages.
We can however, add a new flag for erase operation specifically.
What is the exact use case for how you would use the new flag? Do you erase a lot of chips with the script?
Updated by Vincent Fazio 3 months ago
Anastasia Klimchuk wrote in #note-2:
The flag which is used to detect
verify_after_writeis actually-nordont_verify_it, which makes sense because it should verify by default. It is described asSkip the automatic verification of flash ROM contents after writing.which is consistent with the name.
So the name and description says it's for writing operation only, and the code is consistent.I don't think that we can change the implementation of the existing flag which has been around for ages.
We can however, add a new flag for erase operation specifically.What is the exact use case for how you would use the new flag? Do you erase a lot of chips with the script?
I worded this a bit weird and it should be clarified. It's been a while since I thought about this so I'm trying to jog my own memory...
I think some of this is a semantic thing. I interpret "writing" to a flash chip any action which commits new values to be persisted, so an erase operation is a "write" because it's flipping bits on flash.
I think the biggest issues are:
- when a user says "do not verify these writes", erasures are verified anyway. Regardless of call path (erase or write),
check_erased_rangeperforms verification after blocks are erased and there is no shield from this.- For the
flashrom_image_writecall path, this means we're performing verification on erase and then performing verification after the writes occur (so we're verifying twice) unlessdont_verify_itis specified, but this doesn't shield verifying erasures - For the
flashrom_image_erasecall path, we only perform the forced verification viacheck_erased_range
- For the
- If an erase is unaligned, there is a subsequent write to fill in data that should not have been erased. Some write functions implement a
verify_rangecall, but not all functions look to verify the write was persisted (in fact, it only looks like the WRITE_JEDEC{1} verifies writes).flashrom_image_writecan get away with this because it performs a post-write verification (unless told otherwise) and can spot a problem, however this is not true forflashrom_image_eraseas it only verifies on erase and implements whatever checks are intrinsic to the device write function, which may be nothing.
Of course, i may have misread the code, but this is what it looks like to me.
As for the CLI changes, I realize this wouldn't happen for a while, but large API/interface changes and CLI changes to redefine flags should be allowed for major version bumps, otherwise legacy codebases are stuck maintaining old code forever. Adding new flags don't require this however.
Updated by Anastasia Klimchuk 2 months ago
- Affected versions 1.3 added
Vincent, so sorry for the long delay in reply! But I was thinking about this in the background.
I have just re-read everything, and re-read the code too. I agree that something can be done.
I think we have two main options:
1) Change the meaning of the existing flag -n (dont_verify_it) to also cover erase operation, in addition to covering write.
2) Introduce a new flag which would mean "dont_verify_erase" and would cover exclusively erase. The meaning of the old flag does not change.
It's a good question whether erase counts as writing. Maybe you are right, I can agree that it makes sense.
Am I right that you like option 1 more?
With the choice of options, I am thinking of starting a thread on the mailing list, to see what other regular users think, there may be a bunch of different use cases for how people run flashrom.
The topic can be titled something like "consistency of verification after erase vs write operations"
If there are no responses on the thread, which means no strong opinions either way, we will decide between us two. Would you agree on the plan? And thank you!
Updated by Vincent Fazio about 2 months ago
Anastasia Klimchuk wrote in #note-4:
Am I right that you like option 1 more?
Yes, I think I personally prefer option 1. However, I understand it changes semantics a bit and may need to be behind some larger version bump that denotes a compatibility break with the previous version.
Updated by Anastasia Klimchuk about 2 months ago
I started a thread "Verification after write vs erase operations" on the mailing list.
Updated by Anastasia Klimchuk about 1 month ago
It looks like the option to cover erase operation by existing flag (option 1) will be the way to go.
I think I can produce a patch for this, and see how it goes. However I would do this after https://ticket.coreboot.org/issues/525 is fixed, it looks like a real issue and the fix will change the same code. So once 525 is fixed (which will take some time, mainly because of testing) I will produce a patch for this and add you as reviewer!
Alternatively, if you want, you are very welcome create a patch yourself, but keep in mind that 525 will change the code, so maybe after it.