Re: [PATCH] drm/msm/dp: add .shutdown to mask HPD IRQ before system shutdown

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Redundant synchronize_irq() call.

Pre-existing issues:
- [High] Incomplete fix: Race window in system shutdown still allows IOMMU faults.
- [High] Use-after-free of dp->aux in IRQ handler during driver unbind.
--

--- Patch [1]: [PATCH] drm/msm/dp: add .shutdown to mask HPD IRQ before system shutdown ---
commit d07c91cbfabdf117c1daa68222fb3889fa6527d8
Author: Mahadevan P <mahadevan.p@oss.qualcomm.com>

drm/msm/dp: add .shutdown to mask HPD IRQ before system shutdown

This commit adds a .shutdown callback to mask and flush the HPD IRQ before
system shutdown. This is intended to prevent late hotplug events from
triggering a new modeset that drives hardware through a stale IOMMU mapping
during reboot.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index dc6f33809ca5f..b4fbdeebf952c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1207,6 +1207,14 @@ static void msm_dp_display_remove(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, NULL);
>  }

[Severity: High]
This is a pre-existing issue, but can this lead to a use-after-free during
driver unbind?

Looking at msm_dp_display_remove(), msm_dp_display_deinit_sub_modules() is
called right before platform_set_drvdata():

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_display_remove() {
    ...
    msm_dp_display_deinit_sub_modules(dp);
    platform_set_drvdata(pdev, NULL);
}

This manually frees dp->aux. However, since the IRQ is managed by
devm_request_threaded_irq, it remains registered and enabled until the
devres release phase occurs after the remove callback returns.

If a hotplug interrupt fires during this window, wouldn't the handler
dereference the freed pointer?

msm_dp_display_irq_handler()
    ...
    hpd_isr_status = msm_dp_aux_get_hpd_intr_status(dp->aux);

>  
> +static void msm_dp_display_shutdown(struct platform_device *pdev)
> +{
> +	struct msm_dp_display_private *dp = dev_get_dp_display_private(&pdev->dev);
> +
> +	disable_irq(dp->irq);

[Severity: High]
This is a pre-existing issue, but does this fully resolve the race condition
during system shutdown?

If a hotplug interrupt fires after the DRM master .shutdown callback has
disabled CRTCs but before msm_dp_display_shutdown(), disable_irq() will wait
for the scheduled msm_dp_display_irq_thread to finish. 

If that thread executes drm_client_dev_hotplug() and performs an atomic
commit to turn the display back on, disable_irq() waits for it to complete.

Doesn't this guarantee the display is active when .shutdown returns,
allowing the IOMMU shutdown to trigger the same fatal fault?

> +	synchronize_irq(dp->irq);

[Severity: Low]
Is this call to synchronize_irq() redundant?

Since disable_irq() is not called from an atomic context here, it
inherently calls synchronize_irq() internally to wait for any currently
executing handlers to complete.

> +}
> +
>  static int msm_dp_pm_runtime_suspend(struct device *dev)

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-dpshutdown-v1-1-b062c2f7dfb1@oss.qualcomm.com?part=1