On Mon, 21 Mar 2011 15:52:25 +0000 Toby Gray <toby.gray@xxxxxxxxxxx> 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. For a tty that is normally the right thing to do - no flow control was asserted and the internal 64K of buffering was overrun so discard. > 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. How does ACM handle flow control - is the expectation that it gets flow controlled in hardware by not having the opportunity to send bits to the host end ? If so this seems to make sense. > + copied = 0; Surely copied = buf->size is the no tty assumption "we had the lot and discarded it" > if (tty) { > spin_lock_irqsave(&acm->throttle_lock, flags); > throttled = acm->throttle; > spin_unlock_irqrestore(&acm->throttle_lock, flags); > if (!throttled) { > - tty_insert_flip_string(tty, buf->base, buf->size); > + copied = tty_insert_flip_string(tty, > + buf->base, buf->size); > tty_flip_buffer_push(tty); > } else { > tty_kref_put(tty); > @@ -440,9 +443,26 @@ next_buffer: > } > } > > - spin_lock_irqsave(&acm->read_lock, flags); > - list_add(&buf->list, &acm->spare_read_bufs); > - spin_unlock_irqrestore(&acm->read_lock, flags); > + if (copied == buf->size || !tty) { Which would simplify this lot > + 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"); > + if (copied > 0) { > + memmove(buf->base, > + buf->base + copied, > + buf->size - copied); > + buf->size -= copied; > + } Would it not be cleaner to add a buf->head pointer that could simply be advanced so the code would become buf->head += copied; buf->size -= copied; if (buf->size != 0) list_add(&buf->list, &acm->filled_read_bufs); instead of all the memmove uglies ? -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html