On Fri, Feb 25, 2011 at 06:35:41PM -0800, Paul Zimmerman wrote: > > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > > On Fri, Feb 25, 2011 at 01:45:03PM -0800, Paul Zimmerman wrote: > > > Use the Mult and bMaxBurst values from the endpoint companion > > > descriptor to calculate the max length of an isoc transfer > > > > > > Signed-off-by: Paul Zimmerman <paulz@xxxxxxxxxxxx> > > > --- > > > drivers/usb/core/urb.c | 11 ++++++++++- > > > 1 files changed, 10 insertions(+), 1 deletions(-) > > > > > > diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c > > > index c14fc08..991f33e 100644 > > > --- a/drivers/usb/core/urb.c > > > +++ b/drivers/usb/core/urb.c > > > @@ -366,7 +366,16 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags) > > > if (xfertype == USB_ENDPOINT_XFER_ISOC) { > > > int n, len; > > > > > > - /* FIXME SuperSpeed isoc endpoints have up to 16 bursts */ > > > + /* SuperSpeed isoc endpoints have up to 16 bursts of up to > > > + * 3 packets each > > > + */ > > > + if (dev->speed == USB_SPEED_SUPER) { > > > + int burst = 1 + ep->ss_ep_comp.bMaxBurst; > > > + int mult = 1 + ep->ss_ep_comp.bmAttributes; Hmm, it looks like bmAttributes has some reserved bits in it. This patch is fine for now, but it could cause issues later if the USB-IF choses to extend the spec and use those reserved bits. I'll take this patch for now, but I think I want to include a second patch to extend the #defines in include/linux/usb/ch9.h to have a proper bit mask for bmAttributes: /* USB_DT_SS_ENDPOINT_COMP: SuperSpeed Endpoint Companion descriptor */ struct usb_ss_ep_comp_descriptor { __u8 bLength; __u8 bDescriptorType; __u8 bMaxBurst; __u8 bmAttributes; __u16 wBytesPerInterval; } __attribute__ ((packed)); #define USB_DT_SS_EP_COMP_SIZE 6 /* Bits 4:0 of bmAttributes if this is a bulk endpoint */ #define USB_SS_MAX_STREAMS(p) (1 << (p & 0x1f)) Maybe something like this? /* Bits 1:0 of bmAttributes if this is a isoc endpoint */ #define USB_SS_MULT(p) (1 << (p & 0x3)) > > Paul, please don't create new variables in an if block. Create them at > > the top of the function. > > Hi Sarah, > > I'm sorry, but Linus disagrees with you. He recently expressed a preference > for variables that are only used within a block, to be defined within the > block instead of at the top of the function. Plus I am following the style that > is already used within that function. Please take a closer look. Ah, apparently I am out-of-date. :) Andiry mostly wrote the isochronous code, so I've Cc'd him. > > > + max *= burst; > > > + max *= mult; > > > + } > > > + > > > > Where is max used later? > > max is used about 12 lines further down: > > len = urb->iso_frame_desc[n].length; > if (len < 0 || len > max) > return -EMSGSIZE; > > > I'll have to look further in the code to see if this actually works. I > > don't think you can just use the endpoint calculations, because the > > driver may want to submit a slightly shorter transfer length. It's > > really the USB core's job to check whether the buffer size is bigger > > than what the endpoint can handle. The code should go there instead. > > This code is in the USB core. Please take a closer look at the function, > this is the proper place to check. I have tested this, and without the > patch it is impossible to submit an isoc URB that is larger than a single > packet. With the patch, it works. Ok, that makes sense. I'll queue this patch up. Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
