[lkml]   [2011]   [Mar]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    Patch in this message
    SubjectRe: [PATCH v2] USB: cdc-acm: Prevent data loss when filling tty buffer.
    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?


     \ /
      Last update: 2011-03-22 11:07    [W:0.024 / U:253.868 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site