[PATCH] FT2232 error checking

[Carl-Daniel Hailfinger]

Pretty much everybody who used the FT2232 SPI driver in flashrom had
problems with incorrect reads from time to time.
One reason was that the hardware is pretty timing sensitive even for reads.

The other reason was that the code silently ignored errors. This patch
doesn't add any error recovery, but it will emit error messages if
FT2232 communication goes wrong. That allows us to track down errors
without investing hours in driver debugging.

Jeremy, I'd be very interested in the results of an unmodified flashrom
with only this patch applied (read is sufficient). In theory, you should
either get a working read or loads of error messages about
send_buf/read_buf. If you get no error messages and the image read is
still wrong, libftdi doesn't tell us about the problem. Oh, and please
try in verbose and normal mode. Maybe there's a difference.

This patch will show up at
http://patchwork.coreboot.org/project/flashrom/list/ in a few minutes.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-ft2232_errorcheck/ft2232_spi.c
===================================================================
--- flashrom-ft2232_errorcheck/ft2232_spi.c	(Revision 756)
+++ flashrom-ft2232_errorcheck/ft2232_spi.c	(Arbeitskopie)
@@ -198,7 +198,8 @@
 {
    struct ftdi_context *ftdic = &ftdic_context;
    static unsigned char *buf = NULL;
-	int i = 0, ret = 0;
+	/* failed is special. We use bitwise ops, but it is essentially bool. */
+	int i = 0, ret = 0, failed = 0;

if (writecnt > 65536 || readcnt > 65536)
    	return SPI_INVALID_LENGTH;
@@ -237,6 +238,11 @@
    	buf[i++] = (readcnt - 1) & 0xff;
    	buf[i++] = ((readcnt - 1) >> 8) & 0xff;
    	ret = send_buf(ftdic, buf, i);
+		failed = ret;
+		/* We can't abort here, we still have to deassert CS#. */
+		if (ret)
+			fprintf(stderr, "send_buf failed before read: %i\n",
+				ret);
    	i = 0;
    	if (ret == 0) {
    		/*
@@ -245,6 +251,10 @@
    		 * command. We may be scheduled out etc.
    		 */
    		ret = get_buf(ftdic, readarr, readcnt);
+			failed |= ret;
+			/* We can't abort here either. */
+			if (ret)
+				fprintf(stderr, "get_buf failed: %i\n", ret);
    	}
    }

@@ -252,10 +262,12 @@
    buf[i++] = SET_BITS_LOW;
    buf[i++] = CS_BIT;
    buf[i++] = 0x0b;
-	if (send_buf(ftdic, buf, i))
-		return -1;
+	ret = send_buf(ftdic, buf, i);
+	failed |= ret;
+	if (ret)
+		fprintf(stderr, "send_buf failed at end: %i\n", ret);

-	return ret;
+	return failed ? -1 : 0;
 }

int ft2232_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len)

[Jeremy Buseman]

Carl,
I downloaded a clean copy from SVN and patched it.  I pulled 4 reads without
verbose and 4 with, they are attached.  The reads came back different each
time just as before, but I did not get any error messages so this must be a
libftdi issue.  Let me know if there's more I can do.
Jeremy

On Mon, Nov 9, 2009 at 10:07 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> Pretty much everybody who used the FT2232 SPI driver in flashrom had
> problems with incorrect reads from time to time.
> One reason was that the hardware is pretty timing sensitive even for reads.
>
> The other reason was that the code silently ignored errors. This patch
> doesn't add any error recovery, but it will emit error messages if
> FT2232 communication goes wrong. That allows us to track down errors
> without investing hours in driver debugging.
>
> Jeremy, I'd be very interested in the results of an unmodified flashrom
> with only this patch applied (read is sufficient). In theory, you should
> either get a working read or loads of error messages about
> send_buf/read_buf. If you get no error messages and the image read is
> still wrong, libftdi doesn't tell us about the problem. Oh, and please
> try in verbose and normal mode. Maybe there's a difference.
>
> This patch will show up at
> http://patchwork.coreboot.org/project/flashrom/list/ in a few minutes.
>
> Signed-off-by: Carl-Daniel Hailfinger
> Index: flashrom-ft2232_errorcheck/ft2232_spi.c
>
> --- flashrom-ft2232_errorcheck/ft2232_spi.c     (Revision 756)
> +++ flashrom-ft2232_errorcheck/ft2232_spi.c     (Arbeitskopie)
> @@ -198,7 +198,8 @@
>  {
>        struct ftdi_context *ftdic = &ftdic_context;
>        static unsigned char *buf = NULL;
>   int i = 0, ret = 0;
>   /* failed is special. We use bitwise ops, but it is essentially
>
> bool. */
>   int i = 0, ret = 0, failed = 0;
>
>  if (writecnt > 65536 || readcnt > 65536)
>          return SPI_INVALID_LENGTH;
>
> @@ -237,6 +238,11 @@
>                buf[i++] = (readcnt - 1) & 0xff;
>                buf[i++] = ((readcnt - 1) >> 8) & 0xff;
>                ret = send_buf(ftdic, buf, i);
>           failed = ret;
>           /* We can't abort here, we still have to deassert CS#. */
>           if (ret)
>                   fprintf(stderr, "send_buf failed before read:
>
> %i\n",
>                           ret);
>          i = 0;
>          if (ret == 0) {
>                  /*
>
> @@ -245,6 +251,10 @@
>                         * command. We may be scheduled out etc.
>                         */
>                        ret = get_buf(ftdic, readarr, readcnt);
>                   failed |= ret;
>                   /* We can't abort here either. */
>                   if (ret)
>                           fprintf(stderr, "get_buf failed: %i\n",
>
> ret);
>                }
>        }
>
> @@ -252,10 +262,12 @@
>        buf[i++] = SET_BITS_LOW;
>        buf[i++] = CS_BIT;
>        buf[i++] = 0x0b;
>   if (send_buf(ftdic, buf, i))
>           return -1;
>   ret = send_buf(ftdic, buf, i);
>   failed |= ret;
>   if (ret)
>           fprintf(stderr, "send_buf failed at end: %i\n", ret);
>
>   return ret;
>   return failed ? -1 : 0;
>
> }
>
> int ft2232_spi_read(struct flashchip *flash, uint8_t *buf, int start, int
> len)

A binary attachment has been removed from this mail.
The name of the attachment was "Patched_Reads.tar.gz".

[Carl-Daniel Hailfinger]

Hi Paul, hi Richard,

I remember that one of you mentioned occassional verify failures with
flashrom and the FT2232 code.
The good news is that we recently got a report from another user (Jeremy
Buseman) with whom we could track down some of the issues to timing and
to deficiencies in the software/hardware stack. Running flashrom in
non-verbose mode and with a limit of 36 bytes per SPI command (1 opcode,
3 address, 32 data) gave him 100% reliable results.
I also have a patch pending which allows a specialized test pattern to
be burned, and the results were surprising: Sometimes, the last two
bytes of a 64-byte block are simply missing, then the next 64-byte block
is moved two bytes in the direction of lower addresses, and so on. For
page (256 byte) reads, this could result in up to 8 bytes at the end
being undefined because some or all 64-byte subpages had been trimmed.
Besides that, verbose mode seems to have thrown off the timing enough to
cause read/write errors as well. In theory, that should not happen (and
libftdi/libusb should ensure consistent behaviour), but reality seems a
bit disillusioning.

If you could review the patch below which adds some error checking to
give at least some indications on where the problems may lie, I'd
appreciate it. It is entirely possible that you won't get additional
warnings from the patch, but I'd like to run it past you because you
seem to be responsible for the largest FT2232 flashrom deployment. If
you think the patch is OK, please respond with
> Acked-by: Your Name

On 10.11.2009 04:07, Carl-Daniel Hailfinger wrote:
> Pretty much everybody who used the FT2232 SPI driver in flashrom had
> problems with incorrect reads from time to time.
> One reason was that the hardware is pretty timing sensitive even for reads.
>
> The other reason was that the code silently ignored errors. This patch
> doesn't add any error recovery, but it will emit error messages if
> FT2232 communication goes wrong. That allows us to track down errors
> without investing hours in driver debugging.
>
> Jeremy, I'd be very interested in the results of an unmodified flashrom
> with only this patch applied (read is sufficient). In theory, you should
> either get a working read or loads of error messages about
> send_buf/read_buf. If you get no error messages and the image read is
> still wrong, libftdi doesn't tell us about the problem. Oh, and please
> try in verbose and normal mode. Maybe there's a difference.
>
> Signed-off-by: Carl-Daniel Hailfinger

In case you're not subscribed to the flashrom mailing list, you can
download the patch here:
http://patchwork.coreboot.org/patch/549/raw/

Regards,
Carl-Daniel

[Paul Fox]

hi carl-daniel --

sorry for the delay -- things have been pretty busy recently. 
that patch certainly looks correct.  i haven't been able to try
it, but i'll probably be doing some reflashing soon...

Acked-by: Paul Fox

paul

carl-daniel wrote:
> Hi Paul, hi Richard,
>
> I remember that one of you mentioned occassional verify failures with
> flashrom and the FT2232 code.
> The good news is that we recently got a report from another user (Jeremy
> Buseman) with whom we could track down some of the issues to timing and
> to deficiencies in the software/hardware stack. Running flashrom in
> non-verbose mode and with a limit of 36 bytes per SPI command (1 opcode,
> 3 address, 32 data) gave him 100% reliable results.
> I also have a patch pending which allows a specialized test pattern to
> be burned, and the results were surprising: Sometimes, the last two
> bytes of a 64-byte block are simply missing, then the next 64-byte block
> is moved two bytes in the direction of lower addresses, and so on. For
> page (256 byte) reads, this could result in up to 8 bytes at the end
> being undefined because some or all 64-byte subpages had been trimmed.
> Besides that, verbose mode seems to have thrown off the timing enough to
> cause read/write errors as well. In theory, that should not happen (and
> libftdi/libusb should ensure consistent behaviour), but reality seems a
> bit disillusioning.
>
> If you could review the patch below which adds some error checking to
> give at least some indications on where the problems may lie, I'd
> appreciate it. It is entirely possible that you won't get additional
> warnings from the patch, but I'd like to run it past you because you
> seem to be responsible for the largest FT2232 flashrom deployment. If
> you think the patch is OK, please respond with
> > Acked-by: Your Name
>
> On 10.11.2009 04:07, Carl-Daniel Hailfinger wrote:
> > Pretty much everybody who used the FT2232 SPI driver in flashrom had
> > problems with incorrect reads from time to time.
> > One reason was that the hardware is pretty timing sensitive even for reads.
> >
> > The other reason was that the code silently ignored errors. This patch
> > doesn't add any error recovery, but it will emit error messages if
> > FT2232 communication goes wrong. That allows us to track down errors
> > without investing hours in driver debugging.
> >
> > Jeremy, I'd be very interested in the results of an unmodified flashrom
> > with only this patch applied (read is sufficient). In theory, you should
> > either get a working read or loads of error messages about
> > send_buf/read_buf. If you get no error messages and the image read is
> > still wrong, libftdi doesn't tell us about the problem. Oh, and please
> > try in verbose and normal mode. Maybe there's a difference.
> >
> > Signed-off-by: Carl-Daniel Hailfinger
>
> In case you're not subscribed to the flashrom mailing list, you can
> download the patch here:
> http://patchwork.coreboot.org/patch/549/raw/
>
> Regards,
> Carl-Daniel

=---------------------
 paul fox, [e-mail] (arlington, ma, where it's 55.2 degrees)

[Carl-Daniel Hailfinger]

Jeremy, thanks for testing.
Paul, thanks for the review.

Patch committed in r769.

Regards,
Carl-Daniel

[Jeremy Buseman]

My pleasure.  If there's anything more I can do please let me know.

On 11/21/09, Carl-Daniel Hailfinger  wrote:
> Jeremy, thanks for testing.
> Paul, thanks for the review.
>
> Patch committed in r769.
>
> Regards,
> Carl-Daniel