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.
> 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.
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?
John
--
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
