linux-kernel - Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

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:   Wed, 10 Jan 2018 15:33:25 +0200
From:   Felipe Balbi <balbi@...nel.org>
To:     Roger Quadros <rogerq@...com>
Cc:     vigneshr@...com, gregkh@...uxfoundation.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        "linux-stable # \= v4 . 13" <stable@...r.kernel.org>
Subject: Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume


Hi,

Roger Quadros <rogerq@...com> writes:
> Felipe,
>
> On 10/01/18 15:11, Roger Quadros wrote:
>> The USB PHYs should be requested only once during the life cycle of
>> this driver.
>> 
>> As dwc3_core_init() is called during system suspend/resume
>> it will result in multiple calls to dwc3_core_get_phy() which is wrong.
>> 
>> To prevent that let's move dwc3_core_get_phy() call
>> outside dwc3_core_init().
>> 
>> Fixes: 541768b08a4 ("usb: dwc3: core: Call dwc3_core_get_phy() before initializing phys")
>> Cc: linux-stable <stable@...r.kernel.org> # >= v4.13
>> Signed-off-by: Roger Quadros <rogerq@...com>
>
> FYI. this patch brings the code back to
> revert 541768b08a40	("usb: dwc3: core: Call dwc3_core_get_phy() before initializing phys")
> revert f54edb539c11	("usb: dwc3: core: initialize ULPI before trying to get the PHY")
>
> So looks like this will break ULPI PHY case?
>
> Where do we initialize ULPI PHY, in dwc3_phy_setup()?
>
> if so then 541768b08a40 breaks the ULPI PHY case as well, right?

indeed, that commit regressed ULPI PHYs :-(

Seems like it should be more like below:

@@ -754,15 +754,15 @@ static int dwc3_core_init(struct dwc3 *dwc)
 			dwc->maximum_speed = USB_SPEED_HIGH;
 	}
 
-	ret = dwc3_core_get_phy(dwc);
+	ret = dwc3_phy_setup(dwc);
 	if (ret)
 		goto err0;
 
-	ret = dwc3_core_soft_reset(dwc);
+	ret = dwc3_core_get_phy(dwc);
 	if (ret)
 		goto err0;
 
-	ret = dwc3_phy_setup(dwc);
+	ret = dwc3_core_soft_reset(dwc);
 	if (ret)
 		goto err0;
 
And maybe we rename dwc3_phy_setup() to dwc3_phy_intf_config() just to
make the name match what the function actually does. Can you check that
it won't regress the case reported by Carlos? If that works, then we
would have to move BOTH dwc3_phy_setup() (dwc3_phy_intf_config()) and
dwc3_core_get_phy() outside of dwc3_core_init(), which would mean
duplicated code in suspend/resume handlers.

I'm sure we can sort that out in another way; but the proper order is:

-> initialize ULPI (if necessary)
-> get phy
-> soft reset

-- 
balbi

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