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.
> >> + return -EINVAL;
> >> +
> >> + u3d = ep->u3d;
> >> +
> >> + /* Get the endpoint context address */
> >> + ep_context = ep->ep_context;
> >> +
> >> + direction = mv_u3d_ep_dir(ep);
> >> +
> >> + /* nuke all pending requests (does flush) */
> >> + mv_u3d_nuke(ep, -ESHUTDOWN);
> >> +
> >> + /* Disable the endpoint for Rx or Tx and reset the endpoint type */
> >> + if (direction == MV_U3D_EP_DIR_OUT) {
> >> + epxcr = ioread32(&u3d->vuc_regs->epcr[ep->ep_num].epxoutcr1);
> >> + epxcr &= ~((1 << MV_U3D_EPXCR_EP_ENABLE_SHIFT)
> >> + | USB_ENDPOINT_XFERTYPE_MASK);
> >> + iowrite32(epxcr, &u3d->vuc_regs->epcr[ep->ep_num].epxoutcr1);
> >> + } else {
> >> + epxcr = ioread32(&u3d->vuc_regs->epcr[ep->ep_num].epxincr1);
> >> + epxcr &= ~((1 << MV_U3D_EPXCR_EP_ENABLE_SHIFT)
> >> + | USB_ENDPOINT_XFERTYPE_MASK);
> >> + iowrite32(epxcr, &u3d->vuc_regs->epcr[ep->ep_num].epxincr1);
> >> + }
> >> +
> >> + ep->enabled = 0;
> >> +
> >> + ep->ep.desc = NULL;
> >> + return 0;
> >> +}
> >> +
> >> +static struct usb_request *
> >> +mv_u3d_alloc_request(struct usb_ep *_ep, gfp_t gfp_flags)
> >> +{
> >> + struct mv_u3d_req *req = NULL;
> >> +
> >> + req = kzalloc(sizeof *req, gfp_flags);
> >> + if (!req)
> >> + return NULL;
> >> +
> >> + INIT_LIST_HEAD(&req->queue);
> >> +
> >> + return &req->req;
> >> +}
> >> +
> >> +static void mv_u3d_free_request(struct usb_ep *_ep, struct usb_request *_req)
> >> +{
> >> + struct mv_u3d_req *req = container_of(_req, struct mv_u3d_req, req);
> >> +
> >> + kfree(req);
> >> +}
> >> +
> >> +static void mv_u3d_ep_fifo_flush(struct usb_ep *_ep)
> >> +{
> >> + struct mv_u3d *u3d;
> >> + u32 direction;
> >> + struct mv_u3d_ep *ep = container_of(_ep, struct mv_u3d_ep, ep);
> >> + unsigned int loops;
> >> + u32 tmp;
> >> +
> >> + /* if endpoint is not enabled, cannot flush endpoint */
> >> + if (!ep->enabled)
> >> + return;
> >> +
> >> + u3d = ep->u3d;
> >> + direction = mv_u3d_ep_dir(ep);
> >> +
> >> + /* ep0 need clear bit after flushing fifo. */
> >> + if (!ep->ep_num) {
> >> + if (direction == MV_U3D_EP_DIR_OUT) {
> >> + tmp = ioread32(&u3d->vuc_regs->epcr[0].epxoutcr0);
> >> + tmp |= MV_U3D_EPXCR_EP_FLUSH;
> >> + iowrite32(tmp, &u3d->vuc_regs->epcr[0].epxoutcr0);
> >> + udelay(10);
> >> + tmp &= ~MV_U3D_EPXCR_EP_FLUSH;
> >> + iowrite32(tmp, &u3d->vuc_regs->epcr[0].epxoutcr0);
> >> + } else {
> >> + tmp = ioread32(&u3d->vuc_regs->epcr[0].epxincr0);
> >> + tmp |= MV_U3D_EPXCR_EP_FLUSH;
> >> + iowrite32(tmp, &u3d->vuc_regs->epcr[0].epxincr0);
> >> + udelay(10);
> >> + tmp &= ~MV_U3D_EPXCR_EP_FLUSH;
> >> + iowrite32(tmp, &u3d->vuc_regs->epcr[0].epxincr0);
> >> + }
> >> + return;
> >> + }
> >> +
> >> + if (direction == MV_U3D_EP_DIR_OUT) {
> >> + tmp = ioread32(&u3d->vuc_regs->epcr[ep->ep_num].epxoutcr0);
> >> + tmp |= MV_U3D_EPXCR_EP_FLUSH;
> >> + iowrite32(tmp, &u3d->vuc_regs->epcr[ep->ep_num].epxoutcr0);
> >> +
> >> + /* Wait until flushing completed */
> >> + loops = LOOPS(MV_U3D_FLUSH_TIMEOUT);
> >> + while (ioread32(&u3d->vuc_regs->epcr[ep->ep_num].epxoutcr0) &
> >> + MV_U3D_EPXCR_EP_FLUSH) {
> >> + /*
> >> + * EP_FLUSH bit should be cleared to indicate this
> >> + * operation is complete
> >> + */
> >> + if (loops == 0) {
> >> + dev_dbg(u3d->dev,
> >> + "EP FLUSH TIMEOUT for ep%d%s\n", ep->ep_num,
> >> + direction ? "in" : "out");
> >> + return;
> >> + }
> >> + loops--;
> >> + udelay(LOOPS_USEC);
> >> + }
> >> + } else { /* EP_DIR_IN */
> >> + tmp = ioread32(&u3d->vuc_regs->epcr[ep->ep_num].epxincr0);
> >> + tmp |= MV_U3D_EPXCR_EP_FLUSH;
> >> + iowrite32(tmp, &u3d->vuc_regs->epcr[ep->ep_num].epxincr0);
> >> +
> >> + /* Wait until flushing completed */
> >> + loops = LOOPS(MV_U3D_FLUSH_TIMEOUT);
> >> + while (ioread32(&u3d->vuc_regs->epcr[ep->ep_num].epxincr0) &
> >> + MV_U3D_EPXCR_EP_FLUSH) {
> >> + /*
> >> + * EP_FLUSH bit should be cleared to indicate this
> >> + * operation is complete
> >> + */
> >> + if (loops == 0) {
> >> + dev_dbg(u3d->dev,
> >> + "EP FLUSH TIMEOUT for ep%d%s\n", ep->ep_num,
> >> + direction ? "in" : "out");
> >> + return;
> >> + }
> >> + loops--;
> >> + udelay(LOOPS_USEC);
> >> + }
> >> + }
> >> +}
> >> +
> >> +/* queues (submits) an I/O request to an endpoint */
> >> +static int
> >> +mv_u3d_ep_queue(struct usb_ep *_ep, struct usb_request *_req, gfp_t gfp_flags)
> >> +{
> >> + struct mv_u3d_ep *ep = container_of(_ep, struct mv_u3d_ep, ep);
> >> + struct mv_u3d_req *req = container_of(_req, struct mv_u3d_req, req);
> >> + struct mv_u3d *u3d = ep->u3d;
> >> + unsigned long flags;
> >> + int is_first_req = 0;
> >> +
> >> + if (!ep->ep_num
> >> + && u3d->ep0_state == MV_U3D_STATUS_STAGE
> >> + && !_req->length) {
> >> + dev_dbg(u3d->dev, "ep0 status stage\n");
> >> + u3d->ep0_state = MV_U3D_WAIT_FOR_SETUP;
> >> + return 0;
> >> + }
> >> +
> >> + dev_dbg(u3d->dev, "%s: %s, req: 0x%x\n",
> >> + __func__, _ep->name, (u32)req);
> >> + /* catch various bogus parameters */
> >> + if (!_req || !req->req.complete || !req->req.buf
> >> + || !list_empty(&req->queue)) {
> >> + dev_err(u3d->dev,
> >> + "%s, bad params, _req: 0x%x,"
> >> + "req->req.complete: 0x%x, req->req.buf: 0x%x,"
> >> + "list_empty: 0x%x\n",
> >> + __func__, (u32)_req,
> >> + (u32)req->req.complete, (u32)req->req.buf,
> >> + (u32)list_empty(&req->queue));
> >> + return -EINVAL;
> >> + }
> >> + if (unlikely(!_ep || !ep->ep.desc)) {
> >> + dev_err(u3d->dev, "%s, bad ep\n", __func__);
> >> + return -EINVAL;
> >> + }
> >> + if (ep->ep.desc->bmAttributes == USB_ENDPOINT_XFER_ISOC) {
> >> + if (req->req.length > ep->ep.maxpacket)
> >> + return -EMSGSIZE;
> >> + }
> >> +
> >> + if (!u3d->driver || u3d->gadget.speed == USB_SPEED_UNKNOWN) {
> >> + dev_err(u3d->dev,
> >> + "%s, bad params of driver/speed\n", __func__);
> >
> > do you really need the function name here ??
> >
> It can help to locate the error point if error occurs. What do you think?
it might... but OTOH, this is the only place on the driver where you
print the function name and also the only place which prints "bad params
of driver/speed". So it should be very easy to find with grep, right ?
--
balbi
Attachment:
signature.asc
Description: Digital signature
