[PATCH] Fix data loss in cdc-acm

* [PATCH] Fix data loss in cdc-acm
@ 2015-07-19 21:37 Sven Brauch
  2015-07-20 17:25 ` Johan Hovold
  0 siblings, 1 reply; 21+ messages in thread
From: Sven Brauch @ 2015-07-19 21:37 UTC (permalink / raw)
  To: Linux Kernel Mailing List

[-- Attachment #1: Type: text/plain, Size: 6618 bytes --]

Since acm_process_read_urb does not check the return value
of tty_insert_flip_string, it can happen that not all data
is copied from the urb to the tty if the tty buffer
is full and throttling does not set in quickly enough. This
problem is very evident for devices with high data throughput;
for a device with ~12 MB/s of data transfer, I get a few
missing kB of data every few MB transferred randomly.

To solve this problem, a check is introduced which verifies
that indeed all data was copied from the urb to the tty buffer.
If that is not the case, the urb is held in a queue instead
of resubmitting it to the USB subsystem right away. When new
data arrives or the tty is unthrottled, the queue is emptied
again (as far as possible).

Effectively, this change will force the transmitting USB device
to wait until the tty buffer can accept new data again, instead
of discarding the data in this case.

Please excuse the poor code quality, I have no experience
whatsoever in kernel development.

Signed-off-by: Sven Brauch <mail@svenbrauch.de>
---
 drivers/usb/class/cdc-acm.c | 93 +++++++++++++++++++++++++++++++++++++++++----
 drivers/usb/class/cdc-acm.h |  3 ++
 2 files changed, 89 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 5c8f581..eafe64c 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -402,14 +402,68 @@ 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 int acm_process_read_urb(struct acm *acm, struct urb *urb)
 {
+	int size;
+
 	if (!urb->actual_length)
-		return;
+		return 0;
 
-	tty_insert_flip_string(&acm->port, urb->transfer_buffer,
-			urb->actual_length);
+	size = tty_insert_flip_string(&acm->port, urb->transfer_buffer,
+		urb->actual_length);
 	tty_flip_buffer_push(&acm->port);
+	return size;
+}
+
+static bool acm_push_leftover_data_to_tty(struct acm *acm) {
+	int j, k;
+	int urb_index;
+	unsigned long remaining, offset;
+	int written;
+	struct urb *urb;
+	struct acm_rb *rb;
+	bool may_submit_new = true;
+
+	/* TODO: Does this locking mechanism make any sense? I don't know.
+	         Some sort of lock is certainly required. */
+	unsigned long flags;
+	spin_lock_irqsave(&acm->resubmit_lock, flags);
+
+	/* Loop over the queued read urbs, and submit as much data as possible to the tty. */
+	for (j = 0; j < ACM_NR; j++) {
+		urb_index = acm->held_urbs[j];
+		remaining = acm->remaining_data_in_read_urbs[urb_index];
+		if (remaining == 0)
+			continue;
+
+		urb = acm->read_urbs[urb_index];
+		rb = urb->context;
+		offset = urb->actual_length - remaining;
+		written = tty_insert_flip_string(&acm->port, (char*) (urb->transfer_buffer) + offset,
+			remaining);
+		tty_flip_buffer_push(&acm->port);
+		acm->remaining_data_in_read_urbs[urb_index] = remaining - written;
+		if (remaining == written) {
+			/* The urb's buffer was fully submitted to the tty, it can be passed
+			 * to the USB subsystem again. */
+			set_bit(rb->index, &acm->read_urbs_free);
+			acm_submit_read_urb(acm, rb->index, GFP_ATOMIC);
+			acm->num_held_urbs--;
+		}
+		else {
+			/* There is no point in continuing now, the buffer is full */
+			may_submit_new = false;
+			break;
+		}
+	}
+	/* Move the remaining queued urbs to the beginning of the queue */
+	for (k = 0; k < acm->num_held_urbs; k++) {
+		acm->held_urbs[k] = acm->held_urbs[k+j];
+	}
+
+	spin_unlock_irqrestore(&acm->resubmit_lock, flags);
+
+	return may_submit_new;
 }
 
 static void acm_read_bulk_callback(struct urb *urb)
@@ -418,6 +472,11 @@ static void acm_read_bulk_callback(struct urb *urb)
 	struct acm *acm = rb->instance;
 	unsigned long flags;
 	int status = urb->status;
+	int size;
+	bool may_resubmit, may_submit_new;
+
+	/* First look if any filled urbs are still in the queue */
+	may_submit_new = acm_push_leftover_data_to_tty(acm);
 
 	dev_vdbg(&acm->data->dev, "%s - urb %d, len %d\n", __func__,
 					rb->index, urb->actual_length);
@@ -437,20 +496,36 @@ static void acm_read_bulk_callback(struct urb *urb)
 
 	usb_mark_last_busy(acm->dev);
 
-	acm_process_read_urb(acm, urb);
+	/*
+	 * Only try to post data of this new urb if the queue is empty,
+	 * otherwise put it to the end of the queue.
+	 */
+	size = may_submit_new ? acm_process_read_urb(acm, urb) : 0;
+
+	may_resubmit = true;
 	/*
 	 * Unthrottle may run on another CPU which needs to see events
 	 * in the same order. Submission has an implict barrier
 	 */
 	smp_mb__before_atomic();
-	set_bit(rb->index, &acm->read_urbs_free);
+	if (size != urb->actual_length) {
+		/* do not resubmit the urb, but mark it as filled instead */
+		acm->remaining_data_in_read_urbs[rb->index] = urb->actual_length - size;
+		acm->held_urbs[acm->num_held_urbs++] = rb->index;
+		may_resubmit = false;
+	}
+	else {
+		set_bit(rb->index, &acm->read_urbs_free);
+	}
 
 	/* throttle device if requested by tty */
 	spin_lock_irqsave(&acm->read_lock, flags);
 	acm->throttled = acm->throttle_req;
 	if (!acm->throttled) {
 		spin_unlock_irqrestore(&acm->read_lock, flags);
-		acm_submit_read_urb(acm, rb->index, GFP_ATOMIC);
+		if ( may_resubmit ) {
+			acm_submit_read_urb(acm, rb->index, GFP_ATOMIC);
+		}
 	} else {
 		spin_unlock_irqrestore(&acm->read_lock, flags);
 	}
@@ -774,6 +849,8 @@ static void acm_tty_unthrottle(struct tty_struct *tty)
 	acm->throttle_req = 0;
 	spin_unlock_irq(&acm->read_lock);
 
+	acm_push_leftover_data_to_tty(acm);
+
 	if (was_throttled)
 		acm_submit_read_urbs(acm, GFP_KERNEL);
 }
@@ -1367,6 +1444,8 @@ made_compressed_probe:
 		struct acm_rb *rb = &(acm->read_buffers[i]);
 		struct urb *urb;
 
+		acm->remaining_data_in_read_urbs[i] = 0;
+
 		rb->base = usb_alloc_coherent(acm->dev, readsize, GFP_KERNEL,
 								&rb->dma);
 		if (!rb->base)
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index ffeb3c8..0946b0a 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -93,6 +93,9 @@ struct acm {
 	struct acm_wb wb[ACM_NW];
 	unsigned long read_urbs_free;
 	struct urb *read_urbs[ACM_NR];
+	int held_urbs[ACM_NR+1], num_held_urbs; /* fixed-size queue for not-yet-processed read urbs */
+	unsigned long remaining_data_in_read_urbs[ACM_NR]; /* amount of bytes in each held urb */
+	spinlock_t resubmit_lock;
 	struct acm_rb read_buffers[ACM_NR];
 	int rx_buflimit;
 	int rx_endpoint;
-- 
2.4.6




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply related	[flat|nested] 21+ messages in thread