> -----Original Message----- > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Saturday, February 26, 2011 10:34 PM > To: Paul Zimmerman > Cc: Greg Kroah-Hartman; linux-usb@xxxxxxxxxxxxxxx; Xu, Andiry > Subject: Re: [PATCH] USB: Add support for SuperSpeed isoc endpoints > > 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. > I think the define of ep->ss_ep_comp.bmAttributes bit mask should also be included in this patch. And Paul, you have tested this patch with a SuperSpeed isoc device? Thanks, Andiry -- 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
