linux-kernel - Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature

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: <20230320185206.a4o4bmhml7rlg6f7@synopsys.com>
Date:   Mon, 20 Mar 2023 18:52:05 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Roger Quadros <rogerq@...nel.org>
CC:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "stern@...land.harvard.edu" <stern@...land.harvard.edu>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "vigneshr@...com" <vigneshr@...com>, "srk@...com" <srk@...com>,
        "r-gunasekaran@...com" <r-gunasekaran@...com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 2/2] usb: dwc3: Support
 'snps,gadget-keep-connect-sys-sleep' feature

Hi,

On Mon, Mar 20, 2023, Roger Quadros wrote:
> Implement 'snps,gadget-keep-connect-sys-sleep' property.
> 
> Do not stop the gadget controller and disconnect if this
> property is present and we are connected to a USB Host.
> 
> Prevent System sleep if Gadget is not in USB suspend.
> 
> Signed-off-by: Roger Quadros <rogerq@...nel.org>
> ---
>  drivers/usb/dwc3/core.c   | 25 +++++++++++++++++++------
>  drivers/usb/dwc3/core.h   |  2 ++
>  drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++--
>  3 files changed, 44 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 476b63618511..a47bbaa27302 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>  	dwc->dis_split_quirk = device_property_read_bool(dev,
>  				"snps,dis-split-quirk");
>  
> +	dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev,
> +				"snps,gadget-keep-connect-sys-sleep");
> +
>  	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>  	dwc->tx_de_emphasis = tx_de_emphasis;
>  
> @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>  {
>  	unsigned long	flags;
>  	u32 reg;
> +	int ret;
>  
>  	switch (dwc->current_dr_role) {
>  	case DWC3_GCTL_PRTCAP_DEVICE:
>  		if (pm_runtime_suspended(dwc->dev))
>  			break;
> -		dwc3_gadget_suspend(dwc);
> +		ret = dwc3_gadget_suspend(dwc);
> +		if (ret) {
> +			dev_err(dwc->dev, "gadget not suspended: %d\n", ret);
> +			return ret;
> +		}
>  		synchronize_irq(dwc->irq_gadget);
> -		dwc3_core_exit(dwc);
> +		if(!dwc->gadget_keep_connect_sys_sleep)
> +			dwc3_core_exit(dwc);
>  		break;
>  	case DWC3_GCTL_PRTCAP_HOST:
>  		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
> @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>  
>  	switch (dwc->current_dr_role) {
>  	case DWC3_GCTL_PRTCAP_DEVICE:
> -		ret = dwc3_core_init_for_resume(dwc);
> -		if (ret)
> -			return ret;
> +		if (!dwc->gadget_keep_connect_sys_sleep)
> +		{
> +			ret = dwc3_core_init_for_resume(dwc);
> +			if (ret)
> +				return ret;
> +
> +			dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> +		}
>  
> -		dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>  		dwc3_gadget_resume(dwc);
>  		break;
>  	case DWC3_GCTL_PRTCAP_HOST:
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 582ebd9cf9c2..f84bac815bed 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1328,6 +1328,8 @@ struct dwc3 {
>  	unsigned		dis_split_quirk:1;
>  	unsigned		async_callbacks:1;
>  
> +	unsigned		gadget_keep_connect_sys_sleep:1;
> +
>  	u16			imod_interval;
>  
>  	int			max_cfg_eps;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3c63fa97a680..8062e44f63f6 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc)
>  int dwc3_gadget_suspend(struct dwc3 *dwc)
>  {
>  	unsigned long flags;
> +	int link_state;
>  
>  	if (!dwc->gadget_driver)
>  		return 0;
>  
> -	dwc3_gadget_run_stop(dwc, false, false);
> +	if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) {
> +		link_state = dwc3_gadget_get_link_state(dwc);
> +		/* Prevent PM Sleep if not in U3/L2 */
> +		if (link_state != DWC3_LINK_STATE_U3)
> +			return -EBUSY;
> +
> +		/* don't stop/disconnect */
> +		dwc3_gadget_disable_irq(dwc);

We shouldn't disable event interrupt here. What will happen if the
device is disconnected and reconnect to the host while the device is
still in system suspend? The host would not be able to communicate with
the device then.

BR,
Thinh

> +		return 0;
> +	}
>  
> +	dwc3_gadget_run_stop(dwc, false, false);
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	dwc3_disconnect_gadget(dwc);
>  	__dwc3_gadget_stop(dwc);
> @@ -4588,11 +4599,21 @@ int dwc3_gadget_suspend(struct dwc3 *dwc)
>  
>  int dwc3_gadget_resume(struct dwc3 *dwc)
>  {
> -	int			ret;
> +	int ret;
> +	irqreturn_t irq_t;
>  
>  	if (!dwc->gadget_driver || !dwc->softconnect)
>  		return 0;
>  
> +	if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) {
> +		dwc3_gadget_enable_irq(dwc);
> +		/* check pending events */
> +		irq_t = dwc3_check_event_buf(dwc->ev_buf);
> +		if (irq_t == IRQ_WAKE_THREAD)
> +			dwc3_process_event_buf(dwc->ev_buf);
> +		return 0;
> +	}
> +
>  	ret = __dwc3_gadget_start(dwc);
>  	if (ret < 0)
>  		goto err0;
> -- 
> 2.34.1
>