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@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@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 andwant
* differ.*/result = 1;*first_start = i;}} else {if (result) {/* First location where have andwant
* 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 andwant
* differ.*/result = 1;*first_start = j * 256;}} else {if (result) {/* First location where have andwant
* 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 bugat "
"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);
Good point. Changed.
On Wed, Oct 20, 2010 at 5:16 PM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@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);} elsemsg_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@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 iserase
* because newcontents looks like a completely erased chip, and* oldcontents being completely 0x00 means we have to eraseeverything
* 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