Hi,
> Hi,
>
> On Tue, Dec 17, 2013 at 03:59:31PM +0900, Anton Tikhomirov wrote:
> > In accordance with specification, when sent data length is
>
> please mention section of specification.
USB2.0 spec., 8.5.3.2 Variable-length Data Stage
>
> > an exact multiple of wMaxPacketSize for the pipe and less
> > than requested by host, the function shall return a zero-length
> > packet (ZLP) to indicate the end of the Data stage to a USB host.
>
> hmm... so in USB3 mode that would be host requesting 513 bytes and us
> sending only 512.
Our customer reported this issue. In their case, Windows USB2.0 host
requests Configuration descriptor with wLength = 255. Device replies
with two 64 bytes IN transactions, and stalls EP0 on 3rd IN transaction,
where it has to reply with ZLP. Unfortunately, we don’t have full
picture of what's happening on their side and why host requests
more bytes than actual length of Configuration descriptor.
>
> > @@ -619,6 +619,7 @@ struct dwc3_scratchpad_array {
> > * @three_stage_setup: set if we perform a three phase setup
> > * @ep0_bounced: true when we used bounce buffer
> > * @ep0_expect_in: true when we expect a DATA IN transfer
> > + * @ep0_zlp_sent: true when ZLP was sent
>
> I would rather have a ep0_needs_zlp flag.
ok
>
> > diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> > index 21a3520..cf72561 100644
> > --- a/drivers/usb/dwc3/ep0.c
> > +++ b/drivers/usb/dwc3/ep0.c
> > @@ -782,6 +782,9 @@ static void dwc3_ep0_complete_data(struct dwc3
> *dwc,
> > return;
> > }
> >
> > + if (dwc->ep0_zlp_sent)
> > + goto finish_zlp;
> > +
> > length = trb->size & DWC3_TRB_SIZE_MASK;
> >
> > if (dwc->ep0_bounced) {
> > @@ -802,14 +805,24 @@ static void dwc3_ep0_complete_data(struct dwc3
> *dwc,
> > /* for some reason we did not get everything out */
> >
> > dwc3_ep0_stall_and_restart(dwc);
> > - } else {
> > - /*
> > - * handle the case where we have to send a zero packet.
> This
> > - * seems to be case when req.length > maxpacket. Could it
> be?
> > - */
> > - if (r)
By the way, why do we need this check? If r is NULL, we will have
panic above in this function, where ur is dereferenced.
> > - dwc3_gadget_giveback(ep0, r, 0);
> > + return;
> > }
> > +
> > + /* handle the case where we have to send a zero packet */
> > + if ((epnum & 1) && ur->zero &&
> > + (ur->length % ep0->endpoint.maxpacket == 0)) {
> > + int ret;
> > +
> > + ret = dwc3_ep0_start_trans(dwc, epnum, dwc->ctrl_req_addr,
> 0,
> > + DWC3_TRBCTL_CONTROL_DATA);
> > + WARN_ON(ret < 0);
> > + dwc->ep0_zlp_sent = 1;
> > + return;
> > + }
>
> note that this causes a slight bug. Code expects to receive a
> NRDY_STATUS, but we're sending another CONTROL_DATA which means we will
> receive XFER_COMPLETE_DATA.
>
> It's only working because Control(Data) lost its XferNotReady handling
> due to a silicon bug we found. If someone ever patches that handler,
> this will be a hard-to-track problem.
My bad, will fix this.
>
> how have you tested this ? Any changes to ep0 handling must come with a
> libusb-based testcase so I can make sure nothing else gets broken (yes,
> new requirement :-)
>
> Also make sure to run testusb for control endpoints and leave it
> running
> for weeks. You should be able to survive 4 weeks of stress test without
> any issues.
>
> Don't forget to run through USB30CV ch9 on USB3 and USB2 modes.
>
> Sorry dude, but I can't accept regressions and this code has been
> exercised pretty well.
Thank you for review. I will rework the patch and test carefully.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
