[flashrom] [PATCH] Partial write cleanups
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Thu Nov 4 05:20:26 CET 2010
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 <c-d.hailfinger.devel.2006 at gmx.net>
>>
>> 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 <c-d.hailfinger.devel.2006 at gmx.net>
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 "
"flashrom at flashrom.org\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/
More information about the flashrom
mailing list