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?
>> >> + 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 ?
>
HEHE, no problem, I can remove it:)
> --
> balbi
Thanks,
Yu Xu
--
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
