Re: [flashrom] [PATCH 5/7] Combine serialport_write and serialport_read into serialport_rw.

[Carl-Daniel Hailfinger]

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

[PATCH 5/7] Combine serialport_write and serialport_read into serialport_rw.

[Stefan Tauner]

On Fri, 19 Jul 2013 01:57:30 +0200
Carl-Daniel Hailfinger  wrote:
> 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
> >
> > 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.

I was under the impression that reads and writes are blocking, hence
that these timeout values do not determine the upper bound of the
execution time of one operation. I came to that conclusion because it is
the behavior of my hardware (serprog via cdc-acm) and software (native
linux and wine). I also tested it on native windows.

On non-windows we open the device with O_NDELAY which seems to have been
different to O_NONBLOCK in the distant past but is now semantically
equivalent on most platforms. However, only O_NONBLOCK seems to be in
POSIX and I would like to switch to that.

That being said, apparently we are relying on not too well specified
behavior. AFAIK the tty devices are "character special" files/devices.
The standard specifies the following:

"When opening a block special or character special file that supports
non-blocking opens:
 - If O_NONBLOCK is set, the open() function shall return without
   blocking for the device to be ready or available. *Subsequent
   behavior of the device is device-specific*." (emphasis mine, and I
   am very certain from the other context that this means if reads and
   writes are blocking or not)

So... before I think and write about what we could do, I would like to
hear what you have to say about it.
> > +/* 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?

we do after the ifdefs (marked below) for all platforms.
> > #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."

A character special file is not a pipe.
But even then... we would receive empty reads and timeout quickly. No
harm done AFAICS.
> > +#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;
> > }