Re: [PATCH v4 04/12] phy: phy-mtk-dp: Migrate register offsets to SoC specific pdata

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing NULL check for `device_get_match_data()` leads to a NULL pointer dereference on manual sysfs bind.
- [High] Overlapping devicetree reg property and hardcoded offset prevents successful DT probe.

Pre-existing issues:
- [High] Potential NULL pointer dereference when accessing `dev->platform_data` prior to validation.
--

commit 6b41d18c7c7f31b8549b66b4262af72e2486491d
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: phy-mtk-dp: Migrate register offsets to SoC specific pdata
    
In preparation for adding support for newer SoCs and for adding
more capabilities to this driver in an efficient manner, migrate
all of the hardcoded register offsets to SoC specific pdata and
assign that for both DT and platform probing.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index d241004e28286..20974650d9409 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c
[ ... ]
> @@ -170,12 +241,19 @@ static void mtk_dp_phy_legacy_remove_lookup(void *data)
>  	phy_remove_lookup(phy, "dp", dev_name(dp_phy->dev));
>  }
>  
> +static const struct mtk_dp_phy_pdata mt8195_dp_phy_data;
> +
>  static int mtk_dp_phy_legacy_probe(struct platform_device *pdev, struct mtk_dp_phy *dp_phy)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct phy *phy;
>  	int ret;
>  
> +	/*
> +	 * If legacy platform driver probe, assume this is MT8195 or compatible
> +	 * with a devicetree that was not migrated to the new, proper bindings.
> +	 */
> +	dp_phy->pdata = &mt8195_dp_phy_data;
>  	dp_phy->regmap = *(struct regmap **)dev->platform_data;
>  	if (!dp_phy->regmap)
>  		return dev_err_probe(dev, -EINVAL, "No platform data available\n");

[Severity: High]
This isn't a bug introduced by this patch, but can this dereference
dev->platform_data before checking if it is NULL?

If the driver is manually bound via sysfs or a module without platform data
provided, it looks like it would crash immediately here:

drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_legacy_probe() {
    ...
	dp_phy->regmap = *(struct regmap **)dev->platform_data;
	if (!dp_phy->regmap)
		return dev_err_probe(dev, -EINVAL, "No platform data available\n");
    ...
}

[ ... ]
> @@ -235,6 +313,8 @@ static int mtk_dp_phy_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	dp_phy->pdata = device_get_match_data(dev);
> +
>  	phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops);
>  	if (IS_ERR(phy))
>  		return dev_err_probe(dev, PTR_ERR(phy),

[Severity: High]
Could this result in a NULL pointer dereference?

If the driver is manually bound (e.g., via driver_override in sysfs) to a
device that possesses an of_node but does not match any entry in
mtk_dp_phy_of_match, device_get_match_data() will return NULL.

This NULL pointer would later be dereferenced when accessing dp_phy->pdata
in mtk_dp_phy_init().

[ ... ]
> @@ -249,8 +329,15 @@ static int mtk_dp_phy_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct mtk_dp_phy_pdata mt8195_dp_phy_data = {
> +	.off_dig_glb = 0x1000,
> +	.off_dig_lane = (const u16[]) { 0x1100, 0x1200, 0x1300, 0x1400 },
> +	.regs_dig_glb = mt8195_phy_dig_glb_regs,
> +	.regs_dig_lane = mt8195_phy_dig_lane_regs,
> +};
> +

[Severity: High]
Will these hardcoded offsets prevent a successful devicetree probe?

The driver maps its devicetree reg resource using
devm_platform_ioremap_resource() in mtk_dp_phy_probe():

drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_probe() {
    ...
	base = devm_platform_ioremap_resource(pdev, 0);
	if (IS_ERR(base))
		return PTR_ERR(base);
    ...
}

If the DT reg maps the specific PHY region, the driver will still add
0x1000 to the mapped base, accessing unmapped physical memory. 

If the DT reg maps the entire DP controller region to make the offsets work,
devm_platform_ioremap_resource() will fail with -EBUSY because the parent DP
driver already holds the request_mem_region lock.

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