[flashrom] Using libflashrom from fwupd

[Nico Huber]

On 17.08.2017 14:09, Stefan Tauner wrote:
> <missed the ML in the reply, sorry!>
> 
> On Thu, 17 Aug 2017 11:53:58 +0200
> Nico Huber <nico.huber at secunet.com> wrote:
> 
>> B := we need to give the user a really good reason
>>
>> Having a RESET enum value allows us to fulfill B, thus:
>>
>> RESET -> B
> 
> OK, that's where I don't agree(d) then. I was expecting a "real good
> reason" originating in libflashrom and not being some pre-defined
> obvious string like "things are complicated". :)

There is only one reason in flashrom internally: an erase failed. I
suppose that can be covered by a pre-defined string.

> 
> On Thu, 17 Aug 2017 10:42:14 +0100
> Richard Hughes <hughsient at gmail.com> wrote:
> 
>> On 17 August 2017 at 10:30, Stefan Tauner <stefan.tauner at gmx.at> wrote:
>>> Am I the only one seeing a contradiction to what you wrote half an hour
>>> ago (below)? Did I miss something?  
>>
>> Well, what I was trying to say (perhaps badly) that if we must "go
>> backwards" for a very good technical reason then we need to
>> communicate this to the user. In an ideal world we'd just be able to
>> write 0..100 then verify 0..100, but in the real worlds we have
>> blocks, erases, retries and all that jazz. So if we do need to handle
>> a "retry" we need a way to map the event to something we can show in
>> the UI to explain what we're doing.
> 
> But to do that no additional enum is necessary? The event of a reset
> can easily be determined if the last state is known.
> If we want to make the life of the caller easier and notify it of these
> events we should not use the "enum flashrom_progress_stage" parameter
> for it or rename it. I'd rather separate these two things completely
> like this:
> 
> enum flashrom_progress_stage {
> 	FLASHROM_PROGRESS_READ,
> 	FLASHROM_PROGRESS_WRITE,
> 	FLASHROM_PROGRESS_VERIFY,
> };
> 
> enum flashrom_progress_event {
> 	FLASHROM_PROGEV_RESET_STEP, /* If and when we might back off a little without resetting the whole stage. */ I think this is not implemented yet but we might want to retry writing single blocks if they fail for example.
> 	FLASHROM_PROGEV_RESET_STAGE, /* If and when we need to restart the current stage */ < Not implemented yet either but I forsee this being implemented in case of timeouts or connection problems.
> 	FLASHROM_PROGEV_RESET_PROCESS, /* If and when we need to restart the whole process */ < The typical write/erase fail -> re-read of whole chip case
> 	FLASHROM_PROGEV_SKIP_{STEP, STAGE}, /* If and when we skip a step, or possibly even stage */ < Is this useful at all? Probably not for ordinary progress bars but if you think about something looking like filesystem defragmentation visualization this could be worth it and ignoring is easy :)
> };

Not exactly what I had in mind when I said the interface shouldn't be
"driven by internals" but, IMHO, this clearly goes in the wrong direc-
tion. Not only the interface would be harder to understand and use, I
also don't want to picture how the code would look like internally when
we have not only progress calls but also progress event calls at every
corner.

> 
> typedef void(flashrom_progress_cb)(enum flashrom_progress_stage stage, uint64_t completed, unit64_t total, void *user_data);
> typedef void(flashrom_progress_event_cb)(enum flashrom_progress_event event, char *reason, void *user_data);

`char *reason`? see above.

> 
> flashrom_set_progress_callbacks(flashrom_progress_cb *cb_progress, flashrom_progress_event_cb *cb_event, void *user_data); < We might need to add a flashrom context here? No idea how the current staging implementation works but eventually we want to fix the hidden internal state...
> 
> 
> What's missing above is an indicator of the total progress.
> Essentially this scheme breaks down the total execution into stages and
> gives only feedback on the progress of them individually. To construct
> a useful overall progress indication the caller is required to have
> knowledge about the composition and numbers of stages forming the total
> operation. How could this be solved?

In a simplified version (let's assume resets don't count), we could
simply approximate everything with the size of the flash chip (or size
of included regions if a layout is used). So total amount of progress
for a read/write/verify would be three times the size. Read and verify
would be linear, write rather bumpy, but does it matter?

Any way to smoothen the write progress comes at the cost of an unpredic-
table total. We just can't decide the amount to be written before the
initial read.

Which reminds me: we haven't discussed alternatives to the `enum
flashrom_progress_stage` yet. I said I'm not convinced of it and I'm
even less tonight. The alternative would be to make the whole process
more controllable by the caller.

Richard, would it be feasible for your GUI visualization if you'd do the
steps separately? e.g. you could make a backup, then you could pass the
backup back to libflashrom together with the new data to be written
(which would allow us to skip the usual read before write) and then
finally call a function to verify everything. So basically you'd have
three calls into libflashrom which take some time and would show three
times a progress bar. If you wanted to show a total progress you'd have
to do that manually then.

Nico