Patchwork Paranoid chip test infrastructure, part 1

login
register
about
Submitter Carl-Daniel Hailfinger
Date 2010-10-15 15:18:06
Message ID <4CB870AE.6010502@gmx.net>
Download mbox | patch
Permalink /patch/2116/
State Bitrotted
Headers show

Comments

Carl-Daniel Hailfinger - 2010-10-15 15:18:06
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).

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 <c-d.hailfinger.devel.2006@gmx.net>
Stefan Reinauer - 2010-10-18 23:04:27
* Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> [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 <c-d.hailfinger.devel.2006@gmx.net>

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 <stepan@coreboot.org>

Patch

Index: flashrom-test_chip_infrastructure/flash.h
===================================================================
--- flashrom-test_chip_infrastructure/flash.h	(Revision 1212)
+++ flashrom-test_chip_infrastructure/flash.h	(Arbeitskopie)
@@ -37,6 +37,8 @@ 
 
 /* Error codes */
 #define TIMEOUT_ERROR	-101
+#define VERIFY_ERROR	-201
+#define COMMAND_ERROR	-202
 
 typedef unsigned long chipaddr;
 
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.
+ */
+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;
+	}
+	/* Check integrity of previously written range. */
+	if (verify_range(flash, newcontents, 0, start, NULL)) {
+		msg_cerr("Error in previously written range 0x%x-0x%x\n",
+			 0, start - 1);
+		return VERIFY_ERROR;
+	}
+	/* Check integrity of not yet written range. */
+	if (verify_range(flash, oldcontents + start + len, start + len,
+			 flash->total_size * 1024 - start - len, NULL)) {
+		msg_cerr("Error in not yet written range 0x%x-0x%x\n",
+			 start + len, flash->total_size * 1024 - 1);
+		return VERIFY_ERROR;
+	}
+	/* Check if the write function fails. */
+	if (flash->write(flash, newcontents + start, start, len)) {
+		msg_cerr("Error during write in range 0x%x-0x%x\n",
+			 start, start + len - 1);
+		return COMMAND_ERROR;
+	}
+	/* Check integrity of just written range. */
+	if (verify_range(flash, newcontents + start, start, len, NULL)) {
+		msg_cerr("Error in just written range 0x%x-0x%x\n",
+			 start, start + len - 1);
+		return VERIFY_ERROR;
+	}
+	/* Recheck integrity of previously written range. */
+	if (verify_range(flash, newcontents, 0, start, NULL)) {
+		msg_cerr("Error in previously written range 0x%x-0x%x\n",
+			 0, start - 1);
+		return VERIFY_ERROR;
+	}
+	/* Recheck integrity of not yet written range. */
+	if (verify_range(flash, oldcontents + start + len, start + len,
+			 flash->total_size * 1024 - start - len, NULL)) {
+		msg_cerr("Error in not yet written range 0x%x-0x%x\n",
+			 start + len, flash->total_size * 1024 - 1);
+		return VERIFY_ERROR;
+	}
+	return 0;
+}
+
 static int walk_eraseregions(struct flashchip *flash, int erasefunction,
 			     int (*do_something) (struct flashchip *flash,
 						  unsigned int addr,