[flashrom] Using libflashrom from fwupd

[Nico Huber]

Hi Richard, David,

On 16.08.2017 23:02, David Hendricks wrote:
> On Tue, Aug 15, 2017 at 4:55 AM, Richard Hughes <hughsient at gmail.com> wrote:
> 
>> Hi all,
>>
>> I'm the maintainer of fwupd, which is a daemon for doing firmware
>> updates in Linux. Using fwupd about 200,000 people update firmware
>> every month. At the moment I have an out-of-tree patch to use the
>> flashrom CLI for updating coreboot on Purism laptops, which exec's a
>> flashrom binary with the correct arguments and then screen scrapes the
>> output. This is less than ideal. I saw the appearance of a libflashrom
>> in the staging branch and got very excited. I'm assuming the long term
>> goal here is to install a shared with stable API/ABI library and a
>> pkg-config file. If so, please keep reading.

I'm only using it as a static library, kept reading anyway :) The cur-
rent API is to be considered unstable. We'd like to have it stabilized
before the next release (whenever that may happen), but it's not a top
priority.

>>
>> One thing that is really important for fwupd is progress completion;
>> as flashing the firmware is an inherently scary thing to do users do
>> like to see a progress bar move from 0% to 100% in a gradual way as
>> it's very re-assuring. This is even more important for the GUI, where
>> people that just see a spinner don't know if the process is going to
>> take 10 seconds, or 10 minutes.
>>
> 
> Agreed! Especially since a lot of users have large chips, slow programmers,
> or sometimes both. Simple read operations can take a worryingly long time
> on some common hardware these days.
> 
> 
>>
>> Would it be possible to have a process callback for
>> flashrom_image_read(), flashrom_image_write() and
>> flashrom_image_verify()? It'd need to have some kind of callback
>> userdata too, for instance:

You already helped to improve our API: I forgot that one might need a
userdata pointer for the log callback. Will fix that...

>>
>> int flashrom_image_verify(struct flashrom_flashctx *, const void
>> *buffer, size_t buffer_len, flashrom_progress_callback callback_fn,
>> void *callback_user_data);
>>
>> and also:
>>
>> typedef void(flashrom_progress_callback)(uint64 completed, unit64
>> total, void *user_data)

I guess this would suffice for read and verify, which are simple,
continuous operations. Write, however, is comprised of one, two or three
steps (depending on options and the data): Read (needed to know which
bytes have to change), (comparing to the data to be written) write (if
necessary) and optionally verify. So there is no straight forward way
to tell during read, for instance, if we are at 100% after the read or
not. We could compare the data early, but that might result in an awk-
ward implementation. My idea (not yet convinced if it's any good) would
be to add a `stage` parameter to the callback:

    enum flashrom_progress_stage {
        FLASHROM_PROGRESS_READ,
        FLASHROM_PROGRESS_WRITE,
        FLASHROM_PROGRESS_VERIFY,
    };

>>
> 
> The short answer is yes. Nico also briefly mentioned an idea on IRC to use
> a mechanism similar to flashrom_set_log_callback() to avoid complicating
> function signatures for the read, write, and verify functions.

Correct, I would prefer a method to set the current callback and private
pointer over adding more parameters. Though, we could have another set
of functions ofc, with more parameters.

Richard, please let us know, what you'd prefer. After all, the interface
should be designed to be most usable (and not driven by internals). e.g.
do you expect the callback or the userdata pointer to change between
calls?

> 
>>
>> I'm not familiar at all with flashrom internals, but could try to
>> produce a patch if required -- although to propagate the progress
>> through operations would mean patching a lot of chip->read() callbacks
>> which probably requires a more experienced hand. Ideas welcome,
>> thanks.

I'd be glad to help implementing it. But let's discuss the interface
first.

Nico