Re: [PATCH v4] drm/vblank: Don't arm vblank timer with invalid frame duration

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] AB-BA deadlock between synchronous hrtimer cancellation in drm_crtc_vblank_start_timer() and drm_handle_vblank() taking vblank_time_lock.
--

commit de00db596b26306bffcd319a867015e9f4b1faa6
Author: Roman Ilin <me@romanilin.is>

drm/vblank: Don't arm vblank timer with invalid frame duration

This commit modifies drm_calc_timestamping_constants() to check for INT_MAX
overflows and returns an error instead of proceeding with invalid frame
durations. It also updates drm_crtc_vblank_start_timer() to propagate this
error, allowing the driver to fall back to immediate vblank events.

> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index f90fb2d13e423..72b543bca9e31 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c

[ ... ]

> @@ -2221,6 +2239,7 @@ 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;
> +	int ret;
>  
>  	if (!vtimer->crtc) {
>  		/*
> @@ -2239,7 +2258,9 @@ int drm_crtc_vblank_start_timer(struct drm_crtc *crtc)
>  			hrtimer_try_to_cancel(&vtimer->timer);

[Severity: High]
This is a pre-existing issue, but can this busy-wait loop cause a hard
deadlock?

When CPU 1 enables vblanks, it holds the vblank_time_lock while executing this
synchronous cancellation loop:

CPU 1:
drm_vblank_enable() {
    spin_lock(&dev->vblank_time_lock);
    __enable_vblank(dev, pipe);
    ...
    drm_crtc_vblank_start_timer(crtc);
    while (hrtimer_active(&vtimer->timer))
        hrtimer_try_to_cancel(&vtimer->timer);

If the timer has just fired concurrently, CPU 2 executes the callback which
attempts to acquire the same lock:

CPU 2:
drm_vblank_timer_function() {
    ...
    drm_handle_vblank(dev, pipe);
    spin_lock(&dev->vblank_time_lock);

This creates an AB-BA deadlock where CPU 1 spins forever waiting for the
callback to finish on CPU 2, while CPU 2 spins forever waiting for CPU 1
to release vblank_time_lock. Is it safe to synchronously cancel the timer
here while holding a lock that the timer's callback also takes?

>  	}
>  
> -	drm_calc_timestamping_constants(crtc, &crtc->mode);
> +	ret = drm_calc_timestamping_constants(crtc, &crtc->mode);
> +	if (ret)
> +		return ret;
>  
>  	spin_lock_irqsave(&vtimer->interval_lock, flags);
>  	vtimer->interval = ns_to_ktime(vblank->framedur_ns);

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