linux-kernel - Re: [RFC] usb: dwc3: core: set force_gen1 bit in USB31 devices if max speed is SS

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:   Sat, 13 May 2023 00:53:50 +0530
From:   Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
To:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>
CC:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "quic_ppratap@...cinc.com" <quic_ppratap@...cinc.com>,
        "quic_wcheng@...cinc.com" <quic_wcheng@...cinc.com>,
        "quic_jackp@...cinc.com" <quic_jackp@...cinc.com>
Subject: Re: [RFC] usb: dwc3: core: set force_gen1 bit in USB31 devices if max
 speed is SS



On 5/13/2023 12:45 AM, Krishna Kurapati PSSNV wrote:
> Hi Thinh,
> 
> On 5/13/2023 12:16 AM, Thinh Nguyen wrote:
>> On Fri, May 12, 2023, Krishna Kurapati wrote:
>>> Currently for dwc3_usb31 devices, if maximum_speed is limited to
>>
>> We usually call the controller dwc_usb3, dwc_usb31, or dwc_usb32.
>>
>>> super-speed in DT, then device mode is limited to SS, but host mode
>>> still works in SSP.
>>>
>>> The documentation for max-speed property is as follows:
>>>
>>> "Tells USB controllers we want to work up to a certain speed.
>>> Incase  this isn't passed via DT, USB controllers should default to
>>> their maximum HW capability."
>>>
>>> It doesn't specify that the property is only for device mode.
>>
>> Since this isn't really a fix, can we rephrase the lines below
>>
>>> Fix this by forcing controller supported max speed to Gen1 by
>>> setting LLUCTL.Force_Gen1 bit if controller is DWC3_USB31 and
>>> max speed is mentioned as SS in DT.
>>
>> As follow:
>> There are cases where we need to limit the host's maximum speed to
>> SuperSpeed only. Use this property for host mode to contrain host's
>> speed to SuperSpeed.
>>
>>
> Sure, will rephrase it accordingly. Thanks for the suggestion.
>>>
>>> Signed-off-by: Krishna Kurapati <quic_kriskura@...cinc.com>
>>> ---
>>> Discussion regarding the same at:
>>> https://urldefense.com/v3/__https://lore.kernel.org/all/e465c69c-3a9d-cbdb-d44e-96b99cfa1a92@quicinc.com/__;!!A4F2R9G_pg!YiQpjZIJAw-yu6gEwbKqb5nusjnKQ9dQJrulx39lQP-7JMhcNA2xd8uLJoZ_HE8SuG4Rm2uvhJTSdQ2k0fJVAxU2RWYHHg$
>>>
>>>   drivers/usb/dwc3/core.c | 13 +++++++++++++
>>>   drivers/usb/dwc3/core.h |  4 ++++
>>>   2 files changed, 17 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 0beaab932e7d..989dc76ecbca 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -116,6 +116,18 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
>>>       dwc->current_dr_role = mode;
>>>   }
>>> +static void dwc3_configure_host_speed(struct dwc3 *dwc)
>>> +{
>>> +    u32 reg;
>>> +
>>> +    if (DWC3_IP_IS(DWC31) &&
>>> +       (dwc->maximum_speed == USB_SPEED_SUPER)) {
>>> +        reg = dwc3_readl(dwc->regs, DWC3_LLUCTL);
>>> +        reg |= DWC3_LLUCTL_FORCE_GEN1;
>>> +        dwc3_writel(dwc->regs, DWC3_LLUCTL, reg);
>>> +    }
>>> +}
>>> +
>>>   static void __dwc3_set_mode(struct work_struct *work)
>>>   {
>>>       struct dwc3 *dwc = work_to_dwc(work);
>>> @@ -194,6 +206,7 @@ static void __dwc3_set_mode(struct work_struct 
>>> *work)
>>>       switch (desired_dr_role) {
>>>       case DWC3_GCTL_PRTCAP_HOST:
>>> +        dwc3_configure_host_speed(dwc);
>>  > The LLUCTL doesn't change until there's a Vcc reset. Let's just> 
> initialize it once during dwc3_core_init() if the GHWPARAM indicates the
>> controller is DRD or host only.
>>
> 
> I thought GCTL Core soft reset might clear this bit. That is why I 
> placed it here. For device mode gadget.c takes care of limiting speed. 
> So wanted to do this setting only for host mode, before we invoke host 
> init.
> 
> Thanks for letting know that only VCC reset affects this. Will move this 
> check to core init.
> 
> Regards,
> Krishna,
> 

In fact, The following snippet from programming guide is why I thought 
GCTL reset can affect this register: ( the below is from usb3 prog 
guide, I assume this would be applicable for usb31 as well)

Fields for Register: GCTL
Core Soft Reset (CoreSoftReset)
■ 1'b0 - No soft reset
■ 1'b1 - Soft reset to controller

  Clears the interrupts and all the CSRs except the following
registers:
■ GCTL
■ GUCTL
■ GSTS
■ GSNPSID
■ GGPIO
■ GUID
■ GUSB2PHYCFGn registers
■ GUSB3PIPECTLn registers
■ DCFG
■ DCTL
■ DEVTEN
■ DSTS

If so, don't we need to do this setting after every GCTL core soft reset ?

Regards,
Krishna,

>>>           ret = dwc3_host_init(dwc);
>>>           if (ret) {
>>>               dev_err(dwc->dev, "failed to initialize host\n");
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index d56457c02996..29b780a58dc6 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -121,6 +121,10 @@
>>>   #define DWC3_GPRTBIMAP_FS0    0xc188
>>>   #define DWC3_GPRTBIMAP_FS1    0xc18c
>>>   #define DWC3_GUCTL2        0xc19c
>>> +#define DWC3_LLUCTL        0xd024
>>
>> Please place the register according to its offset order.
>>
>>> +
>>> +/* Force Gen1 speed on Gen2 link */
>>> +#define DWC3_LLUCTL_FORCE_GEN1    BIT(10)
>>>   #define DWC3_VER_NUMBER        0xc1a0
>>>   #define DWC3_VER_TYPE        0xc1a4
>>> -- 
>>> 2.40.0
>>>
>>
>> Thanks,
>> Thinh