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

[Dong, Zhanjun]

> -----Original Message-----
> From: Daniel Vetter <daniel at ffwll.ch>
> Sent: August 22, 2023 9:51 AM
> To: Dong, Zhanjun <zhanjun.dong at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org; Harrison,
> John C <john.c.harrison at intel.com>; Andi Shyti <andi.shyti at linux.intel.com>;
> Daniel Vetter <daniel at ffwll.ch>
> Subject: Re: [PATCH v5] drm/i915: Avoid circular locking dependency when
> flush delayed work on gt reset
> 
> On Fri, Aug 11, 2023 at 11:20:11AM -0700, Zhanjun Dong wrote:
> > This attempts to avoid circular locking dependency between flush delayed
> > work and intel_gt_reset.
> > 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.
> > When intel_gt_reset called, reset_in_progress flag will be set, add code
> > to check the flag, call async verion if reset is in progress.
> >
> > Signed-off-by: Zhanjun Dong <zhanjun.dong at intel.com>
> > Cc: John Harrison <John.C.Harrison at Intel.com>
> > Cc: Andi Shyti <andi.shyti at linux.intel.com>
> > Cc: Daniel Vetter <daniel at ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > 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.
> 
> This is not even close to a locking bugfix. Consider this a formal nack,
> because the issue here is not even close to "needs more comments to
> explain what's going on".
> -Daniel

The purpose of the comment here it is to explain locking issue condition
> 
> > +	 * Check the reset_in_progress flag, call async verion if reset is in
> progress.


The comment here explains check with the flag to avoid locking condition.
The reset process is not considered to be complete in short time, other than that, do we missed anything?

> > +	 */
> > +	if (guc_to_gt(guc)->uc.reset_in_progress)
> > +		cancel_delayed_work(&guc->timestamp.work);
> > +	else
> > +		cancel_delayed_work_sync(&guc->timestamp.work);
> >  }
> >
> >  static void __reset_guc_busyness_stats(struct intel_guc *guc)
> > --
> > 2.34.1
> >
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch