From: Alan S. <st...@ro...> - 2010-10-05 15:08:56
|
This patch fixes the way overflow errors are handled (they can occur in any transaction, not just the last one in a transfer) and it adds tests for other commonly-occuring error statuses. --- Index: libusb/libusb/os/linux_usbfs.c =================================================================== --- libusb.orig/libusb/os/linux_usbfs.c +++ libusb/libusb/os/linux_usbfs.c @@ -1874,38 +1874,34 @@ static int handle_bulk_completion(struct (urb->status == -EOVERFLOW && urb->actual_length > 0)) itransfer->transferred += urb->actual_length; - + /* Many of these errors can occur on *any* urb of a multi-urb transfer. When they do, we have to + tear down the rest of the transfer */ switch (urb->status) { case 0: break; case -EREMOTEIO: /* short transfer */ break; + case -ENOENT: /* cancelled */ + case -ECONNRESET: + break; + case -ESHUTDOWN: + usbi_dbg("device removed"); + goto cancel_remainder; case -EPIPE: usbi_dbg("detected endpoint stall"); - if (tpriv->reap_status == LIBUSB_TRANSFER_COMPLETED) - tpriv->reap_status = LIBUSB_TRANSFER_STALL; goto cancel_remainder; case -EOVERFLOW: - /* overflow can only ever occur in the last urb */ usbi_dbg("overflow, actual_length=%d", urb->actual_length); - if (tpriv->reap_status == LIBUSB_TRANSFER_COMPLETED) - tpriv->reap_status = LIBUSB_TRANSFER_OVERFLOW; - goto completed; + goto cancel_remainder; case -ETIME: case -EPROTO: case -EILSEQ: - /* These can happen on *any* urb of a multi-urb transfer, so need to flag a status and tear down - rest of the transfer before deleting */ usbi_dbg("low level error %d", urb->status); tpriv->reap_action = ERRORED; - if (tpriv->reap_status == LIBUSB_TRANSFER_COMPLETED) - tpriv->reap_status = LIBUSB_TRANSFER_ERROR; goto cancel_remainder; default: usbi_warn(ITRANSFER_CTX(itransfer), "unrecognised urb status %d", urb->status); - if (tpriv->reap_status == LIBUSB_TRANSFER_COMPLETED) - tpriv->reap_status = LIBUSB_TRANSFER_ERROR; goto cancel_remainder; } @@ -1931,6 +1927,8 @@ cancel_remainder: struct linux_device_handle_priv *dpriv = __device_handle_priv(transfer->dev_handle); + if (tpriv->reap_status == LIBUSB_TRANSFER_COMPLETED) + tpriv->reap_status = LIBUSB_TRANSFER_ERROR; if (tpriv->num_retired == num_urbs) { /* no remaining URBs to cancel; this was the last one, so do * the completion */ @@ -2022,6 +2020,11 @@ static int handle_iso_completion(struct switch (urb->status) { case 0: break; + case -ENOENT: /* cancelled */ + break; + case -ESHUTDOWN: + usbi_dbg("device removed"); + break; case -ETIME: case -EPROTO: case -EILSEQ: @@ -2074,6 +2077,12 @@ static int handle_control_completion(str itransfer->transferred = urb->actual_length; status = LIBUSB_TRANSFER_COMPLETED; break; + case -ENOENT: /* cancelled */ + break; + case -ESHUTDOWN: + usbi_dbg("device removed"); + status = LIBUSB_TRANSFER_NO_DEVICE; + break; case -EPIPE: usbi_dbg("unsupported control request"); status = LIBUSB_TRANSFER_STALL; |
From: Peter S. <pe...@st...> - 2010-10-05 16:45:01
|
Alan Stern wrote: > @@ -2022,6 +2020,11 @@ static int handle_iso_completion(struct > switch (urb->status) { > case 0: > break; > + case -ENOENT: /* cancelled */ > + break; > + case -ESHUTDOWN: > + usbi_dbg("device removed"); > + break; > case -ETIME: > case -EPROTO: > case -EILSEQ: > @@ -2074,6 +2077,12 @@ static int handle_control_completion(str > itransfer->transferred = urb->actual_length; > status = LIBUSB_TRANSFER_COMPLETED; > break; > + case -ENOENT: /* cancelled */ > + break; > + case -ESHUTDOWN: > + usbi_dbg("device removed"); > + status = LIBUSB_TRANSFER_NO_DEVICE; > + break; Should all handle_*_completion() be factored together somehow? On purpose no status=NODEVICE for iso transfers? //Peter |
From: Alan S. <st...@ro...> - 2010-10-05 17:28:48
|
On Tue, 5 Oct 2010, Peter Stuge wrote: > Alan Stern wrote: > > @@ -2022,6 +2020,11 @@ static int handle_iso_completion(struct > > switch (urb->status) { > > case 0: > > break; > > + case -ENOENT: /* cancelled */ > > + break; > > + case -ESHUTDOWN: > > + usbi_dbg("device removed"); > > + break; > > case -ETIME: > > case -EPROTO: > > case -EILSEQ: > > @@ -2074,6 +2077,12 @@ static int handle_control_completion(str > > itransfer->transferred = urb->actual_length; > > status = LIBUSB_TRANSFER_COMPLETED; > > break; > > + case -ENOENT: /* cancelled */ > > + break; > > + case -ESHUTDOWN: > > + usbi_dbg("device removed"); > > + status = LIBUSB_TRANSFER_NO_DEVICE; > > + break; > > Should all handle_*_completion() be factored together somehow? That's not a bad idea, if it can be done easily. > On purpose no status=NODEVICE for iso transfers? I probably just forgot it. With iso transfers the overall URB status doesn't mean all that much anyway; the individual packet status values are more important. The only time the URB status matters is when the transfer is cancelled. (Of course, -ESHUTDOWN is an example of a cancellation...) Alan Stern |
From: Peter S. <pe...@st...> - 2010-10-16 22:37:54
|
Alan Stern wrote: > This patch fixes the way overflow errors are handled (they can occur > in any transaction, not just the last one in a transfer) and it adds > tests for other commonly-occuring error statuses. Ok. But.. > case -EPIPE: > usbi_dbg("detected endpoint stall"); > - if (tpriv->reap_status == LIBUSB_TRANSFER_COMPLETED) > - tpriv->reap_status = LIBUSB_TRANSFER_STALL; > goto cancel_remainder; > case -EOVERFLOW: > - /* overflow can only ever occur in the last urb */ > usbi_dbg("overflow, actual_length=%d", urb->actual_length); > - if (tpriv->reap_status == LIBUSB_TRANSFER_COMPLETED) > - tpriv->reap_status = LIBUSB_TRANSFER_OVERFLOW; > - goto completed; > + goto cancel_remainder; Why remove the use of tpriv->reap_status everywhere? //Peter |
From: Alan S. <st...@ro...> - 2010-10-17 02:56:08
|
On Sun, 17 Oct 2010, Peter Stuge wrote: > Alan Stern wrote: > > This patch fixes the way overflow errors are handled (they can occur > > in any transaction, not just the last one in a transfer) and it adds > > tests for other commonly-occuring error statuses. > > Ok. But.. > > > case -EPIPE: > > usbi_dbg("detected endpoint stall"); > > - if (tpriv->reap_status == LIBUSB_TRANSFER_COMPLETED) > > - tpriv->reap_status = LIBUSB_TRANSFER_STALL; > > goto cancel_remainder; > > case -EOVERFLOW: > > - /* overflow can only ever occur in the last urb */ > > usbi_dbg("overflow, actual_length=%d", urb->actual_length); > > - if (tpriv->reap_status == LIBUSB_TRANSFER_COMPLETED) > > - tpriv->reap_status = LIBUSB_TRANSFER_OVERFLOW; > > - goto completed; > > + goto cancel_remainder; > > Why remove the use of tpriv->reap_status everywhere? I didn't remove them -- I combined them all into one place, further down in the code. Alan Stern |
From: Peter S. <pe...@st...> - 2010-10-17 03:34:57
|
Alan Stern wrote: > > > case -EOVERFLOW: > > > - /* overflow can only ever occur in the last urb */ > > > usbi_dbg("overflow, actual_length=%d", urb->actual_length); > > > - if (tpriv->reap_status == LIBUSB_TRANSFER_COMPLETED) > > > - tpriv->reap_status = LIBUSB_TRANSFER_OVERFLOW; > > > - goto completed; > > > + goto cancel_remainder; > > > > Why remove the use of tpriv->reap_status everywhere? > > I didn't remove them -- I combined them all into one place, further > down in the code. Thanks for the pointer. It looks like LIBUSB_TRANSFER_STALL and _OVERFLOW will not be used anymore, and any error results in _ERROR. I think we still want to keep _STALL, but maybe also _OVERFLOW, or no? //Peter |
From: Alan S. <st...@ro...> - 2010-10-17 13:43:34
|
On Sun, 17 Oct 2010, Peter Stuge wrote: > Alan Stern wrote: > > > > case -EOVERFLOW: > > > > - /* overflow can only ever occur in the last urb */ > > > > usbi_dbg("overflow, actual_length=%d", urb->actual_length); > > > > - if (tpriv->reap_status == LIBUSB_TRANSFER_COMPLETED) > > > > - tpriv->reap_status = LIBUSB_TRANSFER_OVERFLOW; > > > > - goto completed; > > > > + goto cancel_remainder; > > > > > > Why remove the use of tpriv->reap_status everywhere? > > > > I didn't remove them -- I combined them all into one place, further > > down in the code. > > Thanks for the pointer. It looks like LIBUSB_TRANSFER_STALL and > _OVERFLOW will not be used anymore, and any error results in _ERROR. > > I think we still want to keep _STALL, but maybe also _OVERFLOW, or no? You're right. I shouldn't have gotten rid of the LIBUSB_TRANSFER_STALL return code; that was a mistake. The _OVERFLOW code doesn't matter so much, but I guess it should be kept also. Clearly those parts of the patch need to be redone. Do you want to do it or shall I? Alan Stern |
From: Peter S. <pe...@st...> - 2010-10-17 16:58:09
|
Alan Stern wrote: > > > > > case -EOVERFLOW: .. > > > > > - if (tpriv->reap_status == LIBUSB_TRANSFER_COMPLETED) > > > > > - tpriv->reap_status = LIBUSB_TRANSFER_OVERFLOW; .. > You're right. I shouldn't have gotten rid of the LIBUSB_TRANSFER_STALL > return code; that was a mistake. The _OVERFLOW code doesn't matter so > much, but I guess it should be kept also. Clearly those parts of the > patch need to be redone. > > Do you want to do it or shall I? I have the patch up in the air here anyway and am happy to do it if it's not too big a change. I think it would be enough to just put the code quoted above back into the stall and overflow cases, right? Once reap_status gets set I don't think it will be overwritten. //Peter |
From: Alan S. <st...@ro...> - 2010-10-17 19:26:29
|
On Sun, 17 Oct 2010, Peter Stuge wrote: > Alan Stern wrote: > > > > > > case -EOVERFLOW: > .. > > > > > > - if (tpriv->reap_status == LIBUSB_TRANSFER_COMPLETED) > > > > > > - tpriv->reap_status = LIBUSB_TRANSFER_OVERFLOW; > .. > > You're right. I shouldn't have gotten rid of the LIBUSB_TRANSFER_STALL > > return code; that was a mistake. The _OVERFLOW code doesn't matter so > > much, but I guess it should be kept also. Clearly those parts of the > > patch need to be redone. > > > > Do you want to do it or shall I? > > I have the patch up in the air here anyway and am happy to do it if > it's not too big a change. I think it would be enough to just put the > code quoted above back into the stall and overflow cases, right? Once > reap_status gets set I don't think it will be overwritten. That's right. Alan Stern |