Re: [PATCH v5 11/16] drm/panthor: Don't update might_have_idle_groups in process_idle_event_locked()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The panthor scheduler acquires `queue->fence_ctx.lock` using `spin_lock()` instead of `spin_lock_irqsave()`, violating the `dma_fence` locking rules and risking a deadlock against hardware interrupts.
--

commit 3ecce8ed069220d684864af5f10bd10ad0ae008b
Author: Boris Brezillon <boris.brezillon@collabora.com>

drm/panthor: Don't update might_have_idle_groups in process_idle_event_locked()

By scheduling an immediate tick, we already force idleness re-evaluation,
which gives the scheduler the opportunity to evict idle groups
and schedule onces that have jobs pending.

> and schedule onces that have jobs pending.

[Severity: Low]
This isn't a bug, but there appears to be a typo (onces instead of ones)
in the commit message.

> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 237f6a75e624c..a5dfb1beafff8 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -1734,8 +1734,6 @@ static void csg_slot_process_idle_event_locked(struct panthor_device *ptdev, u32
>  
>  	lockdep_assert_held(&sched->lock);
>  
> -	sched->might_have_idle_groups = true;
> -

[Severity: High]
This is a pre-existing issue, but looking at panthor_sched.c,
queue->fence_ctx.lock is acquired using plain spin_lock() in multiple
functions:

queue_suspend_timeout()
queue_resume_timeout()
cs_slot_process_fault_event_locked()
queue_check_job_completion()
group_term_post_processing()
queue_run_job()

For example, in queue_check_job_completion():

    spin_lock(&queue->fence_ctx.lock);
    ...

This lock is used as the dma_fence lock for the job's done_fence. Since
dma_fence objects can be shared with other subsystems (like KMS display
drivers), they may be accessed from hardirq context (such as a VBLANK
interrupt calling dma_fence_is_signaled() or dma_fence_add_callback(),
which attempt to acquire the fence lock).

If a hardware interrupt occurs on the same CPU while the panthor driver
holds this lock with interrupts enabled, could the interrupt handler spin
forever attempting to acquire the lock, causing a deadlock?

Should these acquisitions be updated to use spin_lock_irqsave() to comply
with dma_fence locking rules?

>  	/* Schedule a tick so we can evict idle groups and schedule non-idle
>  	 * ones. This will also update runtime PM and devfreq busy/idle states,
>  	 * so the device can lower its frequency or get suspended.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625-panthor-signal-from-irq-v5-0-8836a74e0ef9@collabora.com?part=11