Re: [PATCH 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 2 potential issue(s) to consider:
- [High] Unconditional do-while loop in `mtk_dp_phy_disable_all_lanes` causes an out-of-bounds bitwise shift and undefined behavior when no lanes are initially enabled.
- [High] `mtk_dp_phy_disable_all_lanes` passes a bit index instead of a bitmask to `FIELD_PREP`, failing to disable the correct lanes.
--

commit 25cdd51bd1ac4d69a49e77d93a6b1374ba46dd3c
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: phy-mtk-dp: Support set_lanes in configure and properly cleanup

Add support for enabling a specifically requested number of lanes
in the .configure() callback and disable all lanes in power off
and reset callbacks for proper hardware cleanup.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index 586e727956332..a2cd22b9da066 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c
[ ... ]
> @@ -261,6 +267,35 @@ 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;
> +
> +	ret = regmap_read(dp_phy->regmap, pdata->off_dig_glb + regs[DP_PHYD_TX_CTL_0], &val);
> +	if (ret)
> +		return ret;
> +
> +	/* Get mask of currently enabled lane */
> +	val = FIELD_GET(PHYD_TX_LN_EN, val);
> +
> +	/* 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);
                 ^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this cause undefined behavior if val is initially 0?

If the PHY register reports 0 enabled lanes (which can happen after a
hardware reset or during initial boot), val will be 0. Since this is an
unconditional do-while loop, fls(0) will return 0, leading to lane_num
evaluating to UINT_MAX.

The subsequent BIT(lane_num) attempt would result in an out-of-bounds
bitwise shift. Could this be changed to a while loop, or check if val is 0
before entering the loop?

> +
> +		ret = regmap_clear_bits(dp_phy->regmap,
> +					pdata->off_dig_glb + regs[DP_PHYD_TX_CTL_0],
> +					FIELD_PREP(PHYD_TX_LN_EN, lane_num));
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this clear the correct lanes in the register?

It looks like the bit index lane_num is passed directly to FIELD_PREP here
instead of the bitmask BIT(lane_num), which was correctly used in
mtk_dp_phy_configure().

If lane_num is 3, FIELD_PREP(GENMASK(7, 4), 3) will generate 0x30, which
clears lanes 0 and 1 instead of 0x80 (which would clear lane 3). This seems
like it might repeatedly attempt to clear the wrong lanes and leave higher
lanes enabled.

> +		if (ret)
> +			return ret;
> +	} while (val);
> +
> +	return 0;
> +}

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