Re: [PATCH 08/10] drm/panthor: Automatically enable interrupts in panthor_fw_wait_acks()

[Boris Brezillon <boris.brezillon@collabora.com>]

On Wed, 13 May 2026 16:02:11 +0100
Steven Price <steven.price@arm.com> wrote:

> >>>> It seems to work, although I'm lightly uneasy about this because I'm not
> >>>> entirely sure whether the FW will immediately see the updates to
> >>>> ack_irq_mask and therefore whether there's a possibility to miss an
> >>>> event and be stuck waiting for the timeout.
> >>>>
> >>>> Memory models are not my strong point, OpenAI tells me the sequence
> >>>> should be something like:
> >>>>
> >>>>   scoped_guard(spinlock_irqsave, lock) {
> >>>>   	u32 ack_irq_mask = READ_ONCE(*ack_irq_mask_ptr);
> >>>>
> >>>>   	WRITE_ONCE(*ack_irq_mask_ptr, ack_irq_mask | req_mask);
> >>>>   }    
> >>>
> >>> Is this really needed? In which situation would the compiler/CPU decide
> >>> to re-order this read_update_modify sequence?    
> >>
> >> I think that's the AI being a bit overzealous, but in general WRITE_ONCE
> >> is necessary to avoid some surprising effects. In theory the compiler
> >> can decide to perform multiple writes if it's non-volatile. I.e. a
> >> sequence like:
> >>
> >> 	u32 old_mask = *ack_irq_mask_ptr;
> >> 	if (condition)
> >> 		*ack_irq_mask_ptr = 0;
> >> 	else
> >> 		*ack_irq_mask_ptr |= req_mask;
> >>
> >> Can be 'optimised' to:
> >>
> >> 	u32 old_mask = *ack_irq_mask_ptr;
> >> 	*ack_irq_mask_ptr = 0;
> >> 	if (!condition)
> >> 		*ack_irq_mask_ptr = old_mask | req_mask;
> >>
> >> In which the compiler has changed the (!condition) path to do two writes
> >> one of which "should never be seen".
> >>
> >> Given that the compiler shouldn't be able to move any of the effects
> >> outside of the scoped_guard(), and since there's only one operation then
> >> I can't see how a compiler would screw it up - but the compiler is
> >> technically free to do so.  
> > 
> > Sure, I'm not saying read_modify_write is atomic per-se (even though
> > I'd be surprised if the compiler wasn't generating instructions that
> > are atomic in the end), but it is thread-safe because of the spinlock
> > covering the read_modify_write op.  
> 
> But one of the "threads" is the MCU which isn't using the spinlock -
> which is why it's a problem if the compiler left the value in a 'random'
> state even if it's all fixed up by the time the spinlock is released.

Okay, I see what you mean. I truly hope it's not random values, but if
it goes

	X -> 0 -> X | Y

or

	X -> 0 -> X & ~Y

that's already problematic, because we'd lose events.

> 
> Like you say I would be very surprised if a compiler messed it up in
> this case.

I'll add the READ/WRITE_ONCE() and add a comment to make sure we don't
forget why they are needed (in theory).