Hi,
John Youn <John.Youn@xxxxxxxxxxxx> writes:
> On 12/22/2015 9:31 AM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> John Youn <John.Youn@xxxxxxxxxxxx> writes:
>>> On 12/3/2015 7:18 AM, Felipe Balbi wrote:
>>>> So far, dwc3 has always missed request->zero
>>>> handling for every endpoint. Let's implement
>>>> that so we can handle cases where transfer must
>>>> be finished with a ZLP.
>>>>
>>>> Note that dwc3 is a little special. Even though
>>>> we're dealing with a ZLP, we still need a buffer
>>>> of wMaxPacketSize bytes; to hide that detail from
>>>> every gadget driver, we have a preallocated buffer
>>>> of 1024 bytes (biggest bulk size) to use (and
>>>> share) among all endpoints.
>>>>
>>>> Reported-by: Ravi B <ravibabu@xxxxxx>
>>>> Signed-off-by: Felipe Balbi <balbi@xxxxxx>
>>>> ---
>>>>
>>>> since v1:
>>>> - remove unnecessary 'free_on_complete' flag
>>>>
>>>> since v2:
>>>> - remove unnecessary 'out' label
>>>>
>>>> drivers/usb/dwc3/core.h | 3 +++
>>>> drivers/usb/dwc3/gadget.c | 50 +++++++++++++++++++++++++++++++++++++++++++++--
>>>> 2 files changed, 51 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>>> index 36f1cb74588c..29130682e547 100644
>>>> --- a/drivers/usb/dwc3/core.h
>>>> +++ b/drivers/usb/dwc3/core.h
>>>> @@ -37,6 +37,7 @@
>>>> #define DWC3_MSG_MAX 500
>>>>
>>>> /* Global constants */
>>>> +#define DWC3_ZLP_BUF_SIZE 1024 /* size of a superspeed bulk */
>>>> #define DWC3_EP0_BOUNCE_SIZE 512
>>>> #define DWC3_ENDPOINTS_NUM 32
>>>> #define DWC3_XHCI_RESOURCES_NUM 2
>>>> @@ -647,6 +648,7 @@ struct dwc3_scratchpad_array {
>>>> * @ctrl_req: usb control request which is used for ep0
>>>> * @ep0_trb: trb which is used for the ctrl_req
>>>> * @ep0_bounce: bounce buffer for ep0
>>>> + * @zlp_buf: used when request->zero is set
>>>> * @setup_buf: used while precessing STD USB requests
>>>> * @ctrl_req_addr: dma address of ctrl_req
>>>> * @ep0_trb: dma address of ep0_trb
>>>> @@ -734,6 +736,7 @@ struct dwc3 {
>>>> struct usb_ctrlrequest *ctrl_req;
>>>> struct dwc3_trb *ep0_trb;
>>>> void *ep0_bounce;
>>>> + void *zlp_buf;
>>>> void *scratchbuf;
>>>> u8 *setup_buf;
>>>> dma_addr_t ctrl_req_addr;
>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>> index e341f034296f..e916c11ded59 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -1158,6 +1158,32 @@ out:
>>>> return ret;
>>>> }
>>>>
>>>> +static void __dwc3_gadget_ep_zlp_complete(struct usb_ep *ep,
>>>> + struct usb_request *request)
>>>> +{
>>>> + dwc3_gadget_ep_free_request(ep, request);
>>>> +}
>>>> +
>>>> +static int __dwc3_gadget_ep_queue_zlp(struct dwc3 *dwc, struct dwc3_ep *dep)
>>>> +{
>>>> + struct dwc3_request *req;
>>>> + struct usb_request *request;
>>>> + struct usb_ep *ep = &dep->endpoint;
>>>> +
>>>> + dwc3_trace(trace_dwc3_gadget, "queueing ZLP\n");
>>>> + request = dwc3_gadget_ep_alloc_request(ep, GFP_ATOMIC);
>>>> + if (!request)
>>>> + return -ENOMEM;
>>>> +
>>>> + request->length = 0;
>>>> + request->buf = dwc->zlp_buf;
>>>> + request->complete = __dwc3_gadget_ep_zlp_complete;
>>>> +
>>>> + req = to_dwc3_request(request);
>>>> +
>>>> + return __dwc3_gadget_ep_queue(dep, req);
>>>> +}
>>>> +
>>>> static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request,
>>>> gfp_t gfp_flags)
>>>> {
>>>> @@ -1171,6 +1197,16 @@ static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request,
>>>>
>>>> spin_lock_irqsave(&dwc->lock, flags);
>>>> ret = __dwc3_gadget_ep_queue(dep, req);
>>>> +
>>>> + /*
>>>> + * Okay, here's the thing, if gadget driver has requested for a ZLP by
>>>> + * setting request->zero, instead of doing magic, we will just queue an
>>>> + * extra usb_request ourselves so that it gets handled the same way as
>>>> + * any other request.
>>>> + */
>>>> + if (ret == 0 && request->zero && (request->length % ep->maxpacket == 0))
>>>> + ret = __dwc3_gadget_ep_queue_zlp(dwc, dep);
>>>
>>> Hi Felipe,
>>>
>>> This causes regression with at least mass storage + Windows host.
>>>
>>> When the gadget queues a ZLP, we end up sending two ZLPs which leads
>>> to violating the MSC protocol.
>>
>> heh, no idea why mass storage would set Zero flag in this case :-p
>>
>>> The following fixes it:
>>>
>>> - if (ret == 0 && request->zero && (request->length % ep->maxpacket == 0))
>>> + if (ret == 0 && request->zero && (request->length % ep->maxpacket == 0) &&
>>> + (request->length != 0))
>>
>> Can you send this as a proper patch ?
>
> Sure I can do that. I thought you might want fix it in place since
> it's in your testing/next.
it's already in next :-(
>> And also patch g_mass_storage to
>> _not_ set Zero flag in this case ?
>>
>
> The mass storage driver has always done that and I think it is ok. It
> sets this flag for the mass storage data IN phase and the data might
> be various lengths including zero-length.
* @zero: If true, when writing data, makes the last packet be "short"
* by adding a zero length packet as needed;
I read that as:
if (length % wMaxPacketSize == 0 && zero)
add_zlp();
and I'd assume the gadget driver should be careful when adding zero
flag. In fact, I'd say that "as needed" part should be removed so that
UDC has to only:
if (zero)
add_zlp();
but that's another story entirely :-)
> It should be up to the controller to insert the ZLP as needed to
> terminate the transfer. And if the length is already short or zero,
> then there is no need to do so.
>
> What do you think?
sure, let's go with only your dwc3 fix for now and discuss 'zero' flag
usage during v4.5-rc cycle, if at all ;-)
--
balbi
Attachment:
signature.asc
Description: PGP signature
