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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:

New issues:
- [High] Unconditional NULL pointer dereference of pcs_aon in qmp_combo_com_init() when configuring USB4 mode.
- [High] Double initialization of the USB PHY hardware registers in qmp_combo_usb4_init().
- [High] Type-C mux events are completely lost if the PHY is uninitialized, leading to incorrect hardware configuration.
- [Medium] Resource leak of p2rr2p_pipe_clk on error paths in qmp_combo_usb4_init().
- [Medium] Data race due to unlocked access to qmpphy_mode and orientation in qmp_combo_typec_switch_set().

Pre-existing issues:
- [High] Missing lock in qmp_combo_typec_mux_set() causes race conditions with concurrent PHY initializations.
- [High] Error paths in qmp_combo_com_init() unconditionally decrement init_count, leading to underflow when force=true.
- [High] The return value of qmp_combo_com_init() is ignored during PHY reconfiguration.
--

--- Patch [3]: [PATCH v3 3/5] phy: qualcomm: qmp-combo: Add preliminary USB4 support ---
commit 77ccc077f3b20bfda2985ec877acdf62ac837b2f
Author: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

phy: qualcomm: qmp-combo: Add preliminary USB4 support

Some Combo PHYs (so far only on SC8280XP, X1E80100 and Glymur), come in
a flavor called USB43DP, which as the name implies, features USB4, USB3
and DP signal processing capabilities. This commit adds a new struct phy
to expose the USB4 mode, along with a .set_mode callback to allow
toggling between USB4 and TBT3 submodes.

> 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

[ ... ]

> @@ -3661,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;

[Severity: High]
This is a pre-existing issue, but does calling qmp_combo_com_init(qmp, true)
cause an underflow on error paths?

When force=true, the function skips qmp->init_count++, but if regulators or
clocks fail to enable, it jumps to err_decrement_count and unconditionally
executes qmp->init_count--.

>  
> +	if (qmp->qmpphy_mode == QMPPHY_MODE_USB4) {
> +		pcs = qmp->usb4_pcs;
> +
> +		qphy_setbits(pcs_aon, cfg->regs[QPHY_PCS_USB4_CLAMP_ENABLE], CLAMP_EN);

[Severity: High]
Does this unconditionally dereference pcs_aon? 

Other parts of the driver explicitly check if (pcs_aon) before accessing USB4
clamp registers, suggesting this register block is optional and could be NULL.

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but is it safe to ignore the return value of
qmp_combo_com_init() here? 

If it fails (e.g., failed clock enable), the subsequent call to
qmp_combo_usb_power_on() executes hardware register writes which might cause a
synchronous external abort.

> +
> +	if ((qmp->usb_init_count || qmp->usb4phy_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)
> +		return ret;

[Severity: Medium]
Does this leak the p2rr2p_pipe_clk reference?

If qmp_combo_com_init() or subsequent initialization steps fail, this returns
directly without calling clk_disable_unprepare().

> +
> +	/* 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);

[Severity: High]
Can this cause a double initialization of the USB PHY hardware registers?

If the USB PHY was previously initialized, qmp_combo_reconfigure_phy()
implicitly calls qmp_combo_usb_power_on(qmp->usb_phy). Then this code
immediately calls qmp_combo_usb_power_on(phy) a second time, which might lead
to hardware state machine lockups or polling timeouts.

[ ... ]

> @@ -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]
Can this result in a data race on qmp->qmpphy_mode and qmp->orientation?

This reads qmp->qmpphy_mode and modifies qmp->orientation before acquiring the
mutex guard, while other routines like qmp_combo_reconfigure_phy() modify these
fields concurrently under the lock.

> +
>  	if (orientation == qmp->orientation || orientation == TYPEC_ORIENTATION_NONE)
>  		return 0;
>  
> -	mutex_lock(&qmp->phy_mutex);
> +	guard(mutex)(&qmp->phy_mutex);

[ ... ]

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

[ ... ]

> +	/* The mux still receives Type-C events, even if all PHYs are uninitialized */
> +	if (!qmp->init_count)
> +		return 0;

[Severity: High]
Will Type-C mux events be completely lost if the PHY is uninitialized?

Previously, the incoming new_mode was unconditionally cached in qmp->qmpphy_mode
regardless of init_count. Now that the state save is buried inside
qmp_combo_reconfigure_phy(), returning early here means the requested mode
is lost if the PHY hasn't initialized yet.

>  
> -	return 0;
> +	return qmp_combo_reconfigure_phy(qmp, new_mode);

[Severity: High]
This is a pre-existing issue, but is qmp_combo_typec_mux_set() missing a lock
to prevent race conditions?

This executes without holding qmp->phy_mutex and calls
qmp_combo_reconfigure_phy() which manipulates hardware registers. Concurrently,
qmp_combo_usb_init() or qmp_combo_usb4_init() can execute from other paths that
do hold the lock, potentially clobbering the same registers.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-topic-usb4phy-v3-0-ccf0f7ea98b5@oss.qualcomm.com?part=3

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