Re: [PATCH] phy: qcom: qmp-combo: Leave the USB PCS alone in DP-only mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] phy: qcom: qmp-combo: qmp_combo_typec_switch_set() powers USB in DP-only mode
--

commit ee86c0e81f4849ab16e1b11f61ee95b5a8b6c01a
Author: Joonhoe Kim <26rote@gmail.com>

phy: qcom: qmp-combo: Leave the USB PCS alone in DP-only mode

This commit skips powering on and off the USB PCS in qmp_combo_usb_init() and
qmp_combo_usb_exit() when the PHY is in QMPPHY_MODE_DP_ONLY while continuing
to track qmp->usb_init_count.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 187f0cd11f6c3..6e3406d519d28 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -4459,10 +4459,12 @@ static int qmp_combo_usb_init(struct phy *phy)
>  	if (ret)
>  		return ret;
>  
> -	ret = qmp_combo_usb_power_on(phy);
> -	if (ret) {
> -		qmp_combo_com_exit(qmp, false);
> -		return ret;
> +	if (qmp->qmpphy_mode != QMPPHY_MODE_DP_ONLY) {
> +		ret = qmp_combo_usb_power_on(phy);
> +		if (ret) {
> +			qmp_combo_com_exit(qmp, false);
> +			return ret;
> +		}
>  	}
>  
>  	qmp->usb_init_count++;
> @@ -4484,9 +4486,11 @@ static int qmp_combo_usb_exit(struct phy *phy)
>  		return 0;
>  	}
>  
> -	ret = qmp_combo_usb_power_off(phy);
> -	if (ret)
> -		return ret;
> +	if (qmp->qmpphy_mode != QMPPHY_MODE_DP_ONLY) {
> +		ret = qmp_combo_usb_power_off(phy);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	ret = qmp_combo_com_exit(qmp, false);
>  	if (ret)

[Severity: High]
With qmp_combo_usb_init() and qmp_combo_usb_exit() keeping qmp->usb_init_count
non-zero in QMPPHY_MODE_DP_ONLY while leaving the USB PCS powered off, do
qmp_combo_typec_switch_set() and qmp_combo_reconfigure_phy() in
drivers/phy/qualcomm/phy-qcom-qmp-combo.c also need to check for
QMPPHY_MODE_DP_ONLY before touching the USB PCS?

In drivers/phy/qualcomm/phy-qcom-qmp-combo.c, qmp_combo_typec_switch_set()
checks qmp->usb_init_count without checking qmp->qmpphy_mode:

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_combo_typec_switch_set() {
	...
	if (qmp->init_count) {
		if (qmp->usb_init_count || qmp->usb4_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 || qmp->usb4_init_count)
			qmp_combo_usb_power_on(qmp->usb_phy);
		if (qmp->dp_init_count)
			cfg->dp_aux_init(qmp);
	}
	...
}

When a DP-only monitor (pin assignment C or E) is unplugged,
qmp_combo_typec_switch_set() ignores TYPEC_ORIENTATION_NONE, and
qmp_combo_typec_mux_set() delays the mode change if the Type-C unplug mux
notification arrives before DRM powers off the DP PHY:

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_combo_typec_mux_set() {
	...
	if (qmp->qmpphy_mode != QMPPHY_MODE_USB3_ONLY && qmp->dp_powered_on) {
		dev_dbg(qmp->dev, "typec_mux_set: DP PHY is still in use, delaying switch\n");
		return 0;
	}
	...
}

This leaves qmp->qmpphy_mode set to QMPPHY_MODE_DP_ONLY while disconnected. On
a subsequent connection in the opposite orientation, Type-C drivers (such as
pmic_glink_altmode_worker(), ucsi_handle_connector_change(), tcpm_mux_set(),
and cros_typec_configure_mux()) call typec_switch_set() before
typec_mux_set().

When qmp_combo_typec_switch_set() runs with qmp->qmpphy_mode still set to
QMPPHY_MODE_DP_ONLY and qmp->usb_init_count > 0, it calls
qmp_combo_usb_power_off() on the unpowered USB PCS, re-runs
qmp_combo_com_init() (which configures DP_MODE and leaves the USB3 PHY reset
asserted), and then calls qmp_combo_usb_power_on(). Would this overwrite the
serdes, lane, and PCS tables, time out waiting for PHYSTATUS, and call
clk_disable_unprepare(qmp->pipe_clk) on the error path while qmp->init_count
remains non-zero?

Similarly, when leaving QMPPHY_MODE_DP_ONLY with qmp->usb_init_count > 0,
qmp_combo_reconfigure_phy() unconditionally powers off the USB PCS even though
it was kept powered down in DP-only mode:

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_combo_reconfigure_phy() {
	...
	if (qmp->usb_init_count || qmp->usb4_init_count)
		qmp_combo_usb_power_off(qmp->usb_phy);
	...
}

Should both qmp_combo_typec_switch_set() and the power-off path in
qmp_combo_reconfigure_phy() also skip the USB PCS when qmp->qmpphy_mode is
QMPPHY_MODE_DP_ONLY?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022165219.1368-1-26rote@gmail.com?part=1

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