[PATCH 3/5] phy: qualcomm: qmp-combo: Add preliminary USB4 support
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Mon May 18 04:32:26 PDT 2026
Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [Critical] Accessing hardware registers before enabling regulators and clocks causes a system crash.
- [High] Unconditional dereference of the `pcs_aon` pointer in `qmp_combo_com_init()`.
- [High] Register mismatch when checking for an active DP tunnel breaks the DP PLL.
- [High] Incomplete masking of DP reset bits in `qmp_combo_com_init()` actively forces the DP PHY into reset.
- [High] Type-C mux events are permanently dropped before the PHY is initialized, breaking alt modes.
- [High] Double hardware initialization and `pipe_clk` reference leak in `qmp_combo_usb4_init()`.
- [High] Type-C cable orientation state is discarded for USB3 connections following a USB4 disconnect.
- [High] Error paths in `qmp_combo_usb4_init()` leak the `p2rr2p_pipe_clk` reference.
- [High] TOCTOU data race on `qmpphy_mode` and `orientation` in `qmp_combo_typec_switch_set()`.
- [High] Autonomous mode functions access unclocked `pcs_usb` registers, causing a hardware hang.
- [High] Missing lock in `qmp_combo_typec_mux_set()` leads to concurrent PHY reconfiguration races.
- [High] `init_count` underflows on error paths in `qmp_combo_com_init()` when `force` is true.
--
commit f92b7738f7f78a59373363a1b4f2191edf48f3e5
Author: Konrad Dybcio <konrad.dybcio at oss.qualcomm.com>
phy: qualcomm: qmp-combo: Add preliminary USB4 support
This commit adds preliminary USB4 support for Qualcomm QMP combo PHYs,
introducing a new USB4 submode and handling mutually exclusive USB4 and
USB3/DP operation modes.
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 93f1aa10d4008..898f42de4a086 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_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 force is true, this bypasses the init_count increment. However, if a
failure occurs later in the function (e.g., regulator_bulk_enable fails),
the error path jumps to err_decrement_count and executes qmp->init_count--.
Could this decrement a count that was never incremented, corrupting the
driver state?
>
> + if (qmp->qmpphy_mode == QMPPHY_MODE_USB4) {
> + pcs = qmp->usb4_pcs;
> +
> + qphy_setbits(pcs_aon, cfg->regs[QPHY_PCS_USB4_CLAMP_ENABLE], CLAMP_EN);
Could pcs_aon be NULL here? Other functions like
qmp_combo_enable_autonomous_mode() check if pcs_aon is valid before using it.
If a platform lacks this memory region in the device tree, will this cause a
kernel panic?
Can accessing these hardware registers before enabling regulators and clocks
cause a system crash? The readl() and qphy_setbits() are called before
regulator_bulk_enable() and clk_bulk_prepare_enable(). Accessing unpowered
or unclocked hardware domains often triggers a synchronous external abort.
> +
> + /* 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)
Does this read the wrong register? DP_TUNNELING_CLOCK_GEN_EN is defined as a
bit in QPHY_V3_DP_COM_PHY_MODE_CTRL, rather than QPHY_V3_DP_COM_RESET_OVRD_CTRL.
This might evaluate SW_USB3PHY_RESET_MUX instead, leading to incorrect DP reset
logic.
> + dp_reset_val &= ~SW_DPPHY_RESET_MUX;
> + }
Does this incomplete mask force the DP PHY into reset? dp_reset_val is
initially SW_DPPHY_RESET_MUX | SW_DPPHY_RESET, so this clears the mux bit
but leaves SW_DPPHY_RESET set to 1.
> +
> ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
[ ... ]
> @@ -3695,8 +3737,9 @@ static int qmp_combo_com_init(struct qmp_combo *qmp, bool force)
>
> /* override hardware control for reset of qmp phy */
> qphy_setbits(com, QPHY_V3_DP_COM_RESET_OVRD_CTRL,
> - SW_DPPHY_RESET_MUX | SW_DPPHY_RESET |
> - SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
> + dp_reset_val |
> + SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET |
> + SW_USB4PHY_RESET_MUX | SW_USB4PHY_RESET);
When this is applied, since qphy_setbits() performs a bitwise OR, it cannot
clear SW_DPPHY_RESET_MUX in the hardware if it was already 1. Combined with
SW_DPPHY_RESET still being 1 in dp_reset_val, won't this actively put the
DP PHY into reset and destroy the state it was intended to protect?
[ ... ]
> @@ -3991,6 +4098,119 @@ static int qmp_combo_usb_set_mode(struct phy *phy, enum phy_mode mode, int submo
[ ... ]
> +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;
Do the error paths in this function leak the p2rr2p_pipe_clk reference?
If qmp_combo_com_init(), qmp_combo_reconfigure_phy(), or
qmp_combo_usb_power_on() fail, the function returns directly without calling
clk_disable_unprepare() for p2rr2p_pipe_clk.
> +
> + /* 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);
Can this cause a double hardware initialization and leak the pipe_clk reference?
If the USB3 PHY is already active, qmp_combo_reconfigure_phy() internally
calls qmp_combo_usb_power_on(qmp->usb_phy). Immediately after, this function
unconditionally calls qmp_combo_usb_power_on(phy) a second time. This would
enable the hardware twice and increment the pipe_clk reference count twice,
but usb4_exit() only calls power_off once.
[ ... ]
> @@ -4037,9 +4264,12 @@ static void qmp_combo_enable_autonomous_mode(struct qmp_combo *qmp)
> * Enable i/o clamp_n for autonomous mode
> * V6 and later versions use pcs aon clamp register
> */
> - if (pcs_aon)
> - qphy_clrbits(pcs_aon, cfg->regs[QPHY_PCS_CLAMP_ENABLE], CLAMP_EN);
> - else if (pcs_misc)
> + if (pcs_aon) {
> + if (qmp->qmpphy_mode == QMPPHY_MODE_USB4)
> + qphy_clrbits(pcs_aon, cfg->regs[QPHY_PCS_USB4_CLAMP_ENABLE], CLAMP_EN);
> + else
> + qphy_clrbits(pcs_aon, cfg->regs[QPHY_PCS_CLAMP_ENABLE], CLAMP_EN);
> + } else if (pcs_misc)
> qphy_clrbits(pcs_misc, cfg->regs[QPHY_PCS_CLAMP_ENABLE], CLAMP_EN);
> }
In USB4 mode, qmp_combo_com_init() does not clear SW_USB3PHY_RESET, leaving
the USB3 PCS block in hardware reset. However,
qmp_combo_enable_autonomous_mode() unconditionally accesses pcs_usb registers
earlier in the function (e.g., QPHY_PCS_LFPS_RXTERM_IRQ_CLEAR). Will
accessing an IP block held in reset cause an external abort?
[ ... ]
> @@ -4427,24 +4660,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);
Is there a data race here? The check for qmp->qmpphy_mode and the
modification of qmp->orientation are performed before acquiring
qmp->phy_mutex. If an orientation switch event races with
qmp_combo_usb4_init() modifying qmpphy_mode, switch_set could read a stale
mode and enter the critical section to execute a full PHY reset while the
USB4 router is actively using it.
Also, if a USB4 device is unplugged, usb4_exit is called but qmpphy_mode
remains QMPPHY_MODE_USB4. If a USB3 device is then plugged in, switch_set()
is typically called before mux_set(). Because it sees the stale USB4 mode,
it drops the valid orientation and forces TYPEC_ORIENTATION_NONE.
When mux_set() is subsequently called, it configures USB3 with no orientation.
Will this cause enumeration to fail if the Type-C cable is flipped?
[ ... ]
> @@ -4495,41 +4755,11 @@ static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_s
[ ... ]
> + /* 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);
> }
Does this early return break alt modes by dropping Type-C events? It
bypasses saving the requested mode (qmp->qmpphy_mode = new_mode). Since
Type-C events often arrive before the USB controller initializes the PHY,
the requested mode is lost. When phy_init() is later called, it will
initialize with the stale default mode, breaking USB3 or DP routing.
Additionally, qmp_combo_reconfigure_phy() is called here without acquiring
qmp->phy_mutex. Concurrently, qmp_combo_usb4_init() uses the mutex and also
calls qmp_combo_reconfigure_phy(). Doesn't this lack of synchronization
permit concurrent execution of PHY hardware power down/up sequences,
potentially corrupting register states?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-topic-usb4phy-v1-0-71d827c49dca@oss.qualcomm.com?part=3
More information about the linux-phy
mailing list