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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The return value of `pm_runtime_get_sync()` is ignored, which can lead to hardware register accesses on an unpowered device.
- [Medium] Missing `.remove` callback leaves PM runtime enabled and usage counter unbalanced on driver unbind.

Pre-existing issues:
- [High] Missing `phy_remove_lookup()` causes a memory leak and a dangling pointer in the global PHY lookup list.
- [High] Unconditional dereference of `dev->platform_data` before validating it.
--

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

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

This commit introduces a devicetree match table to decouple the DP PHY driver
from the DP IP driver. It adds an OF PHY provider and MMIO regmap registration
for the devicetree case, while maintaining a legacy probe path for older
devicetrees without a separate PHY node.

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

[ ... ]

> @@ -160,43 +161,92 @@ static const struct phy_ops mtk_dp_phy_dev_ops = {
>  	.owner = THIS_MODULE,
>  };
>  
> +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;
> +
> +	dp_phy->regmap = *(struct regmap **)dev->platform_data;

[Severity: High]
This is a pre-existing issue, but does this code unconditionally dereference
dev->platform_data before verifying it is not NULL?

If the platform device is instantiated without platform data, would this
dereference cause a kernel panic before the subsequent check for
!dp_phy->regmap can execute?

> +	if (!dp_phy->regmap)
> +		return dev_err_probe(dev, -EINVAL, "No platform data available\n");
> +
> +	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");
> +
> +	phy_set_drvdata(phy, dp_phy);
> +	phy_create_lookup(phy, "dp", dev_name(dev));

[Severity: High]
This isn't a bug introduced by this patch, but does this leak the lookup
entry and create a dangling pointer?

phy_create_lookup() dynamically allocates a phy_lookup structure and links it
into the global phys list. Since there is no driver .remove callback calling
phy_remove_lookup(), when the driver unbinds, the devres-managed phy is freed
while still in the global list, which could cause a use-after-free when other
drivers call phy_find().

> +
> +	return 0;
> +}
> +

[ ... ]

>  static int mtk_dp_phy_probe(struct platform_device *pdev)
>  {

[ ... ]

> +	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +	if (IS_ERR(provider))
> +		return PTR_ERR(provider);
> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);

[Severity: High]
Is it safe to ignore the return value of pm_runtime_get_sync() here?

If the generic power domain or clock fails to enable, pm_runtime_get_sync()
will return a negative error code. Without checking this, the PHY provider is
registered even if the hardware is off, which could lead to a synchronous
external abort when attempting to write to the MMIO registers later.

>  
>  	return 0;
>  }
>  
> +static const struct of_device_id mtk_dp_phy_of_match[] = {
> +	{ .compatible = "mediatek,mt8195-dp-phy" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mtk_dp_phy_of_match);
> +
>  static struct platform_driver mtk_dp_phy_driver = {
>  	.probe = mtk_dp_phy_probe,
>  	.driver = {
>  		.name = "mediatek-dp-phy",
> +		.of_match_table = mtk_dp_phy_of_match,
>  	},
>  };

[Severity: Medium]
Does this driver need a .remove callback to clean up the PM runtime state?

Since pm_runtime_enable() and pm_runtime_get_sync() are called in probe
without devm_ handlers or registered teardown actions, unbinding the driver
appears to leak the usage counter and leave PM runtime permanently enabled.

>  module_platform_driver(mtk_dp_phy_driver);
>

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