[PATCH v2 0/3] Rework panthor's cache flush and soft reset locking

[PATCH v2 0/3] Rework panthor's cache flush and soft reset locking

[Nicolas Frattaroli]

There are two problems in panthor's cache flushing/soft reset code
related to locking.

The first problem is that the spinlocks are taken with _irqsave, even
though the contended lock is never acquired in a raw interrupt handler.
Only a threaded handler locks it.

The second problem is that the wait_event condition references
pending_reqs without holding the required lock. This means there's a
data race.

While resolving the first problem is easy, the second problem requires
adding some more macro variants to wait.h, which is done in the first
patch.

New tracepoints to debug cache flushing duration without initial locking
waits is thrown in for good measure as well, to complement what's in
lock_stat and what the function tracer can already do.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
Changes in v2:
- Use trace event template for the two tracepoints (ty Steven Rostedt)
- Link to v1: https://patch.msgid.link/20260729-panthor-cache-flush-fix-v1-0-205921ed3c81@collabora.com

---
Nicolas Frattaroli (3):
      wait: Introduce non-irq variants of wait_event_lock_timeout
      drm/panthor: Revisit reqs_lock handling in flush/reset paths
      drm/panthor: Add tracepoints for cache flushing

 drivers/gpu/drm/panthor/panthor_gpu.c   | 28 +++++------
 drivers/gpu/drm/panthor/panthor_trace.h | 49 +++++++++++++++++++
 include/linux/wait.h                    | 84 +++++++++++++++++++++++++++++++++
 3 files changed, 147 insertions(+), 14 deletions(-)
---
base-commit: 5e24f68d311764bf343f9def49b752b509dfd5fd
change-id: 20260728-panthor-cache-flush-fix-b36cb15f92c3

Best regards,
--  
Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

[PATCH v2 1/3] wait: Introduce non-irq variants of wait_event_lock_timeout

[Nicolas Frattaroli]

For locks that are never acquired in an interrupt context, the non-irq
variants for spin_lock()/spin_unlock() are more efficient. However, so
far, wait.h only provided wait_event_lock_irq_timeout and
wait_event_interruptible_lock_irq_timeout, both of which use
spin_lock_irq()/spin_unlock_irq().

Rectify this by adding new non-irq variants of these two macros.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 include/linux/wait.h | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/include/linux/wait.h b/include/linux/wait.h
index dce055e6add3..cd9b989c120e 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -1174,6 +1174,13 @@ do {										\
 		      __ret = schedule_timeout(__ret);				\
 		      spin_lock_irq(&lock));
 
+#define __wait_event_lock_timeout(wq_head, condition, lock, timeout, state)	\
+	___wait_event(wq_head, ___wait_cond_timeout(condition),			\
+		      state, 0, timeout,					\
+		      spin_unlock(&lock);					\
+		      __ret = schedule_timeout(__ret);				\
+		      spin_lock(&lock))
+
 /**
  * wait_event_interruptible_lock_irq_timeout - sleep until a condition gets
  *		true or a timeout elapses. The condition is checked under
@@ -1219,6 +1226,83 @@ do {										\
 	__ret;									\
 })
 
+/**
+ * wait_event_interruptible_lock_timeout - sleep until a condition gets
+ *		true or a timeout elapses. The condition is checked under
+ *		the lock with IRQs enabled. This is expected to be called
+ *		with the lock taken.
+ * @wq_head: the waitqueue to wait on
+ * @condition: a C expression for the event to wait for
+ * @lock: a spinlock_t that has been locked with spin_lock(), which will be
+ *	  released before schedule() and reacquired afterwards.
+ * @timeout: timeout, in jiffies
+ *
+ * The process is put to sleep (TASK_INTERRUPTIBLE) until the
+ * @condition evaluates to true or signal is received. The @condition is
+ * checked each time the waitqueue @wq_head is woken up.
+ *
+ * wake_up() has to be called after changing any variable that could
+ * change the result of the wait condition.
+ *
+ * This is supposed to be called while holding the lock. The lock is
+ * dropped before going to sleep using spin_unlock() and is reacquired
+ * using spin_lock() afterwards, but without globally disabling IRQs.
+ * This is in contrast to wait_event_interruptible_lock_irq_timeout(),
+ * which uses the spin_lock_irq() and spin_unlock_irq() functions instead.
+ *
+ * The function returns 0 if the @timeout elapsed, -ERESTARTSYS if it
+ * was interrupted by a signal, and the remaining jiffies otherwise
+ * if the condition evaluated to true before the timeout elapsed.
+ */
+#define wait_event_interruptible_lock_timeout(wq_head, condition, lock,		\
+					      timeout)				\
+({										\
+	long __ret = timeout;							\
+	if (!___wait_cond_timeout(condition))					\
+		__ret = __wait_event_lock_timeout(				\
+					wq_head, condition, lock, timeout,	\
+					TASK_INTERRUPTIBLE);			\
+	__ret;									\
+})
+
+/**
+ * wait_event_lock_timeout - sleep until a condition gets true or a timeout
+ *		elapses. The condition is checked under the lock with IRQs
+ *		enabled. This is expected to be called with the lock taken.
+ * @wq_head: the waitqueue to wait on
+ * @condition: a C expression for the event to wait for
+ * @lock: a spinlock_t that has been locked with spin_lock(), which will be
+ *	  released before schedule() and reacquired afterwards.
+ * @timeout: timeout, in jiffies
+ *
+ * The process is put to sleep (TASK_UNINTERRUPTIBLE) until the
+ * @condition evaluates to true or signal is received. The @condition is
+ * checked each time the waitqueue @wq_head is woken up.
+ *
+ * wake_up() has to be called after changing any variable that could
+ * change the result of the wait condition.
+ *
+ * This is supposed to be called while holding the lock. The lock is
+ * dropped before going to sleep using spin_unlock() and is reacquired
+ * using spin_lock() afterwards, but without globally disabling IRQs.
+ * This is in contrast to wait_event_lock_irq_timeout(), which uses the
+ * spin_lock_irq() and spin_unlock_irq() functions instead. Compared to
+ * wait_event_interruptible_lock_timeout(), it uses the %TASK_UNINTERRUPTIBLE
+ * instead of allowing the task to be interrupted.
+ *
+ * The function returns 0 if the @timeout elapsed, or the remaining jiffies
+ * otherwise if the condition evaluated to true before the timeout elapsed.
+ */
+#define wait_event_lock_timeout(wq_head, condition, lock, timeout)		\
+({										\
+	long __ret = timeout;							\
+	if (!___wait_cond_timeout(condition))					\
+		__ret = __wait_event_lock_timeout(				\
+					wq_head, condition, lock, timeout,	\
+					TASK_UNINTERRUPTIBLE);			\
+	__ret;									\
+})
+
 /*
  * Waitqueues which are removed from the waitqueue_head at wakeup time
  */

-- 
2.55.0

Re: [PATCH v2 1/3] wait: Introduce non-irq variants of wait_event_lock_timeout

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The kerneldoc comment for wait_event_lock_timeout incorrectly states that the process can be awakened by a signal.
--

--- Patch [1]: [PATCH v2 1/3] wait: Introduce non-irq variants of wait_event_lock_timeout ---
commit a625303b88e13551964ad465fcf368350aa20d46
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

wait: Introduce non-irq variants of wait_event_lock_timeout

This commit introduces non-irq variants of wait_event_lock_timeout and 
wait_event_interruptible_lock_timeout. These macros are optimized for 
spinlocks that are never acquired in an interrupt context, avoiding the 
overhead of globally disabling IRQs.

> diff --git a/include/linux/wait.h b/include/linux/wait.h
> index dce055e6add39..cd9b989c120e5 100644
> --- a/include/linux/wait.h
> +++ b/include/linux/wait.h
[ ... ]
> +/**
> + * wait_event_lock_timeout - sleep until a condition gets true or a timeout
> + *		elapses. The condition is checked under the lock with IRQs
> + *		enabled. This is expected to be called with the lock taken.
> + * @wq_head: the waitqueue to wait on
> + * @condition: a C expression for the event to wait for
> + * @lock: a spinlock_t that has been locked with spin_lock(), which will be
> + *	  released before schedule() and reacquired afterwards.
> + * @timeout: timeout, in jiffies
> + *
> + * The process is put to sleep (TASK_UNINTERRUPTIBLE) until the
> + * @condition evaluates to true or signal is received. The @condition is
> + * checked each time the waitqueue @wq_head is woken up.

[Severity: Low]
This isn't a bug, but is the mention of a signal being received accurate 
here?

Since wait_event_lock_timeout places the task in TASK_UNINTERRUPTIBLE state, 
it should not be awakened by signals. It looks like this sentence might have 
been copied from the interruptible variant above it.

> + *
> + * wake_up() has to be called after changing any variable that could
> + * change the result of the wait condition.
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-panthor-cache-flush-fix-v2-0-28790478bfff@collabora.com?part=1

Re: [PATCH v2 1/3] wait: Introduce non-irq variants of wait_event_lock_timeout

[Liviu Dudau]

On Thu, Jul 30, 2026 at 01:45:14PM +0200, Nicolas Frattaroli wrote:
> For locks that are never acquired in an interrupt context, the non-irq
> variants for spin_lock()/spin_unlock() are more efficient. However, so
> far, wait.h only provided wait_event_lock_irq_timeout and
> wait_event_interruptible_lock_irq_timeout, both of which use
> spin_lock_irq()/spin_unlock_irq().
> 
> Rectify this by adding new non-irq variants of these two macros.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> ---
>  include/linux/wait.h | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/include/linux/wait.h b/include/linux/wait.h
> index dce055e6add3..cd9b989c120e 100644
> --- a/include/linux/wait.h
> +++ b/include/linux/wait.h
> @@ -1174,6 +1174,13 @@ do {										\
>  		      __ret = schedule_timeout(__ret);				\
>  		      spin_lock_irq(&lock));
>  
> +#define __wait_event_lock_timeout(wq_head, condition, lock, timeout, state)	\
> +	___wait_event(wq_head, ___wait_cond_timeout(condition),			\
> +		      state, 0, timeout,					\
> +		      spin_unlock(&lock);					\
> +		      __ret = schedule_timeout(__ret);				\
> +		      spin_lock(&lock))
> +
>  /**
>   * wait_event_interruptible_lock_irq_timeout - sleep until a condition gets
>   *		true or a timeout elapses. The condition is checked under
> @@ -1219,6 +1226,83 @@ do {										\
>  	__ret;									\
>  })
>  
> +/**
> + * wait_event_interruptible_lock_timeout - sleep until a condition gets
> + *		true or a timeout elapses. The condition is checked under
> + *		the lock with IRQs enabled. This is expected to be called
> + *		with the lock taken.
> + * @wq_head: the waitqueue to wait on
> + * @condition: a C expression for the event to wait for
> + * @lock: a spinlock_t that has been locked with spin_lock(), which will be
> + *	  released before schedule() and reacquired afterwards.
> + * @timeout: timeout, in jiffies
> + *
> + * The process is put to sleep (TASK_INTERRUPTIBLE) until the
> + * @condition evaluates to true or signal is received. The @condition is
> + * checked each time the waitqueue @wq_head is woken up.
> + *
> + * wake_up() has to be called after changing any variable that could
> + * change the result of the wait condition.
> + *
> + * This is supposed to be called while holding the lock. The lock is
> + * dropped before going to sleep using spin_unlock() and is reacquired
> + * using spin_lock() afterwards, but without globally disabling IRQs.
> + * This is in contrast to wait_event_interruptible_lock_irq_timeout(),
> + * which uses the spin_lock_irq() and spin_unlock_irq() functions instead.
> + *
> + * The function returns 0 if the @timeout elapsed, -ERESTARTSYS if it
> + * was interrupted by a signal, and the remaining jiffies otherwise
> + * if the condition evaluated to true before the timeout elapsed.
> + */
> +#define wait_event_interruptible_lock_timeout(wq_head, condition, lock,		\
> +					      timeout)				\
> +({										\
> +	long __ret = timeout;							\
> +	if (!___wait_cond_timeout(condition))					\
> +		__ret = __wait_event_lock_timeout(				\
> +					wq_head, condition, lock, timeout,	\
> +					TASK_INTERRUPTIBLE);			\
> +	__ret;									\
> +})
> +
> +/**
> + * wait_event_lock_timeout - sleep until a condition gets true or a timeout
> + *		elapses. The condition is checked under the lock with IRQs
> + *		enabled. This is expected to be called with the lock taken.
> + * @wq_head: the waitqueue to wait on
> + * @condition: a C expression for the event to wait for
> + * @lock: a spinlock_t that has been locked with spin_lock(), which will be
> + *	  released before schedule() and reacquired afterwards.
> + * @timeout: timeout, in jiffies
> + *
> + * The process is put to sleep (TASK_UNINTERRUPTIBLE) until the
> + * @condition evaluates to true or signal is received. The @condition is
> + * checked each time the waitqueue @wq_head is woken up.
> + *
> + * wake_up() has to be called after changing any variable that could
> + * change the result of the wait condition.
> + *
> + * This is supposed to be called while holding the lock. The lock is
> + * dropped before going to sleep using spin_unlock() and is reacquired
> + * using spin_lock() afterwards, but without globally disabling IRQs.
> + * This is in contrast to wait_event_lock_irq_timeout(), which uses the
> + * spin_lock_irq() and spin_unlock_irq() functions instead. Compared to
> + * wait_event_interruptible_lock_timeout(), it uses the %TASK_UNINTERRUPTIBLE
> + * instead of allowing the task to be interrupted.
> + *
> + * The function returns 0 if the @timeout elapsed, or the remaining jiffies
> + * otherwise if the condition evaluated to true before the timeout elapsed.
> + */
> +#define wait_event_lock_timeout(wq_head, condition, lock, timeout)		\
> +({										\
> +	long __ret = timeout;							\
> +	if (!___wait_cond_timeout(condition))					\
> +		__ret = __wait_event_lock_timeout(				\
> +					wq_head, condition, lock, timeout,	\
> +					TASK_UNINTERRUPTIBLE);			\
> +	__ret;									\
> +})
> +
>  /*
>   * Waitqueues which are removed from the waitqueue_head at wakeup time
>   */
> 
> -- 
> 2.55.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

[PATCH v2 2/3] drm/panthor: Revisit reqs_lock handling in flush/reset paths

[Nicolas Frattaroli]

panthor_gpu_flush_caches() and panthor_gpu_soft_reset() would read (and
even reset) the contents of the pending_reqs register outside of holding
the reqs_lock. Additionally, when it did hold the lock, it did so with
the irqsave/irqrestore variants, even though the spinlock was never
acquired in an atomic context, just the threaded handler.

Use the new wait_event_lock_timeout() macro to check pending_reqs under
the lock, and only do so without disabling interrupts.

Fixes: 5cd894e258c4 ("drm/panthor: Add the GPU logical block")
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_gpu.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
index c013d6bf9a59..f015bde80abf 100644
--- a/drivers/gpu/drm/panthor/panthor_gpu.c
+++ b/drivers/gpu/drm/panthor/panthor_gpu.c
@@ -330,35 +330,34 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
 			     u32 l2, u32 lsc, u32 other)
 {
 	struct panthor_gpu *gpu = ptdev->gpu;
-	unsigned long flags;
 	int ret = 0;
 
 	/* Serialize cache flush operations. */
 	guard(mutex)(&ptdev->gpu->cache_flush_lock);
 
-	spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
+	spin_lock(&ptdev->gpu->reqs_lock);
 	if (!(ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED)) {
 		ptdev->gpu->pending_reqs |= GPU_IRQ_CLEAN_CACHES_COMPLETED;
 		gpu_write(gpu->iomem, GPU_CMD, GPU_FLUSH_CACHES(l2, lsc, other));
 	} else {
 		ret = -EIO;
 	}
-	spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
 
-	if (ret)
+	if (ret) {
+		spin_unlock(&ptdev->gpu->reqs_lock);
 		return ret;
+	}
 
-	if (!wait_event_timeout(ptdev->gpu->reqs_acked,
+	if (!wait_event_lock_timeout(ptdev->gpu->reqs_acked,
 				!(ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED),
-				msecs_to_jiffies(100))) {
-		spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
+				ptdev->gpu->reqs_lock, msecs_to_jiffies(100))) {
 		if ((ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED) != 0 &&
 		    !(gpu_read(gpu->irq.iomem, INT_RAWSTAT) & GPU_IRQ_CLEAN_CACHES_COMPLETED))
 			ret = -ETIMEDOUT;
 		else
 			ptdev->gpu->pending_reqs &= ~GPU_IRQ_CLEAN_CACHES_COMPLETED;
-		spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
 	}
+	spin_unlock(&ptdev->gpu->reqs_lock);
 
 	if (ret) {
 		panthor_device_schedule_reset(ptdev);
@@ -378,27 +377,25 @@ int panthor_gpu_soft_reset(struct panthor_device *ptdev)
 {
 	struct panthor_gpu *gpu = ptdev->gpu;
 	bool timedout = false;
-	unsigned long flags;
 
-	spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
+	guard(spinlock)(&ptdev->gpu->reqs_lock);
+
 	if (!drm_WARN_ON(&ptdev->base,
 			 ptdev->gpu->pending_reqs & GPU_IRQ_RESET_COMPLETED)) {
 		ptdev->gpu->pending_reqs |= GPU_IRQ_RESET_COMPLETED;
 		gpu_write(gpu->irq.iomem, INT_CLEAR, GPU_IRQ_RESET_COMPLETED);
 		gpu_write(gpu->iomem, GPU_CMD, GPU_SOFT_RESET);
 	}
-	spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
 
-	if (!wait_event_timeout(ptdev->gpu->reqs_acked,
+	if (!wait_event_lock_timeout(ptdev->gpu->reqs_acked,
 				!(ptdev->gpu->pending_reqs & GPU_IRQ_RESET_COMPLETED),
+				ptdev->gpu->reqs_lock,
 				msecs_to_jiffies(100))) {
-		spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
 		if ((ptdev->gpu->pending_reqs & GPU_IRQ_RESET_COMPLETED) != 0 &&
 		    !(gpu_read(gpu->irq.iomem, INT_RAWSTAT) & GPU_IRQ_RESET_COMPLETED))
 			timedout = true;
 		else
 			ptdev->gpu->pending_reqs &= ~GPU_IRQ_RESET_COMPLETED;
-		spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
 	}
 
 	if (timedout) {

-- 
2.55.0

Re: [PATCH v2 2/3] drm/panthor: Revisit reqs_lock handling in flush/reset paths

[sashiko-bot]

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

Pre-existing issues:
- [High] Race condition between the threaded IRQ handler and the wait timeout logic causes spurious GPU resets.
--

--- Patch [2]: [PATCH v2 2/3] drm/panthor: Revisit reqs_lock handling in flush/reset paths ---
commit ec14154f1ff35bca85396f0f6f71d7d7b1d6eb6d
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

drm/panthor: Revisit reqs_lock handling in flush/reset paths

The patch updates the handling of reqs_lock in panthor_gpu_flush_caches() and
panthor_gpu_soft_reset(). It replaces wait_event_timeout() with
wait_event_lock_timeout(), and uses spin_lock() instead of spin_lock_irqsave()
since the lock is not acquired in an atomic context.

> diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
> index e52c5675981f5..cd591c17198be 100644
> --- a/drivers/gpu/drm/panthor/panthor_gpu.c
> +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
> @@ -329,35 +329,34 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,

[ ... ]

> -	if (!wait_event_timeout(ptdev->gpu->reqs_acked,
> +	if (!wait_event_lock_timeout(ptdev->gpu->reqs_acked,
>  				!(ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED),
> -				msecs_to_jiffies(100))) {
> -		spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
> +				ptdev->gpu->reqs_lock, msecs_to_jiffies(100))) {
>  		if ((ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED) != 0 &&
>  		    !(gpu_read(gpu->irq.iomem, INT_RAWSTAT) & GPU_IRQ_CLEAN_CACHES_COMPLETED))
>  			ret = -ETIMEDOUT;

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition here
with the threaded IRQ handler?

If panthor_gpu_irq_handler() clears the hardware interrupt status via
INT_CLEAR before acquiring reqs_lock, it can be preempted:

    gpu_write(gpu->irq.iomem, INT_CLEAR, status);
    
    // ... preemptible window ...
    
    spin_lock(&ptdev->gpu->reqs_lock);
    if (status & ptdev->gpu->pending_reqs) {

If the 100ms timeout expires during this precise window, the fallback condition
above evaluates under reqs_lock.

It finds that pending_reqs still indicates the request is pending, but
checking INT_RAWSTAT shows the interrupt is not pending (because it was just
cleared by the preempted thread). 

Could this cause the wait logic to falsely conclude the hardware timed out,
return -ETIMEDOUT, and schedule a spurious full GPU reset?

[ ... ]

> @@ -377,27 +376,25 @@ int panthor_gpu_soft_reset(struct panthor_device *ptdev)

[ ... ]

> -	if (!wait_event_timeout(ptdev->gpu->reqs_acked,
> +	if (!wait_event_lock_timeout(ptdev->gpu->reqs_acked,
>  				!(ptdev->gpu->pending_reqs & GPU_IRQ_RESET_COMPLETED),
> +				ptdev->gpu->reqs_lock,
>  				msecs_to_jiffies(100))) {
> -		spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
>  		if ((ptdev->gpu->pending_reqs & GPU_IRQ_RESET_COMPLETED) != 0 &&
>  		    !(gpu_read(gpu->irq.iomem, INT_RAWSTAT) & GPU_IRQ_RESET_COMPLETED))
>  			timedout = true;

[Severity: High]
This is a pre-existing issue, but does this suffer from the same race condition
with the threaded IRQ handler as the flush caches timeout logic above?

>  		else
>  			ptdev->gpu->pending_reqs &= ~GPU_IRQ_RESET_COMPLETED;
> -		spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-panthor-cache-flush-fix-v2-0-28790478bfff@collabora.com?part=2

Re: [PATCH v2 2/3] drm/panthor: Revisit reqs_lock handling in flush/reset paths

[Liviu Dudau]

On Thu, Jul 30, 2026 at 01:45:15PM +0200, Nicolas Frattaroli wrote:
> panthor_gpu_flush_caches() and panthor_gpu_soft_reset() would read (and
> even reset) the contents of the pending_reqs register outside of holding
> the reqs_lock. Additionally, when it did hold the lock, it did so with
> the irqsave/irqrestore variants, even though the spinlock was never
> acquired in an atomic context, just the threaded handler.
> 
> Use the new wait_event_lock_timeout() macro to check pending_reqs under
> the lock, and only do so without disabling interrupts.
> 
> Fixes: 5cd894e258c4 ("drm/panthor: Add the GPU logical block")
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> ---
>  drivers/gpu/drm/panthor/panthor_gpu.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
> index c013d6bf9a59..f015bde80abf 100644
> --- a/drivers/gpu/drm/panthor/panthor_gpu.c
> +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
> @@ -330,35 +330,34 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
>  			     u32 l2, u32 lsc, u32 other)
>  {
>  	struct panthor_gpu *gpu = ptdev->gpu;
> -	unsigned long flags;
>  	int ret = 0;
>  
>  	/* Serialize cache flush operations. */
>  	guard(mutex)(&ptdev->gpu->cache_flush_lock);
>  
> -	spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
> +	spin_lock(&ptdev->gpu->reqs_lock);
>  	if (!(ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED)) {
>  		ptdev->gpu->pending_reqs |= GPU_IRQ_CLEAN_CACHES_COMPLETED;
>  		gpu_write(gpu->iomem, GPU_CMD, GPU_FLUSH_CACHES(l2, lsc, other));
>  	} else {
>  		ret = -EIO;
>  	}
> -	spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
>  
> -	if (ret)
> +	if (ret) {
> +		spin_unlock(&ptdev->gpu->reqs_lock);
>  		return ret;
> +	}
>  
> -	if (!wait_event_timeout(ptdev->gpu->reqs_acked,
> +	if (!wait_event_lock_timeout(ptdev->gpu->reqs_acked,
>  				!(ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED),
> -				msecs_to_jiffies(100))) {
> -		spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
> +				ptdev->gpu->reqs_lock, msecs_to_jiffies(100))) {
>  		if ((ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED) != 0 &&
>  		    !(gpu_read(gpu->irq.iomem, INT_RAWSTAT) & GPU_IRQ_CLEAN_CACHES_COMPLETED))
>  			ret = -ETIMEDOUT;
>  		else
>  			ptdev->gpu->pending_reqs &= ~GPU_IRQ_CLEAN_CACHES_COMPLETED;
> -		spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
>  	}
> +	spin_unlock(&ptdev->gpu->reqs_lock);
>  
>  	if (ret) {
>  		panthor_device_schedule_reset(ptdev);
> @@ -378,27 +377,25 @@ int panthor_gpu_soft_reset(struct panthor_device *ptdev)
>  {
>  	struct panthor_gpu *gpu = ptdev->gpu;
>  	bool timedout = false;
> -	unsigned long flags;
>  
> -	spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
> +	guard(spinlock)(&ptdev->gpu->reqs_lock);
> +
>  	if (!drm_WARN_ON(&ptdev->base,
>  			 ptdev->gpu->pending_reqs & GPU_IRQ_RESET_COMPLETED)) {
>  		ptdev->gpu->pending_reqs |= GPU_IRQ_RESET_COMPLETED;
>  		gpu_write(gpu->irq.iomem, INT_CLEAR, GPU_IRQ_RESET_COMPLETED);
>  		gpu_write(gpu->iomem, GPU_CMD, GPU_SOFT_RESET);
>  	}
> -	spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
>  
> -	if (!wait_event_timeout(ptdev->gpu->reqs_acked,
> +	if (!wait_event_lock_timeout(ptdev->gpu->reqs_acked,
>  				!(ptdev->gpu->pending_reqs & GPU_IRQ_RESET_COMPLETED),
> +				ptdev->gpu->reqs_lock,
>  				msecs_to_jiffies(100))) {
> -		spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
>  		if ((ptdev->gpu->pending_reqs & GPU_IRQ_RESET_COMPLETED) != 0 &&
>  		    !(gpu_read(gpu->irq.iomem, INT_RAWSTAT) & GPU_IRQ_RESET_COMPLETED))
>  			timedout = true;
>  		else
>  			ptdev->gpu->pending_reqs &= ~GPU_IRQ_RESET_COMPLETED;
> -		spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
>  	}
>  
>  	if (timedout) {
> 
> -- 
> 2.55.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

[PATCH v2 3/3] drm/panthor: Add tracepoints for cache flushing

[Nicolas Frattaroli]

Add two new event tracepoints: gpu_cache_flush_start to be emitted after
acquiring the flush mutex and reqs spinlock, and gpu_cache_flush_end to
be emitted when leaving the function.

This allows debugging the duration a flush takes irrespective of initial
function entry lock contention by subtracting the start tracepoint's
timestamp from the end tracepoint timestamp, and additionally contains
information such as which caches were flushed.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_gpu.c   |  3 ++
 drivers/gpu/drm/panthor/panthor_trace.h | 49 +++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
index f015bde80abf..a25955ad668b 100644
--- a/drivers/gpu/drm/panthor/panthor_gpu.c
+++ b/drivers/gpu/drm/panthor/panthor_gpu.c
@@ -336,6 +336,7 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
 	guard(mutex)(&ptdev->gpu->cache_flush_lock);
 
 	spin_lock(&ptdev->gpu->reqs_lock);
+	trace_gpu_cache_flush_start(ptdev->base.dev, l2, lsc, other);
 	if (!(ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED)) {
 		ptdev->gpu->pending_reqs |= GPU_IRQ_CLEAN_CACHES_COMPLETED;
 		gpu_write(gpu->iomem, GPU_CMD, GPU_FLUSH_CACHES(l2, lsc, other));
@@ -345,6 +346,7 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
 
 	if (ret) {
 		spin_unlock(&ptdev->gpu->reqs_lock);
+		trace_gpu_cache_flush_end(ptdev->base.dev, l2, lsc, other);
 		return ret;
 	}
 
@@ -358,6 +360,7 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
 			ptdev->gpu->pending_reqs &= ~GPU_IRQ_CLEAN_CACHES_COMPLETED;
 	}
 	spin_unlock(&ptdev->gpu->reqs_lock);
+	trace_gpu_cache_flush_end(ptdev->base.dev, l2, lsc, other);
 
 	if (ret) {
 		panthor_device_schedule_reset(ptdev);
diff --git a/drivers/gpu/drm/panthor/panthor_trace.h b/drivers/gpu/drm/panthor/panthor_trace.h
index 6ffeb4fe6599..6951b95b1de7 100644
--- a/drivers/gpu/drm/panthor/panthor_trace.h
+++ b/drivers/gpu/drm/panthor/panthor_trace.h
@@ -76,6 +76,55 @@ TRACE_EVENT(gpu_job_irq,
 		  __entry->events, __entry->duration_ns)
 );
 
+DECLARE_EVENT_CLASS(gpu_cache_flush_template,
+	TP_PROTO(const struct device *dev, u32 l2, u32 lsc, u32 other),
+	TP_ARGS(dev, l2, lsc, other),
+	TP_STRUCT__entry(
+		__string(dev_name, dev_name(dev))
+		__field(u32, l2)
+		__field(u32, lsc)
+		__field(u32, other)
+	),
+	TP_fast_assign(
+		__assign_str(dev_name);
+		__entry->l2     = l2;
+		__entry->lsc    = lsc;
+		__entry->other  = other;
+	),
+	TP_printk("%s: l2=0x%x lsc=0x%x other=0x%x", __get_str(dev_name),
+		  __entry->l2, __entry->lsc, __entry->other)
+);
+
+/**
+ * gpu_cache_flush_start - called after cache flush locks taken, before flush
+ * @dev: pointer to the &struct device, for printing the device name
+ * @l2: "l2" flush flags
+ * @lsc: "lsc" flush flags
+ * @other: "other" flush flags
+ *
+ * Fires after any initial lock contention around the locks needed for flushing
+ * caches, but before the actual cache flush is requested.
+ */
+DEFINE_EVENT(gpu_cache_flush_template, gpu_cache_flush_start,
+	     TP_PROTO(const struct device *dev, u32 l2, u32 lsc, u32 other),
+	     TP_ARGS(dev, l2, lsc, other)
+);
+
+/**
+ * gpu_cache_flush_end - called after cache flush
+ * @dev: pointer to the &struct device, for printing the device name
+ * @l2: "l2" flush flags
+ * @lsc: "lsc" flush flags
+ * @other: "other" flush flags
+ *
+ * Fires after either the cache flush is complete, or has failed. Can be used
+ * together with gpu_cache_flush_start to get how long the flush has taken.
+ */
+DEFINE_EVENT(gpu_cache_flush_template, gpu_cache_flush_end,
+	     TP_PROTO(const struct device *dev, u32 l2, u32 lsc, u32 other),
+	     TP_ARGS(dev, l2, lsc, other)
+);
+
 #endif /* __PANTHOR_TRACE_H__ */
 
 #undef TRACE_INCLUDE_PATH

-- 
2.55.0

Re: [PATCH v2 3/3] drm/panthor: Add tracepoints for cache flushing

[Steven Rostedt]

On Thu, 30 Jul 2026 13:45:16 +0200
Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:

> Add two new event tracepoints: gpu_cache_flush_start to be emitted after
> acquiring the flush mutex and reqs spinlock, and gpu_cache_flush_end to
> be emitted when leaving the function.
> 
> This allows debugging the duration a flush takes irrespective of initial
> function entry lock contention by subtracting the start tracepoint's
> timestamp from the end tracepoint timestamp, and additionally contains
> information such as which caches were flushed.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/gpu/drm/panthor/panthor_gpu.c   |  3 ++
>  drivers/gpu/drm/panthor/panthor_trace.h | 49 +++++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)

From the tracing point of view:

Reviewed-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

Re: [PATCH v2 3/3] drm/panthor: Add tracepoints for cache flushing

[Liviu Dudau]

On Thu, Jul 30, 2026 at 01:45:16PM +0200, Nicolas Frattaroli wrote:
> Add two new event tracepoints: gpu_cache_flush_start to be emitted after
> acquiring the flush mutex and reqs spinlock, and gpu_cache_flush_end to
> be emitted when leaving the function.
> 
> This allows debugging the duration a flush takes irrespective of initial
> function entry lock contention by subtracting the start tracepoint's
> timestamp from the end tracepoint timestamp, and additionally contains
> information such as which caches were flushed.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> ---
>  drivers/gpu/drm/panthor/panthor_gpu.c   |  3 ++
>  drivers/gpu/drm/panthor/panthor_trace.h | 49 +++++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
> index f015bde80abf..a25955ad668b 100644
> --- a/drivers/gpu/drm/panthor/panthor_gpu.c
> +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
> @@ -336,6 +336,7 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
>  	guard(mutex)(&ptdev->gpu->cache_flush_lock);
>  
>  	spin_lock(&ptdev->gpu->reqs_lock);
> +	trace_gpu_cache_flush_start(ptdev->base.dev, l2, lsc, other);
>  	if (!(ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED)) {
>  		ptdev->gpu->pending_reqs |= GPU_IRQ_CLEAN_CACHES_COMPLETED;
>  		gpu_write(gpu->iomem, GPU_CMD, GPU_FLUSH_CACHES(l2, lsc, other));
> @@ -345,6 +346,7 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
>  
>  	if (ret) {
>  		spin_unlock(&ptdev->gpu->reqs_lock);
> +		trace_gpu_cache_flush_end(ptdev->base.dev, l2, lsc, other);
>  		return ret;
>  	}
>  
> @@ -358,6 +360,7 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
>  			ptdev->gpu->pending_reqs &= ~GPU_IRQ_CLEAN_CACHES_COMPLETED;
>  	}
>  	spin_unlock(&ptdev->gpu->reqs_lock);
> +	trace_gpu_cache_flush_end(ptdev->base.dev, l2, lsc, other);
>  
>  	if (ret) {
>  		panthor_device_schedule_reset(ptdev);
> diff --git a/drivers/gpu/drm/panthor/panthor_trace.h b/drivers/gpu/drm/panthor/panthor_trace.h
> index 6ffeb4fe6599..6951b95b1de7 100644
> --- a/drivers/gpu/drm/panthor/panthor_trace.h
> +++ b/drivers/gpu/drm/panthor/panthor_trace.h
> @@ -76,6 +76,55 @@ TRACE_EVENT(gpu_job_irq,
>  		  __entry->events, __entry->duration_ns)
>  );
>  
> +DECLARE_EVENT_CLASS(gpu_cache_flush_template,
> +	TP_PROTO(const struct device *dev, u32 l2, u32 lsc, u32 other),
> +	TP_ARGS(dev, l2, lsc, other),
> +	TP_STRUCT__entry(
> +		__string(dev_name, dev_name(dev))
> +		__field(u32, l2)
> +		__field(u32, lsc)
> +		__field(u32, other)
> +	),
> +	TP_fast_assign(
> +		__assign_str(dev_name);
> +		__entry->l2     = l2;
> +		__entry->lsc    = lsc;
> +		__entry->other  = other;
> +	),
> +	TP_printk("%s: l2=0x%x lsc=0x%x other=0x%x", __get_str(dev_name),
> +		  __entry->l2, __entry->lsc, __entry->other)
> +);
> +
> +/**
> + * gpu_cache_flush_start - called after cache flush locks taken, before flush
> + * @dev: pointer to the &struct device, for printing the device name
> + * @l2: "l2" flush flags
> + * @lsc: "lsc" flush flags
> + * @other: "other" flush flags
> + *
> + * Fires after any initial lock contention around the locks needed for flushing
> + * caches, but before the actual cache flush is requested.
> + */
> +DEFINE_EVENT(gpu_cache_flush_template, gpu_cache_flush_start,
> +	     TP_PROTO(const struct device *dev, u32 l2, u32 lsc, u32 other),
> +	     TP_ARGS(dev, l2, lsc, other)
> +);
> +
> +/**
> + * gpu_cache_flush_end - called after cache flush
> + * @dev: pointer to the &struct device, for printing the device name
> + * @l2: "l2" flush flags
> + * @lsc: "lsc" flush flags
> + * @other: "other" flush flags
> + *
> + * Fires after either the cache flush is complete, or has failed. Can be used
> + * together with gpu_cache_flush_start to get how long the flush has taken.
> + */
> +DEFINE_EVENT(gpu_cache_flush_template, gpu_cache_flush_end,
> +	     TP_PROTO(const struct device *dev, u32 l2, u32 lsc, u32 other),
> +	     TP_ARGS(dev, l2, lsc, other)
> +);
> +
>  #endif /* __PANTHOR_TRACE_H__ */
>  
>  #undef TRACE_INCLUDE_PATH
> 
> -- 
> 2.55.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯