linux-kernel - Re: [PATCH v4] usb: dwc3: Abort suspend on soft disconnect failure

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250422221854.yl7ypzc4kkxbxw2a@synopsys.com>
Date: Tue, 22 Apr 2025 22:19:03 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Kuen-Han Tsai <khtsai@...gle.com>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH v4] usb: dwc3: Abort suspend on soft disconnect failure

On Tue, Apr 22, 2025, Kuen-Han Tsai wrote:
> On Tue, Apr 22, 2025 at 7:20 AM Thinh Nguyen <Thinh.Nguyen@...opsys.com> wrote:
> >
> > On Mon, Apr 21, 2025, Kuen-Han Tsai wrote:
> > > On Sat, Apr 19, 2025 at 9:24 AM Thinh Nguyen <Thinh.Nguyen@...opsys.com> wrote:
> > > >
> > > > On Wed, Apr 16, 2025, Kuen-Han Tsai wrote:
> >
> > <snip>
> >
> > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > > > > index 89a4dc8ebf94..630fd5f0ce97 100644
> > > > > --- a/drivers/usb/dwc3/gadget.c
> > > > > +++ b/drivers/usb/dwc3/gadget.c
> > > > > @@ -4776,26 +4776,22 @@ int dwc3_gadget_suspend(struct dwc3 *dwc)
> > > > >       int ret;
> > > > >
> > > > >       ret = dwc3_gadget_soft_disconnect(dwc);
> > > > > -     if (ret)
> > > > > -             goto err;
> > > > > -
> > > > > -     spin_lock_irqsave(&dwc->lock, flags);
> > > > > -     if (dwc->gadget_driver)
> > > > > -             dwc3_disconnect_gadget(dwc);
> > > > > -     spin_unlock_irqrestore(&dwc->lock, flags);
> > > > > -
> > > > > -     return 0;
> > > > > -
> > > > > -err:
> > > > >       /*
> > > > >        * Attempt to reset the controller's state. Likely no
> > > > >        * communication can be established until the host
> > > > >        * performs a port reset.
> > > > >        */
> > > > > -     if (dwc->softconnect)
> > > > > +     if (ret && dwc->softconnect) {
> > > > >               dwc3_gadget_soft_connect(dwc);
> > > > > +             return -EAGAIN;
> > > >
> > > > This may make sense to have -EAGAIN for runtime suspend. I supposed this
> > > > should be fine for system suspend since it doesn't do anything special
> > > > for this error code.
> > > >
> > > > When you tested runtime suspend, did you observe that the device
> > > > successfully going into suspend on retry?
> > >
> > > Hi Thinh,
> > >
> > > Yes, the dwc3 device can be suspended using either
> > > pm_runtime_suspend() or dwc3_gadget_pullup(), the latter of which
> > > ultimately invokes pm_runtime_put().
> > >
> > > One question: Do you know how to naturally cause a run stop failure?
> > > Based on the spec, the controller cannot halt until the event buffer
> > > becomes empty. If the driver doesn't acknowledge the events, this
> > > should lead to the run stop failure. However, since I cannot naturally
> > > reproduce this problem, I am simulating this scenario by modifying
> > > dwc3_gadget_run_stop() to return a timeout error directly.
> > >
> >
> > I'm not clear what you meant by "naturally" here. The driver is
> > implemented in such a way that this should not happen. If it does, we
> > need to take look to see what we missed.
> >
> > However, to force the driver to hit the controller halt timeout, just
> > wait/generate some events and don't clear the GEVNTCOUNT of event bytes
> > before clearing the run_stop bit.
> >
> > BR,
> > Thinh
> 
> Hi Thinh,
> 
> Thank you for getting back to me and the method to force the timeout!
> 
> By "naturally," I meant reproducing the issue without artificial steps

Ok.

> designed solely to trigger it. You're right; since the driver is
> designed to prevent this state, reproducing it "naturally" is
> difficult.
> 
> I really appreciate your patience, and thank you once more for the
> helpful clarification.
> 

You're welcome.

BR,
Thinh