| Message ID | 1618439388-20427-1-git-send-email-wcheng@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: gadget: Avoid canceling current request for queuing error | expand |
Wesley Cheng <wcheng@codeaurora.org> writes: > If an error is received when issuing a start or update transfer > command, the error handler will stop all active requests (including > the current USB request), and call dwc3_gadget_giveback() to notify > function drivers of the requests which have been stopped. Avoid > having to cancel the current request which is trying to be queued, as > the function driver will handle the EP queue error accordingly. > Simply unmap the request as it was done before, and allow previously > started transfers to be cleaned up. > > Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> > --- > drivers/usb/dwc3/gadget.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index e1b04c97..4200775 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1399,6 +1399,11 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) > if (ret == -EAGAIN) > return ret; > > + /* Avoid canceling current request, as it has not been started */ > + if (req->trb) > + memset(req->trb, 0, sizeof(struct dwc3_trb)); we don't need a full memset. I think ensuring HWO bit is zero is enough.
On 4/14/2021 11:26 PM, Felipe Balbi wrote: > Wesley Cheng <wcheng@codeaurora.org> writes: > >> If an error is received when issuing a start or update transfer >> command, the error handler will stop all active requests (including >> the current USB request), and call dwc3_gadget_giveback() to notify >> function drivers of the requests which have been stopped. Avoid >> having to cancel the current request which is trying to be queued, as >> the function driver will handle the EP queue error accordingly. >> Simply unmap the request as it was done before, and allow previously >> started transfers to be cleaned up. >> >> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> >> --- >> drivers/usb/dwc3/gadget.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index e1b04c97..4200775 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -1399,6 +1399,11 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) >> if (ret == -EAGAIN) >> return ret; >> >> + /* Avoid canceling current request, as it has not been started */ >> + if (req->trb) >> + memset(req->trb, 0, sizeof(struct dwc3_trb)); > > we don't need a full memset. I think ensuring HWO bit is zero is enough. > Hi Felipe, Thanks for the input/review, will make this change to just clear the HWO. Thanks Wesley Cheng
Wesley Cheng wrote: > > > On 4/14/2021 11:26 PM, Felipe Balbi wrote: >> Wesley Cheng <wcheng@codeaurora.org> writes: >> >>> If an error is received when issuing a start or update transfer >>> command, the error handler will stop all active requests (including >>> the current USB request), and call dwc3_gadget_giveback() to notify >>> function drivers of the requests which have been stopped. Avoid >>> having to cancel the current request which is trying to be queued, as >>> the function driver will handle the EP queue error accordingly. >>> Simply unmap the request as it was done before, and allow previously >>> started transfers to be cleaned up. >>> >>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> >>> --- >>> drivers/usb/dwc3/gadget.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index e1b04c97..4200775 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -1399,6 +1399,11 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) >>> if (ret == -EAGAIN) >>> return ret; >>> >>> + /* Avoid canceling current request, as it has not been started */ >>> + if (req->trb) >>> + memset(req->trb, 0, sizeof(struct dwc3_trb)); >> >> we don't need a full memset. I think ensuring HWO bit is zero is enough. >> > Hi Felipe, > > Thanks for the input/review, will make this change to just clear the HWO. > Make sure to increment the dequeue pointer also. I think you can use dwc3_gadget_ep_skip_trbs(). BR, Thinh
Thinh Nguyen wrote: > Wesley Cheng wrote: >> >> >> On 4/14/2021 11:26 PM, Felipe Balbi wrote: >>> Wesley Cheng <wcheng@codeaurora.org> writes: >>> >>>> If an error is received when issuing a start or update transfer >>>> command, the error handler will stop all active requests (including >>>> the current USB request), and call dwc3_gadget_giveback() to notify >>>> function drivers of the requests which have been stopped. Avoid >>>> having to cancel the current request which is trying to be queued, as >>>> the function driver will handle the EP queue error accordingly. >>>> Simply unmap the request as it was done before, and allow previously >>>> started transfers to be cleaned up. >>>> >>>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> >>>> --- >>>> drivers/usb/dwc3/gadget.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>> index e1b04c97..4200775 100644 >>>> --- a/drivers/usb/dwc3/gadget.c >>>> +++ b/drivers/usb/dwc3/gadget.c >>>> @@ -1399,6 +1399,11 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) >>>> if (ret == -EAGAIN) >>>> return ret; >>>> >>>> + /* Avoid canceling current request, as it has not been started */ >>>> + if (req->trb) >>>> + memset(req->trb, 0, sizeof(struct dwc3_trb)); >>> >>> we don't need a full memset. I think ensuring HWO bit is zero is enough. >>> >> Hi Felipe, >> >> Thanks for the input/review, will make this change to just clear the HWO. >> > > Make sure to increment the dequeue pointer also. I think you can use > dwc3_gadget_ep_skip_trbs(). > Nevermind. There maybe a problem with using dwc3_gadget_ep_skip_trbs(). Thinh
On 4/15/2021 12:28 PM, Thinh Nguyen wrote: > Thinh Nguyen wrote: >> Wesley Cheng wrote: >>> >>> >>> On 4/14/2021 11:26 PM, Felipe Balbi wrote: >>>> Wesley Cheng <wcheng@codeaurora.org> writes: >>>> >>>>> If an error is received when issuing a start or update transfer >>>>> command, the error handler will stop all active requests (including >>>>> the current USB request), and call dwc3_gadget_giveback() to notify >>>>> function drivers of the requests which have been stopped. Avoid >>>>> having to cancel the current request which is trying to be queued, as >>>>> the function driver will handle the EP queue error accordingly. >>>>> Simply unmap the request as it was done before, and allow previously >>>>> started transfers to be cleaned up. >>>>> >>>>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> >>>>> --- >>>>> drivers/usb/dwc3/gadget.c | 5 +++++ >>>>> 1 file changed, 5 insertions(+) >>>>> >>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>>> index e1b04c97..4200775 100644 >>>>> --- a/drivers/usb/dwc3/gadget.c >>>>> +++ b/drivers/usb/dwc3/gadget.c >>>>> @@ -1399,6 +1399,11 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) >>>>> if (ret == -EAGAIN) >>>>> return ret; >>>>> >>>>> + /* Avoid canceling current request, as it has not been started */ >>>>> + if (req->trb) >>>>> + memset(req->trb, 0, sizeof(struct dwc3_trb)); >>>> >>>> we don't need a full memset. I think ensuring HWO bit is zero is enough. >>>> >>> Hi Felipe, >>> >>> Thanks for the input/review, will make this change to just clear the HWO. >>> >> >> Make sure to increment the dequeue pointer also. I think you can use >> dwc3_gadget_ep_skip_trbs(). >> > > Nevermind. There maybe a problem with using dwc3_gadget_ep_skip_trbs(). > > Thinh > Hi Thinh, Thank you for your input. In this case (if kick transfer fails w/ an error), would we still need to mess with the enqueue/dequeue pointers? Not sure if my assumption is correct, but the TRB wouldn't have been started, so we can use the same (failed) TRB for future requests, right? I think one thing I will need to update is to loop through num_trbs and clear all HWO bits if the above is not needed. Thanks Wesley Cheng
Wesley Cheng wrote: > > > On 4/15/2021 12:28 PM, Thinh Nguyen wrote: >> Thinh Nguyen wrote: >>> Wesley Cheng wrote: >>>> >>>> >>>> On 4/14/2021 11:26 PM, Felipe Balbi wrote: >>>>> Wesley Cheng <wcheng@codeaurora.org> writes: >>>>> >>>>>> If an error is received when issuing a start or update transfer >>>>>> command, the error handler will stop all active requests (including >>>>>> the current USB request), and call dwc3_gadget_giveback() to notify >>>>>> function drivers of the requests which have been stopped. Avoid >>>>>> having to cancel the current request which is trying to be queued, as >>>>>> the function driver will handle the EP queue error accordingly. >>>>>> Simply unmap the request as it was done before, and allow previously >>>>>> started transfers to be cleaned up. >>>>>> >>>>>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> >>>>>> --- >>>>>> drivers/usb/dwc3/gadget.c | 5 +++++ >>>>>> 1 file changed, 5 insertions(+) >>>>>> >>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>>>> index e1b04c97..4200775 100644 >>>>>> --- a/drivers/usb/dwc3/gadget.c >>>>>> +++ b/drivers/usb/dwc3/gadget.c >>>>>> @@ -1399,6 +1399,11 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) >>>>>> if (ret == -EAGAIN) >>>>>> return ret; >>>>>> >>>>>> + /* Avoid canceling current request, as it has not been started */ >>>>>> + if (req->trb) >>>>>> + memset(req->trb, 0, sizeof(struct dwc3_trb)); >>>>> >>>>> we don't need a full memset. I think ensuring HWO bit is zero is enough. >>>>> >>>> Hi Felipe, >>>> >>>> Thanks for the input/review, will make this change to just clear the HWO. >>>> >>> >>> Make sure to increment the dequeue pointer also. I think you can use >>> dwc3_gadget_ep_skip_trbs(). >>> >> >> Nevermind. There maybe a problem with using dwc3_gadget_ep_skip_trbs(). >> >> Thinh >> > Hi Thinh, > > Thank you for your input. In this case (if kick transfer fails w/ an > error), would we still need to mess with the enqueue/dequeue pointers? > Not sure if my assumption is correct, but the TRB wouldn't have been > started, so we can use the same (failed) TRB for future requests, right? > > I think one thing I will need to update is to loop through num_trbs and > clear all HWO bits if the above is not needed. > No. We track and increment the TRB enqueue counter whenever we prepared one. What you need to do is the opposite as dwc3_gadget_ep_skip_trbs(). You'd need to decrement the enqueue counter of all the TRB prepared by the request. Not every request is the same, and we shouldn't rely on the function driver behavior to reuse/resubmit the same request. Also, we use __dwc3_gadget_kick_transfer() everywhere and not just when queuing new request. Make sure that your change only applies when queuing new request failed and not because of other scenarios. Thanks, Thinh
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index e1b04c97..4200775 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1399,6 +1399,11 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) if (ret == -EAGAIN) return ret; + /* Avoid canceling current request, as it has not been started */ + if (req->trb) + memset(req->trb, 0, sizeof(struct dwc3_trb)); + dwc3_gadget_del_and_unmap_request(dep, req, ret); + dwc3_stop_active_transfer(dep, true, true); list_for_each_entry_safe(req, tmp, &dep->started_list, list)
If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up. Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> --- drivers/usb/dwc3/gadget.c | 5 +++++ 1 file changed, 5 insertions(+)