Linux: try harder not to lose any data

We would previously lose any data that was present on a cancelled URB.
Work harder to make sure this doesn't happen.

2 files changed, 91 insertions, 11 deletions

diff --git a/libusb/core.c b/libusb/core.c
index ce914ce..92eb6bf 100644
--- a/libusb/core.c
+++ b/libusb/core.c
@@ -238,6 +238,62 @@ if (cfg != desired)
  *
  * The above method works because once an interface is claimed, no application
  * or driver is able to select another configuration.
+ *
+ * \section cancel Cancellation, early completion and timeouts
+ *
+ * NOTE: This section is currently Linux-centric. I am not sure if any of these
+ * considerations apply to Darwin or other platforms.
+ *
+ * Cancellation of transfers can produce some slightly undesirable results.
+ * It is important to understand that a transfer timing out can also result in
+ * cancellation, as can a transfer that completes early (with a short packet).
+ *
+ * The timeout case is easy to comprehend; if a transfer times out, libusb
+ * will cancel it at the point of timeout and inform you of this. Hence timeout
+ * equals cancellation.
+ *
+ * The early completion case needs a little more explanation: When you submit
+ * a large transfer, libusb may need to divide it into several smaller
+ * sub-transfers which are then submitted to the host controller in parallel
+ * for performance reasons. If one sub-transfer completes early, libusb then
+ * needs to cancel all the subsequent sub-transfers before returning the result
+ * to you.
+ *
+ * All forms of cancellation including the usual libusb_cancel_transfer() can
+ * add some complications as it is possible that data transfer may be happening
+ * while libusb is performing cancellation. This can lead to the following
+ * kinds of situations:
+ *  - A transfer may timeout or be cancelled by the user. While libusb is
+ *    cancelling the transfer (which may consist of several sub-transfers
+ *    which libusb has to cancel one-by-one), data may start to be transferred.
+ *    libusb will report successful cancellation as usual, and will reflect the
+ *    situation with the \ref libusb_transfer::actual_length "actual_length"
+ *    field of the transfer, and for device-to-host transfers, the appropriate
+ *    amount of data will be present in \ref libusb_transfer::buffer "buffer").
+ *  - Similarly, a few USB-level packets within the transfer may already have
+ *    been transferred when the timeout/cancellation occurred, and more
+ *    packets may complete during the time needed to cancel the transfer.
+ *  - A non-ultimate sub-transfer within a transfer may complete with a short
+ *    packet, prompting libusb to immediately cancel all subsequent
+ *    sub-transfers and report early completion. However, during the time
+ *    needed to cancel the subsequent transfers, more data may be transferred.
+ *    This is a difficult situation because the libusb API does not currently
+ *    have a way of indicating the point at which the transfer ended relative
+ *    to the surplus data that was transferred.
+ *
+ * In all cases where the transfer is a device-to-host transfer and surplus
+ * data is recieved as above, libusb places it in the transfer buffer as if
+ * it had arrived contiguously and updates
+ * \ref libusb_transfer::actual_length "actual_length" accordingly.
+ *
+ * We hope to eliminate some of these difficulties in the near future, but
+ * kernel changes may be required.
+ *
+ * Ultimately, if you cancel a transfer before it has completed then you are
+ * obligated to handle the above caveats and to resynchronize with the device
+ * at the application level. Also, remember that timeouts are simply time-based
+ * cancellations which libusb makes convenient for you, hence the same
+ * considerations apply.
  */
 
 /**
diff --git a/libusb/os/linux_usbfs.c b/libusb/os/linux_usbfs.c
index f9899cf..1280188 100644
--- a/libusb/os/linux_usbfs.c
+++ b/libusb/os/linux_usbfs.c
@@ -1710,22 +1710,41 @@ static int handle_bulk_completion(struct usbi_transfer *itransfer,
 
 	tpriv->num_retired++;
 
-	if (urb->status == 0 ||
-			(urb->status == -EOVERFLOW && urb->actual_length > 0))
-		itransfer->transferred += urb->actual_length;
-
 	if (tpriv->reap_action != NORMAL) {
 		/* cancelled, submit_fail, or completed early */
-		if (urb->status == 0 && tpriv->reap_action == COMPLETED_EARLY) {
-			/* FIXME we could solve this extreme corner case with a memmove
-			 * or something */
-			usbi_warn(ITRANSFER_CTX(itransfer), "SOME DATA LOST! "
-				"(completed early but remaining urb completed)");
+		usbi_dbg("abnormal reap: urb status %d", urb->status);
+
+		/* even though we're in the process of cancelling, it's possible that
+		 * we may receive some data in these URBs that we don't want to lose.
+		 * examples:
+		 * 1. while the kernel is cancelling all the packets that make up an
+		 *    URB, a few of them might complete. so we get back a successful
+		 *    cancellation *and* some data.
+		 * 2. we receive a short URB which marks the early completion condition,
+		 *    so we start cancelling the remaining URBs. however, we're too
+		 *    slow and another URB completes (or at least completes partially).
+		 *
+		 * When this happens, our objectives are not to lose any "surplus" data,
+		 * and also to stick it at the end of the previously-received data
+		 * (closing any holes), so that libusb reports the total amount of
+		 * transferred data and presents it in a contiguous chunk.
+		 */
+		if (urb->actual_length > 0) {
+			struct libusb_transfer *transfer =
+				__USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer);
+			unsigned char *target = transfer->buffer + itransfer->transferred;
+			usbi_dbg("received %d bytes of surplus data", urb->actual_length);
+			if (urb->buffer != target) {
+				usbi_dbg("moving surplus data from offset %d to offset %d",
+					(unsigned char *) urb->buffer - transfer->buffer,
+					target - transfer->buffer);
+				memmove(target, urb->buffer, urb->actual_length);
+			}
+			itransfer->transferred += urb->actual_length;
 		}
-		usbi_dbg("CANCEL: urb status %d", urb->status);
 
 		if (tpriv->num_retired == num_urbs) {
-			usbi_dbg("CANCEL: last URB handled, reporting");
+			usbi_dbg("abnormal reap: last URB handled, reporting");
 			if (tpriv->reap_action == CANCELLED) {
 				free(tpriv->urbs);
 				tpriv->urbs = NULL;
@@ -1739,6 +1758,11 @@ static int handle_bulk_completion(struct usbi_transfer *itransfer,
 		return 0;
 	}
 
+	if (urb->status == 0 ||
+			(urb->status == -EOVERFLOW && urb->actual_length > 0))
+		itransfer->transferred += urb->actual_length;
+
+
 	switch (urb->status) {
 	case 0:
 		break;