Re: [PATCH v2 1/3] phy: qcom: qmp-combo: delay Type-C mux switch while DP PHY is initializing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] phy: qcom: qmp-combo: qmp_combo_typec_mux_set() drops valid
  mode switches
- [Medium] phy: qcom: qmp-combo: commit message confuses orientation and
  mode callbacks
--

commit de650bdad284e255575480f09852006eb78a904a
Author: Saurabh Anand <saurabh.anand@oss.qualcomm.com>

phy: qcom: qmp-combo: delay Type-C mux switch while DP PHY is initializing

This commit extends the guard in qmp_combo_typec_mux_set() to return early
when qmp->dp_init_count is non-zero.

> The Type-C mux switch guard only checked dp_powered_on, which is set in
> qmp_combo_dp_power_on(). However there is a race window between
> qmp_combo_dp_init() and qmp_combo_dp_power_on() during which dp_init_count
> is non-zero but dp_powered_on is still false. A Type-C orientation change
> arriving in this window would proceed with the mux switch while the DP PHY
> is mid-initialization, corrupting the PHY state.

[Severity: Medium]
Does the commit message confuse Type-C orientation changes with mode
changes?

Type-C orientation changes are handled by qmp_combo_typec_switch_set(),
which does not check qmp->dp_powered_on or qmp->dp_init_count before
resetting the common block.  qmp_combo_typec_mux_set() only handles
Type-C mode and SVID transitions.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 61d8b7a5ce8f9..3f93043f94e5c 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -5163,7 +5163,8 @@ static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_s
>  		return 0;
>  	}
>  
> -	if (qmp->qmpphy_mode != QMPPHY_MODE_USB3_ONLY && qmp->dp_powered_on) {
> +	if (qmp->qmpphy_mode != QMPPHY_MODE_USB3_ONLY &&
> +	    (qmp->dp_powered_on || qmp->dp_init_count)) {
>  		dev_dbg(qmp->dev, "typec_mux_set: DP PHY is still in use, delaying switch\n");
>  		return 0;
>  	}

[Severity: High]
Does bailing out when qmp->dp_init_count is non-zero cause regression by
dropping valid Type-C mode switches?

qmp_combo_typec_mux_set() discards new_mode on this path and nothing
replays it when the DP PHY later exits.  Meanwhile,
qmp_combo_reconfigure_phy() already handles reconfiguring the PHY while
qmp->dp_init_count is non-zero and qmp->dp_powered_on is false by powering
down DP AUX, resetting COM with new_mode, and re-running
qmp->cfg->dp_aux_init(qmp):

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_combo_reconfigure_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);
}

During 4-lane DisplayPort Altmode entry, dp_altmode_configured() signals
drm_connector_oob_hotplug_event() before calling dp_altmode_notify() ->
typec_mux_set().  The hotplug event synchronously runs
msm_dp_hpd_plug_handle() -> phy_init(), setting qmp->dp_init_count = 1
while qmp->qmpphy_mode is QMPPHY_MODE_USB3DP.  When typec_mux_set() then
requests QMPPHY_MODE_DP_ONLY, this guard drops the switch and leaves 4-lane
DP broken.

Similarly, on cable unplug in pmic_glink_altmode_worker(),
pmic_glink_altmode_safe() calls typec_mux_set() before
drm_aux_hpd_bridge_notify() triggers phy_exit(), so the transition back to
QMPPHY_MODE_USB3_ONLY is also dropped.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111104434.2222182-1-naidu.nama@oss.qualcomm.com?part=1

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