From: Jeff Vanhoof qjv001@motorola.com
arm-smmu related crashes seen after a Missed ISOC interrupt when no_interrupt=1 is used. This can happen if the hardware is still using the data associated with a TRB after the usb_request's ->complete call has been made. Instead of immediately releasing a request when a Missed ISOC interrupt has occurred, this change will add logic to cancel the request instead where it will eventually be released when the END_TRANSFER command has completed. This logic is similar to some of the cleanup done in dwc3_gadget_ep_dequeue.
Fixes: 6d8a019614f3 ("usb: dwc3: gadget: check for Missed Isoc from event status") Cc: stable@vger.kernel.org Signed-off-by: Jeff Vanhoof qjv001@motorola.com Co-developed-by: Dan Vacura w36195@motorola.com Signed-off-by: Dan Vacura w36195@motorola.com --- V1 -> V3: - no change, new patch in series
drivers/usb/dwc3/core.h | 1 + drivers/usb/dwc3/gadget.c | 38 ++++++++++++++++++++++++++------------ 2 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 8f9959ba9fd4..9b005d912241 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -943,6 +943,7 @@ struct dwc3_request { #define DWC3_REQUEST_STATUS_DEQUEUED 3 #define DWC3_REQUEST_STATUS_STALLED 4 #define DWC3_REQUEST_STATUS_COMPLETED 5 +#define DWC3_REQUEST_STATUS_MISSED_ISOC 6 #define DWC3_REQUEST_STATUS_UNKNOWN -1
u8 epnum; diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 079cd333632e..411532c5c378 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2021,6 +2021,9 @@ static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep) case DWC3_REQUEST_STATUS_STALLED: dwc3_gadget_giveback(dep, req, -EPIPE); break; + case DWC3_REQUEST_STATUS_MISSED_ISOC: + dwc3_gadget_giveback(dep, req, -EXDEV); + break; default: dev_err(dwc->dev, "request cancelled with wrong reason:%d\n", req->status); dwc3_gadget_giveback(dep, req, -ECONNRESET); @@ -3402,21 +3405,32 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep, struct dwc3 *dwc = dep->dwc; bool no_started_trb = true;
- dwc3_gadget_ep_cleanup_completed_requests(dep, event, status); + if (status == -EXDEV) { + struct dwc3_request *tmp; + struct dwc3_request *req;
- if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) - goto out; + if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING)) + dwc3_stop_active_transfer(dep, true, true);
- if (!dep->endpoint.desc) - return no_started_trb; + list_for_each_entry_safe(req, tmp, &dep->started_list, list) + dwc3_gadget_move_cancelled_request(req, + DWC3_REQUEST_STATUS_MISSED_ISOC); + } else { + dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc) && - list_empty(&dep->started_list) && - (list_empty(&dep->pending_list) || status == -EXDEV)) - dwc3_stop_active_transfer(dep, true, true); - else if (dwc3_gadget_ep_should_continue(dep)) - if (__dwc3_gadget_kick_transfer(dep) == 0) - no_started_trb = false; + if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) + goto out; + + if (!dep->endpoint.desc) + return no_started_trb; + + if (usb_endpoint_xfer_isoc(dep->endpoint.desc) && + list_empty(&dep->started_list) && list_empty(&dep->pending_list)) + dwc3_stop_active_transfer(dep, true, true); + else if (dwc3_gadget_ep_should_continue(dep)) + if (__dwc3_gadget_kick_transfer(dep) == 0) + no_started_trb = false; + }
out: /*
On Mon, Oct 17, 2022, Dan Vacura wrote:
From: Jeff Vanhoof qjv001@motorola.com
arm-smmu related crashes seen after a Missed ISOC interrupt when no_interrupt=1 is used. This can happen if the hardware is still using the data associated with a TRB after the usb_request's ->complete call has been made. Instead of immediately releasing a request when a Missed ISOC interrupt has occurred, this change will add logic to cancel the request instead where it will eventually be released when the END_TRANSFER command has completed. This logic is similar to some of the cleanup done in dwc3_gadget_ep_dequeue.
This doesn't sound right. How did you determine that the hardware is still using the data associated with the TRB? Did you check the TRB's HWO bit?
The dwc3 driver would only give back the requests if the TRBs of the associated requests are completed or when the device is disconnected. If the TRB indicated missed isoc, that means that the TRB is completed and its status was updated.
There's a special case which dwc3 may give back requests early is the case of the device disconnecting. The requests should be returned with -ESHUTDOWN, and the gadget driver shouldn't be re-using the requests on de-initialization anyway.
We should not issue End Transfer command just because of missed isoc. We may want issue End Transfer if the gadget driver is too slow and unable to feed requests in time (causing underrun and missed isoc) to resync with the host, but we already handle that.
I'm still not clear what's the problem you're seeing. Do you have the crash log? Tracepoints?
BR, Thinh
Hi Thinh,
On Mon, Oct 17, 2022 at 09:30:38PM +0000, Thinh Nguyen wrote:
On Mon, Oct 17, 2022, Dan Vacura wrote:
From: Jeff Vanhoof qjv001@motorola.com
arm-smmu related crashes seen after a Missed ISOC interrupt when no_interrupt=1 is used. This can happen if the hardware is still using the data associated with a TRB after the usb_request's ->complete call has been made. Instead of immediately releasing a request when a Missed ISOC interrupt has occurred, this change will add logic to cancel the request instead where it will eventually be released when the END_TRANSFER command has completed. This logic is similar to some of the cleanup done in dwc3_gadget_ep_dequeue.
This doesn't sound right. How did you determine that the hardware is still using the data associated with the TRB? Did you check the TRB's HWO bit?
The problem we're seeing was mentioned in the summary of this patch series, issue #1. Basically, with the following patch https://patchwork.kernel.org/project/linux-usb/patch/20210628155311.16762-6-... integrated a smmu panic is occurring on our Android device with the 5.15 kernel which is:
<3>[ 718.314900][ T803] arm-smmu 15000000.apps-smmu: Unhandled arm-smmu context fault from a600000.dwc3!
The uvc gadget driver appears to be the first (and only) gadget that uses the no_interrupt=1 logic, so this seems to be a new condition for the dwc3 driver. In our configuration, we have up to 64 requests and the no_interrupt=1 for up to 15 requests. The list size of dep->started_list would get up to that amount when looping through to cleanup the completed requests. From testing and debugging the smmu panic occurs when a -EXDEV status shows up and right after dwc3_gadget_ep_cleanup_completed_request() was visited. The conclusion we had was the requests were getting returned to the gadget too early.
The dwc3 driver would only give back the requests if the TRBs of the associated requests are completed or when the device is disconnected. If the TRB indicated missed isoc, that means that the TRB is completed and its status was updated.
Interesting, the device is not disconnected as we don't get the -ESHUTDOWN status back and with this patch in place things continue after a -EXDEV status is received.
There's a special case which dwc3 may give back requests early is the case of the device disconnecting. The requests should be returned with -ESHUTDOWN, and the gadget driver shouldn't be re-using the requests on de-initialization anyway.
We should not issue End Transfer command just because of missed isoc. We may want issue End Transfer if the gadget driver is too slow and unable to feed requests in time (causing underrun and missed isoc) to resync with the host, but we already handle that.
Hmm, isn't that what happens when we get into this condition in dwc3_gadget_endpoint_trbs_complete():
if (usb_endpoint_xfer_isoc(dep->endpoint.desc) && list_empty(&dep->started_list) && (list_empty(&dep->pending_list) || status == -EXDEV)) dwc3_stop_active_transfer(dep, true, true);
I'm still not clear what's the problem you're seeing. Do you have the crash log? Tracepoints?
BR, Thinh
Appreciate the support!
Dan
Hi Dan,
On Mon, Oct 17, 2022, Dan Vacura wrote:
Hi Thinh,
On Mon, Oct 17, 2022 at 09:30:38PM +0000, Thinh Nguyen wrote:
On Mon, Oct 17, 2022, Dan Vacura wrote:
From: Jeff Vanhoof qjv001@motorola.com
arm-smmu related crashes seen after a Missed ISOC interrupt when no_interrupt=1 is used. This can happen if the hardware is still using the data associated with a TRB after the usb_request's ->complete call has been made. Instead of immediately releasing a request when a Missed ISOC interrupt has occurred, this change will add logic to cancel the request instead where it will eventually be released when the END_TRANSFER command has completed. This logic is similar to some of the cleanup done in dwc3_gadget_ep_dequeue.
This doesn't sound right. How did you determine that the hardware is still using the data associated with the TRB? Did you check the TRB's HWO bit?
The problem we're seeing was mentioned in the summary of this patch series, issue #1. Basically, with the following patch https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-usb/p... integrated a smmu panic is occurring on our Android device with the 5.15 kernel which is:
<3>[ 718.314900][ T803] arm-smmu 15000000.apps-smmu: Unhandled arm-smmu context fault from a600000.dwc3!The uvc gadget driver appears to be the first (and only) gadget that uses the no_interrupt=1 logic, so this seems to be a new condition for the dwc3 driver. In our configuration, we have up to 64 requests and the no_interrupt=1 for up to 15 requests. The list size of dep->started_list would get up to that amount when looping through to cleanup the completed requests. From testing and debugging the smmu panic occurs when a -EXDEV status shows up and right after dwc3_gadget_ep_cleanup_completed_request() was visited. The conclusion we had was the requests were getting returned to the gadget too early.
As I mentioned, if the status is updated to missed isoc, that means that the controller returned ownership of the TRB to the driver. At least for the particular request with -EXDEV, its TRBs are completed. I'm not clear on your conclusion.
Do we know where did the crash occur? Is it from dwc3 driver or from uvc driver, and at what line? It'd great if we can see the driver log.
The dwc3 driver would only give back the requests if the TRBs of the associated requests are completed or when the device is disconnected. If the TRB indicated missed isoc, that means that the TRB is completed and its status was updated.
Interesting, the device is not disconnected as we don't get the -ESHUTDOWN status back and with this patch in place things continue after a -EXDEV status is received.
Actually, minor correction here: a recent change b44c0e7fef51 ("usb: dwc3: gadget: conditionally remove requests") changed -ESHUTDOWN request status to -ECONNRESET when disable endpoint. This doesn't look right.
While disabling endpoint may also apply for other cases such as switching alternate interface in addition to disconnect, -ESHUTDOWN seems more fitting there.
Hi Michael,
Can you help clarify for the change above? This changed the usage of requests. Now requests returned by disconnection won't be returned as -ESHUTDOWN.
There's a special case which dwc3 may give back requests early is the case of the device disconnecting. The requests should be returned with -ESHUTDOWN, and the gadget driver shouldn't be re-using the requests on de-initialization anyway.
We should not issue End Transfer command just because of missed isoc. We may want issue End Transfer if the gadget driver is too slow and unable to feed requests in time (causing underrun and missed isoc) to resync with the host, but we already handle that.
Hmm, isn't that what happens when we get into this condition in dwc3_gadget_endpoint_trbs_complete():
if (usb_endpoint_xfer_isoc(dep->endpoint.desc) && list_empty(&dep->started_list) && (list_empty(&dep->pending_list) || status == -EXDEV)) dwc3_stop_active_transfer(dep, true, true);
Yes, it's being handled there.
I'm still not clear what's the problem you're seeing. Do you have the crash log? Tracepoints?
Appreciate the support!
Thanks, Thinh
Hi Thinh,
On Tue, Oct 18, 2022 at 06:45:40PM +0000, Thinh Nguyen wrote:
On Mon, Oct 17, 2022, Dan Vacura wrote:
On Mon, Oct 17, 2022 at 09:30:38PM +0000, Thinh Nguyen wrote:
On Mon, Oct 17, 2022, Dan Vacura wrote:
From: Jeff Vanhoof qjv001@motorola.com
arm-smmu related crashes seen after a Missed ISOC interrupt when no_interrupt=1 is used. This can happen if the hardware is still using the data associated with a TRB after the usb_request's ->complete call has been made. Instead of immediately releasing a request when a Missed ISOC interrupt has occurred, this change will add logic to cancel the request instead where it will eventually be released when the END_TRANSFER command has completed. This logic is similar to some of the cleanup done in dwc3_gadget_ep_dequeue.
This doesn't sound right. How did you determine that the hardware is still using the data associated with the TRB? Did you check the TRB's HWO bit?
The problem we're seeing was mentioned in the summary of this patch series, issue #1. Basically, with the following patch https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-usb/p... integrated a smmu panic is occurring on our Android device with the 5.15 kernel which is:
<3>[ 718.314900][ T803] arm-smmu 15000000.apps-smmu: Unhandled arm-smmu context fault from a600000.dwc3!The uvc gadget driver appears to be the first (and only) gadget that uses the no_interrupt=1 logic, so this seems to be a new condition for the dwc3 driver. In our configuration, we have up to 64 requests and the no_interrupt=1 for up to 15 requests. The list size of dep->started_list would get up to that amount when looping through to cleanup the completed requests. From testing and debugging the smmu panic occurs when a -EXDEV status shows up and right after dwc3_gadget_ep_cleanup_completed_request() was visited. The conclusion we had was the requests were getting returned to the gadget too early.
As I mentioned, if the status is updated to missed isoc, that means that the controller returned ownership of the TRB to the driver. At least for the particular request with -EXDEV, its TRBs are completed. I'm not clear on your conclusion.
Do we know where did the crash occur? Is it from dwc3 driver or from uvc driver, and at what line? It'd great if we can see the driver log.
The dwc3 driver would only give back the requests if the TRBs of the associated requests are completed or when the device is disconnected. If the TRB indicated missed isoc, that means that the TRB is completed and its status was updated.
Interesting, the device is not disconnected as we don't get the -ESHUTDOWN status back and with this patch in place things continue after a -EXDEV status is received.
Actually, minor correction here: a recent change b44c0e7fef51 ("usb: dwc3: gadget: conditionally remove requests") changed -ESHUTDOWN request status to -ECONNRESET when disable endpoint. This doesn't look right.
While disabling endpoint may also apply for other cases such as switching alternate interface in addition to disconnect, -ESHUTDOWN seems more fitting there.
Hi Michael,
Can you help clarify for the change above? This changed the usage of requests. Now requests returned by disconnection won't be returned as -ESHUTDOWN.
When writing the patch, I was looking into Documentation/driver-api/usb/error-codes.rst.
After looking into it today, I see that ESHUTDOWN should be send on ep_disable (device disable) and ECONNRESET on stop_active_transfer. So I probably just mixed them up, while writing the patch. :/
The followup patch would then just be to swap the status results of __dwc3_gadget_ep_disable and dwc3_stop_active_transfers on the dwc3_remove_requests call.
Michael
There's a special case which dwc3 may give back requests early is the case of the device disconnecting. The requests should be returned with -ESHUTDOWN, and the gadget driver shouldn't be re-using the requests on de-initialization anyway.
We should not issue End Transfer command just because of missed isoc. We may want issue End Transfer if the gadget driver is too slow and unable to feed requests in time (causing underrun and missed isoc) to resync with the host, but we already handle that.
Hmm, isn't that what happens when we get into this condition in dwc3_gadget_endpoint_trbs_complete():
if (usb_endpoint_xfer_isoc(dep->endpoint.desc) && list_empty(&dep->started_list) && (list_empty(&dep->pending_list) || status == -EXDEV)) dwc3_stop_active_transfer(dep, true, true);
Yes, it's being handled there.
I'm still not clear what's the problem you're seeing. Do you have the crash log? Tracepoints?
Appreciate the support!
Thanks, Thinh
On Tue, Oct 18, 2022, Michael Grzeschik wrote:
Hi Thinh,
On Tue, Oct 18, 2022 at 06:45:40PM +0000, Thinh Nguyen wrote:
On Mon, Oct 17, 2022, Dan Vacura wrote:
On Mon, Oct 17, 2022 at 09:30:38PM +0000, Thinh Nguyen wrote:
On Mon, Oct 17, 2022, Dan Vacura wrote:
From: Jeff Vanhoof qjv001@motorola.com
arm-smmu related crashes seen after a Missed ISOC interrupt when no_interrupt=1 is used. This can happen if the hardware is still using the data associated with a TRB after the usb_request's ->complete call has been made. Instead of immediately releasing a request when a Missed ISOC interrupt has occurred, this change will add logic to cancel the request instead where it will eventually be released when the END_TRANSFER command has completed. This logic is similar to some of the cleanup done in dwc3_gadget_ep_dequeue.
This doesn't sound right. How did you determine that the hardware is still using the data associated with the TRB? Did you check the TRB's HWO bit?
The problem we're seeing was mentioned in the summary of this patch series, issue #1. Basically, with the following patch https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-usb/p... integrated a smmu panic is occurring on our Android device with the 5.15 kernel which is:
<3>[ 718.314900][ T803] arm-smmu 15000000.apps-smmu: Unhandled arm-smmu context fault from a600000.dwc3!The uvc gadget driver appears to be the first (and only) gadget that uses the no_interrupt=1 logic, so this seems to be a new condition for the dwc3 driver. In our configuration, we have up to 64 requests and the no_interrupt=1 for up to 15 requests. The list size of dep->started_list would get up to that amount when looping through to cleanup the completed requests. From testing and debugging the smmu panic occurs when a -EXDEV status shows up and right after dwc3_gadget_ep_cleanup_completed_request() was visited. The conclusion we had was the requests were getting returned to the gadget too early.
As I mentioned, if the status is updated to missed isoc, that means that the controller returned ownership of the TRB to the driver. At least for the particular request with -EXDEV, its TRBs are completed. I'm not clear on your conclusion.
Do we know where did the crash occur? Is it from dwc3 driver or from uvc driver, and at what line? It'd great if we can see the driver log.
The dwc3 driver would only give back the requests if the TRBs of the associated requests are completed or when the device is disconnected. If the TRB indicated missed isoc, that means that the TRB is completed and its status was updated.
Interesting, the device is not disconnected as we don't get the -ESHUTDOWN status back and with this patch in place things continue after a -EXDEV status is received.
Actually, minor correction here: a recent change b44c0e7fef51 ("usb: dwc3: gadget: conditionally remove requests") changed -ESHUTDOWN request status to -ECONNRESET when disable endpoint. This doesn't look right.
While disabling endpoint may also apply for other cases such as switching alternate interface in addition to disconnect, -ESHUTDOWN seems more fitting there.
Hi Michael,
Can you help clarify for the change above? This changed the usage of requests. Now requests returned by disconnection won't be returned as -ESHUTDOWN.
When writing the patch, I was looking into Documentation/driver-api/usb/error-codes.rst.
After looking into it today, I see that ESHUTDOWN should be send on ep_disable (device disable) and ECONNRESET on stop_active_transfer. So I probably just mixed them up, while writing the patch. :/
I think you mean ECONNRESET for ep_dequeue()? dwc3_stop_active_transfer() is called for both scenarios.
The followup patch would then just be to swap the status results of __dwc3_gadget_ep_disable and dwc3_stop_active_transfers on the dwc3_remove_requests call.
Michael
Can you help make a fix?
Thanks! Thinh
Hi Thinh,
On Tue, Oct 18, 2022 at 10:45:16PM +0000, Thinh Nguyen wrote:
On Tue, Oct 18, 2022, Michael Grzeschik wrote:
On Tue, Oct 18, 2022 at 06:45:40PM +0000, Thinh Nguyen wrote:
On Mon, Oct 17, 2022, Dan Vacura wrote:
On Mon, Oct 17, 2022 at 09:30:38PM +0000, Thinh Nguyen wrote:
On Mon, Oct 17, 2022, Dan Vacura wrote:
From: Jeff Vanhoof qjv001@motorola.com
arm-smmu related crashes seen after a Missed ISOC interrupt when no_interrupt=1 is used. This can happen if the hardware is still using the data associated with a TRB after the usb_request's ->complete call has been made. Instead of immediately releasing a request when a Missed ISOC interrupt has occurred, this change will add logic to cancel the request instead where it will eventually be released when the END_TRANSFER command has completed. This logic is similar to some of the cleanup done in dwc3_gadget_ep_dequeue.
This doesn't sound right. How did you determine that the hardware is still using the data associated with the TRB? Did you check the TRB's HWO bit?
The problem we're seeing was mentioned in the summary of this patch series, issue #1. Basically, with the following patch https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-usb/p... integrated a smmu panic is occurring on our Android device with the 5.15 kernel which is:
<3>[ 718.314900][ T803] arm-smmu 15000000.apps-smmu: Unhandled arm-smmu context fault from a600000.dwc3!The uvc gadget driver appears to be the first (and only) gadget that uses the no_interrupt=1 logic, so this seems to be a new condition for the dwc3 driver. In our configuration, we have up to 64 requests and the no_interrupt=1 for up to 15 requests. The list size of dep->started_list would get up to that amount when looping through to cleanup the completed requests. From testing and debugging the smmu panic occurs when a -EXDEV status shows up and right after dwc3_gadget_ep_cleanup_completed_request() was visited. The conclusion we had was the requests were getting returned to the gadget too early.
As I mentioned, if the status is updated to missed isoc, that means that the controller returned ownership of the TRB to the driver. At least for the particular request with -EXDEV, its TRBs are completed. I'm not clear on your conclusion.
Do we know where did the crash occur? Is it from dwc3 driver or from uvc driver, and at what line? It'd great if we can see the driver log.
The dwc3 driver would only give back the requests if the TRBs of the associated requests are completed or when the device is disconnected. If the TRB indicated missed isoc, that means that the TRB is completed and its status was updated.
Interesting, the device is not disconnected as we don't get the -ESHUTDOWN status back and with this patch in place things continue after a -EXDEV status is received.
Actually, minor correction here: a recent change b44c0e7fef51 ("usb: dwc3: gadget: conditionally remove requests") changed -ESHUTDOWN request status to -ECONNRESET when disable endpoint. This doesn't look right.
While disabling endpoint may also apply for other cases such as switching alternate interface in addition to disconnect, -ESHUTDOWN seems more fitting there.
Hi Michael,
Can you help clarify for the change above? This changed the usage of requests. Now requests returned by disconnection won't be returned as -ESHUTDOWN.
When writing the patch, I was looking into Documentation/driver-api/usb/error-codes.rst.
After looking into it today, I see that ESHUTDOWN should be send on ep_disable (device disable) and ECONNRESET on stop_active_transfer. So I probably just mixed them up, while writing the patch. :/
I think you mean ECONNRESET for ep_dequeue()? dwc3_stop_active_transfer() is called for both scenarios.
No, I meant dwc3_stop_active_transfer*s*. On ep_dequeue the request status is already ECONNRESET.
The followup patch would then just be to swap the status results of __dwc3_gadget_ep_disable and dwc3_stop_active_transfers on the dwc3_remove_requests call.
Michael
Can you help make a fix?
Sure, I will write a patch.
Thanks, Michael
Sorry for digging up this grave! :)
I once more came accross the whole situation we are still encountering since one year or so again and found the some reasons why:
#1 there are so many latencies, so that the system is not fast enough to enqueue requests back into an running HW-Transfer. At least on our system setup.
and
#2 there are so many missed transfers leading to broken frames when adding request with no_interrupt set.
For #1: There sometimes are situations in the system where the threaded interrupt handler for the dwc3 is not called fast enough, although the HW-irq was called early and enqueued the irq event and woke the irq thread early. In our case this often happens, when there are other tasks involved on the same CPU and the scheduler is not able to pipeline the irq thread in the necessary time. In our case the main issue is an HW-irq handler of the ethernet controller (cadence macb) that runs berserk on CPU0 and therefor is taking a lot of CPU time. Per default on our system all irq handlers are running on the same CPU. As per definition all interrupt threads will be started on the same CPU as the irq was called, this forces a lot of pressure on one Core. So changing the smp_affinity of the dwc3 irq to the second CPU only, already solves a lot of the underruns.
For #2: I found an issue in the handling of the completion of requests in the started list. When the interrupt handler is *explicitly* calling stop_active_transfer if the overall event of the request was an missed event. This event value only represents the value of the request that was actually triggering the interrupt.
It also calls ep_cleanup_completed_requests and is iterating over the started requests and will call giveback/complete functions of the requests with the proper request status.
So this will also catch missed requests in the queue. However, since there might be, lets say 5 good requests and one missed request, what will happen is, that each complete call for the first good requests will enqueue new requests into the started list and will also call the updatecmd on that transfer that was already missed until the loop will reach the one request with the MISSED status bit set.
So in my opinion the patch from Jeff makes sense when adding the following change aswell. With those both changes the underruns and broken frames finally disappear. I am still unsure about the complete solution about that, since with this the mentioned 5 good requests will be cancelled aswell. So this is still a WIP status here.
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index e031813c5769b..b991d25bbf897 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -3509,6 +3509,45 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, return ret; }
+static int dwc3_gadget_ep_check_missed_requests(struct dwc3_ep *dep) +{ + struct dwc3_request *req; + struct dwc3_request *tmp; + int ret = 0; + + list_for_each_entry_safe(req, tmp, &dep->started_list, list) { + struct dwc3_trb *trb; + + /* TOOD: check if the trb association is correct */ + trb = req->trb; + switch (DWC3_TRB_SIZE_TRBSTS(trb->size)) { + case DWC3_TRBSTS_MISSED_ISOC: + /* Isoc endpoint only */ + ret = -EXDEV; + break; + case DWC3_TRB_STS_XFER_IN_PROG: + /* Applicable when End Transfer with ForceRM=0 */ + case DWC3_TRBSTS_SETUP_PENDING: + /* Control endpoint only */ + case DWC3_TRBSTS_OK: + default: + ret = 0; + break; + } + } + + return ret; +} + static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep, const struct dwc3_event_depevt *event, int status) { @@ -3566,7 +3605,7 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep, struct dwc3 *dwc = dep->dwc; bool no_started_trb = true;
- if (status == -EXDEV) { + if (status == -EXDEV || dwc3_gadget_ep_check_missed_requests(dep)) { struct dwc3_request *tmp; struct dwc3_request *req;
On Mon, Oct 17, 2022 at 03:54:40PM -0500, Dan Vacura wrote:
From: Jeff Vanhoof qjv001@motorola.com
arm-smmu related crashes seen after a Missed ISOC interrupt when no_interrupt=1 is used. This can happen if the hardware is still using the data associated with a TRB after the usb_request's ->complete call has been made. Instead of immediately releasing a request when a Missed ISOC interrupt has occurred, this change will add logic to cancel the request instead where it will eventually be released when the END_TRANSFER command has completed. This logic is similar to some of the cleanup done in dwc3_gadget_ep_dequeue.
Fixes: 6d8a019614f3 ("usb: dwc3: gadget: check for Missed Isoc from event status") Cc: stable@vger.kernel.org Signed-off-by: Jeff Vanhoof qjv001@motorola.com Co-developed-by: Dan Vacura w36195@motorola.com Signed-off-by: Dan Vacura w36195@motorola.com
V1 -> V3:
- no change, new patch in series
drivers/usb/dwc3/core.h | 1 + drivers/usb/dwc3/gadget.c | 38 ++++++++++++++++++++++++++------------ 2 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 8f9959ba9fd4..9b005d912241 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -943,6 +943,7 @@ struct dwc3_request { #define DWC3_REQUEST_STATUS_DEQUEUED 3 #define DWC3_REQUEST_STATUS_STALLED 4 #define DWC3_REQUEST_STATUS_COMPLETED 5 +#define DWC3_REQUEST_STATUS_MISSED_ISOC 6 #define DWC3_REQUEST_STATUS_UNKNOWN -1
u8 epnum; diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 079cd333632e..411532c5c378 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2021,6 +2021,9 @@ static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep) case DWC3_REQUEST_STATUS_STALLED: dwc3_gadget_giveback(dep, req, -EPIPE); break;
case DWC3_REQUEST_STATUS_MISSED_ISOC:dwc3_gadget_giveback(dep, req, -EXDEV);break;@@ -3402,21 +3405,32 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep, struct dwc3 *dwc = dep->dwc; bool no_started_trb = true;
- dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
- if (status == -EXDEV) {
struct dwc3_request *tmp;struct dwc3_request *req;
- if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)
goto out;
if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))dwc3_stop_active_transfer(dep, true, true);
- if (!dep->endpoint.desc)
return no_started_trb;
list_for_each_entry_safe(req, tmp, &dep->started_list, list)dwc3_gadget_move_cancelled_request(req,DWC3_REQUEST_STATUS_MISSED_ISOC);- } else {
dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
list_empty(&dep->started_list) &&(list_empty(&dep->pending_list) || status == -EXDEV))dwc3_stop_active_transfer(dep, true, true);- else if (dwc3_gadget_ep_should_continue(dep))
if (__dwc3_gadget_kick_transfer(dep) == 0)no_started_trb = false;
if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)goto out;if (!dep->endpoint.desc)return no_started_trb;if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&list_empty(&dep->started_list) && list_empty(&dep->pending_list))dwc3_stop_active_transfer(dep, true, true);else if (dwc3_gadget_ep_should_continue(dep))if (__dwc3_gadget_kick_transfer(dep) == 0)no_started_trb = false;- }
out: /* -- 2.34.1
On Thu, Feb 22, 2024, Michael Grzeschik wrote:
Sorry for digging up this grave! :)
I once more came accross the whole situation we are still encountering since one year or so again and found the some reasons why:
#1 there are so many latencies, so that the system is not fast enough to enqueue requests back into an running HW-Transfer. At least on our system setup.
and
#2 there are so many missed transfers leading to broken frames when adding request with no_interrupt set.
For #1: There sometimes are situations in the system where the threaded interrupt handler for the dwc3 is not called fast enough, although the HW-irq was called early and enqueued the irq event and woke the irq thread early. In our case this often happens, when there are other tasks involved on the same CPU and the scheduler is not able to pipeline the irq thread in the necessary time. In our case the main issue is an HW-irq handler of the ethernet controller (cadence macb) that runs berserk on CPU0 and therefor is taking a lot of CPU time. Per default on our system all irq handlers are running on the same CPU. As per definition all interrupt threads will be started on the same CPU as the irq was called, this forces a lot of pressure on one Core. So changing the smp_affinity of the dwc3 irq to the second CPU only, already solves a lot of the underruns.
That's great!
For #2: I found an issue in the handling of the completion of requests in the started list. When the interrupt handler is *explicitly* calling stop_active_transfer if the overall event of the request was an missed event. This event value only represents the value of the request that was actually triggering the interrupt.
It also calls ep_cleanup_completed_requests and is iterating over the started requests and will call giveback/complete functions of the requests with the proper request status.
So this will also catch missed requests in the queue. However, since there might be, lets say 5 good requests and one missed request, what will happen is, that each complete call for the first good requests will enqueue new requests into the started list and will also call the updatecmd on that transfer that was already missed until the loop will reach the one request with the MISSED status bit set.
So in my opinion the patch from Jeff makes sense when adding the following change aswell. With those both changes the underruns and broken frames finally disappear. I am still unsure about the complete solution about that, since with this the mentioned 5 good requests will be cancelled aswell. So this is still a WIP status here.
When the dwc3 driver issues stop_active_transfer(), that means that the started_list is empty and there is an underrun. It treats the incoming requests as staled. However, for UVC, they are still "good".
I think you can just check if the started_list is empty before queuing new requests. If it is, perform stop_active_transfer() to reschedule the incoming requests. None of the newly queue requests will be released yet since they are in the pending_list.
For UVC, perhaps you can introduce a new flag to usb_request called "ignore_queue_latency" or something equivalent. The dwc3 is already partially doing this for UVC. With this new flag, we can rework dwc3 to clearly separate the expected behavior from the function driver.
BR, Thinh
On Thu, Feb 22, 2024 at 01:20:04AM +0000, Thinh Nguyen wrote:
On Thu, Feb 22, 2024, Michael Grzeschik wrote:
For #2: I found an issue in the handling of the completion of requests in the started list. When the interrupt handler is *explicitly* calling stop_active_transfer if the overall event of the request was an missed event. This event value only represents the value of the request that was actually triggering the interrupt.
It also calls ep_cleanup_completed_requests and is iterating over the started requests and will call giveback/complete functions of the requests with the proper request status.
So this will also catch missed requests in the queue. However, since there might be, lets say 5 good requests and one missed request, what will happen is, that each complete call for the first good requests will enqueue new requests into the started list and will also call the updatecmd on that transfer that was already missed until the loop will reach the one request with the MISSED status bit set.
So in my opinion the patch from Jeff makes sense when adding the following change aswell. With those both changes the underruns and broken frames finally disappear. I am still unsure about the complete solution about that, since with this the mentioned 5 good requests will be cancelled aswell. So this is still a WIP status here.
When the dwc3 driver issues stop_active_transfer(), that means that the started_list is empty and there is an underrun.
At this moment this is only the case when both, pending and started list are empty. Or the interrupt event was EXDEV.
The main problem is that the function dwc3_gadget_ep_cleanup_completed_requests(dep, event, status); will issue an complete for each started request, which on the other hand will refill the pending list, and therefor after that refill the stop_active_transfer is currently never hit.
It treats the incoming requests as staled. However, for UVC, they are still "good".
Right, so in that case we can requeue them anyway. But this will have to be done after the stop transfer cmd has finished.
I think you can just check if the started_list is empty before queuing new requests. If it is, perform stop_active_transfer() to reschedule the incoming requests. None of the newly queue requests will be released yet since they are in the pending_list.
So that is basically exactly what my patch is doing. However in the case of an underrun it is not safe to call dwc3_gadget_ep_cleanup_completed_requests as jeff stated. So his underlying patch is really fixing an issue here.
For UVC, perhaps you can introduce a new flag to usb_request called "ignore_queue_latency" or something equivalent. The dwc3 is already partially doing this for UVC. With this new flag, we can rework dwc3 to clearly separate the expected behavior from the function driver.
I don't know why this "extra" flag is even necessary. The code example is already working without that extra flag.
Actually I even came up with an better solution. Additionally of checking if one of the requests in the started list was missed, we can activly check if the trb ring did run dry and if dwc3_gadget_endpoint_trbs_complete is going to enqueue in to the empty trb ring.
So my whole change looks like that:
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index efe6caf4d0e87..2c8047dcd1612 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -952,6 +952,7 @@ struct dwc3_request { #define DWC3_REQUEST_STATUS_DEQUEUED 3 #define DWC3_REQUEST_STATUS_STALLED 4 #define DWC3_REQUEST_STATUS_COMPLETED 5 +#define DWC3_REQUEST_STATUS_MISSED_ISOC 6 #define DWC3_REQUEST_STATUS_UNKNOWN -1
u8 epnum; diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 858fe4c299b7a..a31f4d3502bd3 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2057,6 +2057,9 @@ static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep) req = next_request(&dep->cancelled_list); dwc3_gadget_ep_skip_trbs(dep, req); switch (req->status) { + case 0: + dwc3_gadget_giveback(dep, req, 0); + break; case DWC3_REQUEST_STATUS_DISCONNECTED: dwc3_gadget_giveback(dep, req, -ESHUTDOWN); break; @@ -2066,6 +2069,9 @@ static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep) case DWC3_REQUEST_STATUS_STALLED: dwc3_gadget_giveback(dep, req, -EPIPE); break; + case DWC3_REQUEST_STATUS_MISSED_ISOC: + dwc3_gadget_giveback(dep, req, -EXDEV); + break; default: dev_err(dwc->dev, "request cancelled with wrong reason:%d\n", req->status); dwc3_gadget_giveback(dep, req, -ECONNRESET); @@ -3509,6 +3515,36 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, return ret; }
+static int dwc3_gadget_ep_check_missed_requests(struct dwc3_ep *dep) +{ + struct dwc3_request *req; + struct dwc3_request *tmp; + int ret = 0; + + list_for_each_entry_safe(req, tmp, &dep->started_list, list) { + struct dwc3_trb *trb; + + trb = req->trb; + switch (DWC3_TRB_SIZE_TRBSTS(trb->size)) { + case DWC3_TRBSTS_MISSED_ISOC: + /* Isoc endpoint only */ + ret = -EXDEV; + break; + case DWC3_TRB_STS_XFER_IN_PROG: + /* Applicable when End Transfer with ForceRM=0 */ + case DWC3_TRBSTS_SETUP_PENDING: + /* Control endpoint only */ + case DWC3_TRBSTS_OK: + default: + ret = 0; + break; + } + } + + return ret; +} + static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep, const struct dwc3_event_depevt *event, int status) { @@ -3565,22 +3601,51 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep, { struct dwc3 *dwc = dep->dwc; bool no_started_trb = true; + unsigned int transfer_in_flight = 0; + + /* It is possible that the interrupt thread was delayed by + * scheduling in the system, and therefor the HW has already + * run dry. In that case the last trb in the queue is already + * handled by the hw. By checking the HWO bit we know to restart + * the whole transfer. The condition to appear is more likelely + * if not every trb has the IOC bit set and therefor does not + * trigger the interrupt thread fewer. + */ + if (dep->number && usb_endpoint_xfer_isoc(dep->endpoint.desc)) { + struct dwc3_trb *trb;
- dwc3_gadget_ep_cleanup_completed_requests(dep, event, status); + trb = dwc3_ep_prev_trb(dep, dep->trb_enqueue); + transfer_in_flight = trb->ctrl & DWC3_TRB_CTRL_HWO; + }
- if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) - goto out; + if (status == -EXDEV || !transfer_in_flight) { + struct dwc3_request *tmp; + struct dwc3_request *req;
- if (!dep->endpoint.desc) - return no_started_trb; + if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING)) + dwc3_stop_active_transfer(dep, true, true);
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc) && - list_empty(&dep->started_list) && - (list_empty(&dep->pending_list) || status == -EXDEV)) - dwc3_stop_active_transfer(dep, true, true); - else if (dwc3_gadget_ep_should_continue(dep)) - if (__dwc3_gadget_kick_transfer(dep) == 0) - no_started_trb = false; + list_for_each_entry_safe(req, tmp, &dep->started_list, list) { + dwc3_gadget_move_cancelled_request(req, + (DWC3_TRB_SIZE_TRBSTS(req->trb->size) == DWC3_TRBSTS_MISSED_ISOC) ? + DWC3_REQUEST_STATUS_MISSED_ISOC : 0); + } + } else { + dwc3_gadget_ep_cleanup_completed_requests(dep, event, status); + + if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) + goto out; + + if (!dep->endpoint.desc) + return no_started_trb; + + if (usb_endpoint_xfer_isoc(dep->endpoint.desc) && + list_empty(&dep->started_list) && list_empty(&dep->pending_list)) + dwc3_stop_active_transfer(dep, true, true); + else if (dwc3_gadget_ep_should_continue(dep)) + if (__dwc3_gadget_kick_transfer(dep) == 0) + no_started_trb = false; + }
out: /*
I will seperate the whole hunk into smaller changes and send an v1 the next days to review.
Regards, Michael
On Tue, Feb 27, 2024, Michael Grzeschik wrote:
On Thu, Feb 22, 2024 at 01:20:04AM +0000, Thinh Nguyen wrote:
On Thu, Feb 22, 2024, Michael Grzeschik wrote:
For #2: I found an issue in the handling of the completion of requests in the started list. When the interrupt handler is *explicitly* calling stop_active_transfer if the overall event of the request was an missed event. This event value only represents the value of the request that was actually triggering the interrupt.
It also calls ep_cleanup_completed_requests and is iterating over the started requests and will call giveback/complete functions of the requests with the proper request status.
So this will also catch missed requests in the queue. However, since there might be, lets say 5 good requests and one missed request, what will happen is, that each complete call for the first good requests will enqueue new requests into the started list and will also call the updatecmd on that transfer that was already missed until the loop will reach the one request with the MISSED status bit set.
So in my opinion the patch from Jeff makes sense when adding the following change aswell. With those both changes the underruns and broken frames finally disappear. I am still unsure about the complete solution about that, since with this the mentioned 5 good requests will be cancelled aswell. So this is still a WIP status here.
When the dwc3 driver issues stop_active_transfer(), that means that the started_list is empty and there is an underrun.
At this moment this is only the case when both, pending and started list are empty. Or the interrupt event was EXDEV.
The main problem is that the function dwc3_gadget_ep_cleanup_completed_requests(dep, event, status); will issue an complete for each started request, which on the other hand will refill the pending list, and therefor after that refill the stop_active_transfer is currently never hit.
It treats the incoming requests as staled. However, for UVC, they are still "good".
Right, so in that case we can requeue them anyway. But this will have to be done after the stop transfer cmd has finished.
I think you can just check if the started_list is empty before queuing new requests. If it is, perform stop_active_transfer() to reschedule the incoming requests. None of the newly queue requests will be released yet since they are in the pending_list.
So that is basically exactly what my patch is doing. However in the case of an underrun it is not safe to call dwc3_gadget_ep_cleanup_completed_requests as jeff stated. So his underlying patch is really fixing an issue here.
What I mean is to actively check for started list on every usb_ep_queue() call. Checking during dwc3_gadget_ep_cleanup_completed_requests() is already too late.
For UVC, perhaps you can introduce a new flag to usb_request called "ignore_queue_latency" or something equivalent. The dwc3 is already partially doing this for UVC. With this new flag, we can rework dwc3 to clearly separate the expected behavior from the function driver.
I don't know why this "extra" flag is even necessary. The code example is already working without that extra flag.
The flag is for controller to determine what kinds of behavior the function driver expects. My intention is if this extra flag is not set, the dwc3 driver will not attempt to reshcedule isoc request at all (ie. no stop_active_transfer()).
Actually I even came up with an better solution. Additionally of checking if one of the requests in the started list was missed, we can activly check if the trb ring did run dry and if dwc3_gadget_endpoint_trbs_complete is going to enqueue in to the empty trb ring.
So my whole change looks like that:
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index efe6caf4d0e87..2c8047dcd1612 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -952,6 +952,7 @@ struct dwc3_request { #define DWC3_REQUEST_STATUS_DEQUEUED 3 #define DWC3_REQUEST_STATUS_STALLED 4 #define DWC3_REQUEST_STATUS_COMPLETED 5 +#define DWC3_REQUEST_STATUS_MISSED_ISOC 6 #define DWC3_REQUEST_STATUS_UNKNOWN -1 u8 epnum; diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 858fe4c299b7a..a31f4d3502bd3 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2057,6 +2057,9 @@ static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep) req = next_request(&dep->cancelled_list); dwc3_gadget_ep_skip_trbs(dep, req); switch (req->status) {
case 0:dwc3_gadget_giveback(dep, req, 0);break;@@ -2066,6 +2069,9 @@ static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep) case DWC3_REQUEST_STATUS_STALLED: dwc3_gadget_giveback(dep, req, -EPIPE); break;
case DWC3_REQUEST_STATUS_MISSED_ISOC:dwc3_gadget_giveback(dep, req, -EXDEV);break;@@ -3509,6 +3515,36 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, return ret; } +static int dwc3_gadget_ep_check_missed_requests(struct dwc3_ep *dep) +{
- struct dwc3_request *req;
- struct dwc3_request *tmp;
- int ret = 0;
- list_for_each_entry_safe(req, tmp, &dep->started_list, list) {
struct dwc3_trb *trb;trb = req->trb;switch (DWC3_TRB_SIZE_TRBSTS(trb->size)) {case DWC3_TRBSTS_MISSED_ISOC:/* Isoc endpoint only */ret = -EXDEV;break;case DWC3_TRB_STS_XFER_IN_PROG:/* Applicable when End Transfer with ForceRM=0 */case DWC3_TRBSTS_SETUP_PENDING:/* Control endpoint only */case DWC3_TRBSTS_OK:default:ret = 0;break;}- }
- return ret;
+}
static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep, const struct dwc3_event_depevt *event, int status) { @@ -3565,22 +3601,51 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep, { struct dwc3 *dwc = dep->dwc; bool no_started_trb = true;
- unsigned int transfer_in_flight = 0;
- /* It is possible that the interrupt thread was delayed by
* scheduling in the system, and therefor the HW has already* run dry. In that case the last trb in the queue is already* handled by the hw. By checking the HWO bit we know to restart* the whole transfer. The condition to appear is more likelely* if not every trb has the IOC bit set and therefor does not* trigger the interrupt thread fewer.*/- if (dep->number && usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
struct dwc3_trb *trb;
- dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
trb = dwc3_ep_prev_trb(dep, dep->trb_enqueue);transfer_in_flight = trb->ctrl & DWC3_TRB_CTRL_HWO;- }
- if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)
goto out;
- if (status == -EXDEV || !transfer_in_flight) {
struct dwc3_request *tmp;struct dwc3_request *req;
- if (!dep->endpoint.desc)
return no_started_trb;
if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))dwc3_stop_active_transfer(dep, true, true);
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
list_empty(&dep->started_list) &&(list_empty(&dep->pending_list) || status == -EXDEV))dwc3_stop_active_transfer(dep, true, true);- else if (dwc3_gadget_ep_should_continue(dep))
if (__dwc3_gadget_kick_transfer(dep) == 0)no_started_trb = false;
list_for_each_entry_safe(req, tmp, &dep->started_list, list) {dwc3_gadget_move_cancelled_request(req,(DWC3_TRB_SIZE_TRBSTS(req->trb->size) == DWC3_TRBSTS_MISSED_ISOC) ?DWC3_REQUEST_STATUS_MISSED_ISOC : 0);}- } else {
dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)goto out;if (!dep->endpoint.desc)return no_started_trb;if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&list_empty(&dep->started_list) && list_empty(&dep->pending_list))dwc3_stop_active_transfer(dep, true, true);else if (dwc3_gadget_ep_should_continue(dep))if (__dwc3_gadget_kick_transfer(dep) == 0)no_started_trb = false;- }
out: /*
I will seperate the whole hunk into smaller changes and send an v1 the next days to review.
No, we should not reschedule for every missed-isoc. We only want to target underrun condition.
Thanks, Thinh
On Thu, Mar 07, 2024 at 01:57:44AM +0000, Thinh Nguyen wrote:
On Tue, Feb 27, 2024, Michael Grzeschik wrote:
On Thu, Feb 22, 2024 at 01:20:04AM +0000, Thinh Nguyen wrote:
On Thu, Feb 22, 2024, Michael Grzeschik wrote:
For #2: I found an issue in the handling of the completion of requests in the started list. When the interrupt handler is *explicitly* calling stop_active_transfer if the overall event of the request was an missed event. This event value only represents the value of the request that was actually triggering the interrupt.
It also calls ep_cleanup_completed_requests and is iterating over the started requests and will call giveback/complete functions of the requests with the proper request status.
So this will also catch missed requests in the queue. However, since there might be, lets say 5 good requests and one missed request, what will happen is, that each complete call for the first good requests will enqueue new requests into the started list and will also call the updatecmd on that transfer that was already missed until the loop will reach the one request with the MISSED status bit set.
So in my opinion the patch from Jeff makes sense when adding the following change aswell. With those both changes the underruns and broken frames finally disappear. I am still unsure about the complete solution about that, since with this the mentioned 5 good requests will be cancelled aswell. So this is still a WIP status here.
When the dwc3 driver issues stop_active_transfer(), that means that the started_list is empty and there is an underrun.
At this moment this is only the case when both, pending and started list are empty. Or the interrupt event was EXDEV.
The main problem is that the function dwc3_gadget_ep_cleanup_completed_requests(dep, event, status); will issue an complete for each started request, which on the other hand will refill the pending list, and therefor after that refill the stop_active_transfer is currently never hit.
It treats the incoming requests as staled. However, for UVC, they are still "good".
Right, so in that case we can requeue them anyway. But this will have to be done after the stop transfer cmd has finished.
I think you can just check if the started_list is empty before queuing new requests. If it is, perform stop_active_transfer() to reschedule the incoming requests. None of the newly queue requests will be released yet since they are in the pending_list.
So that is basically exactly what my patch is doing. However in the case of an underrun it is not safe to call dwc3_gadget_ep_cleanup_completed_requests as jeff stated. So his underlying patch is really fixing an issue here.
What I mean is to actively check for started list on every usb_ep_queue() call. Checking during dwc3_gadget_ep_cleanup_completed_requests() is already too late.
I see.
For UVC, perhaps you can introduce a new flag to usb_request called "ignore_queue_latency" or something equivalent. The dwc3 is already partially doing this for UVC. With this new flag, we can rework dwc3 to clearly separate the expected behavior from the function driver.
I don't know why this "extra" flag is even necessary. The code example is already working without that extra flag.
The flag is for controller to determine what kinds of behavior the function driver expects. My intention is if this extra flag is not set, the dwc3 driver will not attempt to reshcedule isoc request at all (ie. no stop_active_transfer()).
Ok.
Actually I even came up with an better solution. Additionally of checking if one of the requests in the started list was missed, we can activly check if the trb ring did run dry and if dwc3_gadget_endpoint_trbs_complete is going to enqueue in to the empty trb ring.
So my whole change looks like that:
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index efe6caf4d0e87..2c8047dcd1612 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -952,6 +952,7 @@ struct dwc3_request { #define DWC3_REQUEST_STATUS_DEQUEUED 3 #define DWC3_REQUEST_STATUS_STALLED 4 #define DWC3_REQUEST_STATUS_COMPLETED 5 +#define DWC3_REQUEST_STATUS_MISSED_ISOC 6 #define DWC3_REQUEST_STATUS_UNKNOWN -1 u8 epnum; diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 858fe4c299b7a..a31f4d3502bd3 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2057,6 +2057,9 @@ static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep) req = next_request(&dep->cancelled_list); dwc3_gadget_ep_skip_trbs(dep, req); switch (req->status) {
case 0:dwc3_gadget_giveback(dep, req, 0);break;@@ -2066,6 +2069,9 @@ static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep) case DWC3_REQUEST_STATUS_STALLED: dwc3_gadget_giveback(dep, req, -EPIPE); break;
case DWC3_REQUEST_STATUS_MISSED_ISOC:dwc3_gadget_giveback(dep, req, -EXDEV);break;@@ -3509,6 +3515,36 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, return ret; } +static int dwc3_gadget_ep_check_missed_requests(struct dwc3_ep *dep) +{
- struct dwc3_request *req;
- struct dwc3_request *tmp;
- int ret = 0;
- list_for_each_entry_safe(req, tmp, &dep->started_list, list) {
struct dwc3_trb *trb;trb = req->trb;switch (DWC3_TRB_SIZE_TRBSTS(trb->size)) {case DWC3_TRBSTS_MISSED_ISOC:/* Isoc endpoint only */ret = -EXDEV;break;case DWC3_TRB_STS_XFER_IN_PROG:/* Applicable when End Transfer with ForceRM=0 */case DWC3_TRBSTS_SETUP_PENDING:/* Control endpoint only */case DWC3_TRBSTS_OK:default:ret = 0;break;}- }
- return ret;
+}
static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep, const struct dwc3_event_depevt *event, int status) { @@ -3565,22 +3601,51 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep, { struct dwc3 *dwc = dep->dwc; bool no_started_trb = true;
- unsigned int transfer_in_flight = 0;
- /* It is possible that the interrupt thread was delayed by
* scheduling in the system, and therefor the HW has already* run dry. In that case the last trb in the queue is already* handled by the hw. By checking the HWO bit we know to restart* the whole transfer. The condition to appear is more likelely* if not every trb has the IOC bit set and therefor does not* trigger the interrupt thread fewer.*/- if (dep->number && usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
struct dwc3_trb *trb;
- dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
trb = dwc3_ep_prev_trb(dep, dep->trb_enqueue);transfer_in_flight = trb->ctrl & DWC3_TRB_CTRL_HWO;- }
- if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)
goto out;
- if (status == -EXDEV || !transfer_in_flight) {
struct dwc3_request *tmp;struct dwc3_request *req;
- if (!dep->endpoint.desc)
return no_started_trb;
if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))dwc3_stop_active_transfer(dep, true, true);
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
list_empty(&dep->started_list) &&(list_empty(&dep->pending_list) || status == -EXDEV))
@[!!here!!]
dwc3_stop_active_transfer(dep, true, true);- else if (dwc3_gadget_ep_should_continue(dep))
if (__dwc3_gadget_kick_transfer(dep) == 0)no_started_trb = false;
list_for_each_entry_safe(req, tmp, &dep->started_list, list) {dwc3_gadget_move_cancelled_request(req,(DWC3_TRB_SIZE_TRBSTS(req->trb->size) == DWC3_TRBSTS_MISSED_ISOC) ?DWC3_REQUEST_STATUS_MISSED_ISOC : 0);}- } else {
dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)goto out;if (!dep->endpoint.desc)return no_started_trb;if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&list_empty(&dep->started_list) && list_empty(&dep->pending_list))dwc3_stop_active_transfer(dep, true, true);else if (dwc3_gadget_ep_should_continue(dep))if (__dwc3_gadget_kick_transfer(dep) == 0)no_started_trb = false;- }
out: /*
I will seperate the whole hunk into smaller changes and send an v1 the next days to review.
I finally send a v1 of my series.
https://lore.kernel.org/linux-usb/20240307-dwc3-gadget-complete-irq-v1-0-4fe...
For the rest of the discussion, I would like to move the conversation to the newly send series.
No, we should not reschedule for every missed-isoc. We only want to target underrun condition.
As you stated above, with reschedule what you mean is calling stop_transfer after a missed transfer was seen?
If so, why is this condition in there already? (@[!!here!!])
Michael
On Thu, Mar 07, 2024, Michael Grzeschik wrote:
On Thu, Mar 07, 2024 at 01:57:44AM +0000, Thinh Nguyen wrote:
On Tue, Feb 27, 2024, Michael Grzeschik wrote:
On Thu, Feb 22, 2024 at 01:20:04AM +0000, Thinh Nguyen wrote:
On Thu, Feb 22, 2024, Michael Grzeschik wrote:
For #2: I found an issue in the handling of the completion of requests in the started list. When the interrupt handler is *explicitly* calling stop_active_transfer if the overall event of the request was an missed event. This event value only represents the value of the request that was actually triggering the interrupt.
It also calls ep_cleanup_completed_requests and is iterating over the started requests and will call giveback/complete functions of the requests with the proper request status.
So this will also catch missed requests in the queue. However, since there might be, lets say 5 good requests and one missed request, what will happen is, that each complete call for the first good requests will enqueue new requests into the started list and will also call the updatecmd on that transfer that was already missed until the loop will reach the one request with the MISSED status bit set.
So in my opinion the patch from Jeff makes sense when adding the following change aswell. With those both changes the underruns and broken frames finally disappear. I am still unsure about the complete solution about that, since with this the mentioned 5 good requests will be cancelled aswell. So this is still a WIP status here.
When the dwc3 driver issues stop_active_transfer(), that means that the started_list is empty and there is an underrun.
At this moment this is only the case when both, pending and started list are empty. Or the interrupt event was EXDEV.
The main problem is that the function dwc3_gadget_ep_cleanup_completed_requests(dep, event, status); will issue an complete for each started request, which on the other hand will refill the pending list, and therefor after that refill the stop_active_transfer is currently never hit.
It treats the incoming requests as staled. However, for UVC, they are still "good".
Right, so in that case we can requeue them anyway. But this will have to be done after the stop transfer cmd has finished.
I think you can just check if the started_list is empty before queuing new requests. If it is, perform stop_active_transfer() to reschedule the incoming requests. None of the newly queue requests will be released yet since they are in the pending_list.
So that is basically exactly what my patch is doing. However in the case of an underrun it is not safe to call dwc3_gadget_ep_cleanup_completed_requests as jeff stated. So his underlying patch is really fixing an issue here.
What I mean is to actively check for started list on every usb_ep_queue() call. Checking during dwc3_gadget_ep_cleanup_completed_requests() is already too late.
I see.
For UVC, perhaps you can introduce a new flag to usb_request called "ignore_queue_latency" or something equivalent. The dwc3 is already partially doing this for UVC. With this new flag, we can rework dwc3 to clearly separate the expected behavior from the function driver.
I don't know why this "extra" flag is even necessary. The code example is already working without that extra flag.
The flag is for controller to determine what kinds of behavior the function driver expects. My intention is if this extra flag is not set, the dwc3 driver will not attempt to reshcedule isoc request at all (ie. no stop_active_transfer()).
Ok.
Actually I even came up with an better solution. Additionally of checking if one of the requests in the started list was missed, we can activly check if the trb ring did run dry and if dwc3_gadget_endpoint_trbs_complete is going to enqueue in to the empty trb ring.
So my whole change looks like that:
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index efe6caf4d0e87..2c8047dcd1612 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -952,6 +952,7 @@ struct dwc3_request { #define DWC3_REQUEST_STATUS_DEQUEUED 3 #define DWC3_REQUEST_STATUS_STALLED 4 #define DWC3_REQUEST_STATUS_COMPLETED 5 +#define DWC3_REQUEST_STATUS_MISSED_ISOC 6 #define DWC3_REQUEST_STATUS_UNKNOWN -1 u8 epnum; diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 858fe4c299b7a..a31f4d3502bd3 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2057,6 +2057,9 @@ static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep) req = next_request(&dep->cancelled_list); dwc3_gadget_ep_skip_trbs(dep, req); switch (req->status) {
case 0:dwc3_gadget_giveback(dep, req, 0);break;@@ -2066,6 +2069,9 @@ static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep) case DWC3_REQUEST_STATUS_STALLED: dwc3_gadget_giveback(dep, req, -EPIPE); break;
case DWC3_REQUEST_STATUS_MISSED_ISOC:dwc3_gadget_giveback(dep, req, -EXDEV);break;@@ -3509,6 +3515,36 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, return ret; } +static int dwc3_gadget_ep_check_missed_requests(struct dwc3_ep *dep) +{
- struct dwc3_request *req;
- struct dwc3_request *tmp;
- int ret = 0;
- list_for_each_entry_safe(req, tmp, &dep->started_list, list) {
struct dwc3_trb *trb;trb = req->trb;switch (DWC3_TRB_SIZE_TRBSTS(trb->size)) {case DWC3_TRBSTS_MISSED_ISOC:/* Isoc endpoint only */ret = -EXDEV;break;case DWC3_TRB_STS_XFER_IN_PROG:/* Applicable when End Transfer with ForceRM=0 */case DWC3_TRBSTS_SETUP_PENDING:/* Control endpoint only */case DWC3_TRBSTS_OK:default:ret = 0;break;}- }
- return ret;
+}
static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep, const struct dwc3_event_depevt *event, int status) { @@ -3565,22 +3601,51 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep, { struct dwc3 *dwc = dep->dwc; bool no_started_trb = true;
- unsigned int transfer_in_flight = 0;
- /* It is possible that the interrupt thread was delayed by
* scheduling in the system, and therefor the HW has already* run dry. In that case the last trb in the queue is already* handled by the hw. By checking the HWO bit we know to restart* the whole transfer. The condition to appear is more likelely* if not every trb has the IOC bit set and therefor does not* trigger the interrupt thread fewer.*/- if (dep->number && usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
struct dwc3_trb *trb;
- dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
trb = dwc3_ep_prev_trb(dep, dep->trb_enqueue);transfer_in_flight = trb->ctrl & DWC3_TRB_CTRL_HWO;- }
- if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)
goto out;
- if (status == -EXDEV || !transfer_in_flight) {
struct dwc3_request *tmp;struct dwc3_request *req;
- if (!dep->endpoint.desc)
return no_started_trb;
if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))dwc3_stop_active_transfer(dep, true, true);
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
list_empty(&dep->started_list) &&(list_empty(&dep->pending_list) || status == -EXDEV))@[!!here!!]
dwc3_stop_active_transfer(dep, true, true);- else if (dwc3_gadget_ep_should_continue(dep))
if (__dwc3_gadget_kick_transfer(dep) == 0)no_started_trb = false;
list_for_each_entry_safe(req, tmp, &dep->started_list, list) {dwc3_gadget_move_cancelled_request(req,(DWC3_TRB_SIZE_TRBSTS(req->trb->size) == DWC3_TRBSTS_MISSED_ISOC) ?DWC3_REQUEST_STATUS_MISSED_ISOC : 0);}- } else {
dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)goto out;if (!dep->endpoint.desc)return no_started_trb;if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&list_empty(&dep->started_list) && list_empty(&dep->pending_list))dwc3_stop_active_transfer(dep, true, true);else if (dwc3_gadget_ep_should_continue(dep))if (__dwc3_gadget_kick_transfer(dep) == 0)no_started_trb = false;- }
out: /*
I will seperate the whole hunk into smaller changes and send an v1 the next days to review.
I finally send a v1 of my series.
https://lore.kernel.org/linux-usb/20240307-dwc3-gadget-complete-irq-v1-0-4fe...
For the rest of the discussion, I would like to move the conversation to the newly send series.
I saw your pushes. Thanks. I'll review and move the discussion there.
No, we should not reschedule for every missed-isoc. We only want to target underrun condition.
As you stated above, with reschedule what you mean is calling stop_transfer after a missed transfer was seen?
If so, why is this condition in there already? (@[!!here!!])
It's only to reschedule if started_list is empty _and_ if there's either no pending request or there's a missed isoc. Not for every missed isoc.
BR, Thinh
linux-stable-mirror@lists.linaro.org
-
Dan Vacura -
Michael Grzeschik -
Thinh Nguyen