On Thu, Oct 23, 2014 at 10:18:41PM -0500, Felipe Balbi wrote:
> HI,
>
> On Fri, Oct 24, 2014 at 10:14:19AM +0900, Anton Tikhomirov wrote:
> >
> > Hi,
> >
> > On Thu, 23 Oct 2014, Alan Stern wrote:
> > > On Thu, 23 Oct 2014, Felipe Balbi wrote:
> > >
> > > > here's v2:
> > > >
> > > > 8<--------------------------------------------------------------
> > > >
> > > > From 1080b54d66e3e77410b41732e76746ed8e2c01c7 Mon Sep 17 00:00:00
> > > 2001
> > > > From: Felipe Balbi <balbi@xxxxxx>
> > > > Date: Tue, 30 Sep 2014 10:39:14 -0500
> > > > Subject: [PATCH] usb: dwc3: ep0: fix Data Phase for transfer sizes
> > > aligned to
> > > > wMaxPacketSize
> > > >
> > > > According to Section 8.5.3.2 of the USB 2.0 specification,
> > > > a USB device must terminate a Data Phase with either a
> > > > short packet or a ZLP (if the previous transfer was
> > > > a multiple of wMaxPacketSize).
> > > >
> > > > For reference, here's what the USB 2.0 specification, section
> > > > 8.5.3.2 says:
> > > >
> > > > "
> > > > 8.5.3.2 Variable-length Data Stage
> > > >
> > > > A control pipe may have a variable-length data phase
> > > > in which the host requests more data than is contained
> > > > in the specified data structure. When all of the data
> > > > structure is returned to the host, the function should
> > > > indicate that the Data stage is ended by returning a
> > > > packet that is shorter than the MaxPacketSize for the
> > > > pipe. If the data structure is an exact multiple of
> > > > wMaxPacketSize for the pipe, the function will return
> > > > a zero-length packet to indicate the end of the Data
> > > > stage.
> > > > "
> > > >
> > > > Signed-off-by: Felipe Balbi <balbi@xxxxxx>
> > > > ---
> > > > drivers/usb/dwc3/ep0.c | 19 +++++++++++++------
> > > > 1 file changed, 13 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> > > > index a47cc1e..ce6b0c7 100644
> > > > --- a/drivers/usb/dwc3/ep0.c
> > > > +++ b/drivers/usb/dwc3/ep0.c
> > > > @@ -828,12 +828,19 @@ static void dwc3_ep0_complete_data(struct dwc3
> > > *dwc,
> > > >
> > > > 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)
> > > > - dwc3_gadget_giveback(ep0, r, 0);
> > > > + dwc3_gadget_giveback(ep0, r, 0);
> > >
> > > Don't you want to wait until the ZLP has completed before doing the
> > > giveback?
> > >
> > > In fact, shouldn't all this ZLP code run when the transfer is
> > > submitted, rather than when it completes? The idea is that you get a
> > > request, you queue up all the necessary TRBs or whatever, and if an
> > > extra ZLP is needed then you queue up an extra TRB.
> >
> > You may want to check my patch one more time. I sent it for review 10
> > monthes ago:
> >
> > [PATCH] usb: dwc3: ep0: Handle variable-length Data Stage
> >
> > It works just fine for us in our custom kernel.
>
> you also said you'd send another version (see [1]) and never did. 10
> months passed and I had even forgotten about this issue until I decided
> to run all gadget drivers through USB[23]0CV and found that g_ncm falls
> into this same bug, so I wrote the patch above.
>
> considering you never sent another version even after 10 months, I'll
> just go ahead with this one and work on improving TRB handling on this
> driver so that when req->zero is true we can actually allocate a
> separate TRB (as has been discussed on this and previous threads).
>
> I'll add a note to commit log stating that you provided the original
> patch but failed to provide a follow up.
actually, I can't do that anymore as I have already moved this commit to
my 'fixes' branch.
--
balbi
Attachment:
signature.asc
Description: Digital signature
