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

[sashiko-bot@kernel.org]

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@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