Re: [PATCH 3/3] drm/xe/guc/ct: Flush g2h worker in case of g2h response timeout

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Oct 09, 2024 at 12:50:29PM -0700, John Harrison wrote:
> On 10/9/2024 03:56, Badal Nilawar wrote:
> > In case if g2h worker doesn't get opportunity to within specified
> 'to run'?
> 
> > timeout delay then flush the g2h worker explicitly.
> > 
> > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/issues/1620
> > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/issues/2902
> > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > Cc: Matthew Auld <matthew.auld@intel.com>
> > Cc: John Harrison <John.C.Harrison@Intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_guc_ct.c | 12 ++++++++++++
> >   1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> > index dcc95c01b6f0..2e2fa59eadfb 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> > @@ -1034,6 +1034,18 @@ static int guc_ct_send_recv(struct xe_guc_ct *ct, const u32 *action, u32 len,
> >   	}
> >   	mutex_unlock(&ct->lock);
> > +	/*
> > +	 * Flush g2h_worker explicitly in case if it didn't get opportunity
> > +	 * to run after it is queued
> > +	 */
> > +	if (!ret) {
> > +		xe_gt_dbg(gt, "Flush G2H worker to service H2G action %#x\n",
> > +			  action[0]);
> Seems like this could be a single line and still not hit the 100 character
> line length limit.
> 

Kinda a bikeshed, but I typically try to wrap at 80 if can.

> > +		flush_work(&ct->g2h_worker);
> > +		if (g2h_fence.done)
> > +			ret = 1;
> > +	}
> Again, are we wanting this to be a permanent change or is it intended as
> just a temporary workaround? If the latter, there needs to be a comment to
> explain the situation.
> 

Agree, see my reply to patch number 2. This applies here too.

> I would also prefer to have the debug print only in the case where
> fence.done was set after the flush. And I wonder if it is also worth making
> it a warn. That way CI will track how often this is happening and on what
> systems.

I think this is a good suggestion so this shows up in our CI.

Matt

> 
> John.
> 
> > +
> >   	/*
> >   	 * Ensure we serialize with completion side to prevent UAF with fence going out of scope on
> >   	 * the stack, since we have no clue if it will fire after the timeout before we can erase
>