linux-kernel - Re: usb: dwc3: gadget: skip pullup and set

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <20210121081333.GA2977@ubuntu>
Date:   Thu, 21 Jan 2021 17:13:33 +0900
From:   Jung Daehwan <dh10.jung@...sung.com>
To:     Wesley Cheng <wcheng@...eaurora.org>,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Cc:     Felipe Balbi <balbi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "supporter:USB SUBSYSTEM open list:DESIGNWARE USB3 DRD IP DRIVER" 
        <linux-usb@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: usb: dwc3: gadget: skip pullup and set_speed after suspend

On Wed, Jan 20, 2021 at 11:44:05PM -0800, Wesley Cheng wrote:
> 
> 
> On 1/20/2021 10:49 PM, Jung Daehwan wrote:
> > Hi,
> > 
> > On Thu, Jan 21, 2021 at 01:00:32AM +0000, Thinh Nguyen wrote:
> >> Hi,
> >>
> >> Daehwan Jung wrote:
> >>> Sometimes dwc3_gadget_pullup and dwc3_gadget_set_speed are called after
> >>> entering suspend. That's why it needs to check whether suspend
> >>>
> >>> 1. dwc3 sends disconnect uevent and turn off. (suspend)
> >>> 2. Platform side causes pullup or set_speed(e.g., adbd closes ffs node)
> >>> 3. It causes unexpected behavior like ITMON error.
> >>>
> >>> Signed-off-by: Daehwan Jung <dh10.jung@...sung.com>
> >>> ---
> >>>  drivers/usb/dwc3/gadget.c | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> >>> index ee44321..d7d4202 100644
> >>> --- a/drivers/usb/dwc3/gadget.c
> >>> +++ b/drivers/usb/dwc3/gadget.c
> >>> @@ -2093,6 +2093,9 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> >>>  	unsigned long		flags;
> >>>  	int			ret;
> >>>  
> >>> +	if (pm_runtime_suspended(dwc->dev))
> >>> +		return 0;
> >>> +
> >>>  	is_on = !!is_on;
> >>>  
> >>>  	/*
> >>> @@ -2403,6 +2406,9 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
> >>>  	unsigned long		flags;
> >>>  	u32			reg;
> >>>  
> >>> +	if (pm_runtime_suspended(dwc->dev))
> >>> +		return;
> >>> +
> >>>  	spin_lock_irqsave(&dwc->lock, flags);
> >>>  	reg = dwc3_readl(dwc->regs, DWC3_DCFG);
> >>>  	reg &= ~(DWC3_DCFG_SPEED_MASK);
> >>
> >> This is already addressed in Wesley Cheng's patches. Can you try the
> >> latest changes of DWC3 in Greg's usb-next branch?
> >>
> >> Thanks,
> >> Thinh
> > 
> > I checked Wesly Cheng's pathces but it's not same.
> > What I want to do for this patch is to avoid pullup from platform side.
> > (android in my case)
> > 
> > It's possible that platform side tries to pullup by UDC_Store after usb is already disconnected.
> > It can finally run controller and enable irq.
> > 
> > I think we have to avoid it and other possible things related to platform side.
> > 
> > 
> 
> Hi Daehwan,
> 
> I think what you're trying to do is to avoid the unnecessary runtime
> resume if the cable is disconnected and userspace attempts to
> bind/unbind the UDC.
> 
> I'm not exactly sure what patches you've pulled in, but assuming you
> didn't pull in any of the recent suspend changes:
> 
> usb: dwc3: gadget: Allow runtime suspend if UDC unbinded
> usb: dwc3: gadget: Preserve UDC max speed setting
> 
> Please consider the following scenario:
> 1.  USB connected
> 2.  UDC unbinded
> 3.  DWC3 will continue to stay in runtime active, since dwc->connected =
> true
> 
> In this scenario, we should allow the DWC3 to enter runtime suspend,
> since runstop has been disabled.
> 
> If you have pulled in the above changes, and adding your changes on top
> of it, then consider the following:
> 1.  USB connected
> 2.  UDC unbinded
> 3.  DWC enters runtime suspend (due to the above changes)
> 4.  UDC binded
> 
> The check for pm_runtime_suspended() will block step#4 from re-enabling
> the runstop bit even if the USB cable is connected.
> 
> Thanks
> Wesley Cheng
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Hi Wesley

The check for runtime_suspended() will block re-enabling the runstop bit as
you said after pulling your patches in.

UDC is contolled by userspace and it's possible UDC can be binded
unexpectedly. That's why I think it needs to handle it even if the
problem is from userspace.

Below is an example in my environment.

1. USB disconnected
2. UDC unbinded
3. DWC enters runtime suspend
4. UDC binded unexpectedly
5. Gadget start and enable irq


Line 9823: [   36.024428][ T2889] dwc3 10e00000.dwc3: Turn off gadget dwc3-gadget
Line 9827: [   36.024572][ T2889] __dwc3_gadget_stop called
Line 9828: [   36.025083][ T2603] android_work: sent uevent USB_STATE=DISCONNECTED
Line 9842: [   36.200896][  T554] usb_gadget_disconnect
Line 9843: [   36.200916][  T554] dwc3_gadget_pullup : 0
Line 9844: [   36.201165][  T554] dwc3_gadget_pullup: get_sync fail
Line 9845: [   36.201197][  T554] dwc3_gadget_stop called
Line 9846: [   36.201250][  T451] android_work: did not send uevent (0 0 0000000000000000)
Line 9849: [   36.202343][    T1] init: processing action (sys.usb.config=adb && sys.usb.configfs=1..
Line 9851: [   36.203622][    T1] dwc3_gadget_start called
Line 9852: [   36.204079][    T1] usb_gadget_connect
Line 9853: [   36.204086][    T1] dwc3_gadget_pullup : 1
Line 9854: [   36.204091][    T1] __dwc3_gadget_start called

Best Regrars,
Jung Daehwan