[PATCH] libusb: Add support for the new USBDEVFS_URB_BULK_CONTINUATION flag

[David Moore]

From: David Moore <dcm@...>

Add support for the new USBDEVFS_URB_BULK_CONTINUATION flag to libusb.

This flag, which is expected to be available in usbfs starting with
kernel 2.6.32, allows the kernel to cancel multiple URBs upon receipt
of a short packet.  This capability allows libusb to preserve data
integrity of large bulk transfers that are split into multiple URBs.
Without this support, these URBs must be canceled in userspace upon
receipt of a short packet, a race condition against future transfers
which might partially fill these canceled URBs.

This patch automatically detects whether a supported kernel is present
and enables the use of the flag when possible.

Signed-off-by: David Moore <dcm@...>
---
 libusb/os/linux_usbfs.c |   39 +++++++++++++++++++++++++++++++++++++--
 libusb/os/linux_usbfs.h |    3 +++
 2 files changed, 40 insertions(+), 2 deletions(-)

This patch deserves extensive testing on kernels with and without
support for USBDEVFS_URB_BULK_CONTINUATION.

diff --git a/libusb/os/linux_usbfs.c b/libusb/os/linux_usbfs.c
index 1280188..9940b67 100644
--- a/libusb/os/linux_usbfs.c
+++ b/libusb/os/linux_usbfs.c
@@ -86,6 +86,8 @@ struct linux_device_priv {
 
 struct linux_device_handle_priv {
  int fd;
+ char supports_flag_short_not_ok;
+ char supports_flag_bulk_continuation;
 };
 
 enum reap_action {
@@ -989,6 +991,8 @@ static int op_open(struct libusb_device_handle *handle)
  return LIBUSB_ERROR_IO;
  }
  }
+ hpriv->supports_flag_short_not_ok = 1;
+ hpriv->supports_flag_bulk_continuation = 1;
 
  return usbi_add_pollfd(HANDLE_CTX(handle), hpriv->fd, POLLOUT);
 }
@@ -1253,6 +1257,28 @@ static void free_iso_urbs(struct linux_transfer_priv *tpriv)
  tpriv->iso_urbs = NULL;
 }
 
+static int submit_bulk_ioctl(struct linux_device_handle_priv *dpriv,
+ struct usbfs_urb *urb)
+{
+ int r = ioctl(dpriv->fd, IOCTL_USBFS_SUBMITURB, urb);
+ if (r < 0 && errno == EINVAL &&
+ (urb->flags & USBFS_URB_BULK_CONTINUATION)) {
+ usbi_dbg("disabled BULK_CONTINUATION flag");
+ dpriv->supports_flag_bulk_continuation = 0;
+ urb->flags &= ~USBFS_URB_BULK_CONTINUATION;
+ r = ioctl(dpriv->fd, IOCTL_USBFS_SUBMITURB, urb);
+ }
+ if (r < 0 && errno == EINVAL &&
+ (urb->flags & USBFS_URB_SHORT_NOT_OK)) {
+ usbi_dbg("disabled SHORT_NOT_OK flag");
+ dpriv->supports_flag_short_not_ok = 0;
+ dpriv->supports_flag_bulk_continuation = 0;
+ urb->flags &= ~USBFS_URB_SHORT_NOT_OK;
+ r = ioctl(dpriv->fd, IOCTL_USBFS_SUBMITURB, urb);
+ }
+ return r;
+}
+
 static int submit_bulk_transfer(struct usbi_transfer *itransfer,
  unsigned char urb_type)
 {
@@ -1300,6 +1326,8 @@ static int submit_bulk_transfer(struct usbi_transfer *itransfer,
  urb->type = urb_type;
  urb->endpoint = transfer->endpoint;
  urb->buffer = transfer->buffer + (i * MAX_BULK_BUFFER_LENGTH);
+ if (dpriv->supports_flag_short_not_ok)
+ urb->flags = USBFS_URB_SHORT_NOT_OK;
  if (i == num_urbs - 1 && last_urb_partial)
  urb->buffer_length = transfer->length % MAX_BULK_BUFFER_LENGTH;
  else if (transfer->length == 0)
@@ -1307,7 +1335,10 @@ static int submit_bulk_transfer(struct usbi_transfer *itransfer,
  else
  urb->buffer_length = MAX_BULK_BUFFER_LENGTH;
 
- r = ioctl(dpriv->fd, IOCTL_USBFS_SUBMITURB, urb);
+ if (i > 0 && dpriv->supports_flag_bulk_continuation)
+ urb->flags |= USBFS_URB_BULK_CONTINUATION;
+
+ r = submit_bulk_ioctl(dpriv, urb);
  if (r < 0) {
  int j;
 
@@ -1758,7 +1789,7 @@ static int handle_bulk_completion(struct usbi_transfer *itransfer,
  return 0;
  }
 
- if (urb->status == 0 ||
+ if (urb->status == 0 || urb->status == -EREMOTEIO ||
  (urb->status == -EOVERFLOW && urb->actual_length > 0))
  itransfer->transferred += urb->actual_length;
 
@@ -1766,6 +1797,8 @@ static int handle_bulk_completion(struct usbi_transfer *itransfer,
  switch (urb->status) {
  case 0:
  break;
+ case -EREMOTEIO: /* short transfer */
+ break;
  case -EPIPE:
  usbi_dbg("detected endpoint stall");
  status = LIBUSB_TRANSFER_STALL;
@@ -1806,6 +1839,8 @@ static int handle_bulk_completion(struct usbi_transfer *itransfer,
  * before reporting results */
  tpriv->reap_action = COMPLETED_EARLY;
  for (i = urb_idx + 1; i < tpriv->num_urbs; i++) {
+ if (tpriv->urbs[i].flags & USBFS_URB_BULK_CONTINUATION)
+ continue;
  int r = ioctl(dpriv->fd, IOCTL_USBFS_DISCARDURB, &tpriv->urbs[i]);
  if (r && errno != EINVAL)
  usbi_warn(TRANSFER_CTX(transfer),
diff --git a/libusb/os/linux_usbfs.h b/libusb/os/linux_usbfs.h
index fdf5e9b..8f0d60d 100644
--- a/libusb/os/linux_usbfs.h
+++ b/libusb/os/linux_usbfs.h
@@ -81,6 +81,9 @@ struct usbfs_iso_packet_desc {
 #define MAX_BULK_BUFFER_LENGTH 16384
 #define MAX_CTRL_BUFFER_LENGTH 4096
 
+#define USBFS_URB_SHORT_NOT_OK 0x01
+#define USBFS_URB_BULK_CONTINUATION 0x04
+
 struct usbfs_urb {
  unsigned char type;
  unsigned char endpoint;
--
1.6.0.6




------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Libusb-devel mailing list
Libusb-devel@...
https://lists.sourceforge.net/lists/listinfo/libusb-devel

[David Moore]

Anybody have any further comments on this patch?  Daniel, is it suitable
for inclusion?

-David

On Sat, 2009-09-05 at 23:12 -0700, David Moore wrote:

> From: David Moore <dcm@...>
>
> Add support for the new USBDEVFS_URB_BULK_CONTINUATION flag to libusb.
>
> This flag, which is expected to be available in usbfs starting with
> kernel 2.6.32, allows the kernel to cancel multiple URBs upon receipt
> of a short packet.  This capability allows libusb to preserve data
> integrity of large bulk transfers that are split into multiple URBs.
> Without this support, these URBs must be canceled in userspace upon
> receipt of a short packet, a race condition against future transfers
> which might partially fill these canceled URBs.
>
> This patch automatically detects whether a supported kernel is present
> and enables the use of the flag when possible.
>
> Signed-off-by: David Moore <dcm@...>
> ---
>  libusb/os/linux_usbfs.c |   39 +++++++++++++++++++++++++++++++++++++--
>  libusb/os/linux_usbfs.h |    3 +++
>  2 files changed, 40 insertions(+), 2 deletions(-)
>
> This patch deserves extensive testing on kernels with and without
> support for USBDEVFS_URB_BULK_CONTINUATION.
>
> diff --git a/libusb/os/linux_usbfs.c b/libusb/os/linux_usbfs.c
> index 1280188..9940b67 100644
> --- a/libusb/os/linux_usbfs.c
> +++ b/libusb/os/linux_usbfs.c
> @@ -86,6 +86,8 @@ struct linux_device_priv {
>  
>  struct linux_device_handle_priv {
>   int fd;
> + char supports_flag_short_not_ok;
> + char supports_flag_bulk_continuation;
>  };
>  
>  enum reap_action {
> @@ -989,6 +991,8 @@ static int op_open(struct libusb_device_handle *handle)
>   return LIBUSB_ERROR_IO;
>   }
>   }
> + hpriv->supports_flag_short_not_ok = 1;
> + hpriv->supports_flag_bulk_continuation = 1;
>  
>   return usbi_add_pollfd(HANDLE_CTX(handle), hpriv->fd, POLLOUT);
>  }
> @@ -1253,6 +1257,28 @@ static void free_iso_urbs(struct linux_transfer_priv *tpriv)
>   tpriv->iso_urbs = NULL;
>  }
>  
> +static int submit_bulk_ioctl(struct linux_device_handle_priv *dpriv,
> + struct usbfs_urb *urb)
> +{
> + int r = ioctl(dpriv->fd, IOCTL_USBFS_SUBMITURB, urb);
> + if (r < 0 && errno == EINVAL &&
> + (urb->flags & USBFS_URB_BULK_CONTINUATION)) {
> + usbi_dbg("disabled BULK_CONTINUATION flag");
> + dpriv->supports_flag_bulk_continuation = 0;
> + urb->flags &= ~USBFS_URB_BULK_CONTINUATION;
> + r = ioctl(dpriv->fd, IOCTL_USBFS_SUBMITURB, urb);
> + }
> + if (r < 0 && errno == EINVAL &&
> + (urb->flags & USBFS_URB_SHORT_NOT_OK)) {
> + usbi_dbg("disabled SHORT_NOT_OK flag");
> + dpriv->supports_flag_short_not_ok = 0;
> + dpriv->supports_flag_bulk_continuation = 0;
> + urb->flags &= ~USBFS_URB_SHORT_NOT_OK;
> + r = ioctl(dpriv->fd, IOCTL_USBFS_SUBMITURB, urb);
> + }
> + return r;
> +}
> +
>  static int submit_bulk_transfer(struct usbi_transfer *itransfer,
>   unsigned char urb_type)
>  {
> @@ -1300,6 +1326,8 @@ static int submit_bulk_transfer(struct usbi_transfer *itransfer,
>   urb->type = urb_type;
>   urb->endpoint = transfer->endpoint;
>   urb->buffer = transfer->buffer + (i * MAX_BULK_BUFFER_LENGTH);
> + if (dpriv->supports_flag_short_not_ok)
> + urb->flags = USBFS_URB_SHORT_NOT_OK;
>   if (i == num_urbs - 1 && last_urb_partial)
>   urb->buffer_length = transfer->length % MAX_BULK_BUFFER_LENGTH;
>   else if (transfer->length == 0)
> @@ -1307,7 +1335,10 @@ static int submit_bulk_transfer(struct usbi_transfer *itransfer,
>   else
>   urb->buffer_length = MAX_BULK_BUFFER_LENGTH;
>  
> - r = ioctl(dpriv->fd, IOCTL_USBFS_SUBMITURB, urb);
> + if (i > 0 && dpriv->supports_flag_bulk_continuation)
> + urb->flags |= USBFS_URB_BULK_CONTINUATION;
> +
> + r = submit_bulk_ioctl(dpriv, urb);
>   if (r < 0) {
>   int j;
>  
> @@ -1758,7 +1789,7 @@ static int handle_bulk_completion(struct usbi_transfer *itransfer,
>   return 0;
>   }
>  
> - if (urb->status == 0 ||
> + if (urb->status == 0 || urb->status == -EREMOTEIO ||
>   (urb->status == -EOVERFLOW && urb->actual_length > 0))
>   itransfer->transferred += urb->actual_length;
>  
> @@ -1766,6 +1797,8 @@ static int handle_bulk_completion(struct usbi_transfer *itransfer,
>   switch (urb->status) {
>   case 0:
>   break;
> + case -EREMOTEIO: /* short transfer */
> + break;
>   case -EPIPE:
>   usbi_dbg("detected endpoint stall");
>   status = LIBUSB_TRANSFER_STALL;
> @@ -1806,6 +1839,8 @@ static int handle_bulk_completion(struct usbi_transfer *itransfer,
>   * before reporting results */
>   tpriv->reap_action = COMPLETED_EARLY;
>   for (i = urb_idx + 1; i < tpriv->num_urbs; i++) {
> + if (tpriv->urbs[i].flags & USBFS_URB_BULK_CONTINUATION)
> + continue;
>   int r = ioctl(dpriv->fd, IOCTL_USBFS_DISCARDURB, &tpriv->urbs[i]);
>   if (r && errno != EINVAL)
>   usbi_warn(TRANSFER_CTX(transfer),
> diff --git a/libusb/os/linux_usbfs.h b/libusb/os/linux_usbfs.h
> index fdf5e9b..8f0d60d 100644
> --- a/libusb/os/linux_usbfs.h
> +++ b/libusb/os/linux_usbfs.h
> @@ -81,6 +81,9 @@ struct usbfs_iso_packet_desc {
>  #define MAX_BULK_BUFFER_LENGTH 16384
>  #define MAX_CTRL_BUFFER_LENGTH 4096
>  
> +#define USBFS_URB_SHORT_NOT_OK 0x01
> +#define USBFS_URB_BULK_CONTINUATION 0x04
> +
>  struct usbfs_urb {
>   unsigned char type;
>   unsigned char endpoint;

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Libusb-devel mailing list
Libusb-devel@...
https://lists.sourceforge.net/lists/listinfo/libusb-devel

[Alan Stern]

On Sun, 20 Sep 2009, David Moore wrote:

> Anybody have any further comments on this patch?  Daniel, is it suitable
> for inclusion?

The comment I made to Michael Lewis applies here too: As written this
code might think an unrelated error means that the SHORT_NOT_OK or
BULK_CONTINUATION flags aren't supported.

> > diff --git a/libusb/os/linux_usbfs.c b/libusb/os/linux_usbfs.c
> > index 1280188..9940b67 100644
> > --- a/libusb/os/linux_usbfs.c
> > +++ b/libusb/os/linux_usbfs.c
> > @@ -86,6 +86,8 @@ struct linux_device_priv {
> >  
> >  struct linux_device_handle_priv {
> >   int fd;
> > + char supports_flag_short_not_ok;
> > + char supports_flag_bulk_continuation;
> >  };

One other thing.  Support of SHORT_NOT_OK and BULK_CONTINUATION is a
property of the kernel, not of each particular USB device.  Hence these
flags should be global variables; there shouldn't be a separate copy
for each device handle.

Alan Stern


------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Libusb-devel mailing list
Libusb-devel@...
https://lists.sourceforge.net/lists/listinfo/libusb-devel

[David Moore]

On Sun, 2009-09-20 at 21:02 -0400, Alan Stern wrote:
> On Sun, 20 Sep 2009, David Moore wrote:
>
> > Anybody have any further comments on this patch?  Daniel, is it suitable
> > for inclusion?
>
> The comment I made to Michael Lewis applies here too: As written this
> code might think an unrelated error means that the SHORT_NOT_OK or
> BULK_CONTINUATION flags aren't supported.
>

That's true, but there are 3 retries and all three will fail if it turns
out to be an unrelated error.  Disabling of the flags will mostly be
moot in that case.  Especially since it's only disabled per-handle as
you noticed below.

> > > diff --git a/libusb/os/linux_usbfs.c b/libusb/os/linux_usbfs.c
> > > index 1280188..9940b67 100644
> > > --- a/libusb/os/linux_usbfs.c
> > > +++ b/libusb/os/linux_usbfs.c
> > > @@ -86,6 +86,8 @@ struct linux_device_priv {
> > >  
> > >  struct linux_device_handle_priv {
> > >   int fd;
> > > + char supports_flag_short_not_ok;
> > > + char supports_flag_bulk_continuation;
> > >  };
>
> One other thing.  Support of SHORT_NOT_OK and BULK_CONTINUATION is a
> property of the kernel, not of each particular USB device.  Hence these
> flags should be global variables; there shouldn't be a separate copy
> for each device handle.

I generally prefer to avoid global variables when I can, so I figured it
wouldn't really hurt anything to do per-handle in this case -- and it
helps the error recovery as discussed above.

-David


------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Libusb-devel mailing list
Libusb-devel@...
https://lists.sourceforge.net/lists/listinfo/libusb-devel

[Daniel Drake]

David Moore wrote:
> Anybody have any further comments on this patch?  Daniel, is it suitable
> for inclusion?

Sorry for the delay on this. It looks good. I think I would prefer to
check the kernel version instead of blindly trying the flag, but this is
a small detail.

It does need testing, as you point out, and with my work and travels
I've been struggling to find  the time to do this. Hopefully I'll be
able to do this towards the end of the month -- but I would also be
happy to trust someone elses test results if someone is interested in
helping me out :)

Daniel

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
_______________________________________________
Libusb-devel mailing list
Libusb-devel@...
https://lists.sourceforge.net/lists/listinfo/libusb-devel