[flashrom] [PATCH] Real partial writes
David Hendricks
dhendrix at google.com
Thu Oct 21 06:50:35 CEST 2010
The code looks good overall, though I haven't had a chance to test it
extensively yet. Thanks for doing this!
Comments below are mostly minor nits.
On Wed, Oct 20, 2010 at 5:16 PM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:
> On 21.10.2010 02:08, Carl-Daniel Hailfinger wrote:
> > I suspect that there are more bugs in the code than I'd like. Please try
> > the patch below which has more debug output.
> >
>
> need_erase used an incorrect offset calculation in the
> write_gran_256bytes case.
>
> This one may even work. Please make sure to test with an image generated
> with a random number generator (like /dev/urandom or similar).
>
> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
>
> 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)
> @@ -793,6 +793,7 @@
> * Check if the buffer @have can be programmed to the content of @want
> without
> * erasing. This is only possible if all chunks of size @gran are either
> kept
> * as-is or changed from an all-ones state to any other state.
> + *
> * The following write granularities (enum @gran) are known:
> * - 1 bit. Each bit can be cleared individually.
> * - 1 byte. A byte can be written once. Further writes to an already
> written
> @@ -803,10 +804,12 @@
> * this function.
> * - 256 bytes. If less than 256 bytes are written, the contents of the
> * unwritten bytes are 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 verified area
> + * @len length of the checked area
> * @gran write granularity (enum, not count)
> * @return 0 if no erase is needed, 1 otherwise
> */
> @@ -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? (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 @@
> break;
> }
> break;
> + default:
> + msg_cerr("%s: Unsupported granularity! Please report a bug
> at "
> + "flashrom at flashrom.org\n", __func__);
> }
> 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.
On Wed, Oct 20, 2010 at 5:16 PM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> 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.
On Wed, Oct 20, 2010 at 5:16 PM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> 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.
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);
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.
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.
--
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20101020/79ca8414/attachment.html>
More information about the flashrom
mailing list