Re: [CI] drm/xe: Disable scheduling early on FD close to avoid CAT error cascade

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `KILL` message handler unconditionally accesses hardware without checking if the device is physically present, which can lead to crashes on hot-unplug.
--

commit 6b34abf1203c0a3c2fe102d1601b9f19c5a65d6b
Author: Matthew Brost <matthew.brost@intel.com>

drm/xe: Disable scheduling early on FD close to avoid CAT error cascade

This commit mitigates CAT error cascades by eagerly sending a self-message
to disable scheduling outside the serialized TDR path when an FD is closed.

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 99d8c807ff056..8b8972b2ebea1 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c

[ ... ]

> @@ -1969,12 +1969,22 @@ static void __guc_exec_queue_process_msg_cgp_sync(struct xe_sched_msg *msg,
>  		guc_exec_queue_send_cgp_sync(q, 0);
>  }
>  
> +static void __guc_exec_queue_process_msg_kill(struct xe_sched_msg *msg)
> +{
> +	struct xe_exec_queue *q = msg->private_data;
> +	struct xe_exec_queue *primary = xe_exec_queue_multi_queue_primary(q);
> +
> +	if (exec_queue_enabled(primary))
> +		disable_scheduling(primary, true);
> +}

[Severity: High]
Could this lead to an MMIO crash on device hot-unplug?

When a hot-unplug event occurs, the device is disconnected and
drm_dev_enter() will return false, indicating the hardware is gone.
If userspace then closes the file descriptor, guc_exec_queue_kill()
will send the KILL message.

Because disable_scheduling() eventually invokes xe_guc_ct_send(), which
performs an unconditional MMIO write (xe_mmio_write32()), calling it
without verifying if the device is still physically present might trigger
a fatal bus error (Machine Check Exception).

Should this handler accept and check the bound state before touching the
hardware, similar to how other message handlers like
__guc_exec_queue_process_msg_cgp_sync() handle it?

>  #define CLEANUP				1	/* Non-zero values to catch uninitialized msg */
>  #define SET_SCHED_PROPS			2
>  #define SUSPEND				3
>  #define RESUME				4
>  #define SET_MULTI_QUEUE_PRIORITY	5
>  #define CGP_SYNC_MSG			6
> +#define KILL				7
>  #define OPCODE_MASK	0xf
>  #define MSG_LOCKED	BIT(8)
>  #define MSG_HEAD	BIT(9)
> @@ -2008,6 +2018,9 @@ static void guc_exec_queue_process_msg(struct xe_sched_msg *msg)
>  	case CGP_SYNC_MSG:
>  		__guc_exec_queue_process_msg_cgp_sync(msg, bound);
>  		break;
> +	case KILL:
> +		__guc_exec_queue_process_msg_kill(msg);
> +		break;

[Severity: High]
Does this miss passing the bound state to the KILL handler?

At the start of guc_exec_queue_process_msg(), the bound state is acquired
via drm_dev_enter():

    bool bound = drm_dev_enter(&xe->drm, &idx);
    
Other handlers like __guc_exec_queue_process_msg_cgp_sync(msg, bound)
receive this state so they can avoid hardware access if the device is gone.
Since __guc_exec_queue_process_msg_kill() isn't receiving bound, it seems
to bypass this safety check.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825201148.1780176-1-matthew.brost@intel.com?part=1