[flashrom] [PATCH 5/7] Combine serialport_write and serialport_read into serialport_rw.
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Fri Jul 19 01:57:30 CEST 2013
Am 10.07.2013 21:21 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. Also, this fixes error detection on Windows.
>
> Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
> ---
> serial.c | 83 ++++++++++++++++++++++++++++++----------------------------------
> 1 file changed, 39 insertions(+), 44 deletions(-)
>
> diff --git a/serial.c b/serial.c
> index 1b394cd..5f74fd1 100644
> --- a/serial.c
> +++ b/serial.c
> @@ -328,66 +330,59 @@ int serialport_shutdown(void *data)
> return 0;
> }
>
> -int serialport_write(unsigned char *buf, unsigned int writecnt)
> +#define EMPTY_TRIES 10
A total of 10 ms timeout is too fast for the Bus Pirate and some slower
serprog implementations AFAICS. I'd prefer a 1 s timeout before
signaling an error. That should be sufficient, and callers are supposed
to check for errors, so the 1 s timeout won't happen multiple times.
> +/* Writes (if \c outdir is true) or reads \c todo bytes from/into \c buf.
> + * Tries up to \c EMPTY_TRIES 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)
> {
> -#ifdef _WIN32
> - DWORD tmp = 0;
> -#else
> - ssize_t tmp = 0;
> -#endif
> - unsigned int empty_writes = 250; /* results in a ca. 125ms timeout */
> + const char * const op = outdir ? "write" : "read";
> + bool err = false;
> + unsigned int empty_tries = EMPTY_TRIES;
>
> - while (writecnt > 0) {
> + while (todo > 0) {
> #ifdef _WIN32
> - WriteFile(sp_fd, buf, writecnt, &tmp, NULL);
> + DWORD cur = 0;
> + if (outdir)
> + err = !WriteFile(sp_fd, buf, todo, &cur, NULL);
> + else
> + err = !ReadFile(sp_fd, buf, todo, &cur, NULL);
Should we check for cur == 0 here? Can that happen?
> #else
> - tmp = write(sp_fd, buf, writecnt);
> -#endif
> - if (tmp == -1) {
> - msg_perr("Serial port write error!\n");
> + ssize_t cur = 0;
> + if (outdir)
> + err = (cur = write(sp_fd, buf, todo)) < 0;
OK.
> + else
> + err = (cur = read(sp_fd, buf, todo)) < 0;
Should be <= 0 instead of < 0 AFAICS. See the POSIX man page of read():
"When attempting to read from an empty pipe or FIFO: If no process has
the pipe open for writing, read() shall return 0 to indicate end-of-file."
> +#endif
> + if (err) {
> + msg_perr("Serial port %s error!\n", op);
> return 1;
> }
> - if (!tmp) {
> - msg_pdbg2("Empty write\n");
> - empty_writes--;
> - programmer_delay(500);
> - if (empty_writes == 0) {
> - msg_perr("Serial port is unresponsive!\n");
> + 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 or disappeared!\n");
> return 1;
> }
> + continue;
> }
> - writecnt -= tmp;
> - buf += tmp;
> + todo -= cur;
> + buf += cur;
> + empty_tries = EMPTY_TRIES;
> }
>
> 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
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the flashrom
mailing list