Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset

[Andi Shyti <andi.shyti@linux.intel.com>]

Hi,

> > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > > index a0e3ef1c65d2..600388c849f7 100644
> > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > > @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc)
> > > >    static void guc_cancel_busyness_worker(struct intel_guc *guc)
> > > >    {
> > > > -	cancel_delayed_work_sync(&guc->timestamp.work);
> > > > +	/*
> > > > +	 * When intel_gt_reset was called, task will hold a lock.
> > > > +	 * To cacel delayed work here, the _sync version will also acquire a lock, which might
> > > > +	 * trigger the possible cirular locking dependency warning.
> > > > +	 * Check the reset_in_progress flag, call async verion if reset is in progress.
> > > > +	 */
> > > This needs to explain in much more detail what is going on and why it is not
> > > a problem. E.g.:
> > > 
> > >     The busyness worker needs to be cancelled. In general that means
> > >     using the synchronous cancel version to ensure that an in-progress
> > >     worker will not keep executing beyond whatever is happening that
> > >     needs the cancel. E.g. suspend, driver unload, etc. However, in the
> > >     case of a reset, the synchronous version is not required and can
> > >     trigger a false deadlock detection warning.
> > > 
> > >     The business worker takes the reset mutex to protect against resets
> > >     interfering with it. However, it does a trylock and bails out if the
> > >     reset lock is already acquired. Thus there is no actual deadlock or
> > >     other concern with the worker running concurrently with a reset. So
> > >     an asynchronous cancel is safe in the case of a reset rather than a
> > >     driver unload or suspend type operation. On the other hand, if the
> > >     cancel_sync version is used when a reset is in progress then the
> > >     mutex deadlock detection sees the mutex being acquired through
> > >     multiple paths and complains.
> > > 
> > >     So just don't bother. That keeps the detection code happy and is
> > >     safe because of the trylock code described above.
> > So why do we even need to cancel anything if it doesn't do anything while
> > the reset is in progress?
> It still needs to be cancelled. The worker only aborts if it is actively
> executing concurrently with the reset. It might not start to execute until
> after the reset has completed. And there is presumably a reason why the
> cancel is being called, a reason not necessarily related to resets at all.
> Leaving the worker to run arbitrarily after the driver is expecting it to be
> stopped will lead to much worse things than a fake lockdep splat, e.g. a use
> after free pointer deref.

I was actually thinking why not leave things as they are and just
disable lockdep from CI. This doesn't look like a relevant report
to me.

Andi