Re: [PATCH v4 3/5] phy: qualcomm: qmp-combo: Add preliminary USB4 support

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:

New issues:
- [Critical] Unclocked hardware register access in `qmp_combo_com_init()` during USB4 mode selection.
- [High] Double initialization of the USB4 PHY hardware in `qmp_combo_usb4_init()`.
- [High] Lockless check of `qmpphy_mode` in `qmp_combo_typec_switch_set()` leads to a TOCTOU race and uncoordinated PHY resets.
- [High] Wiping Type-C orientation state during USB4 mode permanently breaks subsequent fallback to USB3.
- [High] Silently dropping Type-C mux events during USB4 mode permanently strands the PHY.
- [High] PM runtime leak on `usb4_phy` creation failure in `qmp_combo_probe()`.
- [High] Inconsistent PCS selection between `power_on` and `power_off` leaves the USB4 PCS permanently running on error paths.

Pre-existing issues:
- [High] Unbalanced `init_count` decrement on error path when `force == true` in `qmp_combo_com_init()`.
- [High] Missing `phy_mutex` lock in `qmp_combo_typec_mux_set()` causes unsafe concurrent hardware access.
- [Medium] Double-disable of `pipe_clk` on `qmp_combo_usb_power_on()` timeout error.
--

--- Patch [3]: [PATCH v4 3/5] phy: qualcomm: qmp-combo: Add preliminary USB4 support ---

commit 1ed9c8485ed697b644025bad10e28d1de6fda740
Author: Konrad Dybcio [off-list ref]

phy: qualcomm: qmp-combo: Add preliminary USB4 support

Some Combo PHYs (so far only on SC8280XP, X1E80100 and Glymur), come in
a flavor called USB43DP, which as the name implies, features USB4, USB3
and DP signal processing capabilities. In that architecture, USB3 and
USB4 PHYs share the same USB_PLL while featuring separate logic spaces.

[ ... ]

[Severity: Critical]
Does this code execute memory-mapped I/O reads/writes before the
clk_bulk_prepare_enable() call?

When qmp_combo_reconfigure_phy() switches to QMPPHY_MODE_USB4, it calls
qmp_combo_com_exit() to turn off clocks and regulators, then calls
qmp_combo_com_init(). These register accesses happen before the clocks
and regulators are re-enabled, which might cause a synchronous external
abort or bus error.

[ ... ]

[Severity: Medium]
This is a pre-existing issue, but does a timeout error here cause a
double-disable of pipe_clk?

If PHY initialization times out in qmp_combo_usb_power_on(), it jumps to
err_disable_pipe_clk:

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_combo_usb_power_on() {
    ...
err_disable_pipe_clk:
    clk_disable_unprepare(qmp->pipe_clk);
    return ret;
}

It then returns the error to qmp_combo_usb_init(), which calls
qmp_combo_com_exit(). The exit function unconditionally disables
pipe_clk again, potentially leading to a clock reference count underflow.

[ ... ]

[Severity: High]
Can this asymmetry leave the USB4 PCS running on error paths?

In qmp_combo_usb_power_on(), the code uses qmpphy_mode to select usb4_pcs.
Here, qmp_combo_usb_power_off() uses usb4_init_count to select it.

If qmp_combo_usb4_init() encounters a failure and errors out, it does not
increment usb4_init_count. However, the hardware was already started in USB4
mode. A subsequent call to qmp_combo_usb_exit() will see usb4_init_count == 0
and might power off the USB3 PCS instead of the active USB4 PCS.

[ ... ]

+static int qmp_combo_reconfigure_phy(struct qmp_combo *qmp, enum qmpphy_mode new_mode)
+{
+	dev_dbg(qmp->dev, "qmp_combo_reconfigure_phy: switching from qmpphy mode %d to %d\n",
+		qmp->qmpphy_mode, new_mode);
+
+	if (qmp->usb_init_count || qmp->usb4_init_count)
+		qmp_combo_usb_power_off(qmp->usb_phy);
+
+	if (qmp->dp_init_count)
+		writel(DP_PHY_PD_CTL_PSR_PWRDN, qmp->dp_dp_phy + QSERDES_DP_PHY_PD_CTL);
+
+	qmp_combo_com_exit(qmp, true);
+
+	qmp->qmpphy_mode = new_mode;
+
+	/* Now everything's powered down, power up the right PHYs */
+	qmp_combo_com_init(qmp, true);

[Severity: High]
This is a pre-existing issue, but does the error path in qmp_combo_com_init()
cause an unbalanced init_count decrement when force == true?

If force == true, the code skips incrementing qmp->init_count. However, if any
initialization step fails, the function jumps to err_decrement_count:

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_combo_com_init() {
    if (!force && qmp->init_count++)
        return 0;
    ...
    ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
    if (ret) {
        ...
        goto err_decrement_count;
    }
    ...
err_decrement_count:
    qmp->init_count--;
}

This could push the counter to -1, which evaluates to true in subsequent
if (qmp->init_count++) checks, potentially bypassing hardware initialization.

[ ... ]

+	if ((qmp->usb_init_count || qmp->usb4_init_count) &&
+	    new_mode != QMPPHY_MODE_DP_ONLY)
+		qmp_combo_usb_power_on(qmp->usb_phy);
+
+	if ((new_mode == QMPPHY_MODE_USB3DP || new_mode == QMPPHY_MODE_DP_ONLY) &&
+	    qmp->dp_init_count)
+		qmp->cfg->dp_aux_init(qmp);
+
+	return 0;
+}
+
+static int qmp_combo_usb4_init(struct phy *phy)
+{
+	struct qmp_combo *qmp = phy_get_drvdata(phy);
+	int ret;

[ ... ]

+	/* USB4 mode takes precedence to USB3(+DP), force reconfigure the PHY */
+	ret = qmp_combo_reconfigure_phy(qmp, QMPPHY_MODE_USB4);
+	if (ret) {
+		clk_disable_unprepare(qmp->p2rr2p_pipe_clk);
+		qmp_combo_com_exit(qmp, false);
+		return ret;
+	}
+
+	ret = qmp_combo_usb_power_on(phy);

[Severity: High]
Does this code force a redundant hardware power-on sequence on the same
physical block?

When qmp_combo_reconfigure_phy() is called, it already calls
qmp_combo_usb_power_on(qmp->usb_phy) if the USB PHY was active. When execution
returns to qmp_combo_usb4_init(), it unconditionally calls
qmp_combo_usb_power_on(phy) again. This writes initialization sequences and
START bits twice to a running PHY without a power cycle.

[ ... ]

[Severity: High]
Could this destructively clear the Type-C orientation state, causing state
loss upon fallback to USB3?

If qmp_combo_typec_switch_set() forces qmp->orientation = TYPEC_ORIENTATION_NONE,
and later the port falls back to USB3, the TCPM issues a switch_set (which
may drop the orientation due to stale mode) followed by mux_set(USB3). The PHY
might then configure for USB3 with NORMAL orientation, causing the link to
fail if the cable is physically REVERSE.

[Severity: High]
Is qmpphy_mode unsafely accessed outside the lock here?

The check for qmpphy_mode is done before acquiring qmp->phy_mutex. If
qmp_combo_usb4_init() is executing concurrently, it can hold the mutex and
change the mode to QMPPHY_MODE_USB4. If switch_set() evaluates the mode before
this happens, it will block on the mutex and execute a full hardware power
cycle while the host router driver expects exclusive manual control over the
USB4 PHY.

 	if (orientation == qmp->orientation || orientation == TYPEC_ORIENTATION_NONE)
 		return 0;
 
-	mutex_lock(&qmp->phy_mutex);
+	guard(mutex)(&qmp->phy_mutex);

[ ... ]

[Severity: High]
Will returning 0 here silently drop Type-C mux events and permanently strand
the PHY in USB4 mode?

If TCPM issues mux_set(USB3) before the USB4 router driver has finished its
teardown (while usb4_init_count > 0), the event is silently ignored. When
usb4_exit() eventually runs, it leaves the PHY powered off, but the TCPM
believes the USB3 transition already succeeded and might not resend it.

[ ... ]

-		if (new_mode == QMPPHY_MODE_DP_ONLY || new_mode == QMPPHY_MODE_USB3DP) {
-			if (qmp->dp_init_count)
-				cfg->dp_aux_init(qmp);
-		}
-	}
+	/* The mux still receives Type-C events, even if all PHYs are uninitialized */
+	if (!qmp->init_count)
+		return 0;
 
-	return 0;
+	return qmp_combo_reconfigure_phy(qmp, new_mode);
 }

[Severity: High]
This is a pre-existing issue, but does missing a lock here cause unsafe
concurrent hardware access?

qmp_combo_typec_mux_set() directly invokes qmp_combo_reconfigure_phy(),
which modifies shared state (qmpphy_mode, usb_init_count) and performs
full hardware power cycles. This function does not hold qmp->phy_mutex,
meaning concurrent execution with usb4_init or usb_init could cause
simultaneous uncoordinated PHY register writes.

[ ... ]

[Severity: High]
Does this jump bypass the pm_runtime_put_noidle() cleanup?

In qmp_combo_probe(), the device acquires a PM runtime reference. If
devm_phy_create() for usb4_phy fails, the code executes goto err_node_put.
This bypasses the err_pm_put label, leaking the PM runtime reference and
preventing the device from suspending.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820-topic-usb4phy-v4-0-aec9d2cb31f6@oss.qualcomm.com?part=3