[PATCH] rtems_termios_puts: Copy and write more than one char at once

[Sebastian Huber]

Hello Kolja,

looks good, but I have some minor stuff.

On 14/08/14 13:10, Kolja Waschk wrote:
> Beside copying more than one char at once, rtems_termios_refill_transmitter()
> and rtems_termios_puts() now use a common helper that (after checking ORCVXOF)
> calls write() with as much chars as are available consecutively in rawOutBuf.
> Integer types used in new rtems_termios_start_xmit might not be ideal but
> resemble what is used in existing rtems_refill_transmitter code.
>
> ---
>   cpukit/libcsupport/src/termios.c | 133 ++++++++++++++++++++++++---------------
>   1 file changed, 83 insertions(+), 50 deletions(-)
>
> diff --git a/cpukit/libcsupport/src/termios.c b/cpukit/libcsupport/src/termios.c
> index 2448ea1..f14f138 100644
> --- a/cpukit/libcsupport/src/termios.c
> +++ b/cpukit/libcsupport/src/termios.c
> @@ -974,6 +974,47 @@ rtems_termios_ioctl (void *arg)
>   }
>
>   /*
> + * Send as many chars at once as possible to device-specific code.
> + * If stopXmit==true then explicitly write(0) to stop transmitter.
> + */
> +static int
> +rtems_termios_start_xmit (

The internal functions use normally CamelCase in this file.

> +  struct rtems_termios_tty *tty, unsigned int newTail, bool stopXmit)
> +{
> +  int nToSend;

Integer type is wrong.

> +
> +  /* if XOFF was received, do not start output resp. stop */
> +  if ((tty->flow_ctrl & (FL_MDXON | FL_ORCVXOF))
> +      ==                (FL_MDXON | FL_ORCVXOF)) {

Previously we had this and

if (!(tty->flow_ctrl & FL_ORCVXOF)) {

I would use this, since FL_ORCVXOF is only set if FL_MDXON is set.

> +    /* Buffer not empty, but output stops due to XOFF */
> +    /* set flag, that output has been stopped */
> +    tty->flow_ctrl |= FL_OSTOP;
> +    nToSend = 0;
> +    tty->rawOutBufState = rob_busy;
> +    if (stopXmit) {
> +      (*tty->handler.write) (tty, NULL, 0);
> +    }
> +  } else {
> +    if (newTail > tty->rawOutBuf.Head)
> +      nToSend = tty->rawOutBuf.Size - newTail;
> +    else
> +      nToSend = tty->rawOutBuf.Head - newTail;
> +    /* when flow control XON or XOF, don't send blocks of data     */
> +    /* to allow fast reaction on incoming flow ctrl and low latency*/
> +    /* for outgoing flow control                                   */
> +    if (tty->flow_ctrl & (FL_MDXON | FL_MDXOF)) {
> +      nToSend = 1;
> +    }

Yes, the this is the original code, but I think it is a bit odd to discard a 
value computed directly above.  I would rather do the buffer nToSend 
computation in an else block.

> +
> +    tty->rawOutBufState = rob_busy;
> +    (*tty->handler.write)(
> +        tty, &tty->rawOutBuf.theBuf[newTail], nToSend);
> +  }
> +
> +  return nToSend;
> +}
> +
> +/*
>    * Send characters to device-specific code
>    */
>   void
> @@ -989,21 +1030,16 @@ rtems_termios_puts (
>       (*tty->handler.write)(tty, buf, len);
>       return;
>     }
> -  newHead = tty->rawOutBuf.Head;
> +
>     while (len) {
> -    /*
> -     * Performance improvement could be made here.
> -     * Copy multiple bytes to raw buffer:
> -     * if (len > 1) && (space to buffer end, or tail > 1)
> -     *  ncopy = MIN (len, space to buffer end or tail)
> -     *  memcpy (raw buffer, buf, ncopy)
> -     *  buf += ncopy
> -     *  len -= ncopy
> -     *
> -     * To minimize latency, the memcpy should be done
> -     * with interrupts enabled.
> -     */
> -    newHead = (newHead + 1) % tty->rawOutBuf.Size;
> +    size_t nToCopy;
> +    size_t nAvail;
> +
> +    /* Check space for at least one char */
> +    newHead = tty->rawOutBuf.Head + 1;
> +    if (newHead >= tty->rawOutBuf.Size)
> +      newHead -= tty->rawOutBuf.Size;
> +
>       rtems_termios_interrupt_lock_acquire (tty, &lock_context);
>       while (newHead == tty->rawOutBuf.Tail) {
>         tty->rawOutBufState = rob_wait;
> @@ -1014,21 +1050,41 @@ rtems_termios_puts (
>           rtems_fatal_error_occurred (sc);
>         rtems_termios_interrupt_lock_acquire (tty, &lock_context);
>       }
> -    tty->rawOutBuf.theBuf[tty->rawOutBuf.Head] = *buf++;
> +
> +    /* Determine free space up to current tail or end of ring buffer */
> +    nToCopy = len;
> +    if (tty->rawOutBuf.Tail > tty->rawOutBuf.Head) {
> +      /* Available space is contiguous from Head to Tail */
> +      nAvail = tty->rawOutBuf.Tail - tty->rawOutBuf.Head - 1;
> +    } else {
> +      /* Available space wraps at buffer end. To keep code simple, utilize
> +         only the part from Head to end during this iteration */
> +      nAvail = tty->rawOutBuf.Size - tty->rawOutBuf.Head;
> +      /* Head may not touch Tail after wraparound */
> +      if (tty->rawOutBuf.Tail == 0)
> +        nAvail--;
> +    }
> +    if (nToCopy > nAvail)
> +      nToCopy = nAvail;
> +
> +    /* To minimize latency, the memcpy could be done
> +     * with interrupts enabled or with limit on nToCopy (TBD)
> +     */
> +    memcpy(&tty->rawOutBuf.theBuf[tty->rawOutBuf.Head], buf, nToCopy);
> +
> +    newHead = tty->rawOutBuf.Head + nToCopy;
> +    if (newHead >= tty->rawOutBuf.Size)
> +      newHead -= tty->rawOutBuf.Size;
>       tty->rawOutBuf.Head = newHead;
> +
>       if (tty->rawOutBufState == rob_idle) {
> -      /* check, whether XOFF has been received */
> -      if (!(tty->flow_ctrl & FL_ORCVXOF)) {
> -        (*tty->handler.write)(
> -          tty, &tty->rawOutBuf.theBuf[tty->rawOutBuf.Tail],1);
> -      } else {
> -        /* remember that output has been stopped due to flow ctrl*/
> -        tty->flow_ctrl |= FL_OSTOP;
> -      }
> -      tty->rawOutBufState = rob_busy;
> +      (void)rtems_termios_start_xmit (tty, tty->rawOutBuf.Tail, false);

Please don't use this (void).

>       }
> +
>       rtems_termios_interrupt_lock_release (tty, &lock_context);
> -    len--;
> +
> +    buf += nToCopy;
> +    len -= nToCopy;
>     }
>   }
>
> @@ -1678,35 +1734,12 @@ rtems_termios_refill_transmitter (struct rtems_termios_tty *tty)
>         if ( tty->tty_snd.sw_pfn != NULL) {
>           (*tty->tty_snd.sw_pfn)(&tty->termios, tty->tty_snd.sw_arg);
>         }
> -    }
> -    /* check, whether output should stop due to received XOFF */
> -    else if ((tty->flow_ctrl & (FL_MDXON | FL_ORCVXOF))
> -       ==                (FL_MDXON | FL_ORCVXOF)) {
> -      /* Buffer not empty, but output stops due to XOFF */
> -      /* set flag, that output has been stopped */
> -      tty->flow_ctrl |= FL_OSTOP;
> -      tty->rawOutBufState = rob_busy; /*apm*/
> -      (*tty->handler.write) (tty, NULL, 0);
> -      nToSend = 0;
>       } else {
>         /*
> -       * Buffer not empty, start tranmitter
> +       * Buffer not empty, check flow control, start transmitter
>          */
> -      if (newTail > tty->rawOutBuf.Head)
> -        nToSend = tty->rawOutBuf.Size - newTail;
> -      else
> -        nToSend = tty->rawOutBuf.Head - newTail;
> -      /* when flow control XON or XOF, don't send blocks of data     */
> -      /* to allow fast reaction on incoming flow ctrl and low latency*/
> -      /* for outgoing flow control                                   */
> -      if (tty->flow_ctrl & (FL_MDXON | FL_MDXOF)) {
> -        nToSend = 1;
> -      }
> -      tty->rawOutBufState = rob_busy; /*apm*/
> -      (*tty->handler.write)(
> -        tty, &tty->rawOutBuf.theBuf[newTail], nToSend);
> +      nToSend = rtems_termios_start_xmit (tty, newTail, true);
>       }
> -    tty->rawOutBuf.Tail = newTail; /*apm*/
>     }
>
>     rtems_termios_interrupt_lock_release (tty, &lock_context);
>


-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber at embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.