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.

diff --git a/libusb/core.c b/libusb/core.c
index ce914ce..92eb6bf 100644 (file)
--- 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 (file)
--- 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;