* [PATCH v3] usb: dwc3: Remove WARN_ON for device endpoint command timeouts [not found] <CGME20250808125457epcas5p111426353bf9a15dacfa217a9abff6374@epcas5p1.samsung.com> @ 2025-08-08 12:53 ` Selvarasu Ganesan 2025-08-08 13:50 ` Sebastian Andrzej Siewior 2025-08-13 14:33 ` Greg KH 0 siblings, 2 replies; 6+ messages in thread From: Selvarasu Ganesan @ 2025-08-08 12:53 UTC (permalink / raw) To: Thinh.Nguyen, gregkh, m.grzeschik, balbi, bigeasy, linux-usb, linux-kernel Cc: jh0801.jung, dh10.jung, akash.m5, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, muhammed.ali, thiagu.r, Selvarasu Ganesan, stable 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); } } -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] usb: dwc3: Remove WARN_ON for device endpoint command timeouts 2025-08-08 12:53 ` [PATCH v3] usb: dwc3: Remove WARN_ON for device endpoint command timeouts Selvarasu Ganesan @ 2025-08-08 13:50 ` Sebastian Andrzej Siewior 2025-08-13 14:33 ` Greg KH 1 sibling, 0 replies; 6+ messages in thread From: Sebastian Andrzej Siewior @ 2025-08-08 13:50 UTC (permalink / raw) To: Selvarasu Ganesan Cc: Thinh.Nguyen, gregkh, m.grzeschik, balbi, linux-usb, linux-kernel, jh0801.jung, dh10.jung, akash.m5, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, muhammed.ali, thiagu.r, stable 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] usb: dwc3: Remove WARN_ON for device endpoint command timeouts 2025-08-08 12:53 ` [PATCH v3] usb: dwc3: Remove WARN_ON for device endpoint command timeouts Selvarasu Ganesan 2025-08-08 13:50 ` Sebastian Andrzej Siewior @ 2025-08-13 14:33 ` Greg KH 2025-08-14 4:53 ` Selvarasu Ganesan 1 sibling, 1 reply; 6+ messages in thread From: Greg KH @ 2025-08-13 14:33 UTC (permalink / raw) To: Selvarasu Ganesan Cc: Thinh.Nguyen, m.grzeschik, balbi, bigeasy, linux-usb, linux-kernel, jh0801.jung, dh10.jung, akash.m5, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, muhammed.ali, thiagu.r, stable 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: > > 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); > + 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] usb: dwc3: Remove WARN_ON for device endpoint command timeouts 2025-08-13 14:33 ` Greg KH @ 2025-08-14 4:53 ` Selvarasu Ganesan 2025-08-14 5:57 ` Greg KH 0 siblings, 1 reply; 6+ messages in thread From: Selvarasu Ganesan @ 2025-08-14 4:53 UTC (permalink / raw) To: Greg KH Cc: Thinh.Nguyen, m.grzeschik, balbi, bigeasy, linux-usb, linux-kernel, jh0801.jung, dh10.jung, akash.m5, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, muhammed.ali, thiagu.r, stable 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: >> >> 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); >> + > 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] usb: dwc3: Remove WARN_ON for device endpoint command timeouts 2025-08-14 4:53 ` Selvarasu Ganesan @ 2025-08-14 5:57 ` Greg KH 2025-08-15 0:26 ` Thinh Nguyen 0 siblings, 1 reply; 6+ messages in thread From: Greg KH @ 2025-08-14 5:57 UTC (permalink / raw) To: Selvarasu Ganesan Cc: Thinh.Nguyen, m.grzeschik, balbi, bigeasy, linux-usb, linux-kernel, jh0801.jung, dh10.jung, akash.m5, hongpooh.kim, eomji.oh, shijie.cai, alim.akhtar, muhammed.ali, thiagu.r, stable 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: > >> > >> 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); > >> + > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] usb: dwc3: Remove WARN_ON for device endpoint command timeouts 2025-08-14 5:57 ` Greg KH @ 2025-08-15 0:26 ` Thinh Nguyen 0 siblings, 0 replies; 6+ messages in thread From: Thinh Nguyen @ 2025-08-15 0:26 UTC (permalink / raw) To: Greg KH Cc: Selvarasu Ganesan, Thinh Nguyen, m.grzeschik@pengutronix.de, balbi@ti.com, bigeasy@linutronix.de, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, jh0801.jung@samsung.com, dh10.jung@samsung.com, akash.m5@samsung.com, hongpooh.kim@samsung.com, eomji.oh@samsung.com, shijie.cai@samsung.com, alim.akhtar@samsung.com, muhammed.ali@samsung.com, thiagu.r@samsung.com, stable@vger.kernel.org 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: > > >> > > >> 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://urldefense.com/v3/__https://lore.kernel.org/all/20250807014639.1596-1-selvarasu.g@samsung.com/__;!!A4F2R9G_pg!ZUuS_Kx1CqLQnMC177Sy2ptmeKQEiUsNH6WS2so5sPOV0LE7C44xCawe379WzQzFNHw8z7olVaNhm2q13oGFxmHCl8s$ > > >> > > >> 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.thhsgjn73aaov2af@synopsys.com/__;!!A4F2R9G_pg!ZUuS_Kx1CqLQnMC177Sy2ptmeKQEiUsNH6WS2so5sPOV0LE7C44xCawe379WzQzFNHw8z7olVaNhm2q13oGFeZKMSI0$ > > >> --- > > >> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-08-15 0:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20250808125457epcas5p111426353bf9a15dacfa217a9abff6374@epcas5p1.samsung.com>
2025-08-08 12:53 ` [PATCH v3] usb: dwc3: Remove WARN_ON for device endpoint command timeouts Selvarasu Ganesan
2025-08-08 13:50 ` Sebastian Andrzej Siewior
2025-08-13 14:33 ` Greg KH
2025-08-14 4:53 ` Selvarasu Ganesan
2025-08-14 5:57 ` Greg KH
2025-08-15 0:26 ` Thinh Nguyen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox