linux-kernel - Re: [PATCH] usb: dwc3: core: Do not perform GCTL_CORE

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 for Android: free password hash cracker in your pocket

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

Message-ID: <889f7a73-d8d9-2e40-3386-e8f3f30cad24@synopsys.com>
Date:   Thu, 14 Jul 2022 01:06:07 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Rohith Kollalsi <quic_rkollals@...cinc.com>,
        Felipe Balbi <balbi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Philipp Zabel <p.zabel@...gutronix.de>
CC:     "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: dwc3: core: Do not perform GCTL_CORE_SOFTRESET
 during bootup

On 7/12/2022, Rohith Kollalsi wrote:
> According to the programming guide, it is recommended to
> perform a GCTL_CORE_SOFTRESET only when switching the mode
> from device to host or host to device. However, it is found
> that during bootup when __dwc3_set_mode() is called for the
> first time, GCTL_CORESOFTRESET is done with suspendable bit(BIT 17)
> of DWC3_GUSB3PIPECTL set. This some times leads to issues
> like controller going into bad state and controller registers
> reading value zero. Until GCTL_CORESOFTRESET is done and
> run/stop bit is set core initialization is not complete.
> Setting suspendable bit of DWC3_GUSB3PIPECTL and then
> performing GCTL_CORESOFTRESET is therefore not recommended.
> Avoid this by only performing the reset if current_dr_role is set,
> that is, when doing subsequent role switching.
>
> Fixes: f88359e1588b ("usb: dwc3: core: Do core softreset when switch mode")
> Signed-off-by: Rohith Kollalsi <quic_rkollals@...cinc.com>
> ---
>   drivers/usb/dwc3/core.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 050b2ba5986d..7ce0e033d755 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -158,9 +158,13 @@ static void __dwc3_set_mode(struct work_struct *work)
>   		break;
>   	}
>   
> -	/* For DRD host or device mode only */
> -	if ((DWC3_IP_IS(DWC3) || DWC3_VER_IS_PRIOR(DWC31, 190A)) &&
> -	    dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG) {
> +	/*
> +	 * When current_dr_role is zero, do not perform GCTL_CORESOFTRESET.

The line above doesn't really explain why. Can you rephrase it to 
something like this or the equivalent:
"When current_dr_role is not set, there's no role switching. Only 
perform GCTL.CoreSoftReset when there's DRD role switching."

Thanks,
Thinh

> +	 * Perform it only for DRD host or device mode.
> +	 */
> +	if (dwc->current_dr_role && ((DWC3_IP_IS(DWC3) ||
> +			DWC3_VER_IS_PRIOR(DWC31, 190A)) &&
> +			dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG)) {
>   		reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>   		reg |= DWC3_GCTL_CORESOFTRESET;
>   		dwc3_writel(dwc->regs, DWC3_GCTL, reg);