Re: [PATCH v2 08/12] phy: phy-mtk-dp: Add support for digital and analog calibration

[sashiko-bot@kernel.org]

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

New issues:
- [High] The DP PHY is published and exposed to concurrent consumers before its calibration data is initialized in the legacy probe path.
- [Medium] Unconditional `dev_err` logs an error for missing optional NVMEM cells and probe deferrals in `mtk_dp_phy_get_one_cal_para`.

Pre-existing issues:
- [High] Dereferencing `dev->platform_data` without checking for NULL in `mtk_dp_phy_legacy_probe`.
--

--- Patch [8]: [PATCH v2 08/12] phy: phy-mtk-dp: Add support for digital and analog calibration ---
commit 0db0b57c522a5f7eb7d179aeee49708015901046
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: phy-mtk-dp: Add support for digital and analog calibration

This commit adds support for reading DP PHY calibration values from eFuse
via nvmem. If the values are present, they are applied; otherwise, the
driver relies on default calibration data. Analog calibration writing
support is also introduced.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c

[ ... ]

> +static int mtk_dp_phy_get_one_cal_para(struct device *dev, const char *name, u8 max_val)
> +{
> +	u8 buf_byte;
> +	u16 buf;
> +	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, &buf_byte);
> +	if (ret)
> +		ret = nvmem_cell_read_u16(dev, name, &buf);
> +	else
> +		buf = buf_byte;
> +
> +	if (ret) {
> +		dev_err(dev, "Cannot get calibration data for %s: %d\n", name, ret);
> +		return ret;
> +	};

[Severity: Medium]
Will this unconditionally log a hard error if the NVMEM cells are absent
or if the provider returns -EPROBE_DEFER?

Since nvmem_cell_read_u8() returns -ENOENT when optional cells are missing
in the device tree, could this cause spurious error messages in the kernel
log on normal boots? It might also spam the logs during standard probe
deferrals.

[ ... ]

> @@ -444,6 +671,8 @@ static int mtk_dp_phy_legacy_probe(struct platform_device *pdev, struct mtk_dp_p
>  	if (ret)
>  		return ret;
>  
> +	mtk_dp_phy_get_default_cal_data(dp_phy);
> +
>  	return 0;
>  }

[Severity: High]
Is it safe to publish the PHY to consumers before the calibration defaults
are initialized? 

Earlier in mtk_dp_phy_legacy_probe(), phy_create_lookup() exposes the PHY.
If a consumer driver probes concurrently and acquires the PHY right after
that lookup registration, could it call phy_init() while the analog
parameter registers are still zero-initialized by devm_kzalloc(), rather
than having these defaults?

[Severity: High]
This is a pre-existing issue, but can dev->platform_data be NULL earlier
in mtk_dp_phy_legacy_probe()?

Looking at the start of the function:

	dp_phy->regmap = *(struct regmap **)dev->platform_data;
	if (!dp_phy->regmap)
		return dev_err_probe(dev, -EINVAL, "No platform data available\n");

If the driver is bound to a device without platform data (e.g. manually
via sysfs bind), it looks like dereferencing dev->platform_data to read
the regmap will cause a kernel panic before it reaches the NULL check.

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

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