[flashrom] [PATCH] Fix parallel-style programmer access from SPI

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Thu Mar 1 22:26:03 CET 2012


Hi,

this is the same patch, but without the runtime sanity checks.

Am 01.03.2012 20:15 schrieb Michael Karcher:
> Am Donnerstag, den 01.03.2012, 01:42 +0100 schrieb Carl-Daniel
> Hailfinger:
>> RFC: Are those checks with abort a good idea or do they break stuff?
>> exit(1) is not a nice thing for libflashrom, but then again, it's
>> probably better than exploding.
> I don't really feel like checking that at runtime is a good idea, but
> until someone proposes a not too hacky plan how to check that at compile
> time, this definitely has some merits, so it won't be a reason to not
> ack for me.
>
>> I'm also unsure about the parameter order of mmio_readn.
> Yeah, that one is tricky, as it does not match memcpy, which might be
> surprising. OTOH, the "read" in the function name implies that the order
> might be different, as you really "read" what is pointed to by the first
> argument. So this one is OK in some sense.
> Something up to discussion is the length parameter. Traditionally in C,
> the length is passed immediately after the pointer it applies to. Now
> that still does not help us, as the source and the target buffer have
> the same size for obvious reason, so attaching the size to either buffer
> is OK ("read a block of $len bytes at physical address $addr and store
> it in $buf" vs. "read data from physical address $addr and fill the
> buffer $buf of size $len with it). Finally, the length can also be seen
> as detached from either buffer: "Copy $len bytes, with the source being
> at physical address $addr and the destination being at $buf", which
> would also suggest putting size last to me. So for me
>
>   read(src, size, dest)
>   read(src, dest, size)
>
> both make sense in that regard. The variant you use now (the second) has
> some extra bonus in being semantically equivalent to the read system
> call. This semantic equivalence is not a silver bullet, though, as our
> write functions don't model the "file handle first" order of the system
> calls (with the physical address corresponding to the physical address.
>
> So while I can't really push you into either direction, maybe you prefer
> one of the ways to decide on a order that much that you can decide
> yourself.

OK then, it stays like it is in the first version of the patch.

>> The current IT87 SPI code did use the parallel programmer interface for
>> memory mapped reads and writes, but that's the wrong abstraction. It has
>> been fixed to use mmio_read*/mmio_write* for that purpose.
> I like that.
>
>> Introduce sanity checks for all SPI/Parallel-style accesses before a
>> possibly undefined union member is dereferenced.
> See above.
>
>> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
> Acked-by: Michael Karcher <flashrom at mkarcher.dialup.fu-berlin.de>

Thanks for the review!

The IT87 SPI driver has one quirk to speed up reading and writing: If a
flash chip is 512 kByte or less, the flash chip can be completely mapped
in memory and both read and write accesses are faster that way.

The current IT87 SPI code did use the parallel programmer interface for
memory mapped reads and writes, but that's the wrong abstraction. It has
been fixed to use mmio_read*/mmio_write* for that purpose.
Switch internal_chip_readn to use mmio_readn as proper abstraction.

Kudos to Michael Karcher for spotting the bug.


Reported-by: Johan Svensson <flashrom.js at crypt.se>
Tested-by: Johan Svensson <flashrom.js at crypt.se>

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
Acked-by: Michael Karcher <flashrom at mkarcher.dialup.fu-berlin.de>

Index: flashrom-fix_par_programmer_access_from_spi/hwaccess.c
===================================================================
--- flashrom-fix_par_programmer_access_from_spi/hwaccess.c	(Revision 1510)
+++ flashrom-fix_par_programmer_access_from_spi/hwaccess.c	(Arbeitskopie)
@@ -169,6 +169,12 @@
 	return *(volatile uint32_t *) addr;
 }
 
+void mmio_readn(void *addr, uint8_t *buf, size_t len)
+{
+	memcpy(buf, addr, len);
+	return;
+}
+
 void mmio_le_writeb(uint8_t val, void *addr)
 {
 	mmio_writeb(cpu_to_le8(val), addr);
Index: flashrom-fix_par_programmer_access_from_spi/it87spi.c
===================================================================
--- flashrom-fix_par_programmer_access_from_spi/it87spi.c	(Revision 1510)
+++ flashrom-fix_par_programmer_access_from_spi/it87spi.c	(Arbeitskopie)
@@ -330,7 +330,7 @@
 	OUTB(0x06, it8716f_flashport + 1);
 	OUTB(((2 + (fast_spi ? 1 : 0)) << 4), it8716f_flashport);
 	for (i = 0; i < flash->page_size; i++)
-		chip_writeb(flash, buf[i], bios + start + i);
+		mmio_writeb(buf[i], (void *)(bios + start + i));
 	OUTB(0, it8716f_flashport);
 	/* Wait until the Write-In-Progress bit is cleared.
 	 * This usually takes 1-10 ms, so wait in 1 ms steps.
@@ -356,7 +356,7 @@
 	if ((flash->total_size * 1024 > 512 * 1024)) {
 		spi_read_chunked(flash, buf, start, len, 3);
 	} else {
-		read_memmapped(flash, buf, start, len);
+		mmio_readn((void *)(flash->virtual_memory + start), buf, len);
 	}
 
 	return 0;
Index: flashrom-fix_par_programmer_access_from_spi/internal.c
===================================================================
--- flashrom-fix_par_programmer_access_from_spi/internal.c	(Revision 1510)
+++ flashrom-fix_par_programmer_access_from_spi/internal.c	(Arbeitskopie)
@@ -387,6 +387,6 @@
 static void internal_chip_readn(const struct flashctx *flash, uint8_t *buf,
 				const chipaddr addr, size_t len)
 {
-	memcpy(buf, (void *)addr, len);
+	mmio_readn((void *)addr, buf, len);
 	return;
 }
Index: flashrom-fix_par_programmer_access_from_spi/programmer.h
===================================================================
--- flashrom-fix_par_programmer_access_from_spi/programmer.h	(Revision 1510)
+++ flashrom-fix_par_programmer_access_from_spi/programmer.h	(Arbeitskopie)
@@ -312,6 +312,7 @@
 uint8_t mmio_readb(void *addr);
 uint16_t mmio_readw(void *addr);
 uint32_t mmio_readl(void *addr);
+void mmio_readn(void *addr, uint8_t *buf, size_t len);
 void mmio_le_writeb(uint8_t val, void *addr);
 void mmio_le_writew(uint16_t val, void *addr);
 void mmio_le_writel(uint32_t val, void *addr);


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





More information about the flashrom mailing list