Re: [PATCH v3 03/11] drm/mediatek: mtk_dp: Fix hdmi codec and phy driver unregistration

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Double free / use-after-free of mtk_dp->phy_dev on devm_phy_get error path.

Pre-existing issues:
- [High] Asynchronous pm_runtime_put() used immediately before pm_runtime_disable() leaves hardware in incorrect power state.
--

--- Patch [3]: [PATCH v3 03/11] drm/mediatek: mtk_dp: Fix hdmi codec and phy driver unregistration ---
commit ee2a644d342bfbaa916286b22d4e54ddc62e5878
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

drm/mediatek: mtk_dp: Fix hdmi codec and phy driver unregistration

This commit adds devm actions to unregister the HDMI Codec and DisplayPort
PHY platform devices during probe. This prevents multiple instances of these
devices from being registered if the driver probe is deferred.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index eefbc7e0f9c8d..fefd1fae44c90 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
[ ... ]
> @@ -2701,6 +2718,11 @@ static int mtk_dp_register_phy(struct mtk_dp *mtk_dp)
>  		return dev_err_probe(dev, PTR_ERR(mtk_dp->phy_dev),
>  				     "Failed to create device mediatek-dp-phy\n");
>  
> +	ret = devm_add_action_or_reset(dev, mtk_dp_unregister_pdevs, mtk_dp->phy_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to add phy unregister devm action");
> +
>  	mtk_dp_get_calibration_data(mtk_dp);
>  
>  	mtk_dp->phy = devm_phy_get(&mtk_dp->phy_dev->dev, "dp");
>  	if (IS_ERR(mtk_dp->phy)) {
>  		platform_device_unregister(mtk_dp->phy_dev);

[Severity: Critical]
Does this create a double-free on the error path?

Looking at mtk_dp_register_phy() in drivers/gpu/drm/mediatek/mtk_dp.c, if
devm_phy_get() fails (returning e.g. -EPROBE_DEFER), we manually call
platform_device_unregister() here. 

However, since we successfully added the devm action earlier in the function,
returning the error from probe will cause the devres unwind mechanism to
execute mtk_dp_unregister_pdevs().

Won't that call platform_device_unregister() a second time on the already-freed
mtk_dp->phy_dev pointer, resulting in memory corruption?

[ ... ]
> @@ -2870,9 +2892,6 @@ static void mtk_dp_remove(struct platform_device *pdev)
>  	struct mtk_dp *mtk_dp = platform_get_drvdata(pdev);
>  
>  	pm_runtime_put(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to call
pm_runtime_put() immediately before pm_runtime_disable() here in
mtk_dp_remove()?

Since pm_runtime_put() queues an asynchronous suspend, calling
pm_runtime_disable() right after it cancels the pending suspend work.

Doesn't this leave the hardware in an active power state after the driver is
removed? Should this be changed to pm_runtime_put_sync() to ensure the suspend
completes before module unload?

>  	if (mtk_dp->data->bridge_type != DRM_MODE_CONNECTOR_eDP)
>  		timer_delete_sync(&mtk_dp->debounce_timer);
> -	platform_device_unregister(mtk_dp->phy_dev);
> -	if (mtk_dp->audio_pdev)
> -		platform_device_unregister(mtk_dp->audio_pdev);
>  }

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