Re: [PATCH v2 3/3] arm64: dts: qcom: kaanapali: Mark the USB controller as a Type-C mode switch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: dwc3: qcom: race between dwc3_qcom_mux_set() and system suspend/resume
--

commit fe7520845f2780f9c1a4d2eb56c40d6f95200b73
Author: Joonhoe Kim <26rote@gmail.com>

arm64: dts: qcom: kaanapali: Mark the USB controller as a Type-C mode switch

This commit marks the Kaanapali USB controller node with the mode-switch
property so that it switches to the UTMI clock when the USB3-DP combo PHY
enters DisplayPort-only mode and stops the USB3 PIPE clock.

> diff --git a/arch/arm64/boot/dts/qcom/kaanapali.dtsi b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> index e48a179d07220..a06d5d8674885 100644
> --- a/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> +++ b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> @@ -3941,6 +3941,7 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>  			tx-fifo-resize;
>  			dma-coherent;
>  
> +			mode-switch;

[Severity: Medium]
Can enabling mode-switch on the Kaanapali USB controller trigger a race
between dwc3_qcom_mux_set() and system suspend/resume in
drivers/usb/dwc3/dwc3-qcom.c?

On Kaanapali boards (kaanapali-mtp.dts and kaanapali-qrd.dts),
qcom,kaanapali-pmic-glink instantiates UCSI on connector@0. In
ucsi_register_port(), UCSI creates a non-freezable singlethread workqueue
(con->wq) and acquires the USB controller mode switch via typec_mux_get().
If a connector change happens during system sleep transitions,
ucsi_handle_connector_change() runs on con->wq and invokes
typec_set_mode() -> typec_mux_set() -> dwc3_qcom_mux_set():

drivers/usb/dwc3/dwc3-qcom.c:dwc3_qcom_mux_set() {
	...
	if (qcom->ignore_pipe_clk || qcom->pm_suspended ||
	    qcom->dp_only == qcom->utmi_pipe_clk ||
	    pm_runtime_resume_and_get(qcom->dev))
		return 0;

	dwc3_qcom_select_utmi_clk(qcom, qcom->dp_only);
	...
}

In dwc3_qcom_pm_suspend(), dwc3_pm_suspend(&qcom->dwc) runs before acquiring
qcom->pipe_clk_lock and setting qcom->pm_suspended = true, while runtime PM
is still enabled until device_suspend_late():

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

	guard(mutex)(&qcom->pipe_clk_lock);

	ret = dwc3_qcom_suspend(qcom, wakeup);
	if (ret)
		return ret;

	qcom->pm_suspended = true;
	...
}

If qcom->dev was runtime-suspended and dwc3_qcom_mux_set() runs after
dwc3_pm_suspend() finishes but before pipe_clk_lock is acquired,
dwc3_qcom_mux_set() sees qcom->pm_suspended == false and calls
pm_runtime_resume_and_get(qcom->dev). Wouldn't that run
dwc3_qcom_runtime_resume() -> dwc3_runtime_resume(&qcom->dwc) (reinitializing
the core and resuming the PHYs) right before dwc3_qcom_suspend() disables
the glue clocks and interconnects?

Similarly, in dwc3_qcom_pm_resume(), qcom->pm_suspended is cleared and
pipe_clk_lock is released before calling dwc3_pm_resume(&qcom->dwc):

drivers/usb/dwc3/dwc3-qcom.c:dwc3_qcom_pm_resume() {
	...
	scoped_guard(mutex, &qcom->pipe_clk_lock) {
		ret = dwc3_qcom_resume(qcom, wakeup);
		if (ret)
			return ret;

		if (!qcom->ignore_pipe_clk &&
		    qcom->dp_only != qcom->utmi_pipe_clk)
			dwc3_qcom_select_utmi_clk(qcom, qcom->dp_only);

		qcom->pm_suspended = false;
	}

	ret = dwc3_pm_resume(&qcom->dwc);
	...
}

Inside dwc3_pm_resume(), pm_runtime_disable(dev) is held across
pm_runtime_set_active(dev) and dwc3_resume_common(). If qcom->dev was
runtime-suspended prior to system sleep, __pm_runtime_disable() records
dev->power.last_status = RPM_SUSPENDED, so rpm_resume() returns -EACCES while
runtime PM is disabled. If dwc3_qcom_mux_set() runs during that window, it
sees qcom->pm_suspended == false, updates qcom->dp_only, and returns 0 when
pm_runtime_resume_and_get(qcom->dev) fails with -EACCES. Doesn't this skip
dwc3_qcom_select_utmi_clk() and leave qcom->utmi_pipe_clk out of sync with
qcom->dp_only?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103135057.1682-1-26rote@gmail.com?part=3