This commit addresses a rarely observed endpoint command timeout which causes kernel panic due to warn when 'panic_on_warn' is enabled and unnecessary call trace prints when 'panic_on_warn' is disabled. It is seen during fast software-controlled connect/disconnect testcases. The following is one such endpoint command timeout that we observed:
1. Connect ======= ->dwc3_thread_interrupt ->dwc3_ep0_interrupt ->configfs_composite_setup ->composite_setup ->usb_ep_queue ->dwc3_gadget_ep0_queue ->__dwc3_gadget_ep0_queue ->__dwc3_ep0_do_control_data ->dwc3_send_gadget_ep_cmd
2. Disconnect ========== ->dwc3_thread_interrupt ->dwc3_gadget_disconnect_interrupt ->dwc3_ep0_reset_state ->dwc3_ep0_end_control_data ->dwc3_send_gadget_ep_cmd
In the issue scenario, in Exynos platforms, we observed that control transfers for the previous connect have not yet been completed and end transfer command sent as a part of the disconnect sequence and processing of USB_ENDPOINT_HALT feature request from the host timeout. This maybe an expected scenario since the controller is processing EP commands sent as a part of the previous connect. It maybe better to remove WARN_ON in all places where device endpoint commands are sent to avoid unnecessary kernel panic due to warn.
Cc: stable@vger.kernel.org Co-developed-by: Akash M akash.m5@samsung.com Signed-off-by: Akash M akash.m5@samsung.com Signed-off-by: Selvarasu Ganesan selvarasu.g@samsung.com Acked-by: Thinh Nguyen Thinh.Nguyen@synopsys.com ---
Changes in v3: - Added Co-developed-by tags to reflect the correct authorship. - And Added Acked-by tag as well. Link to v2: https://lore.kernel.org/all/20250807014639.1596-1-selvarasu.g@samsung.com/
Changes in v2: - Removed the 'Fixes' tag from the commit message, as this patch does not contain a fix. - And Retained the 'stable' tag, as these changes are intended to be applied across all stable kernels. - Additionally, replaced 'dev_warn*' with 'dev_err*'." Link to v1: https://lore.kernel.org/all/20250807005638.thhsgjn73aaov2af@synopsys.com/ --- drivers/usb/dwc3/ep0.c | 20 ++++++++++++++++---- drivers/usb/dwc3/gadget.c | 10 ++++++++-- 2 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index 666ac432f52d..b4229aa13f37 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -288,7 +288,9 @@ void dwc3_ep0_out_start(struct dwc3 *dwc) dwc3_ep0_prepare_one_trb(dep, dwc->ep0_trb_addr, 8, DWC3_TRBCTL_CONTROL_SETUP, false); ret = dwc3_ep0_start_trans(dep); - WARN_ON(ret < 0); + if (ret < 0) + dev_err(dwc->dev, "ep0 out start transfer failed: %d\n", ret); + for (i = 2; i < DWC3_ENDPOINTS_NUM; i++) { struct dwc3_ep *dwc3_ep;
@@ -1061,7 +1063,9 @@ static void __dwc3_ep0_do_control_data(struct dwc3 *dwc, ret = dwc3_ep0_start_trans(dep); }
- WARN_ON(ret < 0); + if (ret < 0) + dev_err(dwc->dev, + "ep0 data phase start transfer failed: %d\n", ret); }
static int dwc3_ep0_start_control_status(struct dwc3_ep *dep) @@ -1078,7 +1082,12 @@ static int dwc3_ep0_start_control_status(struct dwc3_ep *dep)
static void __dwc3_ep0_do_control_status(struct dwc3 *dwc, struct dwc3_ep *dep) { - WARN_ON(dwc3_ep0_start_control_status(dep)); + int ret; + + ret = dwc3_ep0_start_control_status(dep); + if (ret) + dev_err(dwc->dev, + "ep0 status phase start transfer failed: %d\n", ret); }
static void dwc3_ep0_do_control_status(struct dwc3 *dwc, @@ -1121,7 +1130,10 @@ void dwc3_ep0_end_control_data(struct dwc3 *dwc, struct dwc3_ep *dep) cmd |= DWC3_DEPCMD_PARAM(dep->resource_index); memset(¶ms, 0, sizeof(params)); ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); - WARN_ON_ONCE(ret); + if (ret) + dev_err_ratelimited(dwc->dev, + "ep0 data phase end transfer failed: %d\n", ret); + dep->resource_index = 0; }
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 4a3e97e606d1..4a3d076c1015 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1772,7 +1772,11 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int dep->flags |= DWC3_EP_DELAY_STOP; return 0; } - WARN_ON_ONCE(ret); + + if (ret) + dev_err_ratelimited(dep->dwc->dev, + "end transfer failed: %d\n", ret); + dep->resource_index = 0;
if (!interrupt) @@ -4039,7 +4043,9 @@ static void dwc3_clear_stall_all_ep(struct dwc3 *dwc) dep->flags &= ~DWC3_EP_STALL;
ret = dwc3_send_clear_stall_ep_cmd(dep); - WARN_ON_ONCE(ret); + if (ret) + dev_err_ratelimited(dwc->dev, + "failed to clear STALL on %s\n", dep->name); } }
On 2025-08-08 18:23:05 [+0530], Selvarasu Ganesan wrote:
This commit addresses a rarely observed endpoint command timeout
…
Cc: stable@vger.kernel.org Co-developed-by: Akash M akash.m5@samsung.com Signed-off-by: Akash M akash.m5@samsung.com Signed-off-by: Selvarasu Ganesan selvarasu.g@samsung.com Acked-by: Thinh Nguyen Thinh.Nguyen@synopsys.com
Reviewed-by: Sebastian Andrzej Siewior bigeasy@linutronix.de
Sebastian
On Fri, Aug 08, 2025 at 06:23:05PM +0530, Selvarasu Ganesan wrote:
This commit addresses a rarely observed endpoint command timeout which causes kernel panic due to warn when 'panic_on_warn' is enabled and unnecessary call trace prints when 'panic_on_warn' is disabled. It is seen during fast software-controlled connect/disconnect testcases. The following is one such endpoint command timeout that we observed:
Connect
->dwc3_thread_interrupt ->dwc3_ep0_interrupt ->configfs_composite_setup ->composite_setup ->usb_ep_queue ->dwc3_gadget_ep0_queue ->__dwc3_gadget_ep0_queue ->__dwc3_ep0_do_control_data ->dwc3_send_gadget_ep_cmd
Disconnect
->dwc3_thread_interrupt ->dwc3_gadget_disconnect_interrupt ->dwc3_ep0_reset_state ->dwc3_ep0_end_control_data ->dwc3_send_gadget_ep_cmd
In the issue scenario, in Exynos platforms, we observed that control transfers for the previous connect have not yet been completed and end transfer command sent as a part of the disconnect sequence and processing of USB_ENDPOINT_HALT feature request from the host timeout. This maybe an expected scenario since the controller is processing EP commands sent as a part of the previous connect. It maybe better to remove WARN_ON in all places where device endpoint commands are sent to avoid unnecessary kernel panic due to warn.
Cc: stable@vger.kernel.org Co-developed-by: Akash M akash.m5@samsung.com Signed-off-by: Akash M akash.m5@samsung.com Signed-off-by: Selvarasu Ganesan selvarasu.g@samsung.com Acked-by: Thinh Nguyen Thinh.Nguyen@synopsys.com
Changes in v3:
- Added Co-developed-by tags to reflect the correct authorship.
- And Added Acked-by tag as well.
Link to v2: https://lore.kernel.org/all/20250807014639.1596-1-selvarasu.g@samsung.com/
Changes in v2:
- Removed the 'Fixes' tag from the commit message, as this patch does not contain a fix.
- And Retained the 'stable' tag, as these changes are intended to be applied across all stable kernels.
- Additionally, replaced 'dev_warn*' with 'dev_err*'."
Link to v1: https://lore.kernel.org/all/20250807005638.thhsgjn73aaov2af@synopsys.com/
drivers/usb/dwc3/ep0.c | 20 ++++++++++++++++---- drivers/usb/dwc3/gadget.c | 10 ++++++++-- 2 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index 666ac432f52d..b4229aa13f37 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -288,7 +288,9 @@ void dwc3_ep0_out_start(struct dwc3 *dwc) dwc3_ep0_prepare_one_trb(dep, dwc->ep0_trb_addr, 8, DWC3_TRBCTL_CONTROL_SETUP, false); ret = dwc3_ep0_start_trans(dep);
- WARN_ON(ret < 0);
- if (ret < 0)
dev_err(dwc->dev, "ep0 out start transfer failed: %d\n", ret);
If this fails, why aren't you returning the error and handling it properly? Just throwing an error message feels like it's not going to do much overall.
for (i = 2; i < DWC3_ENDPOINTS_NUM; i++) { struct dwc3_ep *dwc3_ep; @@ -1061,7 +1063,9 @@ static void __dwc3_ep0_do_control_data(struct dwc3 *dwc, ret = dwc3_ep0_start_trans(dep); }
- WARN_ON(ret < 0);
- if (ret < 0)
dev_err(dwc->dev,"ep0 data phase start transfer failed: %d\n", ret);
Same here, why not return the error and propagate it up the call stack?
} static int dwc3_ep0_start_control_status(struct dwc3_ep *dep) @@ -1078,7 +1082,12 @@ static int dwc3_ep0_start_control_status(struct dwc3_ep *dep) static void __dwc3_ep0_do_control_status(struct dwc3 *dwc, struct dwc3_ep *dep) {
- WARN_ON(dwc3_ep0_start_control_status(dep));
- int ret;
- ret = dwc3_ep0_start_control_status(dep);
- if (ret)
dev_err(dwc->dev,"ep0 status phase start transfer failed: %d\n", ret);
Same here. Don't "swallow" errors that you find, that's a sure way to paper over real problems.
Same for all other changes here.
thanks,
greg k-h
On 8/13/2025 8:03 PM, Greg KH wrote:
On Fri, Aug 08, 2025 at 06:23:05PM +0530, Selvarasu Ganesan wrote:
This commit addresses a rarely observed endpoint command timeout which causes kernel panic due to warn when 'panic_on_warn' is enabled and unnecessary call trace prints when 'panic_on_warn' is disabled. It is seen during fast software-controlled connect/disconnect testcases. The following is one such endpoint command timeout that we observed:
Connect
->dwc3_thread_interrupt ->dwc3_ep0_interrupt ->configfs_composite_setup ->composite_setup ->usb_ep_queue ->dwc3_gadget_ep0_queue ->__dwc3_gadget_ep0_queue ->__dwc3_ep0_do_control_data ->dwc3_send_gadget_ep_cmd
Disconnect
->dwc3_thread_interrupt ->dwc3_gadget_disconnect_interrupt ->dwc3_ep0_reset_state ->dwc3_ep0_end_control_data ->dwc3_send_gadget_ep_cmd
In the issue scenario, in Exynos platforms, we observed that control transfers for the previous connect have not yet been completed and end transfer command sent as a part of the disconnect sequence and processing of USB_ENDPOINT_HALT feature request from the host timeout. This maybe an expected scenario since the controller is processing EP commands sent as a part of the previous connect. It maybe better to remove WARN_ON in all places where device endpoint commands are sent to avoid unnecessary kernel panic due to warn.
Cc: stable@vger.kernel.org Co-developed-by: Akash M akash.m5@samsung.com Signed-off-by: Akash M akash.m5@samsung.com Signed-off-by: Selvarasu Ganesan selvarasu.g@samsung.com Acked-by: Thinh Nguyen Thinh.Nguyen@synopsys.com
Changes in v3:
- Added Co-developed-by tags to reflect the correct authorship.
- And Added Acked-by tag as well.
Link to v2: https://lore.kernel.org/all/20250807014639.1596-1-selvarasu.g@samsung.com/
Changes in v2:
- Removed the 'Fixes' tag from the commit message, as this patch does not contain a fix.
- And Retained the 'stable' tag, as these changes are intended to be applied across all stable kernels.
- Additionally, replaced 'dev_warn*' with 'dev_err*'."
Link to v1: https://lore.kernel.org/all/20250807005638.thhsgjn73aaov2af@synopsys.com/
drivers/usb/dwc3/ep0.c | 20 ++++++++++++++++---- drivers/usb/dwc3/gadget.c | 10 ++++++++-- 2 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index 666ac432f52d..b4229aa13f37 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -288,7 +288,9 @@ void dwc3_ep0_out_start(struct dwc3 *dwc) dwc3_ep0_prepare_one_trb(dep, dwc->ep0_trb_addr, 8, DWC3_TRBCTL_CONTROL_SETUP, false); ret = dwc3_ep0_start_trans(dep);
- WARN_ON(ret < 0);
- if (ret < 0)
dev_err(dwc->dev, "ep0 out start transfer failed: %d\n", ret);If this fails, why aren't you returning the error and handling it properly? Just throwing an error message feels like it's not going to do much overall.
Hi Greg,
Thanks for your review comments.
The trigger EP command is followed by an error message in case of failure, but no corrective action is required from the driver's perspective. In this context, returning an error code is not necessary, as the driver's operation can continue uninterrupted.
This approach is consistent with how WARN_ON is handled, as it also does not return a value. Furthermore, This approach aligns with how handled similar situations elsewhere in the code, where added error messages instead of using WARN_ON.
Thanks, Selva
for (i = 2; i < DWC3_ENDPOINTS_NUM; i++) { struct dwc3_ep *dwc3_ep; @@ -1061,7 +1063,9 @@ static void __dwc3_ep0_do_control_data(struct dwc3 *dwc, ret = dwc3_ep0_start_trans(dep); }
- WARN_ON(ret < 0);
- if (ret < 0)
dev_err(dwc->dev,"ep0 data phase start transfer failed: %d\n", ret);Same here, why not return the error and propagate it up the call stack?
} static int dwc3_ep0_start_control_status(struct dwc3_ep *dep) @@ -1078,7 +1082,12 @@ static int dwc3_ep0_start_control_status(struct dwc3_ep *dep) static void __dwc3_ep0_do_control_status(struct dwc3 *dwc, struct dwc3_ep *dep) {
- WARN_ON(dwc3_ep0_start_control_status(dep));
- int ret;
- ret = dwc3_ep0_start_control_status(dep);
- if (ret)
dev_err(dwc->dev,"ep0 status phase start transfer failed: %d\n", ret);Same here. Don't "swallow" errors that you find, that's a sure way to paper over real problems.
Same for all other changes here.
thanks,
greg k-h
On Thu, Aug 14, 2025 at 10:23:38AM +0530, Selvarasu Ganesan wrote:
On 8/13/2025 8:03 PM, Greg KH wrote:
On Fri, Aug 08, 2025 at 06:23:05PM +0530, Selvarasu Ganesan wrote:
This commit addresses a rarely observed endpoint command timeout which causes kernel panic due to warn when 'panic_on_warn' is enabled and unnecessary call trace prints when 'panic_on_warn' is disabled. It is seen during fast software-controlled connect/disconnect testcases. The following is one such endpoint command timeout that we observed:
Connect
->dwc3_thread_interrupt ->dwc3_ep0_interrupt ->configfs_composite_setup ->composite_setup ->usb_ep_queue ->dwc3_gadget_ep0_queue ->__dwc3_gadget_ep0_queue ->__dwc3_ep0_do_control_data ->dwc3_send_gadget_ep_cmd
Disconnect
->dwc3_thread_interrupt ->dwc3_gadget_disconnect_interrupt ->dwc3_ep0_reset_state ->dwc3_ep0_end_control_data ->dwc3_send_gadget_ep_cmd
In the issue scenario, in Exynos platforms, we observed that control transfers for the previous connect have not yet been completed and end transfer command sent as a part of the disconnect sequence and processing of USB_ENDPOINT_HALT feature request from the host timeout. This maybe an expected scenario since the controller is processing EP commands sent as a part of the previous connect. It maybe better to remove WARN_ON in all places where device endpoint commands are sent to avoid unnecessary kernel panic due to warn.
Cc: stable@vger.kernel.org Co-developed-by: Akash M akash.m5@samsung.com Signed-off-by: Akash M akash.m5@samsung.com Signed-off-by: Selvarasu Ganesan selvarasu.g@samsung.com Acked-by: Thinh Nguyen Thinh.Nguyen@synopsys.com
Changes in v3:
- Added Co-developed-by tags to reflect the correct authorship.
- And Added Acked-by tag as well.
Link to v2: https://lore.kernel.org/all/20250807014639.1596-1-selvarasu.g@samsung.com/
Changes in v2:
- Removed the 'Fixes' tag from the commit message, as this patch does not contain a fix.
- And Retained the 'stable' tag, as these changes are intended to be applied across all stable kernels.
- Additionally, replaced 'dev_warn*' with 'dev_err*'."
Link to v1: https://lore.kernel.org/all/20250807005638.thhsgjn73aaov2af@synopsys.com/
drivers/usb/dwc3/ep0.c | 20 ++++++++++++++++---- drivers/usb/dwc3/gadget.c | 10 ++++++++-- 2 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index 666ac432f52d..b4229aa13f37 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -288,7 +288,9 @@ void dwc3_ep0_out_start(struct dwc3 *dwc) dwc3_ep0_prepare_one_trb(dep, dwc->ep0_trb_addr, 8, DWC3_TRBCTL_CONTROL_SETUP, false); ret = dwc3_ep0_start_trans(dep);
- WARN_ON(ret < 0);
- if (ret < 0)
dev_err(dwc->dev, "ep0 out start transfer failed: %d\n", ret);If this fails, why aren't you returning the error and handling it properly? Just throwing an error message feels like it's not going to do much overall.
Hi Greg,
Thanks for your review comments.
The trigger EP command is followed by an error message in case of failure, but no corrective action is required from the driver's perspective. In this context, returning an error code is not necessary, as the driver's operation can continue uninterrupted.
This approach is consistent with how WARN_ON is handled, as it also does not return a value. Furthermore, This approach aligns with how handled similar situations elsewhere in the code, where added error messages instead of using WARN_ON.
Ok, thanks for letting me know, but be prepared for someone in the future to come along and attempt to actually add error handling return logic as it does seem arbitrary to do this for these cases.
greg k-h
Hi,
On Thu, Aug 14, 2025, Greg KH wrote:
On Thu, Aug 14, 2025 at 10:23:38AM +0530, Selvarasu Ganesan wrote:
On 8/13/2025 8:03 PM, Greg KH wrote:
On Fri, Aug 08, 2025 at 06:23:05PM +0530, Selvarasu Ganesan wrote:
This commit addresses a rarely observed endpoint command timeout which causes kernel panic due to warn when 'panic_on_warn' is enabled and unnecessary call trace prints when 'panic_on_warn' is disabled. It is seen during fast software-controlled connect/disconnect testcases. The following is one such endpoint command timeout that we observed:
Connect
->dwc3_thread_interrupt ->dwc3_ep0_interrupt ->configfs_composite_setup ->composite_setup ->usb_ep_queue ->dwc3_gadget_ep0_queue ->__dwc3_gadget_ep0_queue ->__dwc3_ep0_do_control_data ->dwc3_send_gadget_ep_cmd
Disconnect
->dwc3_thread_interrupt ->dwc3_gadget_disconnect_interrupt ->dwc3_ep0_reset_state ->dwc3_ep0_end_control_data ->dwc3_send_gadget_ep_cmd
In the issue scenario, in Exynos platforms, we observed that control transfers for the previous connect have not yet been completed and end transfer command sent as a part of the disconnect sequence and processing of USB_ENDPOINT_HALT feature request from the host timeout. This maybe an expected scenario since the controller is processing EP commands sent as a part of the previous connect. It maybe better to remove WARN_ON in all places where device endpoint commands are sent to avoid unnecessary kernel panic due to warn.
Cc: stable@vger.kernel.org Co-developed-by: Akash M akash.m5@samsung.com Signed-off-by: Akash M akash.m5@samsung.com Signed-off-by: Selvarasu Ganesan selvarasu.g@samsung.com Acked-by: Thinh Nguyen Thinh.Nguyen@synopsys.com
Changes in v3:
- Added Co-developed-by tags to reflect the correct authorship.
- And Added Acked-by tag as well.
Link to v2: https://urldefense.com/v3/__https://lore.kernel.org/all/20250807014639.1596-...
Changes in v2:
- Removed the 'Fixes' tag from the commit message, as this patch does not contain a fix.
- And Retained the 'stable' tag, as these changes are intended to be applied across all stable kernels.
- Additionally, replaced 'dev_warn*' with 'dev_err*'."
Link to v1: https://urldefense.com/v3/__https://lore.kernel.org/all/20250807005638.thhsg...
drivers/usb/dwc3/ep0.c | 20 ++++++++++++++++---- drivers/usb/dwc3/gadget.c | 10 ++++++++-- 2 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index 666ac432f52d..b4229aa13f37 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -288,7 +288,9 @@ void dwc3_ep0_out_start(struct dwc3 *dwc) dwc3_ep0_prepare_one_trb(dep, dwc->ep0_trb_addr, 8, DWC3_TRBCTL_CONTROL_SETUP, false); ret = dwc3_ep0_start_trans(dep);
- WARN_ON(ret < 0);
- if (ret < 0)
dev_err(dwc->dev, "ep0 out start transfer failed: %d\n", ret);If this fails, why aren't you returning the error and handling it properly? Just throwing an error message feels like it's not going to do much overall.
Hi Greg,
Thanks for your review comments.
The trigger EP command is followed by an error message in case of failure, but no corrective action is required from the driver's
This is misleading. We simply don't handle it at the moment. It does not mean that we won't handle it in the future.
perspective. In this context, returning an error code is not necessary, as the driver's operation can continue uninterrupted.
This is also misleading. An command failure can put the controller in a bad state and out of sync with host. We cannot guarantee that the driver can continue to operate as if everything is fine.
This approach is consistent with how WARN_ON is handled, as it also does not return a value. Furthermore, This approach aligns with how handled similar situations elsewhere in the code, where added error messages instead of using WARN_ON.
Ok, thanks for letting me know, but be prepared for someone in the future to come along and attempt to actually add error handling return logic as it does seem arbitrary to do this for these cases.
We're just basically ignoring the failure like before. It just that we won't also bring down the others along with it. Adding code to properly propagate the error code and/or adding recovery will be more involved and require more testing. This change is small enough without much impact to the current logic, and I think it's a step in the right direction.
BR, Thinh
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Sebastian Andrzej Siewior -
Selvarasu Ganesan -
Thinh Nguyen