Re: [PATCH v2] USB: cdc-acm: Prevent data loss when filling ttybuffer.

From: Johan Hovold
Date: Tue Mar 22 2011 - 06:05:38 EST

On Mon, Mar 21, 2011 at 06:04:58PM +0000, Toby Gray wrote:
> When sending large quantities of data through a CDC ACM channel it is possible
> for data to be lost when attempting to copy the data to the tty buffer. This
> occurs due to the return value from tty_insert_flip_string not being checked.
> This patch adds checking for how many bytes have been inserted into the tty
> buffer and returns any remaining bytes back to the filled read buffer list.


> @@ -392,6 +393,7 @@ static void acm_rx_tasklet(unsigned long _acm)


> - spin_lock_irqsave(&acm->read_lock, flags);
> - list_add(&buf->list, &acm->spare_read_bufs);
> - spin_unlock_irqrestore(&acm->read_lock, flags);
> + buf->head += copied;
> + buf->size -= copied;
> +
> + if (buf->size == 0) {
> + spin_lock_irqsave(&acm->read_lock, flags);
> + list_add(&buf->list, &acm->spare_read_bufs);
> + spin_unlock_irqrestore(&acm->read_lock, flags);
> + } else {
> + tty_kref_put(tty);
> + dbg("Partial buffer fill");
> + spin_lock_irqsave(&acm->read_lock, flags);
> + list_add(&buf->list, &acm->filled_read_bufs);
> + spin_unlock_irqrestore(&acm->read_lock, flags);
> + return;
> + }
> +

Say you fill up the tty buffer using the last of the sixteen buffers and
return in the else clause above, how will the tasklet ever get

The problem is that the tasklet is only scheduled on urb completion and
unthrottle (after open), and if you return above no urb will get
re-submitted. So the only way this will work is if it can be guaranteed
that the line discipline will throttle and later unthrottle us. I
doubt that is the case, but perhaps Alan can give a more definite

[By the way, did you see Filippe Balbi's patch posted today claiming to
fix a bug in n_tty which could cause data loss at high speeds?]

I was just about to submit a patch series killing the rx tasklet and
heavily simplifying the cdc-acm driver when you posted last night. I
think that if this mechanism is needed it is more straight-forwardly
implemented on top of those as they removes a lot of complexity and
makes it easier to spot corner cases such as the one pointed out above.

I would also prefer a more generic solution to the problem so that we
don't need to re-introduce driver buffering again. Since we already have
the throttelling mechanism in place, if we could only be notified/find
out that the tty buffers are say half-full, we could throttle (from
within the driver) but still push the remaining buffer still on the wire
as they arrive. It would of course require a guarantee that such a
throttle-is-about-to-happen notification is actually followed by (a
throttle and) unthrottle. Thoughts on that?

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at