On 04/05/2026 12:02, Boris Brezillon wrote:
> On Fri, 1 May 2026 15:20:17 +0100
> Steven Price <[email protected]> wrote:
>
>> On 29/04/2026 10:38, Boris Brezillon wrote:
>>> Rather than assuming an interrupt is always expected for request
>>> acks, temporarily enable the relevant interrupts when the polling-wait
>>> failed. This should hopefully reduce the number of interrupts the CPU
>>> has to process.
>>>
>>> Signed-off-by: Boris Brezillon <[email protected]>
>>
>> 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.
>>
>> /*
>> * The FW interface can be mapped write-combine/Normal-NC.
>
> I'm not too sure I see what the non-cached property has to do with it.
> If it was cached we would still need this memory barrier, and in
> addition, we'd need a cache flush if the FW is not IO-coherent.
I *think* the point the AI was making is that the memory isn't Device.
I.e. it's writeback and the write might not have completed.
>> Make sure the
>> * IRQ mask update is visible to the FW before sleeping waiting for
>> the IRQ.
>> */
>> wmb();
>>
>> Which seems plausible. But I've long ago learnt that plausible doesn't
>> mean much when dealing with memory models!
>
> Yeah, I'm not too sure. I was honestly expecting the spinlock guard to
> act as a memory barrier already, but maybe it's not enough.
So logically it must be enough to enable other CPUs to see writes within
the spinlock - otherwise spinlocks would be completely broken on SMP. I
guess it should be sufficient for the GPU's firmware MCU to see.
This is of course the problem with AI - it's found something 'plausible'
but it's probably completely wrong. I guess we just ignore it unless we
do actually start seeing that timeout happen.
Thanks,
Steve
