Re: [flashrom] [PATCH] Make struct flashchip a field in struct flashctx instead of a complete copy.

[Stefan Tauner]

On Sun, 11 Mar 2012 23:11:20 +0100
Carl-Daniel Hailfinger  wrote:
> 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
>
> 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.

sorry for not touching it so long, but it is one of the *cough* less
exciting patches :)
the free() problem still needs to be addressed before this can be
merged, please see below.
i had to do parts of this twice because i made a mistake that not even
git was able to prevent... so the patch content may not be 100%
reflected by this mail and vice versa, sorry.
> > 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?

done, although i am not sure i really like it...
> > 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?

done against my will. one day i will create a patch that fixes probing
and persuade you... but not today :)
> 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.

due to ", " printing! the first chip does not have one in front.
alternative would be to have the last one as special case. left it
unchanged for now.
> > 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 = &registered_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.

i think it is ok... this is the --force path where multiple chips have
been discovered. we want to just use the first one in that case i
think...
> > 			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.

done (see above).
> > 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.

dunno. i like it :)
if you care, please look into it yourself and tell me what i should
change and where, or better add a patch on top of this one please.
> > 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.

changed back to flashctx
> > @@ -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.

see there...
> > 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?

we can. i would rather do this later, when we have defined the global
error codes. there are lots of other places like this anyway that need
changes after the defines are fixed.
-> unchanged
> > }
> > 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.

after changing it everywhere you noted, it is makes sense. it would
probably have been easier for *both of us* if you would have changed it
yourself though. while this weakens the "capsulation" between the
reviewer and the author i think it would often be way better and faster.
> > 	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.

well yes i have to free it here, but if the function succeeds the
reference escapes inside the function's input parameter struct flashctx
*fill_flash.

i have not thought it through, but maybe we should free it before
mallocing it?
> > 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->...
> […]

done
> > {
> >    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.

reverted
> > 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.

hopefully done, this now includes print.c too.
> 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.

reverted
> > {
> >    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.

with the current implementation this is not needed, but you are right
that his would definitely go into the context and not the flash chip
(although i dont think it will be in the programmer... i think our
region solution will have its own entry in the context because it is
not bound to either the programmer or the flash chip, but both (and
more (i.e. the user!))).
> > {
> >    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.
> […]

added const almost everywhere. const flashctx would also make a lot
of sense on many occasions, but this patch is too big already and i am
fed up already :)