Am 01.04.2013 03:14 schrieb Stefan Tauner: > Because they share almost all code, combine them into serialport_rw > and export the old functions as wrappers. This makes reads "empty reads"- > aware and programmers able to detect such problems even on "blocking" > reads. > > Signed-off-by: Stefan Tauner <[email protected]>
Acked-by: Carl-Daniel Hailfinger <[email protected]> > diff --git a/serial.c b/serial.c > index 49484f5..6679b7c 100644 > --- a/serial.c > +++ b/serial.c > @@ -28,6 +28,7 @@ > #include <sys/stat.h> > #include <errno.h> > #include <inttypes.h> > +#include <stdbool.h> > #ifdef _WIN32 > #include <conio.h> > #else > @@ -328,68 +329,61 @@ int serialport_shutdown(void *data) > return 0; > } > > -int serialport_write(unsigned char *buf, unsigned int writecnt) > +/* Writes (if \c outdir is true) or reads \c todo bytes from/into \c buf. > + * Tries up to 10 times with a timeout of \c ms ms between each try. */ > +static int serialport_rw(unsigned char *buf, unsigned int todo, bool outdir, > unsigned int ms) > { > + const char * const op = outdir ? "write" : "read"; > #ifdef _WIN32 > - DWORD tmp = 0; > + DWORD cur = 0; > #else > - ssize_t tmp = 0; > + ssize_t cur = 0; > #endif > - unsigned int empty_writes = 250; /* results in a ca. 125ms timeout */ > + unsigned int empty_tries = 10; > > - while (writecnt > 0) { > + while (todo > 0) { > #ifdef _WIN32 > - WriteFile(sp_fd, buf, writecnt, &tmp, NULL); > + if (outdir) > + WriteFile(sp_fd, buf, todo, &cur, NULL); > + else > + ReadFile(sp_fd, buf, todo, &cur, NULL); > #else > - tmp = write(sp_fd, buf, writecnt); > + if (outdir) > + cur = write(sp_fd, buf, todo); > + else > + cur = read(sp_fd, buf, todo); > #endif > - if (tmp == -1) { > - msg_perr("Serial port write error!\n"); > + if (cur == -1) { > + msg_perr("Serial port %s error!\n", op); > return 1; The logic of error handling is non-portable (AFAICS the number of read/written bytes for the error case is 0 on Windows and -1 on POSIX). That said, it was non-portable before, so this patch doesn't change it for the worse. > } > - if (!tmp) { > - msg_pdbg2("Empty write\n"); > - empty_writes--; > - programmer_delay(500); > - if (empty_writes == 0) { > + if (cur == 0) { > + msg_pdbg2("Empty %s.\n", op); > + empty_tries--; > + programmer_delay(ms * 1000); > + if (empty_tries == 0) { > msg_perr("Serial port is unresponsive!\n"); That message is actually wrong for the read case. If the serial device (e.g. Bus Pirate) disappears, you'll get 0-byte reads forever, at least on Linux. AFAICS blocking reads shall either return an error or at least 1 byte. That rule seems to be valid on Linux and Windows. Errors can be transient, though (see EINTR and other fun stuff). Suggested alternative wording: "Serial port is unresponsive or disappeared" > return 1; > } > } > - writecnt -= tmp; > - buf += tmp; > + todo -= cur; > + buf += cur; > } > > return 0; > } > > -int serialport_read(unsigned char *buf, unsigned int readcnt) > +int serialport_write(unsigned char *buf, unsigned int writecnt) > { > -#ifdef _WIN32 > - DWORD tmp = 0; > -#else > - ssize_t tmp = 0; > -#endif > - > - while (readcnt > 0) { > -#ifdef _WIN32 > - ReadFile(sp_fd, buf, readcnt, &tmp, NULL); > -#else > - tmp = read(sp_fd, buf, readcnt); > -#endif > - if (tmp == -1) { > - msg_perr("Serial port read error!\n"); > - return 1; > - } > - if (!tmp) > - msg_pdbg2("Empty read\n"); > - readcnt -= tmp; > - buf += tmp; > - } > + return serialport_rw(buf, writecnt, true, 1); > +} > > - return 0; > +int serialport_read(unsigned char *buf, unsigned int readcnt) > +{ > + return serialport_rw(buf, readcnt, false, 10); > } > > + > /* Tries up to timeout ms to read readcnt characters and places them into > the array starting at c. Returns > * 0 on success, positive values on temporary errors (e.g. timeouts) and > negative ones on permanent errors. > * If really_read is not NULL, this function sets its contents to the number > of bytes read successfully. */ Regards, Carl-Daniel -- http://www.hailfinger.org/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
