Re: [PATCH v3 2/2] mailbox: Make mbox_send_message() return error code when tx fails

[Joonwon Kang]

> On Thu, Apr 2, 2026 at 12:07 PM Joonwon Kang <joonwonkang@xxxxxxxxxx> wrote:
> >
> > When the mailbox controller failed transmitting message, the error code
> > was only passed to the client's tx done handler and not to
> > mbox_send_message(). For this reason, the function could return a false
> > success. This commit resolves the issue by introducing the tx status and
> > checking it before mbox_send_message() returns.
> >
> Can you please share the scenario when this becomes necessary? This
> can potentially change the ground underneath some clients, so we have
> to be sure this is really useful.

I would say the problem here is generic enough to apply to all the cases where
the send result needs to be checked. Since the return value of the send API is
not the real send result, any users who believe that this blocking send API
will return the real send result could fall for that. For example, users may
think the send was successful even though it was not actually. I believe it is
uncommon that users have to register a callback solely to get the send result
even though they are using the blocking send API already. Also, I guess there
is no special reason why only the mailbox send API should work this way among
other typical blocking send APIs. For these reasons, this patch makes the send
API return the real send result. This way, users will not need to register the
redundant callback and I think the return value will align with their common
expectation.

Regarding the change in the ground for some clients, could you help to clarify
a bit more on what change, you expect, would surprise the clients?

Thanks,
Joonwon Kang

> 
> Thanks
> Jassi
> 
> 
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Joonwon Kang <joonwonkang@xxxxxxxxxx>
> > ---
> >  drivers/mailbox/mailbox.c          | 20 +++++++++++++++-----
> >  include/linux/mailbox_controller.h |  2 ++
> >  2 files changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> > index d63386468982..ea9aec9dc947 100644
> > --- a/drivers/mailbox/mailbox.c
> > +++ b/drivers/mailbox/mailbox.c
> > @@ -21,7 +21,10 @@
> >  static LIST_HEAD(mbox_cons);
> >  static DEFINE_MUTEX(con_mutex);
> >
> > -static int add_to_rbuf(struct mbox_chan *chan, void *mssg, struct completion *tx_complete)
> > +static int add_to_rbuf(struct mbox_chan *chan,
> > +                      void *mssg,
> > +                      struct completion *tx_complete,
> > +                      int *tx_status)
> >  {
> >         int idx;
> >
> > @@ -34,6 +37,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg, struct completion *tx
> >         idx = chan->msg_free;
> >         chan->msg_data[idx].data = mssg;
> >         chan->msg_data[idx].tx_complete = tx_complete;
> > +       chan->msg_data[idx].tx_status = tx_status;
> >         chan->msg_count++;
> >
> >         if (idx == MBOX_TX_QUEUE_LEN - 1)
> > @@ -91,12 +95,13 @@ static void msg_submit(struct mbox_chan *chan)
> >
> >  static void tx_tick(struct mbox_chan *chan, int r, int idx)
> >  {
> > -       struct mbox_message mssg = {MBOX_NO_MSG, NULL};
> > +       struct mbox_message mssg = {MBOX_NO_MSG, NULL, NULL};
> >
> >         scoped_guard(spinlock_irqsave, &chan->lock) {
> >                 if (idx >= 0 && idx != chan->active_req) {
> >                         chan->msg_data[idx].data = MBOX_NO_MSG;
> >                         chan->msg_data[idx].tx_complete = NULL;
> > +                       chan->msg_data[idx].tx_status = NULL;
> >                         return;
> >                 }
> >
> > @@ -116,8 +121,10 @@ static void tx_tick(struct mbox_chan *chan, int r, int idx)
> >         if (chan->cl->tx_done)
> >                 chan->cl->tx_done(chan->cl, mssg.data, r);
> >
> > -       if (r != -ETIME && chan->cl->tx_block)
> > +       if (r != -ETIME && chan->cl->tx_block) {
> > +               *mssg.tx_status = r;
> >                 complete(mssg.tx_complete);
> > +       }
> >  }
> >
> >  static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
> > @@ -286,15 +293,16 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
> >         int t;
> >         int idx;
> >         struct completion tx_complete;
> > +       int tx_status = 0;
> >
> >         if (!chan || !chan->cl || mssg == MBOX_NO_MSG)
> >                 return -EINVAL;
> >
> >         if (chan->cl->tx_block) {
> >                 init_completion(&tx_complete);
> > -               t = add_to_rbuf(chan, mssg, &tx_complete);
> > +               t = add_to_rbuf(chan, mssg, &tx_complete, &tx_status);
> >         } else {
> > -               t = add_to_rbuf(chan, mssg, NULL);
> > +               t = add_to_rbuf(chan, mssg, NULL, NULL);
> >         }
> >
> >         if (t < 0) {
> > @@ -318,6 +326,8 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
> >                         idx = t;
> >                         t = -ETIME;
> >                         tx_tick(chan, t, idx);
> > +               } else if (tx_status < 0) {
> > +                       t = tx_status;
> >                 }
> >         }
> >
> > diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
> > index 912499ad08ed..890da97bcb50 100644
> > --- a/include/linux/mailbox_controller.h
> > +++ b/include/linux/mailbox_controller.h
> > @@ -117,10 +117,12 @@ struct mbox_controller {
> >   * struct mbox_message - Internal representation of a mailbox message
> >   * @data:              Data packet
> >   * @tx_complete:       Pointer to the transmission completion
> > + * @tx_status:         Pointer to the transmission status
> >   */
> >  struct mbox_message {
> >         void *data;
> >         struct completion *tx_complete;
> > +       int *tx_status;
> >  };
> >
> >  /**
> > --
> > 2.53.0.1185.g05d4b7b318-goog
> >