Re: [PATCH v4 03/12] phy: phy-mtk-dp: Allow probing with devicetree match

[sashiko-bot@kernel.org]

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

New issues:
- [High] Hardcoded register offsets cause incorrect MMIO access or resource conflicts in the new OF probe path.

Pre-existing issues:
- [High] NULL pointer dereference when accessing platform_data before validation.
--

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

phy: phy-mtk-dp: Allow probing with devicetree match

This commit decouples the DisplayPort PHY driver from the DisplayPort IP
driver by allowing it to probe via a devicetree match. When probed via
devicetree, it registers an OF PHY provider and maps its own MMIO regmap,
while retaining the legacy behavior in mtk_dp_phy_legacy_probe().

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index bf7b3a95e72d2..d241004e28286 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c

[ ... ]

> @@ -160,43 +162,109 @@ static const struct phy_ops mtk_dp_phy_dev_ops = {
>  	.owner = THIS_MODULE,
>  };
>  
> +static void mtk_dp_phy_legacy_remove_lookup(void *data)
> +{
> +	struct phy *phy = data;
> +	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
> +
> +	phy_remove_lookup(phy, "dp", dev_name(dp_phy->dev));
> +}
> +
> +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;
> +
> +	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 is a pre-existing issue, but is it possible for dev->platform_data to be
NULL here? If the platform device is instantiated without platform_data
(for example, via a sysfs manual bind or driver core testing), dereferencing
it unconditionally before the validation check will cause a crash.

The code currently does:

dp_phy->regmap = *(struct regmap **)dev->platform_data;

The subsequent check if (!dp_phy->regmap) cannot prevent the crash if
dev->platform_data itself is NULL. Could we validate dev->platform_data
before attempting to dereference it?

> +
> +	phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops);
> +	if (IS_ERR(phy))
> +		return dev_err_probe(dev, PTR_ERR(phy),
> +				     "Failed to create DP PHY\n");

[ ... ]

> +static int mtk_dp_phy_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	struct phy_provider *provider;
>  	struct mtk_dp_phy *dp_phy;
> +	void __iomem *base;
>  	struct phy *phy;
> -	struct regmap *regs;
> -
> -	regs = *(struct regmap **)dev->platform_data;
> -	if (!regs)
> -		return dev_err_probe(dev, -EINVAL,
> -				     "No data passed, requires struct regmap**\n");
> +	int ret;
>  
>  	dp_phy = devm_kzalloc(dev, sizeof(*dp_phy), GFP_KERNEL);
>  	if (!dp_phy)
>  		return -ENOMEM;
>  
> -	dp_phy->regmap = regs;
> +	dp_phy->dev = dev;
> +
> +	/* If there's no devicetree, go for legacy pdev probe */
> +	if (!dev->of_node)
> +		return mtk_dp_phy_legacy_probe(pdev, dp_phy);
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	dp_phy->regmap = devm_regmap_init_mmio(dev, base, &mtk_dp_phy_regmap_cfg);

[Severity: High]
When mapping the dedicated MMIO region for the PHY in the new devicetree
match path, will the existing hardcoded register offsets cause out-of-bounds
accesses?

Looking at the existing phy data structures in this driver, offsets such as
.off_dig_glb = 0x1000 are used. These offsets appear to be relative to the
DP IP base address from the legacy shared regmap.

If the devicetree 'reg' property points to the PHY's actual physical
base address, using these hardcoded offsets on top of the new dedicated
MMIO mapping will overshoot the PHY registers by an additional 0x1000.

Alternatively, if the devicetree tries to compensate by pointing to the
DP IP base, wouldn't this cause a resource conflict (-EBUSY) with the
DP driver's own devm_platform_ioremap_resource() call?

> +	if (IS_ERR(dp_phy->regmap))
> +		return PTR_ERR(dp_phy->regmap);
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +
>  	phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops);

[ ... ]

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