On 06/05/2026 17:08, Boris Brezillon wrote: > On Wed, 6 May 2026 15:35:18 +0100 > Steven Price <[email protected]> wrote: > >> 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. > > 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. Like you say I would be very surprised if a compiler messed it up in this case. >> >>>> >>>> /* >>>> * 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. > > Okay, get it now. > >> >>>> 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. > > For the record, this is currently mapped uncached on both the CPU and > GPU side, because we don't have a way to describe the > shareability properly with the current IOMMU flags. > > So, my understanding was that the smp_wb() (DMB(ISH) on arm64) at > the end of a spin_unlock(), would ensure proper store/load instruction > ordering around this barrier, but that it would only wait for the > content to reach the inner shareable domain before returning, not any > further. But maybe I got that wrong from the start, and DMB(ISH) > doesn't even start the transaction if the access is targeting uncached > memory. In which case, AI is right, a full wmb() is needed, otherwise > there's a chance we'll wait indefinitely because the update didn't make > it to the FW interface in the first place. > > Also, if that's broken for ack_irq_mask, it's also broken in other > places... So I've looked at the firmware implementation and I can say that there's a race condition on the firmware side if we change the mask after sending the START/RESUME request. The firmware currently samples the mask before triggering the update to CSG_ACK and then uses the sampled value to decide whether to trigger an interrupt. This is all fine if the mask is set before the request (so nothing broken with the current code), but we'd need a firmware change before we can safely do what this patch was proposing. And of course we'd have to get our heads round the barriers needed! ;) Thanks, Steve
