[PATCH] usb: dwc3: gadget: set status of request on every completed trb

[PATCH] usb: dwc3: gadget: set status of request on every completed trb

[Michael Grzeschik]

Currently the status of the request being completed comes from the
dwc3_event_depevt status. The resulting status will then be applied to
the request on dwc3_gadget_del_and_unmap_request.

This assigned status is not right in every case. Since it is possible
that more requests can be ready on the interrupt handler we have to set
the actual status for every request from the trbstatus instead.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 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 a0c883f19a417c..760af09d6d8ef7 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -3171,6 +3171,11 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
 	count = trb->size & DWC3_TRB_SIZE_MASK;
 	req->remaining += count;
 
+	if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC)
+		req->request.status = -EXDEV;
+	else if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_OK)
+		req->request.status = 0;
+
 	if ((trb->ctrl & DWC3_TRB_CTRL_HWO) && status != -ESHUTDOWN)
 		return 1;
 
-- 
2.30.2

Re: [PATCH] usb: dwc3: gadget: set status of request on every completed trb

[Thinh Nguyen]

Michael Grzeschik wrote:
> Currently the status of the request being completed comes from the
> dwc3_event_depevt status. The resulting status will then be applied to
> the request on dwc3_gadget_del_and_unmap_request.
> 
> This assigned status is not right in every case. Since it is possible
> that more requests can be ready on the interrupt handler we have to set
> the actual status for every request from the trbstatus instead.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  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 a0c883f19a417c..760af09d6d8ef7 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3171,6 +3171,11 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>  	count = trb->size & DWC3_TRB_SIZE_MASK;
>  	req->remaining += count;
>  
> +	if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC)
> +		req->request.status = -EXDEV;
> +	else if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_OK)
> +		req->request.status = 0;
> +
>  	if ((trb->ctrl & DWC3_TRB_CTRL_HWO) && status != -ESHUTDOWN)
>  		return 1;
>  

req->request.status should only be set at one place, and currently it's
in dwc3_gadget_del_and_unmap_request() when we give back the request.

You need to fix dwc3_gadget_ep_reclaim_completed_trb() to not use the
"status" from the event.

Check and save the first TRB error and report to the request's status.
Don't overwrite it with something new if there are chained TRBs in the
request.

I also notice that DEPEVT_STATUS_BUSERR is not applicable to Transfer in
Progress and Transfer Complete events (that bit is reserved for those
events). While you're working on this, maybe create a separate patch to
remove that check also.

Thanks,
Thinh

Re: [PATCH] usb: dwc3: gadget: set status of request on every completed trb

[Sergey Shtylyov]

Hello!

On 3/8/22 12:46 AM, Michael Grzeschik wrote:

> Currently the status of the request being completed comes from the
> dwc3_event_depevt status. The resulting status will then be applied to
> the request on dwc3_gadget_del_and_unmap_request.
> 
> This assigned status is not right in every case. Since it is possible
> that more requests can be ready on the interrupt handler we have to set
> the actual status for every request from the trbstatus instead.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  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 a0c883f19a417c..760af09d6d8ef7 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3171,6 +3171,11 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>  	count = trb->size & DWC3_TRB_SIZE_MASK;
>  	req->remaining += count;
>  
> +	if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC)
> +		req->request.status = -EXDEV;
> +	else if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_OK)
> +		req->request.status = 0;
> +

   Shouldn't that be a *switch* statement instead?

[...]

MBR, Sergey