On Tue, 2026-06-23 at 08:19 +0100, Tvrtko Ursulin wrote:
> On 17/06/2026 09:38, Philipp Stanner wrote:
> >
> > > @@ -576,6 +578,13 @@ void drm_sched_entity_select_rq(struct
> > > drm_sched_entity *entity)
> > > return;
> > >
> > > spin_lock(&entity->lock);
> > > +
> > > + if (entity->stopped) {
> > > + spin_unlock(&entity->lock);
> > > + return;
> > > +
> > > + }
> >
> > Seems unrelated? Why wasn't this needed semantically before?
>
> It solidifies the guarantee drm_sched_entity_kill() expects that the
> scheduler assigned to an entity cannot change after entity has been stopped.
>
> There we have this sequence:
>
> spin_lock(&entity->lock);
> entity->stopped = true;
> sched = drm_sched_rq_remove_entity(entity->rq, entity);
> spin_unlock(&entity->lock);
>
> if (sched)
> drm_sched_flush_run_work(sched);
>
> That is, without that check, in theory, an evil driver could race
> drm_sched_entity_select_rq() (via drm_sched_job_arm()) and
> drm_sched_entity_kill(). I am not sure if any driver can actually do
> that at the moment but it felt sensible to express it in code.
So, I was mentally in my Rust code the last weeks; re-evaluating this,
I see what you want to achieve.
I think this is then a valid robustness addition. However:
* entity->lock's documentation currently does not state that entity-
>stopped is also protected by that lock. That needs an update.
* Separate patch is desirable
* That patch should investigate whether entity->stopped can
consistently be locked, i.e. drm_sched_entity_is_idle()
* It might be a good opportunity to document the idempotence status of
drm_sched_entity_kill() :)
P.
