Re: [PATCH v5 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:
- [Critical] Accessing PHY registers before enabling clocks in `qmp_combo_com_init` causes a synchronous external abort.
- [High] Type-C mux events fail to update `qmpphy_mode` when the PHY is uninitialized, leading to stale state and broken subsequent connections.
- [High] Double initialization of the USB4 PHY when transitioning from an active USB3 state clobbers the active hardware state.
- [High] PHY mode and power state are not restored if USB4 power-on fails.
- [Medium] Data race on shared state `qmp->orientation` due to unprotected access in `qmp_combo_typec_switch_set`.

Pre-existing issues:
- [High] Preexisting race condition: `qmp_combo_typec_mux_set` modifies shared PHY state and hardware without holding `phy_mutex`.
--

commit e68a240bfd6571e7ac31565bbc5b1911cba807e3
Author: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Subject: phy: qualcomm: qmp-combo: Add preliminary USB4 support

This commit introduces preliminary USB4 support for Combo PHYs on certain
Qualcomm platforms (SC8280XP, X1E80100, Glymur). It adds a new struct phy
to expose the USB4 mode and modifies the PHY mode management to support
mutually exclusive toggling between USB4/TBT3 and USB3/DP operation.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index c39ced168d035..a4f130fc33e39 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
[ ... ]
> @@ -4006,6 +4036,9 @@ 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 *pcs_aon = qmp->pcs_aon;
> +	void __iomem *pcs = qmp->pcs;
>  	void __iomem *com = qmp->com;
>  	void __iomem *aon_toggle;
>  	int ret;
> @@ -4019,6 +4052,16 @@ static int qmp_combo_com_init(struct qmp_combo *qmp, bool force)
>  	else
>  		aon_toggle = qmp->pcs_aon;
>  
> +	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;
> +	}
> +
>  	ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
>  	if (ret) {
>  		dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);

[Severity: Critical]
Does accessing these memory-mapped registers before enabling clocks cause a
synchronous external abort?

When transitioning to USB4 mode, qmp_combo_reconfigure_phy() calls
qmp_combo_com_exit(qmp, true) which disables all PHY clocks. Immediately after,
it calls qmp_combo_com_init(qmp, true), executing this block where it reads
from and writes to the PHY registers before regulator_bulk_enable() and
clk_bulk_prepare_enable() are called below.

[ ... ]
> @@ -4343,6 +4454,125 @@ 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->usb4_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->usb4_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) {
> +		clk_disable_unprepare(qmp->p2rr2p_pipe_clk);
> +		return ret;
> +	}
> +
> +	/* USB4 mode takes precedence to USB3(+DP), force reconfigure the PHY */
> +	ret = qmp_combo_reconfigure_phy(qmp, QMPPHY_MODE_USB4);
> +	if (ret) {
> +		clk_disable_unprepare(qmp->p2rr2p_pipe_clk);
> +		qmp_combo_com_exit(qmp, false);
> +		return ret;
> +	}
> +
> +	ret = qmp_combo_usb_power_on(phy);
> +	if (ret) {
> +		clk_disable_unprepare(qmp->p2rr2p_pipe_clk);
> +		qmp_combo_com_exit(qmp, false);
> +		return ret;
> +	}

[Severity: High]
Does this execute a double initialization of the USB4 PHY?

When qmp_combo_reconfigure_phy() is called and the USB3 PHY is already active
(qmp->usb_init_count > 0), it tears down the connection and calls
qmp_combo_usb_power_on() to start the hardware in the new USB4 mode. Since it
has already started the PHY hardware state machine, does calling
qmp_combo_usb_power_on() again immediately after clobber the active hardware
state?

[Severity: High]
If this second call to qmp_combo_usb_power_on() fails (for example due to a
PHY status poll timeout), does the error path fail to restore the PHY mode and
power state?

qmp_combo_reconfigure_phy() modifies qmp->qmpphy_mode and shuts down the active
USB3/DP PHYs. If power-on fails here, qmp->qmpphy_mode is not restored and the
previously active PHYs are left powered off, leaving the USB3/DP drivers
believing the PHY is initialized.

[ ... ]
> @@ -4779,24 +5022,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);
>  	qmp->orientation = orientation;

[Severity: Medium]
Does writing to qmp->orientation here without holding qmp->phy_mutex introduce
a data race?

This write bypasses the subsequent guard(mutex)(&qmp->phy_mutex) used for
normal orientation caching, creating a concurrent memory access violation if
PHY initialization (such as qmp_combo_com_init) is running in parallel during
Type-C switch events.

[ ... ]
> @@ -4847,41 +5117,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]
Does this early return prevent qmp->qmpphy_mode from tracking the actual
Type-C mux state while the PHY is powered down?

If a USB4 device is unplugged, init_count drops to 0 and qmpphy_mode remains
QMPPHY_MODE_USB4. If a USB3 device is then plugged in, this check returns
early and fails to update qmp->qmpphy_mode. Later, when the USB controller
calls phy_init(), the PHY is programmed using the stale USB4 mode without
orientation, which can break the SuperSpeed link.

[Severity: High]
This is a pre-existing issue, but does calling qmp_combo_reconfigure_phy()
from qmp_combo_typec_mux_set() modify shared PHY state and hardware without
holding qmp->phy_mutex?

qmp_combo_typec_mux_set() is called by the Type-C workqueue without holding the
mutex. It calls qmp_combo_reconfigure_phy(), which updates qmp->qmpphy_mode and
programs PHY hardware registers. This seems to race with PHY lifecycle
operations running in the USB controller thread, which do acquire phy_mutex to
protect these exact same variables and registers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-topic-usb4phy-v5-0-73aac69578ef@oss.qualcomm.com?part=3

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