Re: [PATCH 1/3] drm/xe/guc/ct: Improve g2h request handling during async gt reset

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

On Wed, Oct 09, 2024 at 12:41:36PM -0700, John Harrison wrote:
> On 10/9/2024 03:56, Badal Nilawar wrote:
> > It is possible that a g2h request may be cancelled while waiting for a
> > response due to an asynchronous gt reset. This commit ensures that in
> > such cases, caller will be notified by returning -ECANCELED.
> > 
> > Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> > 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 | 16 ++++++++++++++++
> >   1 file changed, 16 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> > index c7673f56d413..b93b2821e4e8 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> > @@ -512,6 +512,9 @@ void xe_guc_ct_stop(struct xe_guc_ct *ct)
> >   {
> >   	xe_guc_ct_set_state(ct, XE_GUC_CT_STATE_STOPPED);
> >   	stop_g2h_handler(ct);
> > +
> > +	/* Notify callers that CT stopped and G2H requests are cancelled */
> > +	wake_up_all(&ct->g2h_fence_wq);
> >   }
> >   static bool h2g_has_room(struct xe_guc_ct *ct, u32 cmd_len)
> > @@ -1018,6 +1021,19 @@ static int guc_ct_send_recv(struct xe_guc_ct *ct, const u32 *action, u32 len,
> >   	ret = wait_event_timeout(ct->g2h_fence_wq, g2h_fence.done, HZ);
> > +	/*
> > +	 * It is possible that the g2h request may be cancelled while waiting for a response due
> > +	 * to an asynchronous gt reset. In such cases, return -ECANCELED.
> > +	 */
> > +	mutex_lock(&ct->lock);
> > +	if (ct->state == XE_GUC_CT_STATE_STOPPED) {
> > +		xe_gt_dbg(gt, "H2G action %#x canceled as GT reset is in progress\n",
> > +			  action[0]);
> > +		mutex_unlock(&ct->lock);
> > +		return -ECANCELED;
> > +	}
> > +	mutex_unlock(&ct->lock);
> Is the lock worth while? It only protects a single read of a single
> variable. Or is the intention to serialise against any other operations that
> might be in progress and holding the lock? If the latter, it would be better
> to include a comment to that effect.
> 
> Also, the very next statement in this function is 'mutex_lock(&ct->lock);'.
> So now you have unlock/lock back to back which seems redundant.

See my reply to Badal, this flow doesn't look right.

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
>