Re: [PATCH v2 2/2] mailbox: add Axiado AX3005 mailbox driver

On 8/18/2026 11:44 AM, [email protected] wrote:
> 
> 
>> +     for (offset = 0; offset + sizeof(u32) <= msg_len;
>> +          offset += sizeof(u32))
>> +             writel(get_unaligned_le32(buf + offset), priv->mbox_reg);
> 
> [Severity: Medium]
> Since this loop writes bulk data to a FIFO, should it use the stream I/O
> accessor writesl() instead of writel()?
> 
> The io-accessors subsystem guidelines mandate using stream accessors for
> FIFOs to avoid unnecessary per-word memory barriers and redundant endianness
> conversions.
> 

The current per-word get_unaligned_le32()/writel() accesses are
intentional.
The mailbox message format is little-endian, and the buffer is not
guaranteed to be naturally aligned. The stream accessors are raw
transfers and do not provide the same explicit unaligned
little-endian handling, so replacing the current accesses
with writesl() is not a direct substitution.

I'll take another look at whether the stream accessors can be used
without changing those semantics before v3.

>> +     if (csr & AXIADO_MBOX_CSR_EMPTY)
>> +             return IRQ_NONE;
> 
> [Severity: High]
> If the interrupt was asserted solely due to a hardware error (like an
> underflow), axiado_mbox_clear_fifo_errors() successfully clears the condition.
> If the FIFO is empty afterwards, this check returns IRQ_NONE.
> 
> Does this risk permanently disabling the IRQ line? Returning IRQ_NONE after
> successfully servicing a hardware event can trigger the core spurious
> interrupt detector.
> 


Good point. Whether this case can occur depends on whether the FIFO
overflow/underflow status can itself assert the RX interrupt, rather
than the interrupt being driven only by the FIFO not-empty condition.

I'll verify the hardware interrupt semantics and adjust the return
handling in v3 if an error condition can independently cause the
interrupt.

>> +     if (remaining) {
>> +             ret = readl_poll_timeout(priv->csr_reg, csr,
>> +                                      (csr & AXIADO_MBOX_CSR_ERRORS) ||
>> +                                       FIELD_GET(AXIADO_MBOX_CSR_LEVEL, csr) >=
>> +                                       remaining,
> 
> [Severity: High]
> The poll condition succeeds if AXIADO_MBOX_CSR_ERRORS is set, terminating
> early without verifying the FIFO actually reached the required level.
> 
> If the loop exits early due to an error rather than the level check, the code
> continues to the for-loop below and blindly reads remaining words from the
> FIFO. Will this cause hardware underflows and pass garbage data to the
> mailbox client?
> 

Agreed. If the poll exits because AXIADO_MBOX_CSR_ERRORS is set before
the FIFO reaches the required level, the subsequent read loop can
proceed with an incomplete message.

The current code does not distinguish between "the remaining data
arrived" and "an error was observed while the data was still
incomplete". I'll fix this in v3 so that the read loop is entered
only after the required FIFO level has actually been reached, and
re-test the RX path.

>> +             word = readl(priv->mbox_reg);
> 
> [Severity: Medium]
> Similar to the TX path, does this bulk FIFO read need to use readsl() instead
> of readl() to avoid the per-word memory barriers and endianness swaps?
> 

Same consideration as for the TX path above. The current
readl()/put_unaligned_le32() sequence explicitly handles the
little-endian message format and a destination buffer that may not
be naturally aligned, so readsl() is not a direct replacement.
I'll include this in the stream-accessor review for v3.

>> +             put_unaligned_le32(word, buf + i * sizeof(u32));
>> +     }
>> +
>> +     axiado_mbox_clear_fifo_errors(priv);
>> +
>> +     if (READ_ONCE(priv->active))
>> +             mbox_chan_received_data(chan, priv->rx_buffer);
> 
> [Severity: High]
> Can mbox_free_channel() nullify the channel client pointer while this thread
> is preempted?
> 
> If a mailbox client frees the channel, mbox_free_channel() clears chan->cl
> under a lock, releases the lock, and calls axiado_mbox_shutdown(). If this
> threaded IRQ handler evaluates READ_ONCE(priv->active) as true just before
> being preempted, the shutdown routine will block in disable_irq().
> 
> When this handler resumes, will mbox_chan_received_data() dereference a NULL
> chan->cl pointer?
> 

I don't think this race applies here. mbox_free_channel() invokes the
controller's ->shutdown() before chan->cl is cleared, rather than
clearing chan->cl first.

axiado_mbox_shutdown() calls disable_irq(), which waits for any
in-flight interrupt handler, including the threaded handler, to
complete. Therefore, any concurrently running axiado_rx_thread() has
completed by the time shutdown returns, before chan->cl can be cleared.

Based on this ordering, I don't see a path where chan->cl can become
NULL while axiado_rx_thread() is still running.

Thanks,
Swark