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: <87tuumelpf.fsf@kernel.org>
Date:   Thu, 22 Oct 2020 10:58:20 +0300
From:   Felipe Balbi <balbi@...nel.org>
To:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        John Stultz <john.stultz@...aro.org>,
        lkml <linux-kernel@...r.kernel.org>
Cc:     Yu Chen <chenyu56@...wei.com>,
        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


Hi,

Thinh Nguyen <Thinh.Nguyen@...opsys.com> writes:
> John Stultz wrote:
>>  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.

The only reason why this is needed is because SNPS saves some die area
by mapping some host and peripheral register to the same physical area
in the die. I still think a full soft reset is unnecessary as we have
been running this driver without that soft reset for several years now.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (858 bytes)