[flashrom] [PATCH] Make struct flashchip a field in struct flashctx instead of a complete copy.
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Sun Mar 11 23:11:20 CET 2012
Am 26.02.2012 02:05 schrieb Stefan Tauner:
> This is not tested very thoroughly, but it compiles and writing the dummy works.
> Most of the transformation is quite obvious and painless (well, it was no pleasure,
> so i would rather not rebase this often. :)
> I have introduced dedicated struct flashchip variables where the functions use the
> chip field a lot and some of those (not) introduced variables may be debatable,
> but one gets at least a feeling how the code would look like with this change.
>
> One major FIXME is: the code allocates "the new field" while probing and copies
> the data from the *const* struct flashchip in the flashchips.c array into it.
> We need to free that sometime... it is probably obvious (scope of fill_flash), but i
> have not looked into it yet and this is my reminder.
I have answered that in annotations.
> Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
Good idea. It was too dangerous to do this directly before 0.9.5, but
now that the tree has reopened, I think we can merge such a patch early
in the cycle (now) and have enough time to get it tested.
> diff --git a/cli_classic.c b/cli_classic.c
> index 972043b..4bce7d7 100644
> --- a/cli_classic.c
> +++ b/cli_classic.c
> @@ -166,6 +166,7 @@ int main(int argc, char *argv[])
> const struct flashchip *flash;
> struct flashctx flashes[3];
> struct flashctx *fill_flash;
> + const struct flashchip *chip;
> const char *name;
> int namelen, opt, i, j;
> int startchip = -1, chipcount = 0, option_index = 0, force = 0;
> @@ -441,11 +442,13 @@ int main(int argc, char *argv[])
> }
> }
>
> + fill_flash = &flashes[0];
Can you move this assignment back to where it was?
> + chip = fill_flash->chip;
And move this assignment together with the other one?
> if (chipcount > 1) {
> printf("Multiple flash chips were detected: \"%s\"",
> - flashes[0].name);
> + chip->name);
Can you please use flashes[0].chip->name here to keep the code
consistent? Besides that, do you have any idea why the current code is
so screwed up? Moving a printf for flashes[0] outside a perfectly
working for loop seems odd to me. It would be nice if you could just let
the loop below start at i=0 and remove the chip name part from the
printf above.
> for (i = 1; i < chipcount; i++)
> - printf(", \"%s\"", flashes[i].name);
> + printf(", \"%s\"", flashes[i].chip->name);
> printf("\nPlease specify which chip to use with the "
> "-c <chipname> option.\n");
> ret = 1;
> @@ -464,7 +467,7 @@ int main(int argc, char *argv[])
> /* This loop just counts compatible controllers. */
> for (j = 0; j < registered_programmer_count; j++) {
> pgm = ®istered_programmers[j];
> - if (pgm->buses_supported & flashes[0].bustype)
> + if (pgm->buses_supported & chip->bustype)
flashes[0].chip->bustype please.
Note to self: This code needs to be audited again if it should really
run against flashes[0] here. It looks odd, at least when looking only at
the patch.
> compatible_programmers++;
> }
> if (compatible_programmers > 1)
> @@ -491,19 +494,17 @@ int main(int argc, char *argv[])
> goto out_shutdown;
> } else if (!chip_to_probe) {
> /* repeat for convenience when looking at foreign logs */
> - tempstr = flashbuses_to_text(flashes[0].bustype);
> + tempstr = flashbuses_to_text(chip->bustype);
> msg_gdbg("Found %s flash chip \"%s\" (%d kB, %s).\n",
> - flashes[0].vendor, flashes[0].name,
> - flashes[0].total_size, tempstr);
> + chip->vendor, chip->name,
> + chip->total_size, tempstr);
And the flashes[0].chip dance again.
> free(tempstr);
> }
>
> - fill_flash = &flashes[0];
> -
> - check_chip_supported(fill_flash);
> + check_chip_supported(chip);
>
> - size = fill_flash->total_size * 1024;
> - if (check_max_decode(fill_flash->pgm->buses_supported & fill_flash->bustype, size) &&
> + size = chip->total_size * 1024;
> + if (check_max_decode(fill_flash->pgm->buses_supported & chip->bustype, size) &&
> (!force)) {
> fprintf(stderr, "Chip is too big for this programmer "
> "(-V gives details). Use --force to override.\n");
> diff --git a/flash.h b/flash.h
> index 0dac13d..ed64512 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -87,6 +87,7 @@ enum chipbustype {
> #define FEATURE_WRSR_EITHER (FEATURE_WRSR_EWSR | FEATURE_WRSR_WREN)
>
> struct flashctx;
> +typedef int (erasefunc_t)(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
>
> struct flashchip {
> const char *vendor;
> @@ -135,7 +136,7 @@ struct flashchip {
> } eraseblocks[NUM_ERASEREGIONS];
> /* a block_erase function should try to erase one block of size
> * 'blocklen' at address 'blockaddr' and return 0 on success. */
> - int (*block_erase) (struct flashctx *flash, unsigned int blockaddr, unsigned int blocklen);
> + erasefunc_t *block_erase;
I really really hate that typedef. I wonder if we could keep the code
here and use typeof() instead of the typedef elsewhere.
> } block_erasers[NUM_ERASEFUNCTIONS];
>
> int (*printlock) (struct flashctx *flash);
> @@ -148,35 +149,14 @@ struct flashchip {
> } voltage;
> };
>
> -/* struct flashctx must always contain struct flashchip at the beginning. */
> struct flashctx {
> - const char *vendor;
> - const char *name;
> - enum chipbustype bustype;
> - uint32_t manufacture_id;
> - uint32_t model_id;
> - int total_size;
> - int page_size;
> - int feature_bits;
> - uint32_t tested;
> - int (*probe) (struct flashctx *flash);
> - int probe_timing;
> - struct block_eraser block_erasers[NUM_ERASEFUNCTIONS];
> - int (*printlock) (struct flashctx *flash);
> - int (*unlock) (struct flashctx *flash);
> - int (*write) (struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len);
> - int (*read) (struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len);
> - struct voltage voltage;
> - /* struct flashchip ends here. */
> -
> + struct flashchip *chip;
Yay!
> chipaddr virtual_memory;
> /* Some flash devices have an additional register space. */
> chipaddr virtual_registers;
> struct registered_programmer *pgm;
> };
>
> -typedef int (erasefunc_t)(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
> -
> #define TEST_UNTESTED 0
>
> #define TEST_OK_PROBE (1 << 0)
> diff --git a/flashrom.c b/flashrom.c
> index cad043b..0b6cca3 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -522,7 +522,7 @@ char *extract_programmer_param(const char *param_name)
> }
>
> /* Returns the number of well-defined erasers for a chip. */
> -static unsigned int count_usable_erasers(const struct flashctx *flash)
> +static unsigned int count_usable_erasers(const struct flashchip *flash)
Do we want to count usable erasers for the current programmer (for chips
with erase functions limited to certain buses)? If yes, please keep
struct flashctx.
> {
> unsigned int usable_erasefunctions = 0;
> int k;
> @@ -941,32 +942,38 @@ int check_max_decode(enum chipbustype buses, uint32_t size)
> int probe_flash(struct registered_programmer *pgm, int startchip,
> struct flashctx *fill_flash, int force)
> {
> - const struct flashchip *flash;
> + const struct flashchip *chip;
> unsigned long base = 0;
> char location[64];
> uint32_t size;
> enum chipbustype buses_common;
> char *tmp;
>
> - for (flash = flashchips + startchip; flash && flash->name; flash++) {
> - if (chip_to_probe && strcmp(flash->name, chip_to_probe) != 0)
> + for (chip = flashchips + startchip; chip && chip->name; chip++) {
> + if (chip_to_probe && strcmp(chip->name, chip_to_probe) != 0)
> continue;
> - buses_common = pgm->buses_supported & flash->bustype;
> + buses_common = pgm->buses_supported & chip->bustype;
> if (!buses_common)
> continue;
> msg_gdbg("Probing for %s %s, %d kB: ",
> - flash->vendor, flash->name, flash->total_size);
> - if (!flash->probe && !force) {
> + chip->vendor, chip->name, chip->total_size);
> + if (!chip->probe && !force) {
> msg_gdbg("failed! flashrom has no probe function for "
> "this flash chip.\n");
> continue;
> }
>
> - size = flash->total_size * 1024;
> + size = chip->total_size * 1024;
> check_max_decode(buses_common, size);
>
> /* Start filling in the dynamic data. */
> - memcpy(fill_flash, flash, sizeof(struct flashchip));
> + /* FIXME: when to free? */
Tricky, but I think we are lucky here because there is only one path
which returns a non-match and that's the only path where we want to
free. I have annotated that place a few hunks below.
> + fill_flash->chip = malloc(sizeof(struct flashchip));
> + if (fill_flash->chip == NULL) {
> + msg_gerr("%s: out of memory.\n", __func__);
> + return -1;
Can we return an explicit code for OOM here or would that screw up the
caller?
> + }
> + memcpy(fill_flash->chip, chip, sizeof(struct flashchip));
> fill_flash->pgm = pgm;
>
> base = flashbase ? flashbase : (0xffffffff - size + 1);
> @@ -985,11 +992,11 @@ int probe_flash(struct registered_programmer *pgm, int startchip,
> * one for this programmer interface and thus no other chip has
> * been found on this interface.
> */
> - if (startchip == 0 && fill_flash->model_id == SFDP_DEVICE_ID) {
> + if (startchip == 0 && chip->model_id == SFDP_DEVICE_ID) {
We used fill_flash for consistency (i.e. only use the rw copy of the
flashchip entry) here, and if we want to keep that consistency, it's
fill_flash->chip->model_id.
> msg_cinfo("===\n"
> "SFDP has autodetected a flash chip which is "
> "not natively supported by flashrom yet.\n");
> - if (count_usable_erasers(fill_flash) == 0)
> + if (count_usable_erasers(chip) == 0)
See the count_usable_erasers comment a few hunks above.
> msg_cinfo("The standard operations read and "
> "verify should work, but to support "
> "erase, write and all other "
> @@ -1010,15 +1017,15 @@ int probe_flash(struct registered_programmer *pgm, int startchip,
> }
>
> if (startchip == 0 ||
> - ((fill_flash->model_id != GENERIC_DEVICE_ID) &&
> - (fill_flash->model_id != SFDP_DEVICE_ID)))
> + ((chip->model_id != GENERIC_DEVICE_ID) &&
> + (chip->model_id != SFDP_DEVICE_ID)))
See above for fill_flash->chip->model_id.
> break;
>
> notfound:
> programmer_unmap_flash_region((void *)fill_flash->virtual_memory, size);
> }
>
> - if (!flash || !flash->name)
> + if (!chip || !chip->name)
Here you have to free flash->chip. No other freeing is needed AFAICS.
> return -1;
>
> #if CONFIG_INTERNAL == 1
> @@ -1028,27 +1035,27 @@ notfound:
> #endif
> snprintf(location, sizeof(location), "on %s", programmer_table[programmer].name);
>
> - tmp = flashbuses_to_text(flash->bustype);
> + tmp = flashbuses_to_text(chip->bustype);
fill_flash->chip->...
> msg_cinfo("%s %s flash chip \"%s\" (%d kB, %s) %s.\n",
> - force ? "Assuming" : "Found", fill_flash->vendor,
> - fill_flash->name, fill_flash->total_size, tmp, location);
> + force ? "Assuming" : "Found", chip->vendor,
> + chip->name, chip->total_size, tmp, location);
dito
> free(tmp);
>
> /* Flash registers will not be mapped if the chip was forced. Lock info
> * may be stored in registers, so avoid lock info printing.
> */
> if (!force)
> - if (fill_flash->printlock)
> - fill_flash->printlock(fill_flash);
> + if (chip->printlock)
dito
> + chip->printlock(fill_flash);
dito
>
> /* Return position of matching chip. */
> - return flash - flashchips;
> + return chip - flashchips;
> }
>
> int verify_flash(struct flashctx *flash, uint8_t *buf)
> @@ -1308,7 +1315,7 @@ static int walk_eraseregions(struct flashctx *flash, int erasefunction,
> return 0;
> }
>
> -static int check_block_eraser(const struct flashctx *flash, int k, int log)
> +static int check_block_eraser(const struct flashchip *flash, int k, int log)
No conversion, please. See below.
> {
> struct block_eraser eraser = flash->block_erasers[k];
>
> @@ -1357,7 +1366,7 @@ int erase_and_write_flash(struct flashctx *flash, uint8_t *oldcontents,
> break;
> }
> msg_cdbg("Trying erase function %i... ", k);
> - if (check_block_eraser(flash, k, 1))
> + if (check_block_eraser(chip, k, 1))
This one is debatable. We know that some chips support certain erase
functions only on some buses, e.g. Sharp LHF00L04 which can do a
full-chip erase only if it is accesses in A/A Mux mode, not in FWH mode.
My mid-term plan is to augment erase functions (and possibly also
read/write/probe) with a list of valid buses for any given function.
> continue;
> usable_erasefunctions--;
> ret = walk_eraseregions(flash, k, &erase_and_write_block_helper,
> @@ -1550,14 +1559,6 @@ int selfcheck(void)
> msg_gerr("Flashchips table miscompilation!\n");
> ret = 1;
> }
> - /* Check that virtual_memory in struct flashctx is placed directly
> - * after the members copied from struct flashchip.
> - */
> - if (sizeof(struct flashchip) !=
> - offsetof(struct flashctx, virtual_memory)) {
> - msg_gerr("struct flashctx broken!\n");
> - ret = 1;
> - }
Glad to lose this hunk!
> for (flash = flashchips; flash && flash->name; flash++)
> if (selfcheck_eraseblocks(flash))
> ret = 1;
> @@ -1642,7 +1643,7 @@ void check_chip_supported(const struct flashctx *flash)
> /* FIXME: This function signature needs to be improved once doit() has a better
> * function signature.
> */
> -int chip_safety_check(struct flashctx *flash, int force, int read_it,
> +int chip_safety_check(const struct flashchip *flash, int force, int read_it,
Can you please make sure that all pointers to struct flashchip are
called "chip"? Otherwise grepping for "chip->" won't return all accesses
to struct flashchip. The same comment applies to other hunks of the
patch as well.
However, chip_safety_check can not be converted to a different calling
convention because we need struct flashctx here. Admittedly we don't
need it right now, but we will need it once programmer_may_write is part
of the programmer struct in struct flashctx.
> int write_it, int erase_it, int verify_it)
> {
> if (!programmer_may_write && (write_it || erase_it)) {
> @@ -1710,9 +1711,10 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
> uint8_t *oldcontents;
> uint8_t *newcontents;
> int ret = 0;
> - unsigned long size = flash->total_size * 1024;
> + const struct flashchip *chip = flash->chip;
> + unsigned long size = chip->total_size * 1024;
>
> - if (chip_safety_check(flash, force, read_it, write_it, erase_it, verify_it)) {
> + if (chip_safety_check(chip, force, read_it, write_it, erase_it, verify_it)) {
No conversion, please. See above.
> msg_cerr("Aborting.\n");
> ret = 1;
> goto out_nofree;
> @@ -1791,7 +1793,7 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
>
> // This should be moved into each flash part's code to do it
> // cleanly. This does the job.
> - handle_romentries(flash, oldcontents, newcontents);
> + handle_romentries(chip, oldcontents, newcontents);
No conversion, please. See two hunks below.
>
> // ////////////////////////////////////////////////////////////
>
> diff --git a/jedec.c b/jedec.c
> index 69c0c0c..5c549a2 100644
> --- a/jedec.c
> +++ b/jedec.c
> @@ -93,7 +93,7 @@ void data_polling_jedec(const struct flashctx *flash, chipaddr dst,
> msg_cdbg("%s: excessive loops, i=0x%x\n", __func__, i);
> }
>
> -static unsigned int getaddrmask(struct flashctx *flash)
> +static unsigned int getaddrmask(const struct flashchip *flash)
Hm yes, const makes sense. In fact, const makes sense in many more places.
> {
> switch (flash->feature_bits & FEATURE_ADDR_MASK) {
> case FEATURE_ADDR_FULL:
> diff --git a/layout.c b/layout.c
> index 90d3cce..f0f7a6f 100644
> --- a/layout.c
> +++ b/layout.c
> @@ -302,7 +302,7 @@ romlayout_t *get_next_included_romentry(unsigned int start)
> return best_entry;
> }
>
> -int handle_romentries(struct flashctx *flash, uint8_t *oldcontents, uint8_t *newcontents)
> +int handle_romentries(const struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents)
Will we have to change this back once we try to merge the layout
patches? In that case, we need some way to get permissible regions from
the programmer, and that would probably be done via the programmer struct.
> {
> unsigned int start = 0;
> romlayout_t *entry;
> diff --git a/spi25.c b/spi25.c
> index b7e8189..2accb6d 100644
> --- a/spi25.c
> +++ b/spi25.c
> @@ -416,22 +416,23 @@ void spi_prettyprint_status_register_sst25vf040b(uint8_t status)
>
> int spi_prettyprint_status_register(struct flashctx *flash)
> {
> + const struct flashchip *chip = flash->chip;
const struct flashchip here, but just struct flashchip in the next hunk.
What's the reason? Same question applies to other hunks of the patch as
well.
> uint8_t status;
>
> status = spi_read_status_register(flash);
> msg_cdbg("Chip status register is %02x\n", status);
> - switch (flash->manufacture_id) {
> + switch (chip->manufacture_id) {
> case ST_ID:
> - if (((flash->model_id & 0xff00) == 0x2000) ||
> - ((flash->model_id & 0xff00) == 0x2500))
> + if (((chip->model_id & 0xff00) == 0x2000) ||
> + ((chip->model_id & 0xff00) == 0x2500))
> spi_prettyprint_status_register_st_m25p(status);
> break;
> case MACRONIX_ID:
> - if ((flash->model_id & 0xff00) == 0x2000)
> + if ((chip->model_id & 0xff00) == 0x2000)
> spi_prettyprint_status_register_st_m25p(status);
> break;
> case SST_ID:
> - switch (flash->model_id) {
> + switch (chip->model_id) {
> case 0x2541:
> spi_prettyprint_status_register_sst25vf016(status);
> break;
> @@ -862,16 +863,17 @@ static int spi_write_status_register_wren(struct flashctx *flash, int status)
>
> int spi_write_status_register(struct flashctx *flash, int status)
> {
> + struct flashchip *chip = flash->chip;
> int ret = 1;
>
> - if (!(flash->feature_bits & (FEATURE_WRSR_WREN | FEATURE_WRSR_EWSR))) {
> + if (!(chip->feature_bits & (FEATURE_WRSR_WREN | FEATURE_WRSR_EWSR))) {
> msg_cdbg("Missing status register write definition, assuming "
> "EWSR is needed\n");
> - flash->feature_bits |= FEATURE_WRSR_EWSR;
> + chip->feature_bits |= FEATURE_WRSR_EWSR;
> }
> - if (flash->feature_bits & FEATURE_WRSR_WREN)
> + if (chip->feature_bits & FEATURE_WRSR_WREN)
> ret = spi_write_status_register_wren(flash, status);
> - if (ret && (flash->feature_bits & FEATURE_WRSR_EWSR))
> + if (ret && (chip->feature_bits & FEATURE_WRSR_EWSR))
> ret = spi_write_status_register_ewsr(flash, status);
> return ret;
> }
I think the next iteration of this patch can go in.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the flashrom
mailing list