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