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 <[email protected]> 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/[email protected]?part=3
