[PATCH] linux: Don't report LIBUSB_ERROR_NOT_FOUND when cancelling unreaped urbs

[Vitali Lovich]

Shouldn't the correct behaviour be that libusb_cancel_transfer returns an error if the transfers have already completed?  I'm thinking it's an error that reap_action gets set to CANCELLED.  I think it should remain unchanged when discard_urbs detects this scenario.

So the behaviour change I think that represents the better approach is that libusb_cancel_transfer still returns with ERROR_NOT_FOUND meaning the transfer wasn't found to cancel if all URBs have been submitted.  If part of the URBs have been transferred but other haven't been yet, then we return with LIBUSB_ERROR_IO (unless another error takes precedence).

The behaviour is as follows:
cancel when no transfer has occured: LIBUSB_SUCCESS & cancel callback called 
cancel when partial transfer has occured: LIBUSB_ERROR_IO & cancel callback called
cancel when transfer has completed: LIBUSB_ERROR_NOT_FOUND & completion transfer called (unless some error occured where the error handler will be called).

My approach (I apologize if my e-mail client munges the patch):

diff --git a/libusb/os/linux_usbfs.c b/libusb/os/linux_usbfs.c
index 5702ed5..a1d2622 100644
--- a/libusb/os/linux_usbfs.c
+++ b/libusb/os/linux_usbfs.c
@@ -1417,10 +1417,19 @@ static int discard_urbs(struct usbi_transfer *itransfer, int first, int last_plu
 
 		if (EINVAL == errno) {
 			usbi_dbg("URB not found --> assuming ready to be reaped");
-			ret = LIBUSB_ERROR_NOT_FOUND;
+			if (i == last_plus_one - 1) {
+				// last URB has already been processed, so the transfer is actually already
+				// complete just not processed (race condition)
+				ret = LIBUSB_ERROR_NOT_FOUND;
+				break;
+			} else if (ret == 0) {
+				// some set of URBs have already been processed but we're discarding others
+				ret = LIBSUB_ERROR_IO;
+			}
 		} else if (ENODEV == errno) {
 			usbi_dbg("Device not found for URB --> assuming ready to be reaped");
 			ret = LIBUSB_ERROR_NO_DEVICE;
+			break;
 		} else {
 			usbi_warn(TRANSFER_CTX(transfer),
 				"unrecognised discard errno %d", errno);
@@ -1782,6 +1791,7 @@ static int op_cancel_transfer(struct usbi_transfer *itransfer)
 	struct linux_transfer_priv *tpriv = usbi_transfer_get_os_priv(itransfer);
 	struct libusb_transfer *transfer =
 		__USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer);
+	enum reap_action next_reap_action = tpriv->reap_action;
 
 	switch (transfer->type) {
 	case LIBUSB_TRANSFER_TYPE_BULK:
@@ -1791,7 +1801,7 @@ static int op_cancel_transfer(struct usbi_transfer *itransfer)
 	case LIBUSB_TRANSFER_TYPE_CONTROL:
 	case LIBUSB_TRANSFER_TYPE_INTERRUPT:
 	case LIBUSB_TRANSFER_TYPE_ISOCHRONOUS:
-		tpriv->reap_action = CANCELLED;
+		next_reap_action = CANCELLED;
 		break;
 	default:
 		usbi_err(TRANSFER_CTX(transfer),
@@ -1802,7 +1812,11 @@ static int op_cancel_transfer(struct usbi_transfer *itransfer)
 	if (!tpriv->urbs)
 		return LIBUSB_ERROR_NOT_FOUND;
 
-	return discard_urbs(itransfer, 0, tpriv->num_urbs);
+	ret = discard_urbs(itransfer, 0, tpriv->num_urbs);
+	if (ret != LIBUSB_ERROR_NOT_FOUND) {
+		tpriv->reap_action = next_reap_action;
+	}
+	return ret;
 }
 
 static void op_clear_transfer_priv(struct usbi_transfer *itransfer)

On Feb 10, 2012, at 8:54 AM, Hans de Goede wrote:

> IOCTL_USBFS_DISCARDURB sets errno to EINVAL when the kernel could not find the
> urb in the kernels urbs in flight list. Which means that the urb has already
> completed. However:
> 
> 1) The only caller of discard_urbs who checks the return value is
>   op_cancel_transfer
> 2) op_cancel_transfer checks that tpriv->urbs != NULL before calling
>   discard_urbs
> 3) Since tpriv->urbs != NULL we know that:
>   a) The urb has been submitted
>   b) The urb has not been reaped yet
> 4) Since op_cancel_transfer sets tpriv->reap_action to CANCELLED before
>   calling discard_urbs, when the urb eventually does get reaped its callback
>   will get called with a status of LIBUSB_TRANSFER_CANCELLED, even if
>   the IOCTL_USBFS_DISCARDURB call failed with an errno of EINVAL
> 
> Thus I believe it is wrong for op_cancel_transfer to return
> LIBUSB_ERROR_NOT_FOUND in the case when the urb has completed at the kernel
> level, but has not yet been reaped esp. since there is no race free way for an
> app to deal with this, even if an app calls libusb_handle_events immediately
> before calling libusb_cancel_transfer, the urb may still complete in between.
> 
> Moreover in this failure scenario:
> 1) libusb_cancel_transfer returns LIBUSB_ERROR_NOT_FOUND, which one would
>   assume means that the transfers callback function won't get called, since
>   the transfer was appearently not in flight; but
> 2) the transfer callback will still get called as soon as the urb gets
>   reaped (from libusb_handle_events*); *and* not only will it get called, but
> 3) the transfer callback will report a status of LIBUSB_TRANSFER_CANCELLED
>   despite libusb_cancel_transfer reporting a failure to cancel!
> 
> Given all of the above this patch modifies discard_urbs to treat
> an errno of EINVAL as success, since this simply means that an *unreaped*
> urb has already completed, which is an unavoidable scenario and as such
> should not be treated as an error.
> 
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
> libusb/os/linux_usbfs.c |    1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/libusb/os/linux_usbfs.c b/libusb/os/linux_usbfs.c
> index 2b81189..bd9caa3 100644
> --- a/libusb/os/linux_usbfs.c
> +++ b/libusb/os/linux_usbfs.c
> @@ -1466,7 +1466,6 @@ static int discard_urbs(struct usbi_transfer *itransfer, int first, int last_plu
> 
> 		if (EINVAL == errno) {
> 			usbi_dbg("URB not found --> assuming ready to be reaped");
> -			ret = LIBUSB_ERROR_NOT_FOUND;
> 		} else if (ENODEV == errno) {
> 			usbi_dbg("Device not found for URB --> assuming ready to be reaped");
> 			ret = LIBUSB_ERROR_NO_DEVICE;
> -- 
> 1.7.7.6
> 
> 
> _______________________________________________
> libusbx mailing list
> libusbx at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/libusbx