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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 7/9/26 12:46, sashiko-bot@kernel.org wrote:
> 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);

It's really too hot in July in Italy. Uff. :-)

> 
> [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?
> 

Nice. Yes.

>>   	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);
>>   }
>