Re: [PATCH v2 1/3] drm/xe: Read scheduler message opcode under lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe: Data race and incorrect logging of msg->opcode in tracepoint
--

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

drm/xe: Read scheduler message opcode under lock

For static scheduler messages, the opcode is only guaranteed to remain
stable while the message is on the pending message list. Read the opcode
under the message lock before removing the message from the pending list,
and pass it through the processing pipeline.

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 7ce73489c35fc..c4ef3e9bf7a18 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -2056,16 +2056,17 @@ static void __guc_exec_queue_process_msg_cgp_sync(struct xe_sched_msg *msg,
>  #define MSG_HEAD	BIT(9)
>  #define MSG_PM_REF	BIT(10)
>  
> -static void guc_exec_queue_process_msg(struct xe_sched_msg *msg)
> +static void guc_exec_queue_process_msg(struct xe_sched_msg *msg,
> +				       unsigned int opcode)
>  {
>  	struct xe_device *xe = guc_to_xe(exec_queue_to_guc(msg->private_data));
>  	int idx;
> -	bool pm_ref = !!(msg->opcode & MSG_PM_REF);
> +	bool pm_ref = !!(opcode & MSG_PM_REF);
>  	bool bound = drm_dev_enter(&xe->drm, &idx);
>  
>  	trace_xe_sched_msg_recv(msg);

[Severity: Medium]
Since the commit stabilizes the opcode under the message lock and passes it
in as the opcode parameter, does calling trace_xe_sched_msg_recv() outside
the lock still risk a data race?

Looking at drivers/gpu/drm/xe/xe_trace.h:

DECLARE_EVENT_CLASS(xe_sched_msg,
    ...
    TP_fast_assign(
           __assign_str(dev);
           __entry->opcode = msg->opcode;
           ...

It appears the trace macro dereferences msg->opcode unprotected, which
could log the opcode of a re-enqueued message rather than the opcode
currently being processed. Should the trace event be updated to accept the
stabilized opcode parameter instead?

>  
> -	switch (msg->opcode & OPCODE_MASK) {
> +	switch (opcode & OPCODE_MASK) {
>  	case CLEANUP:
>  		__guc_exec_queue_process_msg_cleanup(msg, bound);
>  		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013201508.514480-1-daniel.charles@intel.com?part=1