linux-kernel - Re: [PATCH v2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD

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: <d9c241a5-f31b-b044-bc15-1c5e4d445a69@synopsys.com>
Date:   Thu, 22 Oct 2020 01:17:51 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     John Stultz <john.stultz@...aro.org>,
        lkml <linux-kernel@...r.kernel.org>
CC:     Yu Chen <chenyu56@...wei.com>, Felipe Balbi <balbi@...nel.org>,
        Tejas Joglekar <Tejas.Joglekar@...opsys.com>,
        Yang Fei <fei.yang@...el.com>,
        YongQin Liu <yongqin.liu@...aro.org>,
        Andrzej Pietrasiewicz <andrzej.p@...labora.com>,
        Jun Li <lijun.kernel@...il.com>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: Re: [PATCH v2] usb: dwc3: Trigger a GCTL soft reset when switching
 modes in DRD

John Stultz wrote:
> From: Yu Chen <chenyu56@...wei.com>
>
> With the current dwc3 code on the HiKey960 we often see the
> COREIDLE flag get stuck off in __dwc3_gadget_start(), which
> seems to prevent the reset irq and causes the USB gadget to
> fail to initialize.
>
> We had seen occasional initialization failures with older
> kernels but with recent 5.x era kernels it seemed to be becoming
> much more common, so I dug back through some older trees and
> realized I dropped this quirk from Yu Chen during upstreaming
> as I couldn't provide a proper rational for it and it didn't
> seem to be necessary. I now realize I was wrong.
>
> After resubmitting the quirk Thinh Nguyen pointed out that it
> shouldn't be a quirk and it is actually mentioned in the
> programming guide that it should be done when switching modes
> in DRD.
>
> So, to avoid these !COREIDLE lockups seen on HiKey960, this
> patch issues GCTL soft reset when switching modes if the
> controller is in DRD mode.
>
> Cc: Felipe Balbi <balbi@...nel.org>
> Cc: Tejas Joglekar <tejas.joglekar@...opsys.com>
> Cc: Yang Fei <fei.yang@...el.com>
> Cc: YongQin Liu <yongqin.liu@...aro.org>
> Cc: Andrzej Pietrasiewicz <andrzej.p@...labora.com>
> Cc: Thinh Nguyen <thinhn@...opsys.com>
> Cc: Jun Li <lijun.kernel@...il.com>
> Cc: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: linux-usb@...r.kernel.org
> Signed-off-by: Yu Chen <chenyu56@...wei.com>
> Signed-off-by: John Stultz <john.stultz@...aro.org>
> ---
> v2:
> * Rework to always call the GCTL soft reset in DRD mode,
>   rather then using a quirk as suggested by Thinh Nguyen
>
> ---
>  drivers/usb/dwc3/core.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index bdf0925da6b6..ca94f3a2a83c 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -114,10 +114,24 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
>  	dwc->current_dr_role = mode;
>  }
>  
> +static void dwc3_gctl_core_soft_reset(struct dwc3 *dwc)
> +{
> +	int reg;
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> +	reg |= (DWC3_GCTL_CORESOFTRESET);
> +	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> +	reg &= ~(DWC3_GCTL_CORESOFTRESET);
> +	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +}
> +
>  static void __dwc3_set_mode(struct work_struct *work)
>  {
>  	struct dwc3 *dwc = work_to_dwc(work);
>  	unsigned long flags;
> +	int hw_mode;
>  	int ret;
>  	u32 reg;
>  
> @@ -154,6 +168,11 @@ static void __dwc3_set_mode(struct work_struct *work)
>  		break;
>  	}
>  
> +	/* Execute a GCTL Core Soft Reset when switch mode in DRD*/
> +	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> +	if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD)
> +		dwc3_gctl_core_soft_reset(dwc);
> +

I think this should be done inside the spin_lock.

>  	spin_lock_irqsave(&dwc->lock, flags);
>  
>  	dwc3_set_prtcap(dwc, dwc->desired_dr_role);

The DRD mode change sequence should be like this if we want to switch
from host -> device according to the programming guide (for all DRD IPs):
1. Reset controller with GCTL.CoreSoftReset
2. Set GCTL.PrtCapDir(device)
3. Soft reset with DCTL.CSftRst
4. Then follow up with the initializing registers sequence

However, from code review, with this patch, it follows this sequence:
a. Soft reset with DCTL.CSftRst on driver probe
b. Reset controller with GCTL.CoreSoftReset
c. Set GCTL.PrtCapDir(device)
d. < missing DCTL.CSftRst >
e. Then follow up with initializing registers sequence

It may work, but it doesn't follow the programming guide.

For device -> host, it should be fine because the xHCI driver will do
USBCMD.HCRST during initialization.

BR,
Thinh