Re: [PATCH v8 01/27] drm/vblank_work: Add methods to schedule vblank_work in 2 stages

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm_vblank_work_schedule() fails to update the `armed` state if the target vblank count matches the currently scheduled count, causing state mismatches and resource leaks.
- [Low] Plain reads and writes to `work->armed` race with concurrent lockless accesses, causing potential compiler reordering issues.
--

commit 541484cf3b17ab53b7e81696c1a0711cae54129c
Author: Maarten Lankhorst <dev@lankhorst.se>

drm/vblank_work: Add methods to schedule vblank_work in 2 stages

This commit introduces a 2-stage mechanism for scheduling vblank work. It
allows PREEMPT_RT systems to prepare work in advance, and then arm it later
using a lockless function to maintain constant execution times in disabled
interrupt contexts.

> diff --git a/drivers/gpu/drm/drm_vblank_work.c b/drivers/gpu/drm/drm_vblank_work.c
> index 70f0199251ea0..6acb240b9c112 100644
> --- a/drivers/gpu/drm/drm_vblank_work.c
> +++ b/drivers/gpu/drm/drm_vblank_work.c
[ ... ]
> @@ -139,6 +118,7 @@ int drm_vblank_work_schedule(struct drm_vblank_work *work,
>  		rescheduling = true;
>  	}
>  

[Severity: Medium]
If a driver calls drm_vblank_work_schedule_disabled() followed by
drm_vblank_work_schedule() with the exact same count to enable it, does the
early exit for matching counts bypass updating the armed state?

For example, if __drm_vblank_work_schedule() in
drivers/gpu/drm/drm_vblank_work.c takes this path earlier in the function:

	} else if (work->count == count) {
		/* Already scheduled w/ same vbl count */
		goto out;

If the work item remains unarmed after returning, could this regression leak
the hardware vblank reference since the item will never execute or be removed
from the pending list?

[Severity: Low]
Since the armed state is read locklessly by drm_handle_vblank_works() and
modified locklessly by drm_vblank_work_enable(), should this plain write in
__drm_vblank_work_schedule() use WRITE_ONCE() to prevent C data races and
compiler reordering regressions?

> +	work->armed = armed;
>  	work->count = count;
>  	cur_vbl = drm_vblank_count(dev, vblank->pipe);
>  	passed = drm_vblank_passed(cur_vbl, count);
[ ... ]
> +/**
> + * drm_vblank_work_enable - enable vblank work
> + * @work: vblank work to enable
> + *
> + * This function is specifically only for when drm_vblank_work_schedule_disabled() is
> + * called. It allows for the work to be armed in any context, without any locks.
> + *
> + * The work will be signalled earliest at the @count argument, if it has been passed,
> + * it will signalled at the next vblank.
> + *
> + * This is particularly useful for PREEMPT_RT, where the spin_lock is converted
> + * into a sleeping rtmutex, and vblank evasion requires some work to be
> + * scheduled on completion with interrupts disabled.
> + */
> +void drm_vblank_work_enable(struct drm_vblank_work *work)
> +{

[Severity: Low]
Because drm_vblank_work_enable() in drivers/gpu/drm/drm_vblank_work.c is
designed to be called locklessly, does this plain read of work->armed race
with concurrent modifications?

Should this use READ_ONCE() to avoid KCSAN warnings and compiler optimization
regressions?

> +	WARN_ON(work->armed);
> +	WRITE_ONCE(work->armed, true);
> +}
> +EXPORT_SYMBOL(drm_vblank_work_enable);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615201846.307297-1-dev@lankhorst.se?part=1