| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250815002632.wjazxhscdkaelp5p@synopsys.com>
Date: Fri, 15 Aug 2025 00:26:33 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Greg KH <gregkh@...uxfoundation.org>
CC: Selvarasu Ganesan <selvarasu.g@...sung.com>,
Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
"m.grzeschik@...gutronix.de" <m.grzeschik@...gutronix.de>,
"balbi@...com" <balbi@...com>,
"bigeasy@...utronix.de" <bigeasy@...utronix.de>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"jh0801.jung@...sung.com" <jh0801.jung@...sung.com>,
"dh10.jung@...sung.com" <dh10.jung@...sung.com>,
"akash.m5@...sung.com" <akash.m5@...sung.com>,
"hongpooh.kim@...sung.com" <hongpooh.kim@...sung.com>,
"eomji.oh@...sung.com" <eomji.oh@...sung.com>,
"shijie.cai@...sung.com" <shijie.cai@...sung.com>,
"alim.akhtar@...sung.com" <alim.akhtar@...sung.com>,
"muhammed.ali@...sung.com" <muhammed.ali@...sung.com>,
"thiagu.r@...sung.com" <thiagu.r@...sung.com>,
"stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH v3] usb: dwc3: Remove WARN_ON for device endpoint command
timeouts
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@...r.kernel.org
> > >> Co-developed-by: Akash M <akash.m5@...sung.com>
> > >> Signed-off-by: Akash M <akash.m5@...sung.com>
> > >> Signed-off-by: Selvarasu Ganesan <selvarasu.g@...sung.com>
> > >> Acked-by: Thinh Nguyen <Thinh.Nguyen@...opsys.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
Powered by blists - more mailing lists