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

[Carl-Daniel Hailfinger]

Am 19.08.2012 03:29 schrieb 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

Thanks for the hint. We'll take your version.

 
>>  	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. :)

Hm yes, I should have mentioned forced reads in that comment.

 
>> @@ -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!

Thanks!


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

Right.

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

Hm... while I think that we shouldn't introduce new exit(1), I agree
that this case needs careful audit, and the patch is already complicated
enough as is.


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

You mean having both related checks on one line? Or do you mean using
chip-> instead of flash->chip-> ?

 
>>  			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 :)

_Had_ issues. I really like the new code, because it not only
frees/unmaps stuff, it also leaves no dangling pointers around.


> 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?

Yes. The old !flash check would only have triggered in very odd
circumstances (empty array in flashchips.c), and !flash->name was the
detection for end-of-array (last array member was zeroed).
The new !flash->chip check is way more readable and doesn't rely on
implicit properties of an array in another file.


How do we proceed? Should I repost a fixed version of my patch so you
can merge yours and mine? Can you do the missing dediprog conversion
locally or do you want to take mine?


Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/