[flashrom] Using libflashrom from fwupd

[Stefan Tauner]

<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". :)

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 :)
};

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);

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?

 - The callers have a predefined knowledge about what flashrom does and
   approximate from that (e.g., a "parent" progress bar that shows the
   overall progress in the case of a flashrom -w in 3 giant steps
   similar to what many other programs do (e.g., gparted).
   The disadvantage is that this is completely non-linear speed and
   there need to exist prior knowledge in the caller.

 - Additionally to the "total" values of the current stage we could
   also give a total total. This would solve the problem with the
   preknowledge (unless for some reasons the total number of stages are
   needed). The problem is that I think we won't have this information
   at all times where we need to make these calls. We could update this
   information only sometimes (and use -1 or 0 to indicate no change),
   or alternatively, we could have another dedicated callback to
   describe the overall process. NB: the total amount of net bytes
   transferred might not always equal <number of stages>*<bytes of any single
   stage> (e.g., write-only locks -> initial read and verification are
   impossible thus skipped but we still might need to do a blind write).

 - Go fully berserk and add another callback that informs the
   application of the overall plan/layout of stages flashrom will do.

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner