On Wed, 13 May 2026 16:02:11 +0100
Steven Price <[email protected]> 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).
