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

[sashiko-bot@kernel.org]

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