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

[John Harrison <john.c.harrison@intel.com>]

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.

> +		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.

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.

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