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

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

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

Regards,
  Michael Karcher