linux-kernel - Re: [PATCH v2] usb: dwc3: core: Avoid resume dwc3 if already suspended in pm resume

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: <bed7d2d2-3c66-b174-7b4e-9a2f2c0d5f1c@quicinc.com>
Date:   Fri, 15 Sep 2023 13:17:43 -0700
From:   Wesley Cheng <quic_wcheng@...cinc.com>
To:     William Wu <william.wu@...k-chips.com>,
        <Thinh.Nguyen@...opsys.com>, <gregkh@...uxfoundation.org>
CC:     <linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <frank.wang@...k-chips.com>, <jianwei.zheng@...k-chips.com>,
        <yangbin@...k-chips.com>
Subject: Re: [PATCH v2] usb: dwc3: core: Avoid resume dwc3 if already
 suspended in pm resume

Hi William,

On 9/10/2023 8:31 PM, William Wu wrote:
> If we enable PM runtime auto suspend for dwc3 on rockchip
> platforms (e.g. RK3562), it allows the dwc3 controller to
> enter runtime suspend if usb cable detached and power off
> the power domain of the controller. When system resume, if
> the dwc3 already in runtime suspended, it Shouldn't access
> the dwc3 registers in dwc3_resume() because its power domain
> maybe power off.
> 
> Test on RK3562 tablet, this patch can help to avoid kernel
> panic when accessing the dwc3 registers in dwc3_resume() if
> the dwc3 is in runtime suspended and it's power domain is
> power off.
> 
> Kernel panic - not syncing: Asynchronous SError Interrupt
> Hardware name: Rockchip RK3562 RK817 TABLET LP4 Board (DT)
> Call trace:
> dump_backtrace.cfi_jt+0x0/0x8
>    dump_stack_lvl+0xc0/0x13c
>    panic+0x174/0x468
>    arm64_serror_panic+0x1b0/0x200
>    do_serror+0x184/0x1e4
>    el1_error+0x94/0x118
>    el1_abort+0x40/0x68
>    el1_sync_handler+0x58/0x88
>    el1_sync+0x8c/0x140
>    dwc3_readl+0x30/0x1a0
>    dwc3_phy_setup+0x38/0x510
>    dwc3_core_init+0x68/0xcd4
>    dwc3_core_init_for_resume+0x10c/0x25c
>    dwc3_resume_common+0x44/0x3d0
>    dwc3_resume+0x5c/0xb8
>    dpm_run_callback+0x70/0x488
>    device_resume+0x250/0x2f8
>    dpm_resume+0x258/0x9dc
> 
> Signed-off-by: William Wu <william.wu@...k-chips.com>
> ---
> Changes in v2:
> - Remove Change-Id.
> 
>   drivers/usb/dwc3/core.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 9c6bf054f15d..8274a44f2d6a 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -2185,9 +2185,11 @@ static int dwc3_resume(struct device *dev)
>   
>   	pinctrl_pm_select_default_state(dev);
>   
> -	ret = dwc3_resume_common(dwc, PMSG_RESUME);
> -	if (ret)
> -		return ret;
> +	if (!pm_runtime_suspended(dwc->dev)) {
> +		ret = dwc3_resume_common(dwc, PMSG_RESUME);
> +		if (ret)
> +			return ret;
> +	}
>   
>   	pm_runtime_disable(dev);
>   	pm_runtime_set_active(dev);

In case DWC3 is runtime suspended, then we will avoid the 
dwc3_resume_common() call, but the current flow would also set the RPM 
state to RPM_ACTIVE. (from pm_runtime_set_active())  In this case, what 
happens if there is a pm_runtime_get/resume on the DWC3 device?

I think it would avoid calling rpm_resume() since the RPM state is 
already active, so we wouldn't be calling dwc3_runtime_resume().  Do you 
want to also extend the RPM suspended check to cover if we need to force 
PM runtime state back to active?

Thanks
Wesley Cheng