[XS] Change in flashrom[main]: erasure_layout: Remove redundant `verify_range` call from `erase_write`

[Anastasia Klimchuk (Code Review)]

Attention is currently required from: Aarya, Carly Zlabek, Vincent Fazio.

Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79354?usp=email )

Change subject: erasure_layout: Remove redundant `verify_range` call from `erase_write`
......................................................................

Patch Set 1:

(1 comment)

Patchset:

PS1: 
Vincent, Carly
> Our company leverages flashrom pretty heavily and we're grateful for the project so are happy to contribute patches as we can.

This is really nice, thank you so much! Contributions are most welcome! This is the best way to help flashrom: by doing contributions, patches and testing. Also one more way to help could be to review other patches. If you see someone else's patch which is relevant for you, you can add comments. Just add yourself to the patch and add comments or votes!

I really appreciate your detailed message, I read it carefully and then went through the code and in short: I think you are right. And if there are not one but two redundant verifications, I think the best would be to remove both in one patch (this patch), and then test the patch as a whole.

Your other point is also correct: I can see that `erase_write` code does not check verification flags and doing verification unconditionally. This can be a good idea to fix too, but yes that would be a separate patch.

Specifically for this patch, I agree either 1) or 2) can be removed, and then also 3) , so that if you get to detailed testing and benchmarking, you would test the full solution. Because testing takes your time and effort, so better spend this effort on the full solution.

Between 1) and 2) there can be one more point of difference: if an error happened, 1) will discover it earlier and fail early (`goto _end`) vs 2) will go through the whole region even if the first byte failed.
> I would like us to make sure that we're not missing verification coverage there due to some weird loop quirk.

A note on this: if it helps, and if it can be useful for you, there is a unit test which focuses exactly on various test cases of erasing and writing: `tests/erase_func_algo.c` and you are welcome to add test cases to it. Test has a mock chip with 16 bytes memory, sets up the initial state of memory and what to write. Mock chip logs all invocations of read, erase, write. You can construct any test cases and various layouts.
(for example I am adding here https://review.coreboot.org/c/flashrom/+/78985)

If you end up adding some new test case which is useful, send it as a patch! But even without a patch, it might be useful to run locally many times. Won't test the hardware of course, but will test the logic of erase / write.

Thinking about weird loop quirk: verification on lines #341-344 works precisely on the same memory range as `erasefn` above it on #337. So if theoretically, there is a case with missing verification coverage, this would mean missing erase coverage which would be much bigger issue. Good news: such a test case can be added into a unit test and this way we protect against regressions.

I initially created a test with the mock chip of 16 bytes size, so far it has been enough to describe test cases I could come up with. But if there is a test case which needs 32 bytes and 16 is not enough, mock chip can be extended.