[PATCH v5 3/5] phy: qualcomm: qmp-combo: Add preliminary USB4 support
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Tue Sep 8 01:14:50 PDT 2026
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 <konrad.dybcio at oss.qualcomm.com>
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.
> 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
[ ... ]
> @@ -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.
[ ... ]
> @@ -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.
[ ... ]
> @@ -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.
[ ... ]
> @@ -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
More information about the linux-phy
mailing list