Re: [PATCH v6 11/11] phy: qualcomm: qmp-combo: Fix error propagation in qmp_combo_dp_power_on()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] phy: qcom-qmp-combo: missing dp_aux_cfg2 in nord_usb3dpphy_cfg fails power on
- [High] phy: qcom-qmp-combo: wrong init helper in qmp_v8_configure_dp_phy()
- [High] phy: qcom-qmp-combo: phy_power_on() error leaves DP PHY on and underflows power_count
--

commit 44d1f5b7ba39d3fcf81a7681f3459cbf81e1f3df
Author: Nabige Aala <nabige.aala@oss.qualcomm.com>

phy: qualcomm: qmp-combo: Fix error propagation in qmp_combo_dp_power_on()

This commit updates qmp_combo_dp_power_on() to use guard(mutex) and
propagate errors returned by the configure_dp_phy() callback instead of
ignoring them and unconditionally setting dp_powered_on.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index b34b892c27026..4def2bd991a88 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -4416,8 +4416,9 @@ static int qmp_combo_dp_power_on(struct phy *phy)
>  	const struct qmp_phy_cfg *cfg = qmp->cfg;
>  	void __iomem *tx = qmp->dp_tx;
>  	void __iomem *tx2 = qmp->dp_tx2;
> +	int ret;
>  
> -	mutex_lock(&qmp->phy_mutex);
> +	guard(mutex)(&qmp->phy_mutex);
>  
>  	qmp_combo_dp_serdes_init(qmp);
>  
> @@ -4428,12 +4429,14 @@ static int qmp_combo_dp_power_on(struct phy *phy)
>  	cfg->configure_dp_tx(qmp);
>  
>  	/* Configure link rate, swing, etc. */
> -	cfg->configure_dp_phy(qmp);
> +	ret = cfg->configure_dp_phy(qmp);
> +	if (ret) {
> +		dev_err(qmp->dev, "DP PHY configuration failed: %d\n", ret);
> +		return ret;
> +	}

[Severity: High]
Does propagating the error from cfg->configure_dp_phy() cause DisplayPort PHY
power-on to unconditionally fail with -EINVAL on Nord platforms?

Earlier in this series, commit "phy: qualcomm: qmp-combo: Add
hardware-specific dp_aux_cfg2 and dp_phy_cfg1 fields" populated dp_aux_cfg2
and dp_phy_cfg1 across the V4, V5, and V6 configurations, but omitted them
from nord_usb3dpphy_cfg:

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:
static const struct qmp_phy_cfg nord_usb3dpphy_cfg = {
	...
	.dp_aux_init		= qmp_v4_dp_aux_init,
	.configure_dp_tx	= qmp_v4_configure_dp_tx,
	.configure_dp_clocks	= qmp_v3_configure_dp_clocks,
	.configure_dp_phy	= qmp_v4_configure_dp_phy,

	.regs			= qmp_v6_usb3phy_regs_layout,
	...
};

When phy_power_on() invokes qmp_combo_dp_power_on() ->
qmp_v4_configure_dp_phy() -> qmp_combo_configure_dp_phy_common(), it checks
cfg->dp_aux_cfg2 and returns -EINVAL when it is zero:

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_combo_configure_dp_phy_common() {
	...
	if (!cfg->dp_aux_cfg2) {
		dev_err(qmp->dev, "DP AUX CFG2 value not configured\n");
		return -EINVAL;
	}

	writel(cfg->dp_phy_cfg1, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG_1);
	...
}

Should nord_usb3dpphy_cfg also initialize .dp_aux_cfg2 and .dp_phy_cfg1 to
QSERDES_DP_PHY_AUX_CFG2_V456 and QSERDES_DP_PHY_CFG1_V456?

[Severity: High]
Will checking the return value of cfg->configure_dp_phy() here also cause
DisplayPort PHY power-on to fail with -ETIMEDOUT on Glymur (V8) platforms,
while skipping link clock enablement and status polling on V4, V5, and V6
platforms?

In commit "phy: qualcomm: qmp-combo: Extract common DP PHY init sequence",
both qmp_v4_configure_dp_phy() and qmp_v8_configure_dp_phy() started with
identical calls to qmp_v456_configure_dp_phy(qmp). It looks like the hunk
replacing qmp_v456_configure_dp_phy(qmp) with
qmp_combo_configure_dp_phy_common(qmp) was applied to
qmp_v4_configure_dp_phy() instead of qmp_v8_configure_dp_phy():

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_v4_configure_dp_phy() {
	...
	ret = qmp_combo_configure_dp_phy_common(qmp);
	if (ret < 0)
		return ret;

	/*
	 * At least for 7nm DP PHY this has to be done after enabling link
	 * clock.
	 */
	...
}

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_v8_configure_dp_phy() {
	...
	ret = qmp_v456_configure_dp_phy(qmp);
	if (ret < 0)
		return ret;
	...
}

As a result, on V8 platforms, qmp_combo_dp_power_on() ->
qmp_v8_configure_dp_phy() -> qmp_v456_configure_dp_phy() writes 0x19 to
QSERDES_DP_PHY_CFG before V8 TX/TSYNC setup and polls QPHY_DP_PHY_STATUS for
BIT(1), which times out with -ETIMEDOUT on V8 hardware and now aborts
qmp_combo_dp_power_on(). Meanwhile, qmp_v4_configure_dp_phy() bypasses
qmp_v456_configure_dp_phy() and never enables the link clock or polls
QPHY_DP_PHY_STATUS before configuring the TX lanes.

[Severity: High]
When cfg->configure_dp_phy() fails and qmp_combo_dp_power_on() returns an
error, can this leave the DP PHY hardware partially powered on and cause
phy->power_count and runtime PM refcount underflows in the caller?

Before cfg->configure_dp_phy() fails (for example with -ETIMEDOUT on PLL
lock), qmp_combo_dp_serdes_init() (whose return value is also unchecked),
qmp_configure_lane(), cfg->configure_dp_tx(), and the initial writes in
cfg->configure_dp_phy() have already deasserted power-down bits in
QSERDES_DP_PHY_PD_CTL, enabled TX drivers and biases, and started the reset
state machine. Returning directly on error does not write
DP_PHY_PD_CTL_PSR_PWRDN to QSERDES_DP_PHY_PD_CTL as qmp_combo_dp_power_off()
does.

Furthermore, when qmp_combo_dp_power_on() returns a negative error to
phy_power_on(), phy->power_count remains 0 and phy_power_on() drops its
runtime PM reference:

drivers/phy/phy-core.c:phy_power_on() {
	...
	if (phy->power_count == 0 && phy->ops->power_on) {
		ret = phy->ops->power_on(phy);
		if (ret < 0) {
			dev_err(&phy->dev, "phy poweron failed --> %d\n", ret);
			goto err_pwr_on;
		}
	}
	++phy->power_count;
	...
}

However, the Qualcomm DP controller in msm_dp_ctrl_enable_mainlink_clocks()
does not check the return value of phy_power_on(phy):

drivers/gpu/drm/msm/dp/dp_ctrl.c:msm_dp_ctrl_enable_mainlink_clocks() {
	...
	phy_configure(phy, &ctrl->phy_opts);
	phy_power_on(phy);
	...
}

When link training subsequently retries at a lower link rate in
msm_dp_ctrl_reinitialize_mainlink() or tears down in
msm_dp_ctrl_deinitialize_mainlink() and msm_dp_ctrl_off_link(), it
unconditionally calls phy_power_off(phy). Because phy->power_count is 0
instead of 1, phy_power_off() skips phy->ops->power_off(phy), decrements
phy->power_count to -1, drops an extra runtime PM reference, and causes the
subsequent phy_power_on(phy) retry to skip calling qmp_combo_dp_power_on()
altogether.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-glymur-phy-v3-v6-0-1f1d2e565a66@oss.qualcomm.com?part=11

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