[PATCH 0/2] drm/vblank: timer: Fix race condition in enable_vblank

[PATCH 0/2] drm/vblank: timer: Fix race condition in enable_vblank

[Thomas Zimmermann]

(was: drm/vblank: Restart enabling vblank timer on possible deadlock)

Busy-waiting in drm_crtc_vblank_start_timer() until a previous vblank
timer has completed can deadlock. Rework how the vblank timer's start
and cancel helpers interact with the timer function.

Patch 1 fixes the possible deadlock by removing the busy-wait loop from
drm_crtc_vblank_start_timer(). To avoid possible interference between
_start_timer()'s hrtimer_start() and hrtimer_forward_now() in the timer
function, extend the interval_lock around both. Both, _start_timer() and
the timer function, can still overlap, but hrtimer initialization should
work reliably. At most, the first vblank event after changing the interval
might be off.

Fix the possible overlap by canceling the vblank timer's internal hrtimer.
Do this outside of the vblank code's critical section after switching
the vblank timer off. With the hrtimer being canceled, the next vblank
event will be sent at the correct time.

Tested with bochs on qemu.

Additional notes: there have been reports about vblank timers not happening
in time, such as [1]. We suspected that might happen due to guest systems
running under high CPU load. But the problem could also be caused by the
deadlock fixed in patch 1.

[1] https://lore.kernel.org/dri-devel/6a176ae6.d07c18fe.1069e4.0016.GAE@google.com/

Thomas Zimmermann (2):
  drm/vblank: timer: Do not block within critical section
  drm/vblank: Cancel vblank timer outside of critical section

 drivers/gpu/drm/drm_vblank.c         | 87 ++++++++++++++++++----------
 drivers/gpu/drm/drm_vblank_helper.c  | 16 +++++
 drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c  |  3 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c |  2 +
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c |  2 +
 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c |  2 +-
 include/drm/drm_crtc.h               |  9 +++
 include/drm/drm_vblank.h             | 16 +++++
 include/drm/drm_vblank_helper.h      |  2 +
 9 files changed, 105 insertions(+), 34 deletions(-)


base-commit: a3c3ff69be4e55d4f8f3d93789381ebb23684b17
-- 
2.56.0

[PATCH 1/2] drm/vblank: timer: Do not block within critical section

[Thomas Zimmermann]

Remove try-to-cancel loop from drm_crtc_vblank_start_timer() to avoid
a possible livelock while spinning within the loop. Hold interval_lock
when starting and forwarding the vblank timer's hrtimer to avoid
forwarding an already queued timer.

The vblank helper drm_vblank_disable_and_save() and the vblank IRQ
handler hold the spin lock vblank_time_lock at some point. This can
result in a deadlock if the vblank timer's disable code in
drm_crtc_vblank_cancel_timer() calls hrtimer_cancel() directly: the
helper waits for the timer function to complete and the timer function
waits for the helper to release vblank_time_lock.

drm_crtc_vblank_cancel_timer() works around this problem by calling
hrtimer_try_to_cancel() instead. For re-enabling the vblank timer,
drm_crtc_vblank_start_timer() currently waits for the vblank timer
to stop. It runs into the same locking problem while waiting for the
timer function to complete.

Resolve the locking problem by removing hrtimer_try_to_cancel() from
drm_crtc_vblank_start_timer(). Hold interval_lock while starting or
forwarding the hrtimer. Without holding interval_lock,
drm_crtc_vblank_start_timer() could set an interval and start the
hrtimer; while a concurrent timer function forwards the timer with the
old interval. This can corrupt the ordering of the hrtimer's internal
queue. Holding the lock prevents this from happening.

When starting the vblank timer, it is still possible that an already
running timer sends out a vblank event at the previous vblank interval.
This is merely an inconsistency and not critical.

Closes: https://lore.kernel.org/dri-devel/20260914111129.300691F00898@smtp.kernel.org/
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 74afeb812850 ("drm/vblank: Add vblank timer")
Cc: Louis Chauvet <louis.chauvet@bootlin.com>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: David Airlie <airlied@gmail.com>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v6.19+
---
 drivers/gpu/drm/drm_vblank.c | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index f90fb2d13e42..8cc8d018d353 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -2179,19 +2179,18 @@ static enum hrtimer_restart drm_vblank_timer_function(struct hrtimer *timer)
 	struct drm_crtc *crtc = vtimer->crtc;
 	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
 	struct drm_device *dev = crtc->dev;
-	unsigned long flags;
-	ktime_t interval;
-	u64 ret_overrun;
+	u64 ret_overrun = 1;
 	bool succ;
 
-	spin_lock_irqsave(&vtimer->interval_lock, flags);
-	interval = vtimer->interval;
-	spin_unlock_irqrestore(&vtimer->interval_lock, flags);
+	scoped_guard(spinlock_irqsave, &vtimer->interval_lock) {
+		ktime_t interval = vtimer->interval;
 
-	if (!interval)
-		return HRTIMER_NORESTART;
+		if (!interval || hrtimer_is_queued(timer))
+			return HRTIMER_NORESTART;
+
+		ret_overrun = hrtimer_forward_now(timer, interval);
+	}
 
-	ret_overrun = hrtimer_forward_now(&vtimer->timer, interval);
 	if (ret_overrun != 1)
 		drm_dbg_vbl(dev, "vblank timer overrun\n");
 
@@ -2220,7 +2219,6 @@ int drm_crtc_vblank_start_timer(struct drm_crtc *crtc)
 {
 	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
 	struct drm_vblank_crtc_timer *vtimer = &vblank->vblank_timer;
-	unsigned long flags;
 
 	if (!vtimer->crtc) {
 		/*
@@ -2230,22 +2228,14 @@ int drm_crtc_vblank_start_timer(struct drm_crtc *crtc)
 		spin_lock_init(&vtimer->interval_lock);
 		hrtimer_setup(&vtimer->timer, drm_vblank_timer_function,
 			      CLOCK_MONOTONIC, HRTIMER_MODE_REL);
-	} else {
-		/*
-		 * Timer should not be active. If it is, wait for the
-		 * previous cancel operations to finish.
-		 */
-		while (hrtimer_active(&vtimer->timer))
-			hrtimer_try_to_cancel(&vtimer->timer);
 	}
 
 	drm_calc_timestamping_constants(crtc, &crtc->mode);
 
-	spin_lock_irqsave(&vtimer->interval_lock, flags);
-	vtimer->interval = ns_to_ktime(vblank->framedur_ns);
-	spin_unlock_irqrestore(&vtimer->interval_lock, flags);
-
-	hrtimer_start(&vtimer->timer, vtimer->interval, HRTIMER_MODE_REL);
+	scoped_guard(spinlock_irqsave, &vtimer->interval_lock) {
+		vtimer->interval = ns_to_ktime(vblank->framedur_ns);
+		hrtimer_start(&vtimer->timer, vtimer->interval, HRTIMER_MODE_REL);
+	}
 
 	return 0;
 }
-- 
2.56.0

[PATCH 2/2] drm/vblank: Cancel vblank timer outside of critical section

[Thomas Zimmermann]

Cancel the vblank timer's hrtimer after disabling the vblank timer to
avoid running timer function with outdated interval.

Stopping and restarting the vblank timer with a different interval can
leave behind an already-running timer function that uses the previous
interval. This can result in a vblank event with inconsistent timing. At
the same time, canceling the hrtimer in drm_crtc_vblank_cancel_timer() with
hrtimer_cancel() is not possible due to a deadlock between vblank_time_lock
and the hrtimer.

Resolve this problem by calling hrtimer_cancel() outside of
vblank_time_lock's critical section. Implement this with the new callback
sync_vblank in struct drm_crtc_funcs. Invoke this callback after switching
off the vblank. Some internal bookkeeping is necessary to ensure that the
timer is really switched off.

Implement sync_vblank for vblank timers with the new helper
drm_crtc_vblank_helper_sync_vblank_timer(). From there, the vblank function
drm_crtc_blank_stop_timer() stops the hrtimer unconditionally. Install
drm_crtc_vblank_helper_sync_vblank_timer() in most drivers via
DRM_CRTC_VBLANK_TIMER_FUNCS. Update vmwgfx, which does not use this macro.

Implement the locking in drm_crtc_vblank_cancel_timer() with a guard. Also
remove the call to hrtimer_try_to_cancel() from that function. It is no
longer needed with sync_vblank invoking hrtimer_cancel().

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_vblank.c         | 53 ++++++++++++++++++++++------
 drivers/gpu/drm/drm_vblank_helper.c  | 16 +++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c  |  3 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c |  2 ++
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c |  2 ++
 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c |  2 +-
 include/drm/drm_crtc.h               |  9 +++++
 include/drm/drm_vblank.h             | 16 +++++++++
 include/drm/drm_vblank_helper.h      |  2 ++
 9 files changed, 93 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 8cc8d018d353..ca3a6f17564b 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1403,6 +1403,9 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
 
 	spin_unlock_irq(&dev->event_lock);
 
+	if (crtc->funcs->sync_vblank)
+		crtc->funcs->sync_vblank(crtc);
+
 	/* Will be reset by the modeset helpers when re-enabling the crtc by
 	 * calling drm_calc_timestamping_constants(). */
 	vblank->hwmode.crtc_clock = 0;
@@ -2246,28 +2249,58 @@ EXPORT_SYMBOL(drm_crtc_vblank_start_timer);
  * @crtc: the CRTC
  *
  * Drivers should call this function from their CRTC's disable_vblank
- * function to stop a vblank timer.
+ * function to stop a vblank timer. Only signals the timer to stop at
+ * the next invocation. Non-blocking. To fully stop the vblank timer,
+ * drivers should invoke drm_crtc_vblank_stop_timer().
  */
 void drm_crtc_vblank_cancel_timer(struct drm_crtc *crtc)
 {
 	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
 	struct drm_vblank_crtc_timer *vtimer = &vblank->vblank_timer;
-	unsigned long flags;
 
 	/*
-	 * Calling hrtimer_cancel() can result in a deadlock with DRM's
-	 * vblank_time_lime_lock and hrtimers' softirq_expiry_lock. So
+	 * Calling hrtimer_cancel() here can result in a deadlock between
+	 * DRM's vblank_time_lock and hrtimers' softirq_expiry_lock. So
 	 * clear interval and indicate cancellation. The timer function
-	 * will cancel itself on the next invocation.
+	 * will cancel itself on the next invocation. Otherwise cancel it
+	 * in drm_crtc_vblank_stop_canceled_timer().
 	 */
+	scoped_guard(spinlock_irqsave, &vtimer->interval_lock)
+		vtimer->interval = 0;
+}
+EXPORT_SYMBOL(drm_crtc_vblank_cancel_timer);
+
+/**
+ * drm_crtc_vblank_stop_canceled_timer - Stops a CRTC's previously canceled vblank timer
+ * @crtc: the CRTC
+ *
+ * Drivers should call this function from their CRTC's sync_vblank
+ * function to fully stop a vblank timer. The timer must have been
+ * canceled before. Use drm_crtc_vblank_stop_timer() to stop a vblank
+ * timer unconditionally.
+ *
+ * The function blocks until a running timer function has returned.
+ */
+void drm_crtc_vblank_stop_canceled_timer(struct drm_crtc *crtc)
+{
+	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
+	struct drm_vblank_crtc_timer *vtimer = &vblank->vblank_timer;
+	ktime_t interval;
 
-	spin_lock_irqsave(&vtimer->interval_lock, flags);
-	vtimer->interval = 0;
-	spin_unlock_irqrestore(&vtimer->interval_lock, flags);
+	if (!vtimer->crtc)
+		return;
 
-	hrtimer_try_to_cancel(&vtimer->timer);
+	/*
+	 * The vblank timer is off if it has an interval of zero. Cancel
+	 * it before drm_crtc_vblank_start_timer() reprograms it.
+	 */
+	scoped_guard(spinlock_irqsave, &vtimer->interval_lock)
+		interval = vtimer->interval;
+
+	if (!interval)
+		hrtimer_cancel(&vtimer->timer);
 }
-EXPORT_SYMBOL(drm_crtc_vblank_cancel_timer);
+EXPORT_SYMBOL(drm_crtc_vblank_stop_canceled_timer);
 
 /**
  * drm_crtc_vblank_get_vblank_timeout - Returns the vblank timeout
diff --git a/drivers/gpu/drm/drm_vblank_helper.c b/drivers/gpu/drm/drm_vblank_helper.c
index d3f8147ecdc1..7465d74ce29e 100644
--- a/drivers/gpu/drm/drm_vblank_helper.c
+++ b/drivers/gpu/drm/drm_vblank_helper.c
@@ -146,6 +146,22 @@ void drm_crtc_vblank_helper_disable_vblank_timer(struct drm_crtc *crtc)
 }
 EXPORT_SYMBOL(drm_crtc_vblank_helper_disable_vblank_timer);
 
+/**
+ * drm_crtc_vblank_helper_sync_vblank_timer - Implements struct &drm_crtc_funcs.sync_vblank
+ * @crtc: The CRTC
+ *
+ * The helper drm_crtc_vblank_helper_sync_vblank_timer() implements
+ * sync_vblank of struct drm_crtc_funcs for CRTCs that require a
+ * VBLANK timer.
+ *
+ * See also struct &drm_crtc_funcs.sync_vblank.
+ */
+void drm_crtc_vblank_helper_sync_vblank_timer(struct drm_crtc *crtc)
+{
+	drm_crtc_vblank_stop_canceled_timer(crtc);
+}
+EXPORT_SYMBOL(drm_crtc_vblank_helper_sync_vblank_timer);
+
 /**
  * drm_crtc_vblank_helper_get_vblank_timestamp_from_timer -
  *	Implements struct &drm_crtc_funcs.get_vblank_timestamp
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
index 0dea09cc29a7..e8c7100a3dc8 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
@@ -33,7 +33,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_fourcc.h>
-
+#include <drm/drm_vblank_helper.h>
 
 #define vmw_crtc_to_ldu(x) \
 	container_of(x, struct vmw_legacy_display_unit, base.crtc)
@@ -256,6 +256,7 @@ static const struct drm_crtc_funcs vmw_legacy_crtc_funcs = {
 	.page_flip = drm_atomic_helper_page_flip,
 	.enable_vblank          = vmw_vkms_enable_vblank,
 	.disable_vblank         = vmw_vkms_disable_vblank,
+	.sync_vblank            = drm_crtc_vblank_helper_sync_vblank_timer,
 	.get_vblank_timestamp   = vmw_vkms_get_vblank_timestamp,
 };
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
index 39d58fc392bf..a74cafb1769e 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
@@ -35,6 +35,7 @@
 #include <drm/drm_damage_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_vblank.h>
+#include <drm/drm_vblank_helper.h>
 
 #define vmw_crtc_to_sou(x) \
 	container_of(x, struct vmw_screen_object_unit, base.crtc)
@@ -312,6 +313,7 @@ static const struct drm_crtc_funcs vmw_screen_object_crtc_funcs = {
 	.page_flip = drm_atomic_helper_page_flip,
 	.enable_vblank          = vmw_vkms_enable_vblank,
 	.disable_vblank         = vmw_vkms_disable_vblank,
+	.sync_vblank            = drm_crtc_vblank_helper_sync_vblank_timer,
 	.get_vblank_timestamp   = vmw_vkms_get_vblank_timestamp,
 };
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index b505f8ba3e83..d3f7a5c2d131 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -37,6 +37,7 @@
 #include <drm/drm_damage_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_vblank.h>
+#include <drm/drm_vblank_helper.h>
 
 #define vmw_crtc_to_stdu(x) \
 	container_of(x, struct vmw_screen_target_display_unit, base.crtc)
@@ -785,6 +786,7 @@ static const struct drm_crtc_funcs vmw_stdu_crtc_funcs = {
 	.page_flip = drm_atomic_helper_page_flip,
 	.enable_vblank		= vmw_vkms_enable_vblank,
 	.disable_vblank		= vmw_vkms_disable_vblank,
+	.sync_vblank            = drm_crtc_vblank_helper_sync_vblank_timer,
 	.get_vblank_timestamp	= vmw_vkms_get_vblank_timestamp,
 	.get_crc_sources	= vmw_vkms_get_crc_sources,
 	.set_crc_source		= vmw_vkms_set_crc_source,
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
index 3d0d5dfa869f..8ce10cf9c19e 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
@@ -299,7 +299,7 @@ vmw_vkms_crtc_cleanup(struct drm_crtc *crtc)
 	struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
 
 	if (vmw->vkms_enabled)
-		drm_crtc_vblank_cancel_timer(crtc);
+		drm_crtc_vblank_stop_timer(crtc);
 
 	if (du->vkms.surface)
 		vmw_surface_unreference(&du->vkms.surface);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index be5dca538d87..46546006f361 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -908,6 +908,15 @@ struct drm_crtc_funcs {
 	 */
 	void (*disable_vblank)(struct drm_crtc *crtc);
 
+	/**
+	 * @sync_vblank:
+	 *
+	 * Synchronizes vblank hardware and software state after switching off
+	 * vblanks for the given CRTC. Called without holding vblank locks. Allows
+	 * the driver to clean up vblank state outside of critical sections.
+	 */
+	void (*sync_vblank)(struct drm_crtc *crtc);
+
 	/**
 	 * @get_vblank_timestamp:
 	 *
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 2fcef9c0f5b1..257b83d145ce 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -319,6 +319,22 @@ void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
 
 int drm_crtc_vblank_start_timer(struct drm_crtc *crtc);
 void drm_crtc_vblank_cancel_timer(struct drm_crtc *crtc);
+void drm_crtc_vblank_stop_canceled_timer(struct drm_crtc *crtc);
+
+/**
+ * drm_crtc_vblank_stop_timer - Stops a CRTC's vblank timer
+ * @crtc: the CRTC
+ *
+ * Drivers should call this function to fully stop a CRTC's vblank timer.
+ *
+ * The function blocks until a running timer function has returned.
+ */
+static inline void drm_crtc_vblank_stop_timer(struct drm_crtc *crtc)
+{
+	drm_crtc_vblank_cancel_timer(crtc);
+	drm_crtc_vblank_stop_canceled_timer(crtc);
+}
+
 void drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_time);
 
 /*
diff --git a/include/drm/drm_vblank_helper.h b/include/drm/drm_vblank_helper.h
index 28051f08d0f4..dd0029e045be 100644
--- a/include/drm/drm_vblank_helper.h
+++ b/include/drm/drm_vblank_helper.h
@@ -37,6 +37,7 @@ void drm_crtc_vblank_atomic_disable(struct drm_crtc *crtc,
 
 int drm_crtc_vblank_helper_enable_vblank_timer(struct drm_crtc *crtc);
 void drm_crtc_vblank_helper_disable_vblank_timer(struct drm_crtc *crtc);
+void drm_crtc_vblank_helper_sync_vblank_timer(struct drm_crtc *crtc);
 bool drm_crtc_vblank_helper_get_vblank_timestamp_from_timer(struct drm_crtc *crtc,
 							    int *max_error,
 							    ktime_t *vblank_time,
@@ -51,6 +52,7 @@ bool drm_crtc_vblank_helper_get_vblank_timestamp_from_timer(struct drm_crtc *crt
 #define DRM_CRTC_VBLANK_TIMER_FUNCS \
 	.enable_vblank = drm_crtc_vblank_helper_enable_vblank_timer, \
 	.disable_vblank = drm_crtc_vblank_helper_disable_vblank_timer, \
+	.sync_vblank = drm_crtc_vblank_helper_sync_vblank_timer, \
 	.get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp_from_timer
 
 #endif
-- 
2.56.0

Re: [PATCH 2/2] drm/vblank: Cancel vblank timer outside of critical section

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/vblank: uninitialized spinlock in drm_crtc_vblank_cancel_timer()
--

commit 0925c513110481f6c38259e0df588ad4194770ca
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/vblank: Cancel vblank timer outside of critical section

This commit introduces a sync_vblank callback in struct drm_crtc_funcs so
that vblank timers can be synchronously canceled via hrtimer_cancel() outside
of vblank_time_lock. It also adds drm_crtc_vblank_stop_canceled_timer() and
drm_crtc_vblank_stop_timer(), and updates vmwgfx and the vblank timer helpers
to use them.

> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 8cc8d018d353a..ca3a6f17564b5 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
[ ... ]
> @@ -2246,28 +2249,58 @@ EXPORT_SYMBOL(drm_crtc_vblank_start_timer);
>   * @crtc: the CRTC
>   *
>   * Drivers should call this function from their CRTC's disable_vblank
> - * function to stop a vblank timer.
> + * function to stop a vblank timer. Only signals the timer to stop at
> + * the next invocation. Non-blocking. To fully stop the vblank timer,
> + * drivers should invoke drm_crtc_vblank_stop_timer().
>   */
>  void drm_crtc_vblank_cancel_timer(struct drm_crtc *crtc)
>  {
>  	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
>  	struct drm_vblank_crtc_timer *vtimer = &vblank->vblank_timer;
> -	unsigned long flags;
>  
>  	/*
> -	 * Calling hrtimer_cancel() can result in a deadlock with DRM's
> -	 * vblank_time_lime_lock and hrtimers' softirq_expiry_lock. So
> +	 * Calling hrtimer_cancel() here can result in a deadlock between
> +	 * DRM's vblank_time_lock and hrtimers' softirq_expiry_lock. So
>  	 * clear interval and indicate cancellation. The timer function
> -	 * will cancel itself on the next invocation.
> +	 * will cancel itself on the next invocation. Otherwise cancel it
> +	 * in drm_crtc_vblank_stop_canceled_timer().
>  	 */
> +	scoped_guard(spinlock_irqsave, &vtimer->interval_lock)
> +		vtimer->interval = 0;
> +}
> +EXPORT_SYMBOL(drm_crtc_vblank_cancel_timer);
> +
> +/**
> + * drm_crtc_vblank_stop_canceled_timer - Stops a CRTC's previously canceled vblank timer
> + * @crtc: the CRTC
> + *
> + * Drivers should call this function from their CRTC's sync_vblank
> + * function to fully stop a vblank timer. The timer must have been
> + * canceled before. Use drm_crtc_vblank_stop_timer() to stop a vblank
> + * timer unconditionally.
> + *
> + * The function blocks until a running timer function has returned.
> + */
> +void drm_crtc_vblank_stop_canceled_timer(struct drm_crtc *crtc)
> +{
> +	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
> +	struct drm_vblank_crtc_timer *vtimer = &vblank->vblank_timer;
> +	ktime_t interval;
>  
> -	spin_lock_irqsave(&vtimer->interval_lock, flags);
> -	vtimer->interval = 0;
> -	spin_unlock_irqrestore(&vtimer->interval_lock, flags);
> +	if (!vtimer->crtc)
> +		return;

[Severity: High]
Should drm_crtc_vblank_cancel_timer() also check for !vtimer->crtc before
acquiring vtimer->interval_lock?

In drm_vblank_init(), dev->vblank is allocated with drmm_kcalloc(), leaving
vtimer zero-initialized. Both vtimer->crtc and vtimer->interval_lock are
only initialized lazily on the first call to drm_crtc_vblank_start_timer():

drivers/gpu/drm/drm_vblank.c:drm_crtc_vblank_start_timer() {
	if (!vtimer->crtc) {
		/*
		 * Set up the data structures on the first invocation.
		 */
		vtimer->crtc = crtc;
		spin_lock_init(&vtimer->interval_lock);
		hrtimer_setup(&vtimer->timer, drm_vblank_timer_function,
			      CLOCK_MONOTONIC, HRTIMER_MODE_REL);
	}

While drm_crtc_vblank_stop_canceled_timer() guards against an uninitialized
timer with if (!vtimer->crtc) return, drm_crtc_vblank_stop_timer() calls
drm_crtc_vblank_cancel_timer() first:

vmw_vkms_crtc_cleanup()
  -> drm_crtc_vblank_stop_timer()
       -> drm_crtc_vblank_cancel_timer()
            -> scoped_guard(spinlock_irqsave, &vtimer->interval_lock)
       -> drm_crtc_vblank_stop_canceled_timer()

When vmw_vkms_crtc_cleanup() runs during CRTC teardown on a display unit
where vblank was never enabled, wouldn't drm_crtc_vblank_cancel_timer() lock
vtimer->interval_lock before spin_lock_init() has ever been called on it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105145946.896674-1-tzimmermann@suse.de?part=2