lkml.org 
[lkml]   [2011]   [Mar]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    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
    re-scheduled?

    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
    answer?

    [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?

    Thanks,
    Johan


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