Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm_kms_helper_poll_fini() fails to synchronize with in-flight driver HPD threads and introduces a TOCTOU race with userspace probing, leaving the original late-modeset bug unfixed. - [Medium] Calling drm_kms_helper_poll_fini() inside drm_atomic_helper_shutdown() introduces a regression causing a kernel warning stack trace (drm_WARN_ON) during driver unload. --
--- Patch [1]: [PATCH v2] drm/atomic-helper: tear down HPD/polling in drm_atomic_helper_shutdown() --- commit e1bd39c97d78edddf053e357bf0f99d72ae55494 Author: Mahadevan P <[email protected]> drm/atomic-helper: tear down HPD/polling in drm_atomic_helper_shutdown() This commit adds a call to drm_kms_helper_poll_fini() within drm_atomic_helper_shutdown() to tear down output polling and HPD interrupts. The goal is to prevent late hotplug events from triggering modesets after the display hardware and IOMMU mappings have been disabled during shutdown. Link: https://lore.kernel.org/r/[email protected] > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > index 285aac3554dfd..80c67623180a2 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c [ ... ] > @@ -3676,6 +3677,8 @@ EXPORT_SYMBOL(drm_atomic_helper_reset_crtc); > * > * This is just a convenience wrapper around drm_atomic_helper_disable_all(), > * and it is the atomic version of drm_helper_force_disable_all(). > + * > + * This also tears down output polling and HPD via > drm_kms_helper_poll_fini(). > */ > void drm_atomic_helper_shutdown(struct drm_device *dev) > { > @@ -3685,6 +3688,8 @@ void drm_atomic_helper_shutdown(struct drm_device *dev) > if (dev == NULL) > return; > > + drm_kms_helper_poll_fini(dev); [Severity: High] Does calling drm_kms_helper_poll_fini() here fully synchronize with in-flight driver HPD threads? While this masks hardware interrupts, it doesn't appear to flush pending IRQ threads since there is no synchronize_irq() equivalent here. If an HPD interrupt fires just before shutdown and schedules a driver's threaded IRQ handler (like msm_dp_display_irq_thread), it seems the thread could wake up, block on mode_config.mutex, and then trigger an unsolicited modeset after drm_atomic_helper_shutdown() finishes and releases the locks. Also, can this introduce a race with userspace probing? drm_kms_helper_poll_fini() modifies poll_enabled locklessly. A concurrent GETCONNECTOR ioctl holding mode_config.mutex might read poll_enabled as true and call drm_kms_helper_poll_enable(), which could turn HPD back on during the shutdown sequence. [Severity: Medium] Could this cause a kernel warning during driver unload? drm_kms_helper_poll_fini() sets dev->mode_config.poll_enabled to false. However, many drivers explicitly call drm_kms_helper_poll_disable() during their own teardown sequences after calling drm_atomic_helper_shutdown() (for example, in nouveau_display_fini() in the nouveau driver). Since drm_kms_helper_poll_disable() has a guard check: if (drm_WARN_ON(dev, !dev->mode_config.poll_enabled)) return; It looks like drivers performing standard unload routines will hit this warning stack trace because the core already silently finalized the polling state. > + > DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); > > ret = drm_atomic_helper_disable_all(dev, &ctx); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
