Re: [PATCH v1 4/4] drm/panthor: Relax check in panthor_sched_pre_reset()

[Boris Brezillon <boris.brezillon@collabora.com>]

On Fri, 7 Nov 2025 16:40:53 +0000
Steven Price <steven.price@arm.com> wrote:

> On 31/10/2025 15:48, Boris Brezillon wrote:
> > A group can become runnable even after reset.in_progress has
> > been set to true and panthor_sched_suspend() has been called,
> > because the drm_sched queues are still running at that point,
> > and ::run_job() might call group_schedule_locked() which moves
> > the group to the runnable list. And that's fine, because we're
> > moving those groups to the stopped list anyway when we call
> > panthor_group_stop(), so just drop the misleading WARN_ON().  
> 
> If we've got another thread mutating the runnable list between
> panthor_sched_suspend() and list_for_each_entry_safe(), doesn't that
> make the list iterator unsafe? (_safe only protects against deleting the
> current item, not against concurrent access).

I'm not too sure actually. There's an
atomic_read(&sched->reset.in_progress) to check if we're about to reset
in group_schedule_locked() and cancel the insertion into the runnable
list in that case, meaning we're sure nothing new will be inserted after
we've set the in_progress=true in panthor_sched_pre_reset().

> 
> It feels to me like we should be holding the sched mutex - at least
> while iterating. I agree the WARN_ON is unnecessary, and will need
> removing if we simply guard the iteration - the alternative is to
> recolour panthor_sched_suspend() to assume the lock is held (and take
> the lock in panthor_sched_pre_reset), but I suspect that's a more ugly
> change.

I'd rather ensure that nothing new is inserted in the runnable/idle
lists after sched->reset.in_progress is set to true. Note that
sched->reset.in_progress is set to true with the sched lock held,
meaning any path modifying the sched lists (must be done with the sched
lock held) should complete before we set this to true. As long as those
paths also skip the list insertion, or, for things happening in a work
context (thinking of the tick work here), as long as the work is not
rescheduled until we get a chance to disable this work, we should be
good, no?