| Message ID | 1449155873-22988-1-git-send-email-balbi@ti.com |
|---|---|
| State | Accepted |
| Commit | 04c03d10e507052cfce6910ddf34091196e79e1c |
| Headers | show |
Hi, John Youn <John.Youn@synopsys.com> 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@ti.com> >> Signed-off-by: Felipe Balbi <balbi@ti.com> >> --- >> >> 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 ? And also patch g_mass_storage to _not_ set Zero flag in this case ? thanks -- balbi
Hi, John Youn <John.Youn@synopsys.com> writes: > On 12/22/2015 9:31 AM, Felipe Balbi wrote: >> >> Hi, >> >> John Youn <John.Youn@synopsys.com> 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@ti.com> >>>> Signed-off-by: Felipe Balbi <balbi@ti.com> >>>> --- >>>> >>>> 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
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); + spin_unlock_irqrestore(&dwc->lock, flags); return ret; @@ -2743,6 +2779,12 @@ int dwc3_gadget_init(struct dwc3 *dwc) goto err3; } + dwc->zlp_buf = kzalloc(DWC3_ZLP_BUF_SIZE, GFP_KERNEL); + if (!dwc->zlp_buf) { + ret = -ENOMEM; + goto err4; + } + dwc->gadget.ops = &dwc3_gadget_ops; dwc->gadget.max_speed = USB_SPEED_SUPER; dwc->gadget.speed = USB_SPEED_UNKNOWN; @@ -2762,16 +2804,19 @@ int dwc3_gadget_init(struct dwc3 *dwc) ret = dwc3_gadget_init_endpoints(dwc); if (ret) - goto err4; + goto err5; ret = usb_add_gadget_udc(dwc->dev, &dwc->gadget); if (ret) { dev_err(dwc->dev, "failed to register udc\n"); - goto err4; + goto err5; } return 0; +err5: + kfree(dwc->zlp_buf); + err4: dwc3_gadget_free_endpoints(dwc); dma_free_coherent(dwc->dev, DWC3_EP0_BOUNCE_SIZE, @@ -2804,6 +2849,7 @@ void dwc3_gadget_exit(struct dwc3 *dwc) dwc->ep0_bounce, dwc->ep0_bounce_addr); kfree(dwc->setup_buf); + kfree(dwc->zlp_buf); dma_free_coherent(dwc->dev, sizeof(*dwc->ep0_trb), dwc->ep0_trb, dwc->ep0_trb_addr);
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@ti.com> Signed-off-by: Felipe Balbi <balbi@ti.com> --- 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(-) -- 2.6.3 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html