Re: [PATCH v5 3/5] phy: qualcomm: qmp-combo: Add preliminary USB4 support
From: sashiko-bot@kernel.org
Date: 2026-09-08 08:14:51
Also in:
linux-devicetree
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: New issues: - [Critical] Accessing PHY registers before enabling clocks in `qmp_combo_com_init` causes a synchronous external abort. - [High] Type-C mux events fail to update `qmpphy_mode` when the PHY is uninitialized, leading to stale state and broken subsequent connections. - [High] Double initialization of the USB4 PHY when transitioning from an active USB3 state clobbers the active hardware state. - [High] PHY mode and power state are not restored if USB4 power-on fails. - [Medium] Data race on shared state `qmp->orientation` due to unprotected access in `qmp_combo_typec_switch_set`. Pre-existing issues: - [High] Preexisting race condition: `qmp_combo_typec_mux_set` modifies shared PHY state and hardware without holding `phy_mutex`. -- commit e68a240bfd6571e7ac31565bbc5b1911cba807e3 Author: Konrad Dybcio [off-list ref] Subject: phy: qualcomm: qmp-combo: Add preliminary USB4 support This commit introduces preliminary USB4 support for Combo PHYs on certain Qualcomm platforms (SC8280XP, X1E80100, Glymur). It adds a new struct phy to expose the USB4 mode and modifies the PHY mode management to support mutually exclusive toggling between USB4/TBT3 and USB3/DP operation.
quoted hunk ↗ jump to hunk
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c index c39ced168d035..a4f130fc33e39 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -4006,6 +4036,9 @@ static int qmp_combo_dp_calibrate(struct phy *phy) static int qmp_combo_com_init(struct qmp_combo *qmp, bool force) { const struct qmp_phy_cfg *cfg = qmp->cfg; + u32 dp_reset_val = SW_DPPHY_RESET_MUX | SW_DPPHY_RESET; + void __iomem *pcs_aon = qmp->pcs_aon; + void __iomem *pcs = qmp->pcs; void __iomem *com = qmp->com; void __iomem *aon_toggle; int ret;@@ -4019,6 +4052,16 @@ static int qmp_combo_com_init(struct qmp_combo *qmp, bool force) else aon_toggle = qmp->pcs_aon; + if (qmp->qmpphy_mode == QMPPHY_MODE_USB4) { + pcs = qmp->usb4_pcs; + + qphy_setbits(pcs_aon, cfg->regs[QPHY_PCS_USB4_CLAMP_ENABLE], CLAMP_EN); + + /* Do not disturb the DP PLL in case there's an active DP tunnel */ + if (readl(com + QPHY_V3_DP_COM_RESET_OVRD_CTRL) & DP_TUNNELING_CLOCK_GEN_EN) + dp_reset_val &= ~SW_DPPHY_RESET_MUX; + } + ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs); if (ret) { dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
[Severity: Critical] Does accessing these memory-mapped registers before enabling clocks cause a synchronous external abort? When transitioning to USB4 mode, qmp_combo_reconfigure_phy() calls qmp_combo_com_exit(qmp, true) which disables all PHY clocks. Immediately after, it calls qmp_combo_com_init(qmp, true), executing this block where it reads from and writes to the PHY registers before regulator_bulk_enable() and clk_bulk_prepare_enable() are called below. [ ... ]
quoted hunk ↗ jump to hunk
@@ -4343,6 +4454,125 @@ static int qmp_combo_usb_set_mode(struct phy *phy, enum phy_mode mode, int submo return 0; } +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); + + 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; + + guard(mutex)(&qmp->phy_mutex); + + if (!qmp->p2rr2p_pipe_clk) { + dev_err(qmp->dev, "missing p2rr2p_pipe clock handle. Update your Device Tree.\n"); + return -EINVAL; + } + + ret = clk_prepare_enable(qmp->p2rr2p_pipe_clk); + if (ret) { + dev_err(qmp->dev, "p2rr2p_pipe enable failed: %d\n", ret); + return ret; + } + + ret = qmp_combo_com_init(qmp, false); + if (ret) { + clk_disable_unprepare(qmp->p2rr2p_pipe_clk); + return 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); + if (ret) { + clk_disable_unprepare(qmp->p2rr2p_pipe_clk); + qmp_combo_com_exit(qmp, false); + return ret; + }
[Severity: High] Does this execute a double initialization of the USB4 PHY? When qmp_combo_reconfigure_phy() is called and the USB3 PHY is already active (qmp->usb_init_count > 0), it tears down the connection and calls qmp_combo_usb_power_on() to start the hardware in the new USB4 mode. Since it has already started the PHY hardware state machine, does calling qmp_combo_usb_power_on() again immediately after clobber the active hardware state? [Severity: High] If this second call to qmp_combo_usb_power_on() fails (for example due to a PHY status poll timeout), does the error path fail to restore the PHY mode and power state? qmp_combo_reconfigure_phy() modifies qmp->qmpphy_mode and shuts down the active USB3/DP PHYs. If power-on fails here, qmp->qmpphy_mode is not restored and the previously active PHYs are left powered off, leaving the USB3/DP drivers believing the PHY is initialized. [ ... ]
quoted hunk ↗ jump to hunk
@@ -4779,24 +5022,29 @@ static int qmp_combo_typec_switch_set(struct typec_switch_dev *sw, struct qmp_combo *qmp = typec_switch_get_drvdata(sw); const struct qmp_phy_cfg *cfg = qmp->cfg; + if (qmp->qmpphy_mode == QMPPHY_MODE_USB4) { + /* QMPPHY has no orientation handling in USB4 mode, don't cache the setting */ + qmp->orientation = TYPEC_ORIENTATION_NONE; + return 0; + } + if (orientation == qmp->orientation || orientation == TYPEC_ORIENTATION_NONE) return 0; - mutex_lock(&qmp->phy_mutex); + guard(mutex)(&qmp->phy_mutex); qmp->orientation = orientation;
[Severity: Medium] Does writing to qmp->orientation here without holding qmp->phy_mutex introduce a data race? This write bypasses the subsequent guard(mutex)(&qmp->phy_mutex) used for normal orientation caching, creating a concurrent memory access violation if PHY initialization (such as qmp_combo_com_init) is running in parallel during Type-C switch events. [ ... ]
quoted hunk ↗ jump to hunk
@@ -4847,41 +5117,11 @@ static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_s return 0; } - dev_dbg(qmp->dev, "typec_mux_set: switching from qmpphy mode %d to %d\n", - qmp->qmpphy_mode, new_mode); - - qmp->qmpphy_mode = new_mode; - - if (qmp->init_count) { - if (qmp->usb_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); - - /* Now everything's powered down, power up the right PHYs */ - qmp_combo_com_init(qmp, true); - - if (new_mode == QMPPHY_MODE_DP_ONLY) { - if (qmp->usb_init_count) - qmp->usb_init_count--; - } - - if (new_mode == QMPPHY_MODE_USB3DP || new_mode == QMPPHY_MODE_USB3_ONLY) { - qmp_combo_usb_power_on(qmp->usb_phy); - if (!qmp->usb_init_count) - qmp->usb_init_count++; - } - - 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] Does this early return prevent qmp->qmpphy_mode from tracking the actual Type-C mux state while the PHY is powered down? If a USB4 device is unplugged, init_count drops to 0 and qmpphy_mode remains QMPPHY_MODE_USB4. If a USB3 device is then plugged in, this check returns early and fails to update qmp->qmpphy_mode. Later, when the USB controller calls phy_init(), the PHY is programmed using the stale USB4 mode without orientation, which can break the SuperSpeed link. [Severity: High] This is a pre-existing issue, but does calling qmp_combo_reconfigure_phy() from qmp_combo_typec_mux_set() modify shared PHY state and hardware without holding qmp->phy_mutex? qmp_combo_typec_mux_set() is called by the Type-C workqueue without holding the mutex. It calls qmp_combo_reconfigure_phy(), which updates qmp->qmpphy_mode and programs PHY hardware registers. This seems to race with PHY lifecycle operations running in the USB controller thread, which do acquire phy_mutex to protect these exact same variables and registers. -- Sashiko AI review · https://sashiko.dev/#/patchset/20260908-topic-usb4phy-v5-0-73aac69578ef@oss.qualcomm.com?part=3 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy