[flashrom] [PATCH] Real partial writes
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Thu Oct 21 17:31:49 CEST 2010
On 21.10.2010 06:50, David Hendricks wrote:
> The code looks good overall, though I haven't had a chance to test it
> extensively yet. Thanks for doing this!
>
Thanks for your review. Comments inline below.
> Comments below are mostly minor nits.
>
> On Wed, Oct 20, 2010 at 5:16 PM, Carl-Daniel Hailfinger wrote:
>
>> need_erase used an incorrect offset calculation in the
>> write_gran_256bytes case.
>>
>> Index: flashrom-partial_write_rolling_erase_write/flashrom.c
>> ===================================================================
>> --- flashrom-partial_write_rolling_erase_write/flashrom.c (Revision
>> 1215)
>> +++ flashrom-partial_write_rolling_erase_write/flashrom.c
>> (Arbeitskopie)
>> @@ -838,7 +841,7 @@
>> continue;
>> /* have needs to be in erased state. */
>> for (i = 0; i < limit; i++)
>> - if (have[i] != 0xff) {
>> + if (have[j * 256 + i] != 0xff) {
>> result = 1;
>> break;
>> }
>>
>>
>
> This definitely makes more sense :-)
>
> Maybe you can just rename i and j to offset and page to make their purpose a
> bit more clear?
>
The big problem is that page is a misnomer for chips with non-256-byte
pages. base would be a more suitable name, but it may cause confusion
with flashbase. The alternative would be to add some annotation ("page
is not really a page, it is the write granularity length") which makes
things a bit clearer. I'd like to keep i for the inner counter, though.
> (note: i generally have no problem with i/j/k/l for simple iterators)
>
> On Wed, Oct 20, 2010 at 5:16 PM, Carl-Daniel Hailfinger <
> c-d.hailfinger.devel.2006 at gmx.net> wrote:
>
>> @@ -846,10 +849,102 @@
>> return result;
>> }
>>
>> +/**
>> + * Check if the buffer @have needs to be programmed to get the content of
>> @want.
>> + * If yes, return 1 and fill in first_start with the start address of the
>> + * write operation and first_len with the length of the first
>> to-be-written
>> + * chunk. If not, return 0 and leave first_start and first_len undefined.
>> + *
>> + * Warning: This function assumes that @have and @want point to naturally
>> + * aligned regions.
>> + *
>> + * @have buffer with current content
>> + * @want buffer with desired content
>> + * @len length of the checked area
>> + * @gran write granularity (enum, not count)
>> + * @return 0 if no write is needed, 1 otherwise
>> + * @first_start offset of the first byte which needs to be written
>> + * @first_len length of the first contiguous area which needs to be
>> written
>>
>>
>
> Not sure what the preferred style is but @return should probably be listed
> below the variables, preferably separated by a newline.
>
Right. first_start and first_len are some sort of return variables as
well. That's the reason I had them grouped after return.
I have reordered them and extended the comments. The newline separation
is something I'm not so sure about given the explanation above.
That said, maybe it makes sense returning first_len as value of the
function. first_len=0 would mean no write is needed. That would kill one
variable and still keep full functionality.
> On Wed, Oct 20, 2010 at 5:16 PM, Carl-Daniel Hailfinger wrote:
>
>> + *
>> + * FIXME: This function needs a parameter which tells it about coalescing
>> + * in relation to the max write length of the programmer and the max write
>> + * length of the chip.
>> + */
>> +static int get_next_write(uint8_t *have, uint8_t *want, int len,
>> + int *first_start, int *first_len,
>> + enum write_granularity gran)
>> +{
>> + int result = 0;
>> + int i, j, limit;
>>
>>
>
> i and j are used for the same purpose in this function -- you can eliminate
> one of them.
>
Done.
> On Wed, Oct 20, 2010 at 5:16 PM, Carl-Daniel Hailfinger wrote
>> + /* The passed in variable might be uninitialized. */
>> + *first_len = 0;
>>
>
> The usage model of first_len and first_start is kind of weird... Perhaps in
> this case it is more straightforward to pass the info between functions
> using a struct. Would it make more sense to create such a struct, alloc it
> in get_next_write, and change the return type of the function to point at
> the struct if write is needed / NULL if not needed? e.g.
>
I think using first_len as return value as suggested above makes most
sense. We'd avoid dealing with a struct and only first_start would be
passed in. Then again, having straightforward
return-values-must-be-returned style would be good as well. Comments?
> struct next_write {
> unsigned short write_needed; /* takes place of "result" flag
> used in get_next_write */
> int start;
> int len;
> }
>
> static struct next_write *get_next_write(uint8_t *have, uint8_t *want, int
> len, enum write_granularity gran)
> {
> struct next_write *next_write;
> ...
> return next_write; /* NULL if we have what we want */
> }
>
> On Wed, Oct 20, 2010 at 5:16 PM, Carl-Daniel Hailfinger <
> c-d.hailfinger.devel.2006 at gmx.net> wrote:
>
>
>> + switch (gran) {
>> + case write_gran_1bit:
>> + case write_gran_1byte:
>> + for (i = 0; i < len; i++) {
>> + if (have[i] != want[i]) {
>> + if (!result) {
>> + /* First location where have and
>> want
>> + * differ.
>> + */
>> + result = 1;
>> + *first_start = i;
>> + }
>> + } else {
>> + if (result) {
>> + /* First location where have and
>> want
>> + * do not differ anymore.
>> + */
>> + *first_len = i - *first_start;
>> + break;
>> + }
>> + }
>> + }
>> + /* Did the loop terminate without setting first_len? */
>> + if (result && ! *first_len)
>> + *first_len = i - *first_start;
>> + break;
>> + case write_gran_256bytes:
>> + for (j = 0; j < len / 256; j++) {
>> + limit = min(256, len - j * 256);
>> + /* Are 'have' and 'want' identical? */
>> + if (memcmp(have + j * 256, want + j * 256, limit))
>> {
>> + if (!result) {
>> + /* First location where have and
>> want
>> + * differ.
>> + */
>> + result = 1;
>> + *first_start = j * 256;
>> + }
>> + } else {
>> + if (result) {
>> + /* First location where have and
>> want
>> + * do not differ anymore.
>> + */
>> + *first_len = j * 256 -
>> *first_start;
>> + break;
>> + }
>> + }
>> + }
>> + /* Did the loop terminate without setting first_len? */
>> + if (result && ! *first_len)
>> + *first_len = min(j * 256 - *first_start, len);
>> + break;
>> + default:
>> + msg_cerr("%s: Unsupported granularity! Please report a bug
>> at "
>> + "flashrom at flashrom.org\n", __func__);
>> + }
>> + return result;
>> +}
>>
>>
>
> The write_gran_1byte and write_gran_256byte cases look practically identical
> -- you are really only changing the granularity, which is a simple
> multiplier. Perhaps you can generalize the write_gran_256byte case to
> include the 1byte case as well by simply replacing the granularity.
>
> switch(gran) {
> case write_gran_1bit:
> case write_gran_1byte:
> stride = 1;
> break;
> case write_gran_256bytes:
> stride = 256;
> break;
> }
>
> for (j = 0; j < len / stride; j++) {
> limit = min(stride, len - j * stride);
> /* Are 'have' and 'want' identical? */
> if (memcmp(have + j * stride, want + j * stride, limit)) { /* same as
> have[i] != want[i] above */
> if (!result) {
> /* First location where have and want
> * differ.
> */
> result = 1;
> *first_start = j * stride;
> }
> } else {
> if (result) {
> /* First location where have and want
> * do not differ anymore.
> */
> *first_len = j * stride - *first_start;
> break;
> }
> }
> }
> /* Did the loop terminate without setting first_len? */
> if (result && ! *first_len)
> *first_len = min(j * stride - *first_start, len);
>
Good point. Changed.
> On Wed, Oct 20, 2010 at 5:16 PM, Carl-Daniel Hailfinger <
> c-d.hailfinger.devel.2006 at gmx.net> wrote:
>
>
>> + /* FIXME: Assume 256 byte granularity for now to play it safe. */
>> + if (need_erase(oldcontents, newcontents, len, gran)) {
>> + msg_cdbg(" E");
>> + ret = erasefn(flash, start, len);
>> + if (ret)
>> + return ret;
>> + /* Erase was successful. Adjust oldcontents. */
>> + memset(oldcontents + start, 0xff, len);
>> + } else
>> + msg_cdbg(" e");
>>
>>
> + while (get_next_write(oldcontents + starthere,
>
>> + newcontents + starthere,
>> + len - starthere, &starthere,
>> + &lenhere, gran)) {
>> + msg_cdbg("W");
>> + /* Needs the partial write function signature. */
>> + ret = flash->write(flash, newcontents + starthere, start +
>> starthere, lenhere);
>> + if (ret)
>> + break;
>> + starthere += lenhere;
>> + }
>> + return ret;
>>
>>
>
> Another minor style nit: IMHO it's best to use braces consistently for both
> if and else portions. So even though you are technically correct, encasing
> the else clause in braces provides a useful visual cue that the logic is
> finished.
>
> Also, it's not clear what the difference is between "E" and "e". If
> !need_erase(), then does that imply that you skipped the region? If so,
> perhaps an "s" is more appropriate.
>
Both points already fixed in a version sent shortly before your review
arrived. We now have E, W, S.
> On Wed, Oct 20, 2010 at 5:16 PM, Carl-Daniel Hailfinger <
> c-d.hailfinger.devel.2006 at gmx.net> wrote:
>
>
>> + oldcontents = (uint8_t *) malloc(size);
>> + /* Assume worst case: All bits are 0. */
>> + memset(oldcontents, 0x00, size);
>> + newcontents = (uint8_t *) malloc(size);
>> + /* Assume best case: All bits should be 1. */
>> + memset(newcontents, 0xff, size);
>> + /* Side effect of the assumptions above: Default write action is
>> erase
>> + * because newcontents looks like a completely erased chip, and
>> + * oldcontents being completely 0x00 means we have to erase
>> everything
>> + * before we can write.
>> + */
>> +
>>
>>
>
> Due to my lack of experience, it seemed to me like the best thing to do is
> assume no action should be taken. That is, set both buffers to 0xff (equal
> and corresponding to an erased chip).
> ..
> ..
> ..
> But then I looked several lines down and saw that erase_it depends on
> newcontents being 0xff and oldcontents complementing it.
>
> Perhaps the thing to do is move the memsets down to the erase_it block. For
> the other stuff, newcontents comes from a file and oldcontents comes from
> the chip.
>
The code flow in doit() is still very much subject to change or rather
replacement. It is made more complicated by the fact that some people
want to avoid the initial read at all costs, and simply run a full erase
first, then write. I plan to revamp doit() shortly to finally get us
libflashrom.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the flashrom
mailing list