Re: [PATCH 5/5] SLIP: Fix transmission segmentation mechanism

[Peter Hurley <peter@hurleysoftware.com>]

On 07/02/2013 11:31 AM, Dean Jenkins wrote:
> Binding SLIP with a PTY/TTY can cause truncated SLIP frames to be
> transmitted when the first write to the TTY fails to consume all
> the characters of the SLIP frame.
>
> Asynchronous to the write function is a wakeup event from the TTY
> that indicates the TTY can accept more data. The wakeup event
> calls tty_wakeup() which calls slip_write_wakeup() when
> TTY_DO_WRITE_WAKEUP is set.
>
> To complete the transmission of a SLIP frame to the TTY,
> slip_write_wakeup() must run at least once. Unfortunately, pty_write()
> also calls tty_wakeup() and when TTY_DO_WRITE_WAKEUP is set,
> slip_write_wakeup() is called. xleft is always zero and
> causes slip_write_wakeup() to complete the transmission and
> clears the TTY_DO_WRITE_WAKEUP flag. This can cause a truncated
> SLIP frame because any remaining characters will not get sent.
>
> The wakeup event is unable to process the remaining characters
> because the TTY_DO_WRITE_WAKEUP flag has been cleared.
>
> The code modification fixes the transmission segmentation
> mechanism by preventing pty_write() from calling
> slip_write_wakeup() by clearing TTY_DO_WRITE_WAKEUP before calling
> pty_write(). After pty_write() returns, TTY_DO_WRITE_WAKEUP is set
> to allow the TTY wakeup event to call slip_write_wakeup() to
> attempt to complete the transmission of the SLIP frame.
>
> This may not be foolproof because a timeout is needed to
> break out of the cycle of transmission attempts. Note error
> codes from the TTY layer will break out of the cycle of
> transmission attempts.
>
> Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
> ---
>   drivers/net/slip/slip.c | 29 ++++++++++++++++++++---------
>   1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
> index e2eff84..0ded23d 100644
> --- a/drivers/net/slip/slip.c
> +++ b/drivers/net/slip/slip.c
> @@ -404,15 +404,13 @@ static void sl_encaps(struct slip *sl, unsigned char *icp, int len)
>   	 */
>   	sl->xleft = 0;
>
> -	/* Order of next two lines is *very* important.
> -	 * When we are sending a little amount of data,
> -	 * the transfer may be completed inside the ops->write()
> -	 * routine, because it's running with interrupts enabled.
> -	 * In this case we *never* got WRITE_WAKEUP event,
> -	 * if we did not request it before write operation.
> -	 *       14 Oct 1994  Dmitry Gorodchanin.
> +	/* ensure slip_write_wakeup() does not run due to write()
> +	 * or write_wakeup event and this prevents slip_write_wakeup()
> +	 * responding to an out of date xleft value.
>   	 */
> -	set_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);
> +	clear_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);
> +
> +	/* attempt to write the SLIP frame to the TTY buffer */
>   	err = sl->tty->ops->write(sl->tty, sl->xbuff, count);
>
>   	if (err < 0) {
> @@ -432,6 +430,11 @@ static void sl_encaps(struct slip *sl, unsigned char *icp, int len)
>   	/* VSV */
>   	clear_bit(SLF_OUTWAIT, &sl->flags);	/* reset outfill flag */
>   #endif
> +	/* xleft will be zero when all characters have been written.
> +	 * if xleft is positive then additional writes are needed.
> +	 * write_wakeup event is needed to complete the transmission.
> +	 */
> +	set_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);

What happens if the tty_wakeup() has already happened on another
CPU before TTY_DO_WRITE_WAKEUP was set?

>   }
>
>   /*
> @@ -447,15 +450,18 @@ static void slip_write_wakeup(struct tty_struct *tty)
>   	if (!sl || sl->magic != SLIP_MAGIC || !netif_running(sl->dev))
>   		return;
>
> +	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
> +
>   	if (sl->xleft <= 0)  {
> +		/* Whole SLIP frame has been written. */
>   		/* Now serial buffer is almost free & we can start
>   		 * transmission of another packet */
>   		sl->dev->stats.tx_packets++;
> -		clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
>   		sl_unlock(sl);
>   		return;
>   	}
>
> +	/* attempt to write the remaining SLIP frame characters */
>   	err = tty->ops->write(tty, sl->xhead, sl->xleft);
>
>   	if (err < 0) {
> @@ -468,6 +474,11 @@ static void slip_write_wakeup(struct tty_struct *tty)
>
>   	sl->xleft -= actual;
>   	sl->xhead += actual;
> +
> +	/* allow the next tty wakeup event to attempt to complete
> +	 * the transmission
> +	 */
> +	set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);

Same here.

>   }
>
>   static void sl_tx_timeout(struct net_device *dev)
>