Hi,
On Wed, Jun 06, 2012 at 03:38:25PM +0800, Yu Xu wrote:
> Hi Balbi,
>
> 2012/6/6 Felipe Balbi <balbi@xxxxxx>:
> > Hi,
> >
> > On Wed, Jun 06, 2012 at 10:10:28AM +0800, Yu Xu wrote:
> >> >> + _ep->maxburst = maxburst = 1;
> >> >
> >> > we generally avoid multiple assignments in one line. We also avoid magic
> >> > constants. Why is this always 1 and why do you overwrite ep->maxburst ?
> >> >
> >> I'll separate the line.
> >> For the maxburst value, if the code run here, it means the
> >> _ep->maxburst is wrong,
> >> so we need to handle it and set it to one to make it according to the SPEC.
> >> And we also handle other type of endpoint like this.
> >> But actually, the function driver should ensure the _ep->maxburst is correct,
> >> the code should not be touch in the normal case.
> >>
> >> >> + }
> >> >> + dev_dbg(u3d->dev,
> >> >> + "maxburst: %d on bulk %s\n", maxburst, ep->name);
> >> >> + break;
> >> >> + case USB_ENDPOINT_XFER_CONTROL:
> >> >> + if (maxburst != 1) {
> >> >> + dev_err(u3d->dev,
> >> >> + "max burst should be 1 on control ep\n");
> >> >> + _ep->maxburst = maxburst = 1;
> >> >
> >> > if any gadget driver puts MaxBurst to anything other than zero, you
> >> > should have a big giant warning about it...
> >> >
> >> Control transfer has the fixed max burst size of one. So we check it here.
> >> Do you mean that?
> >
> > yes I mean that. Look at composite.c:
> >
> >> if (g->speed == USB_SPEED_SUPER) {
> >> switch (usb_endpoint_type(_ep->desc)) {
> >> case USB_ENDPOINT_XFER_ISOC:
> >> /* mult: bits 1:0 of bmAttributes */
> >> _ep->mult = comp_desc->bmAttributes & 0x3;
> >> case USB_ENDPOINT_XFER_BULK:
> >> case USB_ENDPOINT_XFER_INT:
> >> _ep->maxburst = comp_desc->bMaxBurst;
> >> break;
> >> default:
> >> /* Do nothing for control endpoints */
> >> break;
> >> }
> >> }
> >
> > as you can see, control endpoint's maxburst won't even be initialized.
> > In fact, I can see a bug on this code. This should read as:
> >
> > if (g->speed == USB_SPEED_SUPER) {
> > switch (usb_endpoint_type(_ep->desc)) {
> > case USB_ENDPOINT_XFER_ISOC:
> > /* mult: bits 1:0 of bmAttributes */
> > _ep->mult = comp_desc->bmAttributes & 0x3;
> > case USB_ENDPOINT_XFER_BULK:
> > case USB_ENDPOINT_XFER_INT:
> > _ep->maxburst = comp_desc->bMaxBurst + 1;
> > break;
> > default:
> > /* Do nothing for control endpoints */
> > if (comp_desc->bMaxBurst != 0)
> > ERROR(cdev, "ep0 bMaxBurst must be zero\n");
> > _ep->maxburst = 1;
> > break;
> > }
> > }
> >
> > I will fix that and push to v3.6 merge window.
> >
> What do you suggest? So I do not check it here, but set the maxburst
> as 1, right?
For control endpoints you should assume it's correct I guess. For all
other endpoints, you should fix based on your HW's limitations but pring
a dev_dbg() or dev_vdbg() in that case.
--
balbi
Attachment:
signature.asc
Description: Digital signature
