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

[Nicolas Frattaroli]

On Monday, 3 August 2026 15:13:25 Central European Summer Time Nicolas Frattaroli wrote:
> On Monday, 3 August 2026 10:53:24 Central European Summer Time Boris Brezillon wrote:
> > Hello Nicolas,
> > 
> > On Thu, 30 Jul 2026 13:45:15 +0200
> > Nicolas Frattaroli <nicolas.frattaroli@xxxxxxxxxxxxx> 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.
> > 
> > Can you elaborate a bit on the race being fixed here? If pending_reqs bits
> > are truly cleared before the wake_up_all() call (which would require a
> > WRITE_ONCE() to be enforced, admittedly), there's no risk for the
> > wait_event() call to do a test before the bits have been updated,
> > and this holds even if the test is done without the lock held.
> > 
> > The other race I could think of is two threads calling
> > panthor_gpu_flush_caches() concurrently, and the second one stealing
> > the FLUSH_COMPLETED event the first thread waits on and re-issuing a
> > second flush on top, thus delaying the completion for the first thread.
> > But that should be covered by the cache_flush_lock.
> 
> panthor_gpu_flush_caches() is not the only thing that sets/gets
> pending_reqs. Notably, the threaded interrupt handler does, as
> well as any other functionality using the same member for reqs
> tracking (e.g. the soft reset).
> 
> Consider the following serialisation of events:
> 1. T1 asks to flush caches by writing GPU_CMD and setting pending_reqs
> 2. T1 drops reqs_lock.
> 3. T2 enters IRQ handler for flush complete, spins lock waiting for
>    reqs_lock

Minor correction: Imagine 2 and 3 reversed here in a non-IRQ-disabling,
variant, otherwise "spins lock" does not make sense. With an IRQ-disabling
variant, ignore the "spins lock waiting for reqs_lock" part. 

> 4. T1 sleeps at wait_event_timeout
> 5. T2 updates pending_reqs and wakes up the waiter in any order, since
>    the effects of those two can't consistently be observed as sequential
>    logic without the outer reqs_lock being held by the observer
> 6. T1 wakes up, checks pending_reqs, but since pending_reqs is checked
>    without holding any lock, so we implictly depend on the synchronisation
>    point that is the waitqueue's lock rather than the reqs_lock spinlock,
>    which says nothing about whether the pending_reqs change materialised
>    on T1's side yet as far as I can tell?
> 7. T1 sees that pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED is still != 0,
>    so goes back to sleep for some future wake-up of reqs_acked or a timeout.
>