On 03.11.2010 23:44, Michael Karcher wrote: > Am Mittwoch, den 03.11.2010, 03:51 +0100 schrieb Carl-Daniel Hailfinger: > >> Signed-off-by: Carl-Daniel Hailfinger <[email protected]> >> >> diff -u flashrom-partial_write_rolling_erase_write/flashrom.c >> flashrom-partial_write_rolling_erase_write_cleanup/flashrom.c >> --- flashrom-partial_write_rolling_erase_write/flashrom.c (Arbeitskopie) >> +++ flashrom-partial_write_rolling_erase_write_cleanup/flashrom.c >> (Arbeitskopie) >> @@ -878,14 +878,11 @@ >> * 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 *first_start, enum write_granularity gran) >> { >> - int result = 0, rel_start = 0; >> + int result = 0, rel_start = 0, first_len = 0; >> > As "result" no longer is the return value, please rename the variable. > Something like "write_needed" would make sense, but... > > >> @@ -903,14 +900,14 @@ >> /* First location where have and want >> * do not differ anymore. >> */ >> - *first_len = i - rel_start; >> + first_len = i - rel_start; >> break; >> } >> } >> } >> /* Did the loop terminate without setting first_len? */ >> - if (result && ! *first_len) >> - *first_len = i - rel_start; >> + if (result && ! first_len) >> + first_len = i - rel_start; >> > ... you could also simplify this stuff by having a variable > "in_write_region" instead of result, which you clear in the block where > you set "first_len", and then the "if" would just need to check the > "in_write_region" flag instead of combining two things. > > Or, wait a moment! Do you see some similarity between the assignment in > the if and the assignment before the break? Yes! The expressions are > identical. So: remove the first_len assignment before the break, you > also don't need to clear a flag in that block, just break. And then you > do the first_len assignment if the variable till now known as result is > set. The logic is that the loop always terminates at the end of the > region to write. Which might either be because of the loop end condition > or the matching compare - who cares, actually. >
I actually had that cleanup included in an earlier partial write patch, but decided to scratch it because I was still searching for the bug in the partial write patch back then. And once the partial write patch started to work for everyone, I was hesitant to reintroduce the cleanup and thus invalidate all tests. Now that partial write is merged and we know it works, I may as well merge that cleanup again. > No Ack yet as I want to see the result if incorporating my comments into > this function before. > > >> +out: >> + free(oldcontents); >> + free(newcontents); >> +out_nofree: >> programmer_shutdown(); >> > if you initialize oldcontents and newcontents with NULL, free is > guaranteed by ANSI C to be a no-op, so you might get rid of the > out_nofree label. Whether one wants to make use of that feature or > considers not calling free at all if there was no malloc is a matter of > taste. > That's a good point. I believe we don't handle this consistently in flashrom. The Linux kernel usually uses out: and out_free: labels, and IIRC that applies even to cases where free() would be harmless, but it's been a while since I read huge chunks of kernel code, so that may have changed. Regards, Carl-Daniel Avoid two memory leaks in doit() which were unproblematic for flashrom because flashrom terminates after finishing doit(). Rename oldcontents to curconents in erase_and_write_block_helper(). Unify the code for all granularities in get_next_write(). Return write length from get_next_write() instead of filling it as referenced parameter. Thanks to Michael Karcher for pointing out the first two issues. Thanks to David Hendricks for pointing out the third issue and suggesting a way to unify that code. Signed-off-by: Carl-Daniel Hailfinger <[email protected]> Index: flashrom-partial_write_rolling_erase_write_cleanup/flashrom.c =================================================================== --- flashrom-partial_write_rolling_erase_write_cleanup/flashrom.c (Revision 1224) +++ flashrom-partial_write_rolling_erase_write_cleanup/flashrom.c (Arbeitskopie) @@ -869,81 +869,62 @@ * @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 + * @first_start offset of the first byte which needs to be written (passed in + * value is increased by the offset of the first needed write + * relative to have/want or unchanged if no write is needed) + * @return length of the first contiguous area which needs to be written + * 0 if no write is needed * * 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 *first_start, enum write_granularity gran) { - int result = 0, rel_start = 0; - int i, limit; + int need_write = 0, rel_start = 0, first_len = 0; + int i, limit, stride; - /* The passed in variable might be uninitialized. */ - *first_len = 0; 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; - rel_start = i; - } - } else { - if (result) { - /* First location where have and want - * do not differ anymore. - */ - *first_len = i - rel_start; - break; - } - } - } - /* Did the loop terminate without setting first_len? */ - if (result && ! *first_len) - *first_len = i - rel_start; + stride = 1; break; case write_gran_256bytes: - for (i = 0; i < len / 256; i++) { - limit = min(256, len - i * 256); - /* Are 'have' and 'want' identical? */ - if (memcmp(have + i * 256, want + i * 256, limit)) { - if (!result) { - /* First location where have and want - * differ. - */ - result = 1; - rel_start = i * 256; - } - } else { - if (result) { - /* First location where have and want - * do not differ anymore. - */ - *first_len = i * 256 - rel_start; - break; - } - } - } - /* Did the loop terminate without setting first_len? */ - if (result && ! *first_len) - *first_len = min(i * 256 - rel_start, len); + stride = 256; break; default: msg_cerr("%s: Unsupported granularity! Please report a bug at " "[email protected]\n", __func__); + /* Claim that no write was needed. A write with unknown + * granularity is too dangerous to try. + */ + return 0; } + for (i = 0; i < len / stride; i++) { + limit = min(stride, len - i * stride); + /* Are 'have' and 'want' identical? */ + if (memcmp(have + i * stride, want + i * stride, limit)) { + if (!need_write) { + /* First location where have and want differ. */ + need_write = 1; + rel_start = i * stride; + } + } else { + if (need_write) { + /* First location where have and want + * do not differ anymore. + */ + first_len = i * stride - rel_start; + break; + } + } + } + /* Did the loop terminate without setting first_len? */ + if (need_write && ! first_len) + first_len = min(i * stride - rel_start, len); *first_start += rel_start; - return result; + return first_len; } /* This function generates various test patterns useful for testing controller @@ -1370,7 +1351,7 @@ static int erase_and_write_block_helper(struct flashchip *flash, unsigned int start, unsigned int len, - uint8_t *oldcontents, + uint8_t *curcontents, uint8_t *newcontents, int (*erasefn) (struct flashchip *flash, unsigned int addr, @@ -1383,26 +1364,26 @@ int writecount = 0; enum write_granularity gran = write_gran_256bytes; /* FIXME */ - /* oldcontents and newcontents are opaque to walk_eraseregions, and + /* curcontents and newcontents are opaque to walk_eraseregions, and * need to be adjusted here to keep the impression of proper abstraction */ - oldcontents += start; + curcontents += start; newcontents += start; msg_cdbg(":"); /* FIXME: Assume 256 byte granularity for now to play it safe. */ - if (need_erase(oldcontents, newcontents, len, gran)) { + if (need_erase(curcontents, newcontents, len, gran)) { msg_cdbg("E"); ret = erasefn(flash, start, len); if (ret) return ret; - /* Erase was successful. Adjust oldcontents. */ - memset(oldcontents, 0xff, len); + /* Erase was successful. Adjust curcontents. */ + memset(curcontents, 0xff, len); skip = 0; } - while (get_next_write(oldcontents + starthere, - newcontents + starthere, - len - starthere, &starthere, - &lenhere, gran)) { + /* get_next_write() sets starthere to a new value after the call. */ + while ((lenhere = get_next_write(curcontents + starthere, + newcontents + starthere, + len - starthere, &starthere, gran))) { if (!writecount++) msg_cdbg("W"); /* Needs the partial write function signature. */ @@ -1796,8 +1777,8 @@ if (chip_safety_check(flash, force, filename, read_it, write_it, erase_it, verify_it)) { msg_cerr("Aborting.\n"); - programmer_shutdown(); - return 1; + ret = 1; + goto out_nofree; } /* Given the existence of read locks, we want to unlock for read, @@ -1808,8 +1789,7 @@ if (read_it) { ret = read_flash_to_file(flash, filename); - programmer_shutdown(); - return ret; + goto out_nofree; } oldcontents = (uint8_t *) malloc(size); @@ -1835,14 +1815,13 @@ emergency_help_message(); ret = 1; } - programmer_shutdown(); - return ret; + goto out; } if (write_it || verify_it) { if (read_buf_from_file(newcontents, size, filename)) { - programmer_shutdown(); - return 1; + ret = 1; + goto out; } #if CONFIG_INTERNAL == 1 @@ -1859,8 +1838,8 @@ */ msg_cdbg("Reading old flash chip contents...\n"); if (flash->read(flash, oldcontents, 0, size)) { - programmer_shutdown(); - return 1; + ret = 1; + goto out; } // This should be moved into each flash part's code to do it @@ -1878,13 +1857,13 @@ msg_cinfo("Good. It seems nothing was " "changed.\n"); nonfatal_help_message(); - programmer_shutdown(); - return 1; + ret = 1; + goto out; } } emergency_help_message(); - programmer_shutdown(); - return 1; + ret = 1; + goto out; } } @@ -1900,7 +1879,10 @@ emergency_help_message(); } +out: + free(oldcontents); + free(newcontents); +out_nofree: programmer_shutdown(); - return ret; } -- http://www.hailfinger.org/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
