On Mon, 18 May 2026 10:16:56 +0200 Boris Brezillon <[email protected]> wrote:
> On Thu, 14 May 2026 15:25:07 +0100 > Steven Price <[email protected]> wrote: > > > On 12/05/2026 12:37, 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]> > > > > As mentioned in the other thread[1] it turns out this won't work with > > the current firmware. > > > > The firmware checks the interrupt mask before signalling the ACK - so > > enabling the bit in the mask just before waiting for it is problematic - > > the firmware may not see the addition in the mask and will not trigger > > the interrupt. > > Is it a problem though? wait_event_timeout() will evaluate the > condition before going to sleep, so, if the FW raced with the > input->ack_irq_mask update, I assume the condition will evaluate to > true and wait_event_timeout() would return immediately. The only issue > is if the FW updates the output->ack register after reading > input->ack_irq_mask, but that would be weird, since the output->ack > update doesn't depend on input->ack_irq_mask, and raising an interrupt > before updating output->ack would be racy anyway. > > Am I missing something? > Quoting your reply to v1, for the context, because I missed it when replying here: > 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. So, the race is: CPU MCU CSG_REQ.STATE = START atomic_poll(CSG_ACK & STATE_MASK) process_state_change() ... mask = CSG_ACK_IRQ_MASK ... // done CSG_ACK_IRQ_MASK |= STATE_MASK wait(CSG_ACK & STATE_MASK) CSG_ACK = (CSG_ACK & STATE_MASK) | CSG_REQ.STATE if (mask & STATE_MASK) // evaluates to false raise_int() It's a bit unfortunate, because conditionally enabling IRQs only after the polling period expires reduces the number of interrupts directly caused by requests initiated by the CPU since it's assumed those requests will complete relatively quickly and avoid a sleep+interrupt+wake-up round-trip. We could of course update CSG_ACK_IRQ_MASK at the time we update CSG_REQ, but that means we'll still get an interrupt, so I guess we should stick to what exists now if there's no other option. Oh well. > > 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! ;) :-(
