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

[sashiko-bot@kernel.org]

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy