Re: [flashrom] [PATCH] Paranoid chip test infrastructure, part 1

[Stefan Reinauer]

* Carl-Daniel Hailfinger  [101015 17:18]:
> Add a function which checks erase and write operations and makes sure
> that neither touch anything outside the desired region.
> Runtime is O(n^2).

What's n in this case? While in theory it would not matter, in practice
n being the number of bytes of flash would have significantly more
impact than n being the number of erase blocks of the chip, or blocks
to actually write.
> A less paranoid version can have O(n) runtime, but I think it is
> valuable to have the paranoid option.
>
> Signed-off-by: Carl-Daniel Hailfinger

It's very unclear from this patch what the impact on flashrom would be
later on as it just adds dead, untestable code. ;-)

What's the use case? Since you mention test patterns in the code below
the implication is that this code is only used to verify whether a
flash chip driver is consistent, so it would be something that a normal
user does not have to suffer from.
> Index: flashrom-test_chip_infrastructure/flashrom.c
>
> --- flashrom-test_chip_infrastructure/flashrom.c	(Revision 1212)
> +++ flashrom-test_chip_infrastructure/flashrom.c	(Arbeitskopie)
> @@ -1271,6 +1271,65 @@
>    return ret;
>  }
>
> +/* This function expects the previous flash contents in oldcontents, and the
> wanted flash contents in newcontents. Usually both are test patterns, and
> they should be distinct. The region from start to start+len-1 is expected
> to be already erased.
> */

Not for this patch, but have you considered using doxygen style
comments?
> +int autotest_write_eraseregion_paranoid(struct flashchip *flash,
> 			unsigned int start, unsigned int len,
> 			uint8_t *oldcontents,
> 			uint8_t *newcontents)
>
> +{
> /* Checking integrity of just erased range is not needed, the erase
> * function will do that itself. Do it anyway for nicer diagnostics.
> */
> if (check_erased_range(flash, start, len)) {
> msg_cerr("Error in just erased range 0x%x-0x%x\n",
> 	 start, start + len - 1);
> return VERIFY_ERROR;
> }

Is there a better way to have nicer diagnostics? If this is only used
for driver verification I guess it does not matter. Otherwise it would
be really ugly.

So, in the case that this is driver verification code only:

Acked-by: Stefan Reinauer