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