RE: [PATCH] usb: dwc2: fix a race, don't power off/on phy for dual-role mode

[Minas Harutyunyan]

Hi Fabrice,

>On 3/15/2023 6:45 PM, Fabrice Gasnier <fabrice.gasnier@xxxxxxxxxxx> wrote:
>From: Fabrice Gasnier <fabrice.gasnier@xxxxxxxxxxx>
>Sent: Wednesday, March 15, 2023 6:45 PM
>To: Minas Harutyunyan <hminas@xxxxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx;
>quentin.schulz@xxxxxxxxxxxxxxxxxxxxx
>Cc: linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-stm32@st-
>md-mailman.stormreply.com; amelie.delaunay@xxxxxxxxxxx;
>alexandre.torgue@xxxxxxxxxxx; fabrice.gasnier@xxxxxxxxxxx
>Subject: [PATCH] usb: dwc2: fix a race, don't power off/on phy for dual-role
>mode
>
>When in dual role mode (dr_mode == USB_DR_MODE_OTG), platform probe
>successively basically calls:
>- dwc2_gadget_init()
>- dwc2_hcd_init()
>- dwc2_lowlevel_hw_disable() since recent change [1]
>- usb_add_gadget_udc()
>
>The PHYs (and so the clocks it may provide) shouldn't be disabled for all
>SoCs, in OTG mode, as the HCD part has been initialized.
>
>On STM32 this creates some weird race condition upon boot, when:
>- initially attached as a device, to a HOST
>- and there is a gadget script invoked to setup the device part.
>Below issue becomes systematic, as long as the gadget script isn't started
>by userland: the hardware PHYs (and so the clocks provided by the
>PHYs) remains disabled.
>It ends up in having an endless interrupt storm, before the watchdog resets
>the platform.
>
>[   16.924163] dwc2 49000000.usb-otg: EPs: 9, dedicated fifos, 952 entries
>in SPRAM
>[   16.962704] dwc2 49000000.usb-otg: DWC OTG Controller
>[   16.966488] dwc2 49000000.usb-otg: new USB bus registered, assigned bus
>number 2
>[   16.974051] dwc2 49000000.usb-otg: irq 77, io mem 0x49000000
>[   17.032170] hub 2-0:1.0: USB hub found
>[   17.042299] hub 2-0:1.0: 1 port detected
>[   17.175408] dwc2 49000000.usb-otg: Mode Mismatch Interrupt: currently in
>Host mode
>[   17.181741] dwc2 49000000.usb-otg: Mode Mismatch Interrupt: currently in
>Host mode
>[   17.189303] dwc2 49000000.usb-otg: Mode Mismatch Interrupt: currently in
>Host mode
>...
>
>The host part is also not functional, until the gadget part is configured.
>
>The HW may only be disabled for peripheral mode (original init), e.g.
>dr_mode == USB_DR_MODE_PERIPHERAL, until the gadget driver initializes.
>
>But when in USB_DR_MODE_OTG, the HW should remain enabled, as the HCD part
>is able to run, while the gadget part isn't necessarily configured.
>
>I don't fully get the of purpose the original change, that claims disabling
>the hardware is missing. It creates conditions on SOCs using the PHY
>initialization to be completely non working in OTG mode. Original change [1]
>should be reworked to be platform specific.
>
>[1] https://urldefense.com/v3/__https://lore.kernel.org/r/20221206-dwc2-
>gadget-dual-role-v1-2-36515e1092cd@theobroma-
>systems.com__;!!A4F2R9G_pg!Y21e8pRbIOVyLQTRP5HjdeDUHpSjbtiRQVFGVCOBBDu9yH32W
>tdppqmP-8TLyGrBjyOBG5iI4qw6XMFEdfRJDhZ8HVc$
>
>Fixes: ade23d7b7ec5 ("usb: dwc2: power on/off phy for peripheral mode in
>dual-role mode")
>Signed-off-by: Fabrice Gasnier <fabrice.gasnier@xxxxxxxxxxx>

Acked-by: Minas Harutyunyan <hminas@xxxxxxxxxxxx>

>---
> drivers/usb/dwc2/gadget.c   | 6 ++----
> drivers/usb/dwc2/platform.c | 3 +--
> 2 files changed, 3 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index
>62fa6378d2d7..8b15742d9e8a 100644
>--- a/drivers/usb/dwc2/gadget.c
>+++ b/drivers/usb/dwc2/gadget.c
>@@ -4549,8 +4549,7 @@ static int dwc2_hsotg_udc_start(struct usb_gadget
>*gadget,
> 	hsotg->gadget.dev.of_node = hsotg->dev->of_node;
> 	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
>
>-	if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL ||
>-	    (hsotg->dr_mode == USB_DR_MODE_OTG && dwc2_is_device_mode(hsotg)))
>{
>+	if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL) {
> 		ret = dwc2_lowlevel_hw_enable(hsotg);
> 		if (ret)
> 			goto err;
>@@ -4612,8 +4611,7 @@ static int dwc2_hsotg_udc_stop(struct usb_gadget
>*gadget)
> 	if (!IS_ERR_OR_NULL(hsotg->uphy))
> 		otg_set_peripheral(hsotg->uphy->otg, NULL);
>
>-	if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL ||
>-	    (hsotg->dr_mode == USB_DR_MODE_OTG && dwc2_is_device_mode(hsotg)))
>+	if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL)
> 		dwc2_lowlevel_hw_disable(hsotg);
>
> 	return 0;
>diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index
>23ef75996823..262c13b6362a 100644
>--- a/drivers/usb/dwc2/platform.c
>+++ b/drivers/usb/dwc2/platform.c
>@@ -576,8 +576,7 @@ static int dwc2_driver_probe(struct platform_device
>*dev)
> 	dwc2_debugfs_init(hsotg);
>
> 	/* Gadget code manages lowlevel hw on its own */
>-	if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL ||
>-	    (hsotg->dr_mode == USB_DR_MODE_OTG && dwc2_is_device_mode(hsotg)))
>+	if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL)
> 		dwc2_lowlevel_hw_disable(hsotg);
>
> #if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || \
>--
>2.25.1