Re: [PATCH 3/3] usb: dwc3: qcom: pass live SS-link state into PHY at suspend entry

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] usb: dwc3: qcom: phy_set_mode_ext() called after PHY runtime
  suspend
- [Medium] usb: dwc3: qcom: dwc3_qcom_read_ss_state() broken when
  CONFIG_USB=m
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] usb: dwc3: qcom: NULL pointer dereference in
  dwc3_qcom_read_usb2_speed() [1]
--

commit 98eee22a6981ce3ef452cc8f9ec999a2bf823513
Author: Tapas Rana <tapas.rana@oss.qualcomm.com>

usb: dwc3: qcom: pass live SS-link state into PHY at suspend entry

This commit adds dwc3_qcom_read_ss_state() to check whether a SuperSpeed
device is attached on each port's USB3 roothub and passes that state to
phy_set_mode_ext() during dwc3_qcom_suspend().

> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index ac68b4218b56b..d4ba095e47eb0 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
[ ... ]
> @@ -251,6 +253,34 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom, i
>  	return udev->speed;
>  }
>  
> +static enum qcom_usb_ss_state dwc3_qcom_read_ss_state(struct dwc3_qcom *qcom, int port_index)
> +{
> +	struct usb_device *udev;
> +	struct usb_hcd __maybe_unused *hcd, *usb3_hcd;
> +	struct dwc3 *dwc = &qcom->dwc;
> +
> +	/*
> +	 * FIXME: Fix this layering violation.
> +	 */
> +	hcd = platform_get_drvdata(dwc->xhci);
> +	if (!hcd)
> +		return QCOM_USB_SS_ABSENT;

[Severity: High]
This problem wasn't introduced by this patch, but can
dwc3_qcom_read_usb2_speed() hit a NULL pointer dereference on hcd
[1] if the xHCI driver fails to probe?

dwc3_host_init() assigns dwc->xhci before platform_device_add(), and
platform_device_add() succeeds even if driver binding fails, leaving
dwc->xhci non-NULL with NULL drvdata.  During suspend,
dwc3_qcom_is_host() sees dwc->xhci and calls dwc3_qcom_read_usb2_speed()
right before dwc3_qcom_read_ss_state(), which dereferences
hcd->self.root_hub without the !hcd check added here:

drivers/usb/dwc3/dwc3-qcom.c:dwc3_qcom_read_usb2_speed() {
	...
	hcd = platform_get_drvdata(dwc->xhci);

#ifdef CONFIG_USB
	udev = usb_hub_find_child(hcd->self.root_hub, port_index + 1);
	...
}

> +
> +#ifdef CONFIG_USB
> +	usb3_hcd = hcd->shared_hcd ? hcd->shared_hcd : hcd;
> +	if (!usb3_hcd->self.root_hub)
> +		return QCOM_USB_SS_ABSENT;
> +
> +	udev = usb_hub_find_child(usb3_hcd->self.root_hub, port_index + 1);
> +#else
> +	udev = NULL;
> +#endif
> +	if (!udev)
> +		return QCOM_USB_SS_ABSENT;

[Severity: Medium]
Does #ifdef CONFIG_USB compile out the roothub lookup when CONFIG_USB=m?
Because USB is a tristate Kconfig symbol, modular builds define
CONFIG_USB_MODULE rather than CONFIG_USB, so dwc3_qcom_read_ss_state()
always takes the #else branch and returns QCOM_USB_SS_ABSENT even when a
SuperSpeed device is attached.  Should this use IS_REACHABLE(CONFIG_USB)
or IS_ENABLED(CONFIG_USB)?

[ ... ]
> @@ -359,8 +389,12 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>  	 * freezable workqueue.
>  	 */
>  	if (dwc3_qcom_is_host(qcom) && wakeup) {
> -		for (i = 0; i < qcom->num_ports; i++)
> +		for (i = 0; i < qcom->num_ports; i++) {
>  			qcom->ports[i].usb2_speed = dwc3_qcom_read_usb2_speed(qcom, i);
> +			qcom->ports[i].ss_state = dwc3_qcom_read_ss_state(qcom, i);
> +			phy_set_mode_ext(qcom->dwc.usb3_generic_phy[i], PHY_MODE_USB_HOST,
> +					 qcom->ports[i].ss_state);
> +		}
>  		dwc3_qcom_enable_interrupts(qcom);
>  	}

[Severity: High]
Isn't phy_set_mode_ext() called here after the USB3 PHY has already
entered runtime suspend?

Both dwc3_qcom_pm_suspend() and dwc3_qcom_runtime_suspend() call
dwc3_pm_suspend() or dwc3_runtime_suspend() before dwc3_qcom_suspend():

drivers/usb/dwc3/dwc3-qcom.c:dwc3_qcom_runtime_suspend() {
	...
	ret = dwc3_runtime_suspend(&qcom->dwc);
	if (ret)
		return ret;

	return dwc3_qcom_suspend(qcom, true);
}

Inside dwc3_suspend_common(), phy_pm_runtime_put_sync() drops the
runtime PM reference on dwc->usb3_generic_phy[i], which schedules
qmp_combo_runtime_suspend() on the parent QMP combo PHY device while
dwc3_qcom_suspend() sleeps in clk_bulk_disable_unprepare() and
dwc3_qcom_interconnect_disable().

Because qmp_combo_usb_set_mode() only records qmp->ss_state in memory and
QPHY_PCS_AUTONOMOUS_MODE_CTRL is programmed in
qmp_combo_runtime_suspend() -> qmp_combo_enable_autonomous_mode() before
the PHY clocks are disabled, updating ss_state here happens after the
PHY has already programmed autonomous mode with QCOM_USB_SS_UNKNOWN (or
the previous cycle's state).

[1] https://sashiko.dev/#/bug/linux-14f73483-dc6c-4250-802a-9e0e3203cc91

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106082716.128968-1-tapas.rana@oss.qualcomm.com?part=3

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy