Re: [PATCH 07/12] phy: phy-mtk-dp: Support setting volt swing and preemphasis values

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The newly added PHYD_DP_TX_FORCE_VOLT_SWING_EN bit is never set or cleared, causing the hardware to likely ignore the configured voltage swing and pre-emphasis values.
- [High] Undefined behavior in `mtk_dp_phy_disable_all_lanes` when no lanes are currently enabled.

Pre-existing issues:
- [High] Missing `.remove` callback leads to Use-After-Free and unbalanced PM runtime states on module unload.
--

commit 3443073c225a00d56dcccbe80409c8537773ab64
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: phy-mtk-dp: Support setting volt swing and preemphasis values

Add support for honoring the set_voltages request to set the voltage swing
and preemphasis values in the .configure() callback. It also updates the
.reset() callback to clean up the newly configured values.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index a2cd22b9da066..17d871530ccaf 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c
> @@ -235,6 +242,28 @@ static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
>  				   PHYD_TX_LN_EN, val);
>  	}
>  
> +	if (opts->dp.set_voltages) {
> +		const u32 reg_drv_force = pdata->regs_dig_lane[DP_PHYD_LAN_DRIVING_FORCE];
> +
> +		if (opts->dp.lanes > 4) {
> +			dev_err(&phy->dev, "Wrong lanes config %u\n", opts->dp.lanes);
> +			return -EINVAL;
> +		}
> +
> +		for (i = 0; i < opts->dp.lanes; i++) {
> +			const u32 off_dig_lane = pdata->off_dig_lane[i];
> +			u32 val;
> +
> +			val = FIELD_PREP(PHYD_DP_TX_FORCE_VOLT_SWING_VAL, opts->dp.voltage[i]);
> +			val |= FIELD_PREP(PHYD_DP_TX_FORCE_PRE_EMPH_VAL, opts->dp.pre[i]);
> +
> +			regmap_update_bits(dp_phy->regmap, off_dig_lane + reg_drv_force,
> +					   PHYD_DP_TX_FORCE_VOLT_SWING_VAL |
> +					   PHYD_DP_TX_FORCE_PRE_EMPH_VAL,
> +					   val);

[Severity: High]
Does this override hardware configuration actually take effect?

Since PHYD_DP_TX_FORCE_VOLT_SWING_EN is not included in the update mask or
set in the value here, the hardware force enable bit remains unset. Will
the hardware ignore these override values without the enable bit being
asserted?

> +		}
> +	}
> +
>  	regmap_update_bits(dp_phy->regmap,
[ ... ]
> @@ -345,6 +375,15 @@ static int mtk_dp_phy_reset(struct phy *phy)
>  	if (ret)
>  		dev_err(dp_phy->dev, "Could not disable lanes during reset!\n");
>  

[Severity: High]
Is it possible for the loop in mtk_dp_phy_disable_all_lanes() to execute
with val initially set to 0?

drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_disable_all_lanes() {
    ...
    /* 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);
    ...
}

If no lanes are currently enabled, fls(0) - 1 evaluates to -1, which can
cause undefined behavior when shifting ~BIT(-1). Should this be a
while (val) loop instead of do { ... } while (val)?

> +	/* Reset Voltage Swing and Preemphasis values */
> +	for (i = 0; i < MTK_DP_PHY_MAX_LANES; i++) {
> +		const u32 off_dig_lane = pdata->off_dig_lane[i];
> +
> +		regmap_clear_bits(dp_phy->regmap, off_dig_lane + reg_drv_force,
> +				   PHYD_DP_TX_FORCE_VOLT_SWING_VAL |
> +				   PHYD_DP_TX_FORCE_PRE_EMPH_VAL);

[Severity: High]
Does this correctly clear the override state?

Similar to the configuration above, this clears the swing and pre-emphasis
values but leaves the PHYD_DP_TX_FORCE_VOLT_SWING_EN bit untouched. If
the force mode was enabled elsewhere, could it remain active after reset?

> +	}
> +
>  	return 0;
>  }


[Severity: High]
This is a pre-existing issue, but I noticed the platform driver lacks a
.remove callback:

static struct platform_driver mtk_dp_phy_driver = {
	.probe = mtk_dp_phy_probe,
	.driver = { ... },
};

During probe, pm_runtime_enable() and pm_runtime_get_sync() are called, and
a phy_lookup structure is dynamically allocated via phy_create_lookup().

Because there is no .remove callback, unloading the module leaves PM runtime
unbalanced and leaks the phy_lookup struct, which remains on the global phys
list while pointing to a devres-freed phy device. Could this lead to a
use-after-free on subsequent lookups?

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