Attention is currently required from: Aarya, Anastasia Klimchuk, Nikolai Artemiev, Peter Marheine.
Sergii Dmytruk has posted comments on this change by Sergii Dmytruk. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email )
Change subject: More adequate progress ......................................................................
Patch Set 1:
(7 comments)
Patchset:
PS1:
Sergii, thanks a lot for the patch, that's a great help. […]
Sure, please go for it. I can review and provide more details as needed, just don't have time/need to finish this properly.
Commit Message:
https://review.coreboot.org/c/flashrom/+/84102/comment/7fa24368_d2e255aa?usp... : PS1, Line 46: : CLI shares terminal with the rest of the code
What does it mean? I don't understand :(
I just meant that flashrom can print log output at any point (like it prints `DONE something`) so CLI implementation can't assume that nothing interferes with its output.
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/84102/comment/2af2d3e3_1145c745?usp... : PS1, Line 58: unsigned long long delay_ns;
This is changing to nano seconds, is it to be able to set a more fine-grained delay? […]
I think insufficient range was my first guess, before I noticed `(1000000 * 8) / freq` below. If frequency of 1Hz isn't necessary in 32-bit builds, then `long int` should be enough. 64MHz wasn't supported because `(1000000 * 8) / 64000000` results in zero delay (like any frequency above 8MHz).
Type of `freq` below doesn't need to change in any case.
File include/flash.h:
https://review.coreboot.org/c/flashrom/+/84102/comment/74c4eb9a_2775229e?usp... : PS1, Line 544: struct stage_progress { : size_t current; : size_t total; : };
And can this go to libflashrom. […]
This is not part of API, nothing in `libflashrom.h` uses this structure.
https://review.coreboot.org/c/flashrom/+/84102/comment/8edbf977_85a786b5?usp... : PS1, Line 587: struct stage_progress stage_progress[FLASHROM_PROGRESS_NR];
Maybe this can go inside `struct flashrom_progress` ? Why do we need two structs on the same level, […]
That would change API and ABI. This was added for internal use to keep track of progress, while `struct flashrom_progress` is a parameter for a user callback which can also be allocated on client side and changed at will, potentially messing with flashrom's view of things.
File linux_mtd.c:
https://review.coreboot.org/c/flashrom/+/84102/comment/4caa98f3_c3707e9c?usp... : PS1, Line 298: update_progress(flash, FLASHROM_PROGRESS_ERASE, u + data->erasesize, len);
Why you removed this? Should it be […]
It's for erasing which I think is handled uniformly in `erasure_layout.c`.
File spi.c:
https://review.coreboot.org/c/flashrom/+/84102/comment/a9799407_12dbdc0e?usp... : PS1, Line 118: update_progress(flash, FLASHROM_PROGRESS_READ, start - start_address + to_read, end_address);
Why removing, should this be […]
`flash->mst->spi.read` updates progress, keeping this line results in progress on reading reaching over 100%.
This could be a single place to report read progress (like for erasing) and drop all similar calls in programmers, but granularity would likely be too coarse (erasing is different, there can be a command to erase the whole chip at once and you can only report it in one way: 0% -> 100%).