| Message ID | b81cd5b5281cfbfdadb002c4bcf5c9be7c017cfd.1609828485.git.Thinh.Nguyen@synopsys.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a5c7682aaaa10e42928d73de1c9e1e02d2b14c2e |
| Headers | show |
| Series | [v2] usb: dwc3: gadget: Clear wait flag on dequeue | expand |
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > If an active transfer is dequeued, then the endpoint is freed to start a > new transfer. Make sure to clear the endpoint's transfer wait flag for > this case. > > Cc: stable@vger.kernel.org > Fixes: e0d19563eb6c ("usb: dwc3: gadget: Wait for transfer completion") > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > --- > Changes in v2: > - Only clear the wait flag if the selected request is of an active transfer. > Otherwise, any dequeue will change the endpoint's state even if it's for > some random request. > > drivers/usb/dwc3/gadget.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 78cb4db8a6e4..9a00dcaca010 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1763,6 +1763,8 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep, > list_for_each_entry_safe(r, t, &dep->started_list, list) > dwc3_gadget_move_cancelled_request(r); > > + dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE; I'm not sure this is correct. This could create a race condition between clearing this bit and getting the transfer complete interrupt. It also seems to break the assumptions made by dwc3_gadget_endpoint_trbs_complete() (actually its users), specially regarding ISOC endpoints. Have you verified all transfer types with this commit?
Hi Felipe, Felipe Balbi wrote: > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >> If an active transfer is dequeued, then the endpoint is freed to start a >> new transfer. Make sure to clear the endpoint's transfer wait flag for >> this case. >> >> Cc: stable@vger.kernel.org >> Fixes: e0d19563eb6c ("usb: dwc3: gadget: Wait for transfer completion") >> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> --- >> Changes in v2: >> - Only clear the wait flag if the selected request is of an active transfer. >> Otherwise, any dequeue will change the endpoint's state even if it's for >> some random request. >> >> drivers/usb/dwc3/gadget.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 78cb4db8a6e4..9a00dcaca010 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -1763,6 +1763,8 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep, >> list_for_each_entry_safe(r, t, &dep->started_list, list) >> dwc3_gadget_move_cancelled_request(r); >> >> + dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE; > I'm not sure this is correct. This could create a race condition between > clearing this bit and getting the transfer complete interrupt. It also > seems to break the assumptions made by > dwc3_gadget_endpoint_trbs_complete() (actually its users), specially > regarding ISOC endpoints. > > Have you verified all transfer types with this commit? > It shouldn't race. It's protected by the spinlock irq and it doesn't matter whether dwc3_gadget_endpoint_trbs_complete() or this dequeue function clears it first. The flag DWC3_EP_WAIT_TRANSFER_COMPLETE is only applicable to stream transfer as the driver needs to wait for 1 stream to finish before starting another. This is verified with our test environment (which includes UASP CV and others). BR, Thinh
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>> If an active transfer is dequeued, then the endpoint is freed to start a >>> new transfer. Make sure to clear the endpoint's transfer wait flag for >>> this case. >>> >>> Cc: stable@vger.kernel.org >>> Fixes: e0d19563eb6c ("usb: dwc3: gadget: Wait for transfer completion") >>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >>> --- >>> Changes in v2: >>> - Only clear the wait flag if the selected request is of an active transfer. >>> Otherwise, any dequeue will change the endpoint's state even if it's for >>> some random request. >>> >>> drivers/usb/dwc3/gadget.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index 78cb4db8a6e4..9a00dcaca010 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -1763,6 +1763,8 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep, >>> list_for_each_entry_safe(r, t, &dep->started_list, list) >>> dwc3_gadget_move_cancelled_request(r); >>> >>> + dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE; >> I'm not sure this is correct. This could create a race condition between >> clearing this bit and getting the transfer complete interrupt. It also >> seems to break the assumptions made by >> dwc3_gadget_endpoint_trbs_complete() (actually its users), specially >> regarding ISOC endpoints. >> >> Have you verified all transfer types with this commit? >> > > It shouldn't race. It's protected by the spinlock irq and it doesn't > matter whether dwc3_gadget_endpoint_trbs_complete() or this dequeue > function clears it first. The flag DWC3_EP_WAIT_TRANSFER_COMPLETE is > only applicable to stream transfer as the driver needs to wait for 1 > stream to finish before starting another. > > This is verified with our test environment (which includes UASP CV and > others). Fair enough, I'll take your word for it :-) Acked-by: Felipe Balbi <balbi@kernel.org>
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 78cb4db8a6e4..9a00dcaca010 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1763,6 +1763,8 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep, list_for_each_entry_safe(r, t, &dep->started_list, list) dwc3_gadget_move_cancelled_request(r); + dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE; + goto out; } }
If an active transfer is dequeued, then the endpoint is freed to start a new transfer. Make sure to clear the endpoint's transfer wait flag for this case. Cc: stable@vger.kernel.org Fixes: e0d19563eb6c ("usb: dwc3: gadget: Wait for transfer completion") Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> --- Changes in v2: - Only clear the wait flag if the selected request is of an active transfer. Otherwise, any dequeue will change the endpoint's state even if it's for some random request. drivers/usb/dwc3/gadget.c | 2 ++ 1 file changed, 2 insertions(+) base-commit: 2edc7af892d0913bf06f5b35e49ec463f03d5ed8