[PATCH v2 3/5] phy: qualcomm: qmp-combo: Add preliminary USB4 support
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Tue Jul 28 03:23:05 PDT 2026
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues:
- [High] Resource leak of p2rr2p_pipe_clk on error paths and early returns.
- [High] Type-C orientation is silently discarded after a USB4 disconnect, breaking subsequent USB3 connections.
- [High] Incorrect register offset is used to check for active DP tunnels, leading to incorrect DP PHY reset behavior.
- [Medium] Lockless write to qmp->orientation in qmp_combo_typec_switch_set() introduces a data race.
Pre-existing issues:
- [High] Missing mutex protection in qmp_combo_typec_mux_set() leads to severe data races on PHY state and hardware registers.
- [High] Ignored return value of qmp_combo_com_init() in reconfiguration paths leads to hardware crashes.
--
--- Patch [3]: [PATCH v2 3/5] phy: qualcomm: qmp-combo: Add preliminary USB4 support ---
commit 52b7ebb494f878e59b52ed97e3df1d0893347c98
Author: Konrad Dybcio <konrad.dybcio at oss.qualcomm.com>
phy: qualcomm: qmp-combo: Add preliminary USB4 support
This patch adds preliminary USB4 support to some QMP Combo PHYs. It introduces
a new struct phy for USB4, along with a set_mode callback for toggling
between USB4 and TBT3 submodes. The Type-C handling is delegated to the Host
Router when USB4 PHY mode is active.
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index cdcfad2e86b1d..b80eac9d71401 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
[ ... ]
> @@ -3691,14 +3691,26 @@ 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 *com = qmp->com;
> void __iomem *pcs_aon = qmp->pcs_aon;
> + void __iomem *pcs = qmp->pcs;
> int ret;
> u32 val;
>
> if (!force && qmp->init_count++)
> return 0;
>
> + 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;
> + }
[Severity: High]
Is the DP_TUNNELING_CLOCK_GEN_EN mask applied to the wrong register offset here?
DP_TUNNELING_CLOCK_GEN_EN is defined as BIT(3) for QPHY_V3_DP_COM_PHY_MODE_CTRL,
but this checks QPHY_V3_DP_COM_RESET_OVRD_CTRL instead. Reading the wrong
register might misread the hardware state and lead to incorrect DP PHY reset
behavior.
[ ... ]
> @@ -3993,6 +4100,119 @@ 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->usb4phy_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->usb4phy_init_count) &&
> + new_mode != QMPPHY_MODE_DP_ONLY)
> + qmp_combo_usb_power_on(qmp->usb_phy);
[Severity: High]
This is a pre-existing issue, but does ignoring the return value of
qmp_combo_com_init() here lead to SoC crashes if the initialization fails?
Accessing PHY MMIO registers in qmp_combo_usb_power_on() when regulators or
clocks are powered off could trigger an unrecoverable external abort.
> +
> + 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)
> + return ret;
> +
> + /* USB4 mode takes precedence to USB3(+DP), force reconfigure the PHY */
> + ret = qmp_combo_reconfigure_phy(qmp, QMPPHY_MODE_USB4);
> + if (ret)
> + return ret;
> +
> + ret = qmp_combo_usb_power_on(phy);
> + if (ret) {
> + qmp_combo_com_exit(qmp, false);
> + return ret;
> + }
[Severity: High]
Does this code leak the p2rr2p_pipe_clk clock?
If any of qmp_combo_com_init(), qmp_combo_reconfigure_phy(), or
qmp_combo_usb_power_on() fail, the function returns the error directly without
calling clk_disable_unprepare(qmp->p2rr2p_pipe_clk).
> +
> + /*
> + * Due to the SoC design, the PHY only has a single valid consumer and
> + * preventing it from having sole ownership of the PHY's power state
> + * makes suspending/resuming the router impossible.
> + */
> + WARN_ON(qmp->usb4phy_init_count++);
> +
> + return 0;
> +}
> +
> +static int qmp_combo_usb4_exit(struct phy *phy)
> +{
> + struct qmp_combo *qmp = phy_get_drvdata(phy);
> + int ret;
> +
> + guard(mutex)(&qmp->phy_mutex);
> +
> + ret = qmp_combo_usb_power_off(qmp->usb_phy);
> + if (ret)
> + return ret;
> +
> + ret = qmp_combo_com_exit(qmp, false);
> + if (ret)
> + return ret;
[Severity: High]
Are there clock leaks on these early return paths?
If qmp_combo_usb_power_off() or qmp_combo_com_exit() return an error, the
function exits early, bypassing the clk_disable_unprepare() step.
> +
> + /*
> + * Mark the USB4 PHY uninitialized and wait for a mux_set event to determine the correct
> + * setting. This will always be possible because USB4 requires Type-C.
> + */
> + qmp->usb4phy_init_count--;
> +
> + clk_disable_unprepare(qmp->p2rr2p_pipe_clk);
> +
> + return 0;
> +}
[Severity: High]
Does exiting USB4 mode fail to clear qmp->qmpphy_mode?
If qmp->qmpphy_mode is not reset here, when the next device (like a standard
USB3 device) is connected, qmp_combo_typec_switch_set() will still see
qmpphy_mode as QMPPHY_MODE_USB4. This could cause the valid Type-C
orientation to be incorrectly discarded.
[ ... ]
> @@ -4429,24 +4662,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;
> + }
[Severity: Medium]
Does writing to qmp->orientation without holding qmp->phy_mutex introduce a
data race?
Concurrent threads, such as the USB controller driver executing
qmp_combo_usb_init(), hold the phy_mutex and call qmp_combo_com_init() which
reads qmp->orientation.
>
> if (orientation == qmp->orientation || orientation == TYPEC_ORIENTATION_NONE)
> return 0;
>
> - mutex_lock(&qmp->phy_mutex);
> + guard(mutex)(&qmp->phy_mutex);
> qmp->orientation = orientation;
>
> if (qmp->init_count) {
> - if (qmp->usb_init_count)
> + if (qmp->usb_init_count || qmp->usb4phy_init_count)
> qmp_combo_usb_power_off(qmp->usb_phy);
> qmp_combo_com_exit(qmp, true);
>
> qmp_combo_com_init(qmp, true);
> - if (qmp->usb_init_count)
> + if (qmp->usb_init_count || qmp->usb4phy_init_count)
> qmp_combo_usb_power_on(qmp->usb_phy);
[Severity: High]
This is a pre-existing issue, but does ignoring the return value of
qmp_combo_com_init() here have the same hardware crash potential?
If qmp_combo_com_init() fails, qmp_combo_usb_power_on() is called
unconditionally, which might access MMIO registers while unpowered.
[ ... ]
> @@ -4497,41 +4757,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]
This is a pre-existing issue, but does qmp_combo_typec_mux_set() lack mutex
protection?
The function performs hardware reconfigurations via qmp_combo_reconfigure_phy()
and modifies shared state such as qmp->qmpphy_mode without acquiring
qmp->phy_mutex. This can race with phy_ops callbacks that do hold the mutex.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-topic-usb4phy-v2-0-5d9dd5149ec7@oss.qualcomm.com?part=3
More information about the linux-phy
mailing list