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

[John Harrison]

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 at intel.com>
> Cc: Matthew Brost <matthew.brost at intel.com>
> Cc: Matthew Auld <matthew.auld at intel.com>
> Cc: John Harrison <John.C.Harrison at 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.

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