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 places data which hasn't been inserted into a pending buffer list.
The pending buffer list is pushed to the tty buffer when the tty unthrottles
Signed-off-by: Toby Gray <[email protected]>
---
drivers/usb/class/cdc-acm.c | 95 ++++++++++++++++++++++++++++++++++++------
drivers/usb/class/cdc-acm.h | 6 ++-
2 files changed, 85 insertions(+), 16 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 395a347..9f1d3ff 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -361,23 +361,65 @@ static int acm_submit_read_urbs(struct acm *acm, gfp_t mem_flags)
return 0;
}
-static void acm_process_read_urb(struct acm *acm, struct urb *urb)
+static void acm_process_read_buf(struct acm *acm, struct acm_rb *rb)
{
struct tty_struct *tty;
+ int copied;
- if (!urb->actual_length)
+ if (!rb->size)
return;
tty = tty_port_tty_get(&acm->port);
- if (!tty)
+ if (!tty) {
+ /* drop all data */
+ rb->size = 0;
return;
+ }
- tty_insert_flip_string(tty, urb->transfer_buffer, urb->actual_length);
+ copied = tty_insert_flip_string(tty, rb->head, rb->size);
tty_flip_buffer_push(tty);
+ rb->head += copied;
+ rb->size -= copied;
+
tty_kref_put(tty);
}
+static void acm_process_pending_read_bufs(struct acm *acm)
+{
+ unsigned long flags;
+ struct acm_rb *buf;
+
+ spin_lock_irqsave(&acm->read_lock, flags);
+ while (!list_empty(&acm->pending_read_bufs)) {
+ buf = list_entry(acm->pending_read_bufs.next,
+ struct acm_rb, list);
+ list_del(&buf->list);
+ spin_unlock_irqrestore(&acm->read_lock, flags);
+
+ acm_process_read_buf(acm, buf);
+
+ spin_lock_irqsave(&acm->read_lock, flags);
+ if (buf->size > 0) {
+ /* Managed to fill tty buffer again, so readd to
+ * the head before giving up. */
+ list_add(&buf->list, &acm->pending_read_bufs);
+ spin_unlock_irqrestore(&acm->read_lock, flags);
+ return;
+ }
+
+ set_bit(buf->index, &acm->read_urbs_free);
+ }
+ /* Submit any free urbs if not throttled */
+ if (!acm->throttled && !acm->susp_count) {
+ spin_unlock_irqrestore(&acm->read_lock, flags);
+ acm_submit_read_urbs(acm, GFP_KERNEL);
+ } else {
+ spin_unlock_irqrestore(&acm->read_lock, flags);
+ }
+ return;
+}
+
static void acm_read_bulk_callback(struct urb *urb)
{
struct acm_rb *rb = urb->context;
@@ -386,24 +428,52 @@ static void acm_read_bulk_callback(struct urb *urb)
dev_vdbg(&acm->data->dev, "%s - urb %d, len %d\n", __func__,
rb->index, urb->actual_length);
- set_bit(rb->index, &acm->read_urbs_free);
if (!acm->dev) {
+ set_bit(rb->index, &acm->read_urbs_free);
dev_dbg(&acm->data->dev, "%s - disconnected\n", __func__);
return;
}
usb_mark_last_busy(acm->dev);
if (urb->status) {
+ set_bit(rb->index, &acm->read_urbs_free);
dev_dbg(&acm->data->dev, "%s - non-zero urb status: %d\n",
__func__, urb->status);
return;
}
- acm_process_read_urb(acm, urb);
+ rb->head = urb->transfer_buffer;
+ rb->size = urb->actual_length;
- /* throttle device if requested by tty */
spin_lock_irqsave(&acm->read_lock, flags);
- acm->throttled = acm->throttle_req;
+
+ if (!list_empty(&acm->pending_read_bufs)) {
+ /* Data from previous reads is still pending, so
+ * add to the end of that. */
+ list_add_tail(&rb->list, &acm->pending_read_bufs);
+ } else {
+ spin_unlock_irqrestore(&acm->read_lock, flags);
+
+ acm_process_read_buf(acm, rb);
+
+ spin_lock_irqsave(&acm->read_lock, flags);
+ /* Bulk endpoints are recommended by the ACM specification
+ * to be used for hardware flow control. If bulk endpoints
+ * are being used then check that all data was passed to
+ * the tty.
+ */
+ if (!acm->is_int_ep && rb->size > 0) {
+ /* acm->throttled might not yet be true as throttling is
+ * detect by the ldisc handler, however it will be
+ * detected eventually and therefore unthrottled.
+ */
+ list_add_tail(&rb->list, &acm->pending_read_bufs);
+ } else {
+ set_bit(rb->index, &acm->read_urbs_free);
+ }
+ }
+
+ /* throttle device if requested by tty */
if (!acm->throttled && !acm->susp_count) {
spin_unlock_irqrestore(&acm->read_lock, flags);
acm_submit_read_urb(acm, rb->index, GFP_ATOMIC);
@@ -651,26 +721,22 @@ static void acm_tty_throttle(struct tty_struct *tty)
return;
spin_lock_irq(&acm->read_lock);
- acm->throttle_req = 1;
+ acm->throttled = 1;
spin_unlock_irq(&acm->read_lock);
}
static void acm_tty_unthrottle(struct tty_struct *tty)
{
struct acm *acm = tty->driver_data;
- unsigned int was_throttled;
if (!ACM_READY(acm))
return;
spin_lock_irq(&acm->read_lock);
- was_throttled = acm->throttled;
acm->throttled = 0;
- acm->throttle_req = 0;
spin_unlock_irq(&acm->read_lock);
- if (was_throttled)
- acm_submit_read_urbs(acm, GFP_KERNEL);
+ acm_process_pending_read_bufs(acm);
}
static int acm_tty_break_ctl(struct tty_struct *tty, int state)
@@ -1078,6 +1144,7 @@ made_compressed_probe:
spin_lock_init(&acm->read_lock);
mutex_init(&acm->mutex);
acm->rx_endpoint = usb_rcvbulkpipe(usb_dev, epread->bEndpointAddress);
+ INIT_LIST_HEAD(&acm->pending_read_bufs);
acm->is_int_ep = usb_endpoint_xfer_int(epread);
if (acm->is_int_ep)
acm->bInterval = epread->bInterval;
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index ca7937f..995333b 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -72,8 +72,10 @@ struct acm_wb {
};
struct acm_rb {
+ struct list_head list;
int size;
unsigned char *base;
+ unsigned char *head;
dma_addr_t dma;
int index;
struct acm *instance;
@@ -96,6 +98,7 @@ struct acm {
struct acm_rb read_buffers[ACM_NR];
int rx_buflimit;
int rx_endpoint;
+ struct list_head pending_read_bufs;
spinlock_t read_lock;
int write_used; /* number of non-empty write buffers */
int transmitting;
@@ -113,8 +116,7 @@ struct acm {
unsigned int susp_count; /* number of suspended interfaces */
unsigned int combined_interfaces:1; /* control and data collapsed */
unsigned int is_int_ep:1; /* interrupt endpoints contrary to spec used */
- unsigned int throttled:1; /* actually throttled */
- unsigned int throttle_req:1; /* throttle requested */
+ unsigned int throttled:1; /* tty is throttling */
u8 bInterval;
struct acm_wb *delayed_wb; /* write queued for a device about to be woken */
};
--
1.7.0.4
> This patch adds checking for how many bytes have been inserted into the tty
> buffer and places data which hasn't been inserted into a pending buffer list.
> The pending buffer list is pushed to the tty buffer when the tty unthrottles
I don't think this is the way to tackle it.
If the kernel tty buffer code is shedding data then not only have we
asserted a throttle but 64K has been accumulated or the system is out of
memory.
So the questions to me are:
a) Why is your setup filling 64K in the time it takes the throttle
response to occur
b) Do we care (is the right thing to do to lose bits anyway at that point)
c) What should we do about it in the *general* case
It seems to me either its a cdc-acm specific problem with throttle
response behaviour, or its a general case at very high speed (which seems
more likely). In neither case is adding an extra queue to paper over it
the right solution.
On 08/06/2011 16:46, Alan Cox wrote:
>> This patch adds checking for how many bytes have been inserted into the tty
>> buffer and places data which hasn't been inserted into a pending buffer list.
>> The pending buffer list is pushed to the tty buffer when the tty unthrottles
> I don't think this is the way to tackle it.
Fair enough. I wasn't 100% sure that adding what is really an extra
layer of buffering in the driver was the best solution.
> If the kernel tty buffer code is shedding data then not only have we
> asserted a throttle but 64K has been accumulated or the system is out of
> memory.
>
> So the questions to me are:
>
> a) Why is your setup filling 64K in the time it takes the throttle
> response to occur
The setup is a Nokia mobile phone sending some application data over a
USB connection. Applications running on Nokia's Symbian based mobile
phones which wish to communicate with a PC over USB are offered a
virtual COM port on the mobile phone side. This COM port appears as a
second cdc-acm device to the PC.
In this particular instance the device is sending a large quantity of
data (about 1MB) down the COM port. This communication is one way, so
the device keeps sending the data as fast as the PC requests more USB
bulk transfers.
If it hasn't been told to throttle then the cdc-acm code will issue more
bulk reads as soon as the last one has completed. This means that it's
possible for the tty to not have a chance to be throttle by
flush_to_ldisc before too much data has been received over USB.
> b) Do we care (is the right thing to do to lose bits anyway at that point)
The cdc-acm driver which Nokia provide for Windows doesn't drop data
when transferring large volumes of data, so most application making use
of the mobile phone to PC USB connection won't have implemented any form
of error correction or retransmission. If we don't care about data loss
on Linux then it will force the application layer to implement some way
of detecting these losses and to then retransmit the data.
> c) What should we do about it in the *general* case
> It seems to me either its a cdc-acm specific problem with throttle
> response behaviour, or its a general case at very high speed (which seems
> more likely). In neither case is adding an extra queue to paper over it
> the right solution.
Agreed. I'm not sure about how specific to cdc-acm it is though. I think
the reason that I'm seeing this problem is due to the high data rate of
USB combined with the protocol that is being used over the cdc-acm
channel not having any flow control or acknowledgements. I have no idea
how specialist this situation is though.
I think a possible alternative solution for the cdc-acm would be for it
to make sure that tty_buffer_request_room returns enough space for any
pending URB requests. However I think I originally dismissed this as a
possibility as I couldn't think of an easy way for the cdc-acm driver to
be told that more buffer room has been made available.
Regards,
Toby