Re: [PATCH v5 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 1 potential issue(s) to consider:
- [Medium] phy: missing DP PHY power down on qmp_combo_dp_power_on() error path
--

commit 12c88a7f83dcdbd1fafc3b878f7302691ae24c8d
Author: Nabige Aala <nabige.aala@oss.qualcomm.com>

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

The configure_dp_phy() callback can return an error (e.g. -ETIMEDOUT on
PLL lock failure), but qmp_combo_dp_power_on() was ignoring the return
value and always returning 0. Capture the return value, skip setting
dp_powered_on on failure, and propagate the error to the caller.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 5bbb2b5f8c74d..13d5586b981c8 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c

[ ... ]

> @@ -4388,13 +4389,17 @@ 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);
> +		goto out_unlock;
> +	}

[Severity: Medium]
If cfg->configure_dp_phy(qmp) fails (e.g., due to a PLL lock timeout), does
this error path leave the DP PHY permanently powered up?

The Linux PHY core framework does not automatically invoke phy_power_off() if
a power_on callback fails, as seen in drivers/phy/phy-core.c:phy_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;
	}

Because configure_dp_phy() internally clears DP_PHY_PD_CTL_PSR_PWRDN to take
the PHY out of power down, returning an error here without re-asserting this
bit leaves the DP PHY powered up.

qmp_combo_dp_power_off() performs this cleanup during normal operation:

	writel(DP_PHY_PD_CTL_PSR_PWRDN, qmp->dp_dp_phy + QSERDES_DP_PHY_PD_CTL);

Should the error path manually assert DP_PHY_PD_CTL_PSR_PWRDN or perform other
necessary cleanup before returning?

>  
>  	qmp->dp_powered_on = true;
>  
> +out_unlock:
>  	mutex_unlock(&qmp->phy_mutex);
> -
> -	return 0;
> +	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-glymur-phy-v3-v5-0-3848c2615d5a@oss.qualcomm.com?part=11

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