linux-kernel - Re: [PATCH] usb: dwc3: core: configure PHY before initializing host in dwc3_set

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: <dcbf54db-3cc5-6863-2805-4aa1adce234a@gmail.com>
Date:   Sun, 13 Nov 2022 00:07:22 +0100
From:   Ferry Toth <fntoth@...il.com>
To:     Sven Peter <sven@...npeter.dev>
Cc:     Felipe Balbi <felipe.balbi@...ux.intel.com>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH] usb: dwc3: core: configure PHY before initializing host
 in dwc3_set_mode

Hi,

Op 12-11-2022 om 23:46 schreef Sven Peter:
> Hi Ferry,
> 
> On Sat, Nov 12, 2022, at 23:15, Ferry Toth wrote:
>> Hi Sven,
>>
>> Op 12-11-2022 om 11:25 schreef Sven Peter:
>>> Usually, first the PHY is set to the correct mode and then the host or
>>> device side of the controller is initialized afterwards. Otherwise a PHY
>>> that's already used has to be reconfigured.
>>> dwc3_core_init_mode() does this correctly for both host and device and
>>> __dwc3_set_mode() does it correctly when switching to device mode.
>>> When setting up host mode however it first initializes xhci and only
>>> then changes the PHY's mode. Let's also do the operations in the correct
>>> order here.
>>>
>>> Fixes: 958d1a4c40dd ("usb: dwc3: core: program PHY for proper DRD modes")
>>> Signed-off-by: Sven Peter <sven@...npeter.dev>
>>> ---
>>>    drivers/usb/dwc3/core.c | 9 +++++----
>>>    1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index ad4d644e21a4..759d23d908fa 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -212,14 +212,15 @@ static void __dwc3_set_mode(struct work_struct *work)
>>>    
>>>    	switch (dwc->desired_dr_role) {
>>>    	case DWC3_GCTL_PRTCAP_HOST:
>>> +		if (dwc->usb2_phy)
>>> +			otg_set_vbus(dwc->usb2_phy->otg, true);
>>> +		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
>>> +		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
>>> +
>>>    		ret = dwc3_host_init(dwc);
>>>    		if (ret) {
>>>    			dev_err(dwc->dev, "failed to initialize host\n");
>>>    		} else {
>>> -			if (dwc->usb2_phy)
>>> -				otg_set_vbus(dwc->usb2_phy->otg, true);
>>> -			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
>>> -			phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
>>>    			if (dwc->dis_split_quirk) {
>>>    				reg = dwc3_readl(dwc->regs, DWC3_GUCTL3);
>>>    				reg |= DWC3_GUCTL3_SPLITDISABLE;
>> This patch breaks usb host mode on Intel Merrifield platform. I am
>> testing this on top of v6.0 +
>> * my 2 "usb: dwc3: core: defer probe on ulpi_read_id timeout" patches
>> (otherwise tusb1210 will not be probed on this platform)
>> * Revert "usb: dwc3: disable USB core PHY management" (with/without this
>> patch makes no difference)
>> * usb: dwc3: Do not get extcon device when usb-role-switch is used
>> (with/without this patch makes no difference)
>>
>> ftrace shows tusb1210 is indeed still probed, nevertheless in host mode
>> it seems vbus is not powered as my connected smsc95xx based hub is not
>> active (seems not plugged).
>>
>> Flipping the switch to device mode gadgets work fine.
>>
>> Could it be dwc3_host_init() needs to be called prior to otg_set_vbus()?
> 
> Huh, thanks for testing!
> 
> For the platform I'm working on I need to set the phy mode before dwc3_host_init()
> and then found the same code further below in dwc3_core_init_mode,
> 
> static int dwc3_core_init_mode(struct dwc3 *dwc)
> {
> [...]
> 	case USB_DR_MODE_HOST:
> 		dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
> 
> 		if (dwc->usb2_phy)
> 			otg_set_vbus(dwc->usb2_phy->otg, true);
> 		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
> 		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
> 
> 		ret = dwc3_host_init(dwc);
> 		if (ret)
> 			return dev_err_probe(dev, ret, "failed to initialize host\n");
> 		break;
> [...]
> }
> 
> so I'm quite surprised it causes issue in __dwc3_set_mode now.
> If otg_set_vbus indeed needs to happen after dwc3_host_init it should probably
> also be called afterwards in dwc3_core_init_mode as well.

Maybe there is a difference between the case USB_DR_MODE_HOST above and 
case USB_DR_MODE_OTG touched by this patch?
> 
> Best,
> 
> 
> Sven