[flashrom] [PATCH] Combine serialport_write and serialport_read into serialport_rw.

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Fri May 31 00:36:17 CEST 2013


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 <stefan.tauner at student.tuwien.ac.at>

Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>


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





More information about the flashrom mailing list