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@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@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
*/
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@gmx.net> wrote:
@@ -846,10 +849,102 @@
break;
}
break;
+ default:
+ msg_cerr("%s: Unsupported granularity! Please report a bug at "
+ "
flashrom@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.
+ *
+ * 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.
+ /* 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 */
}
+ 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@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);
+ /* 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.
+ 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.