From fe43cf02f416417f2cdf59524abb84356d36d496 Mon Sep 17 00:00:00 2001 From: Swark Yang Date: Thu, 3 Sep 2026 00:17:52 -0700 Subject: [PATCH] mailbox: axiado: fix RX frame draining and error handling Two RX-path bugs found during bring-up: 1. axiado_rx_thread() previously consumed only one frame per invocation. Multiple frames can accumulate in the FIFO while the threaded handler is running. Split the single-frame handling into axiado_mbox_receive_one_frame() and drain all currently queued frames before returning from the handler. A cond_resched() in the drain loop prevents a continuously-fed channel from monopolizing the CPU. 2. readl_poll_timeout() waiting for the remainder of a frame can return success either because the required FIFO level was reached or because a FIFO error appeared -- it does not distinguish the two. The old code did not check for that case and could therefore enter the read loop without enough words actually being present. Explicitly inspect the error status after the poll: if an error occurs while waiting for the remainder of an already-started frame, treat that frame as unusable and flush the FIFO to resynchronize, rather than risk delivering incomplete or garbage data to the client. While here: - Track whether the handler actually serviced a hardware condition, such as clearing a FIFO error or consuming frame data. Return IRQ_HANDLED whenever something was serviced, even if the FIFO is empty afterward, instead of reporting IRQ_NONE after handling a hardware condition. - Use readsl() for the RX FIFO payload. The destination is the driver-owned rx_buffer allocated by devm_kzalloc(), so it has suitable alignment for the stream accessor. readsl() is appropriate for copying a sequence of words from the FIFO into that buffer. The TX path intentionally stays on get_unaligned_le32()/writel(): its buffer is supplied by an external mailbox client, the mailbox API gives no guarantee that buffer is naturally aligned, and the TX path also explicitly handles a partial final word. mbox_free_channel() invokes ->shutdown() before chan->cl is cleared, and axiado_mbox_shutdown() calls disable_irq(), which waits for the in-flight threaded handler to complete, so no functional change is needed for that path. Verified: builds clean with the aarch64 cross-compiler, passes checkpatch.pl, and was regression-tested with live mailbox traffic on AX3005. Fixes: 14af7a96afa3 ("mailbox: add Axiado AX3005 mailbox driver") Signed-off-by: Swark Yang Signed-off-by: Jassi Brar --- drivers/mailbox/axiado-mailbox.c | 114 ++++++++++++++++++++++++------- 1 file changed, 89 insertions(+), 25 deletions(-) diff --git a/drivers/mailbox/axiado-mailbox.c b/drivers/mailbox/axiado-mailbox.c index 3a3cf7421863..aa1dcc1b267d 100644 --- a/drivers/mailbox/axiado-mailbox.c +++ b/drivers/mailbox/axiado-mailbox.c @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -36,6 +37,12 @@ #define AXIADO_MBOX_CSR_ERRORS (AXIADO_MBOX_CSR_OVERFLOW | \ AXIADO_MBOX_CSR_UNDERFLOW) +enum axiado_mbox_frame_result { + AXIADO_MBOX_FRAME_EMPTY, + AXIADO_MBOX_FRAME_OK, + AXIADO_MBOX_FRAME_BAD, /* malformed/incomplete; FIFO flushed */ +}; + struct axiado_mbox_data { u8 num_chans; u16 msg_size; @@ -116,6 +123,13 @@ static int axiado_mbox_send_data(struct mbox_chan *chan, void *data) return -EBUSY; } + /* + * Per-word get_unaligned_le32()/writel(), not writesl(): @data is + * supplied by whatever client calls mbox_send_message(), and + * mbox_chan_ops.send_data(void *data) makes no alignment guarantee + * for that buffer. writesl() dereferences it as a native u32 *, + * which is unsafe unless alignment is known. + */ for (offset = 0; offset + sizeof(u32) <= msg_len; offset += sizeof(u32)) writel(get_unaligned_le32(buf + offset), priv->mbox_reg); @@ -131,31 +145,42 @@ static int axiado_mbox_send_data(struct mbox_chan *chan, void *data) return 0; } -static irqreturn_t axiado_rx_thread(int irq, void *dev_id) +/* + * Read and, if valid, deliver exactly one length-prefixed frame currently + * at the head of the RX FIFO. Does not loop -- the caller decides whether + * to call this again. May clear a pending FIFO error at several points + * below, wherever a new one could plausibly have appeared since the last + * check; each check is a cheap readl() when nothing is pending. + */ +static enum axiado_mbox_frame_result axiado_mbox_receive_one_frame(struct axiado_channel_data *priv, + bool *serviced) { - struct axiado_channel_data *priv = dev_id; struct mbox_chan *chan = priv->chan; struct axiado_mbox *mb = dev_get_drvdata(chan->mbox->dev); u8 *buf = priv->rx_buffer; unsigned int num_words; unsigned int remaining; - unsigned int i; u32 msg_len; u32 word; u32 csr; int ret; /* - * Clear and log/count any pending errors, but don't discard data - * on their account alone: a rejected overflow write doesn't - * corrupt what was already safely queued ahead of it, so let the - * length-based read below decide whether what's here is usable. + * Don't discard data on a cleared error's account alone: a rejected + * overflow write doesn't corrupt what was already safely queued + * ahead of it, so let the length-based read below decide whether + * what's here is usable. Count clearing a real error as @serviced + * so the handler reports IRQ_HANDLED whenever it actually handled + * a hardware condition. */ - axiado_mbox_clear_fifo_errors(priv); + if (axiado_mbox_clear_fifo_errors(priv)) + *serviced = true; csr = readl(priv->csr_reg); if (csr & AXIADO_MBOX_CSR_EMPTY) - return IRQ_NONE; + return AXIADO_MBOX_FRAME_EMPTY; + + *serviced = true; /* * The first word contains the total message length in bytes, @@ -165,8 +190,12 @@ static irqreturn_t axiado_rx_thread(int irq, void *dev_id) msg_len = word; put_unaligned_le32(word, buf); - if (msg_len < sizeof(u32) || msg_len > mb->drv_data->msg_size) - goto invalid_message; + if (msg_len < sizeof(u32) || msg_len > mb->drv_data->msg_size) { + dev_warn_ratelimited(chan->mbox->dev, + "Channel %u invalid frame length %u (max %u)\n", + priv->channel_num, msg_len, mb->drv_data->msg_size); + goto bad_frame; + } num_words = DIV_ROUND_UP(msg_len, sizeof(u32)); remaining = num_words - 1; @@ -182,33 +211,68 @@ static irqreturn_t axiado_rx_thread(int irq, void *dev_id) remaining, AXIADO_MBOX_RX_POLL_US, AXIADO_MBOX_RX_TIMEOUT_US); - if (ret) - goto incomplete_message; + /* + * The wait condition above is satisfied by EITHER the + * expected word count landing OR a FIFO error appearing. + * ret == 0 only means "didn't time out" - it does not mean + * the words are actually there. Check @csr for the error + * bit explicitly; otherwise an underflow can present as + * "success" and the read loop below pulls words that were + * never pushed, handing the client garbage. + */ + if (ret || (csr & AXIADO_MBOX_CSR_ERRORS)) { + dev_warn_ratelimited(chan->mbox->dev, + "Channel %u incomplete frame: got %lu/%u words (%s)\n", + priv->channel_num, + FIELD_GET(AXIADO_MBOX_CSR_LEVEL, csr), + num_words, + (csr & AXIADO_MBOX_CSR_ERRORS) ? + "FIFO error" : "timed out"); + goto bad_frame; + } axiado_mbox_clear_fifo_errors(priv); } - for (i = 1; i < num_words; i++) { - word = readl(priv->mbox_reg); - put_unaligned_le32(word, buf + i * sizeof(u32)); - } + /* + * readsl() streams the payload words directly into rx_buffer, which + * is allocated by this driver via devm_kzalloc() and therefore has + * suitable alignment for a stream accessor. + */ + readsl(priv->mbox_reg, buf + sizeof(u32), remaining); axiado_mbox_clear_fifo_errors(priv); if (READ_ONCE(priv->active)) mbox_chan_received_data(chan, priv->rx_buffer); - return IRQ_HANDLED; + return AXIADO_MBOX_FRAME_OK; -incomplete_message: - dev_warn_ratelimited(chan->mbox->dev, - "Channel %u received an incomplete message\n", - priv->channel_num); - -invalid_message: +bad_frame: + axiado_mbox_clear_fifo_errors(priv); writel(AXIADO_MBOX_CSR_FLUSH, priv->csr_reg); - return IRQ_HANDLED; + return AXIADO_MBOX_FRAME_BAD; +} + +static irqreturn_t axiado_rx_thread(int irq, void *dev_id) +{ + struct axiado_channel_data *priv = dev_id; + enum axiado_mbox_frame_result result; + bool handled = false; + + /* + * Drain all frames currently queued in the FIFO before returning. + * Multiple frames may accumulate while the threaded handler is + * running. cond_resched() prevents a continuously-fed channel from + * monopolizing the CPU. + */ + do { + result = axiado_mbox_receive_one_frame(priv, &handled); + cond_resched(); + } while (result == AXIADO_MBOX_FRAME_OK); + + return handled ? IRQ_HANDLED : IRQ_NONE; } static int axiado_mbox_startup(struct mbox_chan *chan)