Re: [PATCH] synclink_gt fix transmit race and timeout

Andrew Morton
2009-06-23T06:20:27Z

On Tue, 16 Jun 2009 14:44:11 -0500 Paul Fulghum <paulkf@microgate.com> wrote:

> Fix race condition when adding transmit data to active DMA buffer ring
> that can cause transmit stall.
> Update transmit timeout when adding data to active DMA buffer ring.
> Base transmit timeout on amount of buffered data instead of
> using fixed value.
>

It's not a terribly good changelog, sorry.

It fails to describe the race condition?

It fails to explain the user-visible effects of the bug which was
fixed. Those who need to make should-we-backport-this-to-stable
decisions need to know this. Often we are told, often we can guess.
Sometimes neither.

>
> --- a/drivers/char/synclink_gt.c 2009-06-09 22:05:27.000000000 -0500
> +++ b/drivers/char/synclink_gt.c 2009-06-16 13:58:14.000000000 -0500
> @@ -463,7 +463,6 @@ static unsigned int free_tbuf_count(stru
> static unsigned int tbuf_bytes(struct slgt_info *info);
> static void reset_tbufs(struct slgt_info *info);
> static void tdma_reset(struct slgt_info *info);
> -static void tdma_start(struct slgt_info *info);
> static void tx_load(struct slgt_info *info, const char *buf, unsigned int count);
>
> static void get_signals(struct slgt_info *info);
> @@ -791,6 +790,18 @@ static void set_termios(struct tty_struc
> }
> }
>
> +static void update_tx_timer(struct slgt_info *info)
> +{
> + /*
> + * use worst case speed of 1200bps to calculate transmit timeout
> + * based on data in buffers (tbuf_bytes) and FIFO (128 bytes)
> + */
> + if (info->params.mode == MGSL_MODE_HDLC) {
> + int timeout = (tbuf_bytes(info) * 7) + 1000;
> + mod_timer(&info->tx_timer, jiffies + msecs_to_jiffies(timeout));
> + }
> +}

Where did the "7" come from?

> static int write(struct tty_struct *tty,
> const unsigned char *buf, int count)
> {
> @@ -834,8 +845,18 @@ start:
> spin_lock_irqsave(&info->lock,flags);
> if (!info->tx_active)
> tx_start(info);
> - else
> - tdma_start(info);
> + else if (!(rd_reg32(info, TDCSR) & BIT0)) {
> + /* transmit still active but transmit DMA stopped */
> + unsigned int i = info->tbuf_current;
> + if (!i)
> + i = info->tbuf_count;
> + i--;
> + /* if DMA buf unsent must try later after tx idle */
> + if (desc_count(info->tbufs[i]))
> + ret = 0;
> + }
> + if (ret > 0)
> + update_tx_timer(info);
> spin_unlock_irqrestore(&info->lock,flags);
> }
>
> @@ -1498,10 +1519,9 @@ static int hdlcdev_xmit(struct sk_buff *
> /* save start time for transmit timeout detection */
> dev->trans_start = jiffies;
>
> - /* start hardware transmitter if necessary */
> spin_lock_irqsave(&info->lock,flags);
> - if (!info->tx_active)
> - tx_start(info);
> + tx_start(info);
> + update_tx_timer(info);
> spin_unlock_irqrestore(&info->lock,flags);
>
> return 0;
> @@ -3886,50 +3906,19 @@ static void tx_start(struct slgt_info *i
> slgt_irq_on(info, IRQ_TXUNDER + IRQ_TXIDLE);
> /* clear tx idle and underrun status bits */
> wr_reg16(info, SSR, (unsigned short)(IRQ_TXIDLE + IRQ_TXUNDER));
> - if (info->params.mode == MGSL_MODE_HDLC)
> - mod_timer(&info->tx_timer, jiffies +
> - msecs_to_jiffies(5000));
> } else {
> slgt_irq_off(info, IRQ_TXDATA);
> slgt_irq_on(info, IRQ_TXIDLE);
> /* clear tx idle status bit */
> wr_reg16(info, SSR, IRQ_TXIDLE);
> }
> - tdma_start(info);
> + /* set 1st descriptor address and start DMA */
> + wr_reg32(info, TDDAR, info->tbufs[info->tbuf_start].pdesc);
> + wr_reg32(info, TDCSR, BIT2 + BIT0);
> info->tx_active = true;
> }
> }
>
> ...
>
> @@ -4942,8 +4931,7 @@ static void tx_timeout(unsigned long con
> info->icount.txtimeout++;
> }
> spin_lock_irqsave(&info->lock,flags);
> - info->tx_active = false;
> - info->tx_count = 0;
> + tx_stop(info);

I have a suspicion that tx_stop() should use del_timer_sync(), not
del_timer(). What happens if the timer handler is concurrently
running?