Re: [PATCH v2 2/3] drm/panthor: Report the CS fatal errors through GROUP_GET_STATE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/panthor: race reading fatal_queues in panthor_group_get_state()
--

commit 8ad04a984a0eee7e92a001d9035eba4354298f3e
Author: Christian Gmeiner <cgmeiner@igalia.com>

drm/panthor: Report the CS fatal errors through GROUP_GET_STATE

This commit stores the raw CS_FATAL and CS_FATAL_INFO values in the
panthor_queue that hit a fatal error and exposes them to userspace via
an array in DRM_IOCTL_PANTHOR_GROUP_GET_STATE.

Link: https://lore.kernel.org/r/20251215115457.2137485-3-lukas.zapolskas@arm.com
Link: https://lore.kernel.org/r/20251215115457.2137485-6-lukas.zapolskas@arm.com

> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index a343c5180e01e..40400b2e45518 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c

[ ... ]

> @@ -1412,10 +1421,20 @@ cs_slot_process_fatal_event_locked(struct panthor_device *ptdev,
>  	info = cs_iface->output->fatal_info;
>  
>  	if (group) {
> +		struct panthor_queue *queue = cs_id < group->queue_count ?
> +					      group->queues[cs_id] : NULL;
> +
>  		drm_warn(&ptdev->base, "CS_FATAL: pid=%d, comm=%s\n",
>  			 group->task_info.pid, group->task_info.comm);
>  
>  		atomic_or(BIT(cs_id), &group->fatal_queues);
> +
> +		if (queue) {
> +			queue->fatal = (struct drm_panthor_queue_fatal){
> +				.exception = fatal,
> +				.exception_info = info,
> +			};
> +		}
>  	}

[ ... ]

> @@ -3844,6 +3869,11 @@ int panthor_group_get_state(struct panthor_file *pfile,
>  
>  	if (group->innocent)
>  		get_state->state |= DRM_PANTHOR_GROUP_STATE_INNOCENT;
> +
> +	scoped_guard(spinlock, &sched->events_lock) {
> +		for (u32 i = 0; i < *fatal_count; i++)
> +			fatals[i] = group->queues[i]->fatal;
> +	}
>  	mutex_unlock(&sched->lock);

[Severity: Medium]
Can panthor_group_get_state() return an inconsistent snapshot to userspace
when racing with a CS_FATAL interrupt?

In the threaded interrupt path, panthor_job_irq_threaded_handler() ->
panthor_job_irq_handler() -> panthor_sched_report_fw_events() ->
sched_process_csg_irq_locked() -> cs_slot_process_fatal_event_locked() runs
under sched->events_lock without holding sched->lock, updating both
group->fatal_queues and queue->fatal.

Meanwhile, panthor_group_get_state() reads group->fatal_queues before taking
sched->events_lock:

drivers/gpu/drm/panthor/panthor_sched.c:panthor_group_get_state() {
	...
	mutex_lock(&sched->lock);
	if (atomic_read(&group->timedout))
		get_state->state |= DRM_PANTHOR_GROUP_STATE_TIMEDOUT;

	get_state->fatal_queues = atomic_read(&group->fatal_queues);
	if (get_state->fatal_queues)
		get_state->state |= DRM_PANTHOR_GROUP_STATE_FATAL_FAULT;

	if (group->innocent)
		get_state->state |= DRM_PANTHOR_GROUP_STATE_INNOCENT;

	scoped_guard(spinlock, &sched->events_lock) {
		for (u32 i = 0; i < *fatal_count; i++)
			fatals[i] = group->queues[i]->fatal;
	}
	...
}

If cs_slot_process_fatal_event_locked() runs on another CPU after
atomic_read(&group->fatal_queues) but before sched->events_lock is acquired,
get_state->fatal_queues and DRM_PANTHOR_GROUP_STATE_FATAL_FAULT will remain
unset while fatals[cs_id].exception contains a non-zero CS_FATAL value.

Should group->fatal_queues be read inside the sched->events_lock critical
section alongside group->queues[i]->fatal so that fatal_queues, state, and
fatals[] are consistent?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-panthor-fault-info-v2-v2-0-673b31f3840c@igalia.com?part=2