Re: [PATCH v2 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:
- [High] Wrong device pointer passed to devm_add_action_or_reset() for the audio codec platform device causes a leak and double-free.
- [High] Manual platform_device_unregister() calls in the devm_add_action_or_reset() error paths lead to double unregistrations.
--

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

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

This commit attempts to fix platform device leaks during probe deferrals
by adding devm_add_action_or_reset() calls to automatically unregister
the HDMI codec and DisplayPort PHY platform devices.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index eefbc7e0f9c8d..2c738c16dafab 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c

[ ... ]

> @@ -2680,18 +2687,29 @@ static int mtk_dp_register_audio_driver(struct device *dev)
>  		.data = mtk_dp,
>  		.no_capture_mute = 1,
>  	};
> +	int ret;
>  
>  	mtk_dp->audio_pdev = platform_device_register_data(dev,
>  							   HDMI_CODEC_DRV_NAME,
>  							   PLATFORM_DEVID_AUTO,
>  							   &codec_data,
>  							   sizeof(codec_data));
> -	return PTR_ERR_OR_ZERO(mtk_dp->audio_pdev);
> +	if (IS_ERR(mtk_dp->audio_pdev))
> +		return PTR_ERR(mtk_dp->audio_pdev);
> +
> +	ret = devm_add_action_or_reset(dev, mtk_dp_unregister_pdevs, mtk_dp->phy_dev);
                                                                     ^^^^^^^^^^^^^^^

[Severity: High]
Should this be passing mtk_dp->audio_pdev instead of mtk_dp->phy_dev?

Passing mtk_dp->phy_dev here registers a second unregister action for the phy
device, which could lead to a double-free on driver unbind, while leaving
the audio device leaked.

> +	if (ret) {
> +		platform_device_unregister(mtk_dp->audio_pdev);

[Severity: High]
Will this cause a double unregistration if devm_add_action_or_reset() fails?

Since devm_add_action_or_reset() automatically executes the action callback
(mtk_dp_unregister_pdevs) upon failure, wouldn't calling
platform_device_unregister() here unregister it a second time?

> +		return dev_err_probe(dev, ret,
> +				     "Failed to add codec unregister devm action");
> +	}
> +	return 0;
>  }
>  
>  static int mtk_dp_register_phy(struct mtk_dp *mtk_dp)
>  {
>  	struct device *dev = mtk_dp->dev;
> +	int ret;
>  
>  	mtk_dp->phy_dev = platform_device_register_data(dev, "mediatek-dp-phy",
>  							PLATFORM_DEVID_AUTO,
> @@ -2701,6 +2719,13 @@ 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) {
> +		platform_device_unregister(mtk_dp->phy_dev);

[Severity: High]
Similarly, does this result in a double unregistration if the devm action
fails to allocate?

devm_add_action_or_reset() will call mtk_dp_unregister_pdevs(mtk_dp->phy_dev)
internally if it returns an error, making this manual unregister redundant and
potentially unsafe.

> +		return dev_err_probe(dev, ret,
> +				     "Failed to add phy unregister devm action");
> +	}
> +
>  	mtk_dp_get_calibration_data(mtk_dp);

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