Re: [PATCH 09/12] phy: phy-mtk-dp: Rewrite and document default driving param macros

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The refactoring introduces incorrect integer values for two DisplayPort PHY driving parameters, silently altering the underlying hardware configuration.
- [High] Integer underflow and undefined behavior in mtk_dp_phy_disable_all_lanes when no lanes are enabled.
- [High] Flawed PM runtime initialization, race condition, and unhandled errors in probe.
- [Medium] Incorrect endianness conversion by pointer aliasing a u16 as a u8*.
--

commit 3ae397642076d659c70475bdbefbc3286e0e7c1f
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Subject: phy: phy-mtk-dp: Rewrite and document default driving param macros

This patch uses FIELD_PREP_CONST and adds definitions and macros to build
the default driving parameters for the PHY. It also adds comments explaining
what they are supposed to set in the PHY.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index d7024a1443358..1d73a622ef322 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c
[ ... ]
> +/* MT8195: Logic State Change Point (LC TX C) */
> +#define MT8195_DRIVING_PARAM_3_DEFAULT	BUILD_DRIVING_PARAM_0( 16, 20, 24, 32)
> +#define MT8195_DRIVING_PARAM_4_DEFAULT	BUILD_DRIVING_PARAM_12(24, 30, 60, 32)

[Severity: High]
Does this unintentionally change the default for SW1_PRE2?

The conversion to macros incorrectly alters the sw1_pre2 value. The
original macro XTP_LN_TX_LCTXC0_SW1_PRE2_DEFAULT was (BIT(18) | BIT(21)),
which evaluates to 36 (0x24) when shifted. This macro now sets it to
60 (0x3c).

> +#define MT8195_DRIVING_PARAM_5_DEFAULT	BUILD_DRIVING_PARAM_23(8, 48)

[Severity: High]
Is there a similar issue with the default for SW2_PRE1?

The original value was 40 (0x28), but this is now passing 8 (0x08), which
drops BIT(5). These altered driving parameters can degrade signal integrity
and cause connection instability.

[ ... ]
> +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);
> +
> +		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));
> +		if (ret)
> +			return ret;
> +	} while (val);

[Severity: High]
Can this do-while loop execute with val initially at 0?

If the PHY is already fully disabled when this is called, val will be 0.
The loop blindly executes once, fls(0) returns 0, and lane_num underflows
to 0xffffffff.

Using ~BIT(0xffffffff) invokes undefined behavior, and passing 0xffffffff
to FIELD_PREP() extracts unpredictable bits, causing a spurious out-of-bounds
register write when clearing the PHY control registers.

> +
> +	return 0;
> +}
[ ... ]
> +static int mtk_dp_phy_get_one_cal_para(struct device *dev, const char *name, u8 max_val)
> +{
> +	u16 buf = 0;
> +	int ret;
> +
> +	/*
> +	 * All of the calibrations are always max 8 bits long, but some may
> +	 * be split between two different 8-bits cells: handle this corner
> +	 * case by retrying reading as u16.
> +	 */
> +	ret = nvmem_cell_read_u8(dev, name, (u8 *)&buf);

[Severity: Medium]
Will this lead to an endianness violation on Big Endian platforms?

Casting a u16 buffer pointer to (u8 *) will cause the single byte to be
written into the most significant byte of buf instead of the least
significant byte on Big Endian architectures.

For instance, reading 0x0F would result in buf holding 0x0F00 (3840), which
would falsely trigger the subsequent if (buf > max_val) validation check.

> +	if (ret)
> +		ret = nvmem_cell_read_u16(dev, name, &buf);
[ ... ]
> +static int mtk_dp_phy_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	phy_set_drvdata(phy, dp_phy);
> +
> +	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +	if (IS_ERR(provider))
> +		return PTR_ERR(provider);

[Severity: High]
Could registering the provider before enabling PM runtime cause a race condition?

Exposing the PHY to consumers here means a consumer could claim the PHY and
attempt to power it on before pm_runtime_enable() is called below. This
would return -EACCES and break the consumer initialization.

> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);

[Severity: High]
Should the return value of pm_runtime_get_sync(dev) be checked?

Ignoring the return value means the probe silently succeeds even if the
hardware cannot be powered.

Additionally, this driver lacks a .remove callback. Since
pm_runtime_get_sync() increases the usage counter without it ever being
released, and pm_runtime_disable() is never called, unloading the module
permanently leaks the PM reference and breaks PM runtime for this device.

> +
> +	return 0;
> +}

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