linux-kernel - Re: [PATCH v3 2/2] usb: dwc3: Fix DRD mode change sequence following programming guide

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]

Date:   Mon, 29 Mar 2021 15:17:54 -0700
From:   Wesley Cheng <wcheng@...eaurora.org>
To:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        John Stultz <john.stultz@...aro.org>,
        lkml <linux-kernel@...r.kernel.org>
Cc:     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 v3 2/2] usb: dwc3: Fix DRD mode change sequence following
 programming guide



On 3/6/2021 3:39 PM, Thinh Nguyen wrote:
> Wesley Cheng wrote:
>>
>> On 1/7/2021 5:51 PM, John Stultz wrote:
>>> In reviewing the previous patch, Thinh Nguyen pointed out that
>>> the DRD mode change sequence should be like the following when
>>> switching 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
>>>
>>> The current code does:
>>> a. Soft reset with DCTL.CSftRst on driver probe
>>> b. Reset controller with GCTL.CoreSoftReset (added in previous
>>>    patch)
>>> c. Set GCTL.PrtCapDir(device)
>>> d. < missing DCTL.CSftRst >
>>> e. Then follow up with initializing registers sequence
>>>
>>> So this patch adds the DCTL.CSftRst soft reset that was currently
>>> missing from the dwc3 mode switching.
>>>
>>> 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: John Stultz <john.stultz@...aro.org>
>>> ---
>>> Feedback would be appreciated. I'm a little worried I should be
>>> conditionalizing the DCTL.CSftRst on DRD mode controllers, but
>>> I'm really not sure what the right thing to do is for non-DRD
>>> mode controllers.
>>> ---
>>>  drivers/usb/dwc3/core.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index b6a6b90eb2d5..71f8b07ecb99 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -40,6 +40,8 @@
>>>  
>>>  #define DWC3_DEFAULT_AUTOSUSPEND_DELAY	5000 /* ms */
>>>  
>>> +static int dwc3_core_soft_reset(struct dwc3 *dwc);
>>> +
>>>  /**
>>>   * dwc3_get_dr_mode - Validates and sets dr_mode
>>>   * @dwc: pointer to our context structure
>>> @@ -177,6 +179,7 @@ static void __dwc3_set_mode(struct work_struct *work)
>>>  
>>>  	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
>>>  
>>> +	dwc3_core_soft_reset(dwc);
>> Hi John/Thinh/Felipe,
>>
>> I actually added this change into my local branch, because we were
>> seeing an issue when switching from host mode --> peripheral mode.  What
>> was happening was that the RXFIFO register did not update back to the
>> expected value for peripheral mode by the time
>> dwc3_gadget_init_out_endpoint() was executed.  With the logic to
>> calculate the EP max packet limit based on RXFIFO reg, this caused all
>> EPs to be set with an EP max limit of 0.
>>
>> With this change, it seemed to help with the above issue.  However, can
>> we consider moving the core soft reset outside the spinlock?  At least
>> with our PHY init routines, we have some msleep() calls for waiting for
>> the PHYs to be ready, which will end up as a sleeping while atomic bug.
>> (not sure if PHY init is required to be called in atomic context)
>>
>> Thanks
>> Wesley Cheng
> 
> Hi Wesley,
> 
> Thanks for letting us know the issue you're having also.
> 
> Yes, you need to wait a certain amount of time to synchronize with the
> PHY (at least 50ms for dwc_usb32 and dwc_usb31 v1.80a and above, and
> less for older versions). When removing the spinlock to use msleep(),
> just make sure that there's no race issue. BTW, how long does your setup
> need to msleep()?
> 
Hi Thinh,

Sorry for the late response.  My mistake, its actually just a usleep()
for a less than 100uS (polling for a status bit change, so it will exit
early if possible).  For this change, can we just move the
dwc3_core_soft_reset() outside of the spinlock?

Thanks
Wesley Cheng

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project