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

[Stefan Tauner]

On Sun, 19 Aug 2012 01:51:16 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Counter-proposal (conversion from scratch). AFAICS the only differences
> to your version regarding code flow are in cli_classic.c and flashrom.c.

this is not a full review, i just looked at flashrom.c and cli_classic.c

> Index: flashrom-flashctx_separate_struct_flashchip/cli_classic.c
> ===================================================================
> --- flashrom-flashctx_separate_struct_flashchip/cli_classic.c	(Revision 1576)
> +++ flashrom-flashctx_separate_struct_flashchip/cli_classic.c	(Arbeitskopie)
> @@ -109,8 +109,8 @@
>  {
>  	unsigned long size;
>  	/* Probe for up to three flash chips. */
> -	const struct flashchip *flash;
> -	struct flashctx flashes[3];
> +	const struct flashchip *chip;
> +	struct flashctx flashes[3] = {};

this is not standard C afaik, but gcc does not accept (the standard) {0}
without warning either, hence my {{0}} suggestion.
http://stackoverflow.com/questions/1352370/c-static-array-initialization-how-verbose-do-i-need-to-be/1352379#1352379

>  	struct flashctx *fill_flash;
>  	const char *name;
>  	int namelen, opt, i, j;
> @@ -389,17 +389,16 @@
>  	}
>  	/* Does a chip with the requested name exist in the flashchips array? */
>  	if (chip_to_probe) {
> -		for (flash = flashchips; flash && flash->name; flash++)
> -			if (!strcmp(flash->name, chip_to_probe))
> +		for (chip = flashchips; chip && chip->name; chip++)
> +			if (!strcmp(chip->name, chip_to_probe))
>  				break;
> -		if (!flash || !flash->name) {
> +		if (!chip || !chip->name) {
>  			msg_cerr("Error: Unknown chip '%s' specified.\n", chip_to_probe);
>  			msg_gerr("Run flashrom -L to view the hardware supported in this flashrom version.\n");
>  			ret = 1;
>  			goto out;
>  		}
> -		/* Clean up after the check. */
> -		flash = NULL;
> +		/* Keep chip around for later usage. */
>  	}

nasty hack for the evil hack named forced reads. :)

> @@ -456,9 +452,15 @@
>  			/* 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)
> +				/* chip is still set from the chip_to_probe earlier in this function. */
> +				if (pgm->buses_supported & chip->bustype)
>  					compatible_programmers++;
>  			}
> +			if (!compatible_programmers) {
> +				msg_cinfo("No compatible controller found for the requested flash chip.\n");
> +				ret = 1;
> +				goto out_shutdown;
> +			}

good catch!

> Index: flashrom-flashctx_separate_struct_flashchip/flashrom.c
> ===================================================================
> --- flashrom-flashctx_separate_struct_flashchip/flashrom.c	(Revision 1576)
> +++ flashrom-flashctx_separate_struct_flashchip/flashrom.c	(Arbeitskopie)
> @@ -950,44 +950,52 @@
>  	return 1;
>  }
>  
> -int probe_flash(struct registered_programmer *pgm, int startchip,
> -		struct flashctx *fill_flash, int force)
> +int probe_flash(struct registered_programmer *pgm, int startchip, struct flashctx *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);

this can be combined with the line before easily.

> +		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);

hm.. return value of check_max_decode is ignored here, and checked
later in main()... quite odd, but not related to flashctx.

>  
>  		/* Start filling in the dynamic data. */
> -		memcpy(fill_flash, flash, sizeof(struct flashchip));
> -		fill_flash->pgm = pgm;
> +		flash->chip = calloc(1, sizeof(struct flashchip));

right. my patch has the potential to explode spectacularly due to a
normal malloc here and the code expecting 0 to recognize end of lists
etc :/

> +		if (!flash->chip) {
> +			msg_gerr("Out of memory!\n");
> +			// FIXME: Is -1 the right return code?
> +			return -1;

hm... well it breaks the outer loop, but there is no real error handling
there. better add an exit(1) here (yes, really :) for now and fix it
when the error definitions are merged (which has rather high priority
for me). just returning with the current code would postpone the
explosion to doit(). NB: i havent checked that too thoroughly.

> +		}
> +		memcpy(flash->chip, chip, sizeof(struct flashchip));
> +		flash->pgm = pgm;
>  
>  		base = flashbase ? flashbase : (0xffffffff - size + 1);
> -		fill_flash->virtual_memory = (chipaddr)programmer_map_flash_region("flash chip", base, size);
> +		flash->virtual_memory = (chipaddr)programmer_map_flash_region("flash chip", base, size);
>  
> +		/* We handle a forced match like a real match, we just avoid probing. Note that probe_flash()
> +		 * is only called with force=1 after normal probing failed.
> +		 */

yay more comments in the hard parts of flashrom :)

>  		if (force)
>  			break;
>  
> -		if (fill_flash->probe(fill_flash) != 1)
> +		if (flash->chip->probe(flash) != 1)
>  			goto notfound;
>  
>  		/* If this is the first chip found, accept it.
> @@ -1022,15 +1030,18 @@
>  		}
>  
>  		if (startchip == 0 ||
> -		    ((fill_flash->model_id != GENERIC_DEVICE_ID) &&
> -		     (fill_flash->model_id != SFDP_DEVICE_ID)))
> +		    ((flash->chip->model_id != GENERIC_DEVICE_ID) &&
> +		     (flash->chip->model_id != SFDP_DEVICE_ID)))

i like that bit better in my patch, because it groups together what
belongs together.

>  			break;
>  
>  notfound:
> -		programmer_unmap_flash_region((void *)fill_flash->virtual_memory, size);
> +		programmer_unmap_flash_region((void *)flash->virtual_memory, size);
> +		flash->virtual_memory = (chipaddr)NULL;
> +		free(flash->chip);
> +		flash->chip = NULL;

pretty clear indication that this function has issues imho :)
those lines are a candidate to form a function flashctx_init or
something like that, if we need this more often (i really hope we dont.)

>  	}
>  
> -	if (!flash || !flash->name)
> +	if (!flash->chip)
>  		return -1;

i wondered too why that ->name check was there, any ideas?

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner