Re: [PATCH v4 06/12] phy: phy-mtk-dp: Support set_lanes in configure and properly cleanup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mtk_dp_phy_disable_all_lanes() ignores the return value of regmap_read() and unconditionally returns 0, leading to uninitialized memory use and dead code.
--

commit 96237b9548980d0963b925da5fbae9fec554beb6
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Subject: phy: phy-mtk-dp: Support set_lanes in configure and properly cleanup

This commit adds support for configuring a specific number of lanes
via the .configure() callback. It also introduces functionality to
disable all lanes during power-off and reset to ensure proper hardware
state cleanup.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index 4247506ceb996..276391d991aae 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c

[ ... ]

> @@ -254,12 +272,39 @@ static int mtk_dp_phy_power_on(struct phy *phy)
>  	return 0;
>  }
>  
> +static int mtk_dp_phy_disable_all_lanes(struct mtk_dp_phy *dp_phy)
> +{
> +	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
> +	const u8 *regs = pdata->regs_dig_glb;
> +	int ret;
> +	u32 val;
> +
> +	/* Get mask of currently enabled lane */
> +	regmap_read(dp_phy->regmap, pdata->off_dig_glb + regs[DP_PHYD_TX_CTL_0], &val);

[Severity: High]
Does this code handle the possibility of regmap_read() failing?

If regmap_read() returns an error (e.g., if the underlying bus or clock is in
an invalid state), it will not initialize val. In this scenario, the subsequent
operations would use uninitialized stack memory, potentially leading to random
lanes being disabled based on garbage data.

> +	val = FIELD_GET(PHYD_TX_LN_EN, val);
> +	if (val == 0)
> +		return 0;
> +
> +	/* Disable all lanes (needs to be done one by one, from last to first) */
> +	do {
> +		u32 lane_num = fls(val) - 1;
> +		val &= ~BIT(lane_num);
> +
> +		regmap_clear_bits(dp_phy->regmap,
> +				  pdata->off_dig_glb + regs[DP_PHYD_TX_CTL_0],
> +				  FIELD_PREP(PHYD_TX_LN_EN, BIT(lane_num)));
> +	} while (val);
> +
> +	return 0;

[Severity: High]
Should this function return the error state from the regmap operations?

It declares a ret variable but never uses it, and unconditionally returns 0.

> +}
> +
>  static int mtk_dp_phy_power_off(struct phy *phy)
>  {
>  	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
>  	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
>  	const u8 *regs_dig = pdata->regs_dig_glb;
>  	const u8 *regs_ana = pdata->regs_ana_glb;
> +	int ret;
>  
>  	regmap_set_bits(dp_phy->regmap,
>  			pdata->off_ana_glb + regs_ana[DP_PHYA_GLB_FORCE_CTRL_1],
> @@ -269,6 +314,12 @@ static int mtk_dp_phy_power_off(struct phy *phy)
>  	regmap_write(dp_phy->regmap,
>  		     pdata->off_dig_glb + regs_dig[DP_PHYD_AUX_RX_CTL], 0);
>  
> +	ret = mtk_dp_phy_disable_all_lanes(dp_phy);
> +	if (ret) {
> +		dev_err(dp_phy->dev, "Could not disable lanes for poweroff!\n");
> +		return ret;
> +	}

[Severity: High]
Since mtk_dp_phy_disable_all_lanes() unconditionally returns 0, this error
handling block appears to be dead code, preventing the driver from properly
detecting hardware state failures during cleanup.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910092038.48291-1-angelogioacchino.delregno@collabora.com?part=6