Re: [PATCH] mailbox: Fix NULL message support in mbox_send_message()

[Joonwon Kang <joonwonkang@google.com>]

> On Mon, Mar 23, 2026 at 12:14 AM Joonwon Kang <joonwonkang@google.com> wrote:
> >
> > > The active_req field serves double duty as both the "is a TX in
> > > flight" flag (NULL means idle) and the storage for the in-flight
> > > message pointer. When a client sends NULL via mbox_send_message(),
> > > active_req is set to NULL, which the framework misinterprets as
> > > "no active request". This breaks the TX state machine by:
> > >
> > >  - tx_tick() short-circuits on (!mssg), skipping the tx_done
> > >    callback and the tx_complete completion
> > >  - txdone_hrtimer() skips the channel entirely since active_req
> > >    is NULL, so poll-based TX-done detection never fires.
> > >
> > > Fix this by introducing a MBOX_NO_MSG sentinel value that means
> > > "no active request," freeing NULL to be valid message data. The
> > > sentinel is defined in the subsystem-internal mailbox.h so that
> > > controller drivers within drivers/mailbox/ can reference it, but
> > > it is not exposed to clients outside the subsystem.
> >
> > It sounds that it allows future controller drivers also to refer to the
> > new sentinel pointer value.
> >
> Sentinel value is not the problem, active_req should have been hidden
> from controllers. Which is actually respected by all controllers
> except the tegra-hsp.c
> 
> > >
> > > Fifteen in-tree callers send NULL (doorbell-style IPCs on Qualcomm,
> > > Tegra, TI, Xilinx, i.MX, SCMI, and PCC platforms). All were
> > > audited for regression:
> > >
> > >  - Most already work around the bug via knows_txdone=true with a
> > >    manual mbox_client_txdone() call, making the framework's
> > >    tracking irrelevant. These are unaffected.
> > >
> > >  - Poll-based callers (Xilinx zynqmp/r5) are strictly better off:
> > >    the poll timer now correctly detects NULL-active channels
> > >    instead of silently skipping them.
> > >
> > >  - irq-qcom-mpm.c was a pre-existing bug -- the only Qualcomm
> > >    caller that omitted the knows_txdone + mbox_client_txdone()
> > >    pattern. Fixed in a companion commit ("irqchip/qcom-mpm: Fix
> > >    missing mailbox TX done acknowledgment").
> > >
> > >  - No caller sets both a tx_done callback and sends NULL, nor
> > >    combines tx_block=true with NULL sends, so the newly reachable
> > >    callback/completion paths are never exercised.
> > >
> > > Also update tegra-hsp's flush callback, which directly inspects
> > > active_req to wait for the channel to drain: the old "!= NULL"
> > > check becomes "!= MBOX_NO_MSG", otherwise flush spins until
> > > timeout since the sentinel is non-NULL.
> > >
> > > The only tradeoff is that 'MBOX_NO_MSG' can not be used as a message
> > > by clients.
> >
> > The other, but I guess more important, tradeoff is that future controller
> > driver developers should now know that the pointer value of `->active->req`
> > could be -1(== MBOX_NO_MSG) other than conventional pointer value(memory
> > address, NULL, or error-encoded pointer value).
> >
> That should not be a concern. Controller drivers shouldn't peek into
> mailbox internals

Thanks for this clarification on your intention. This resolves the afore-
mentioned concerns.

> and if they do they will know the sentinel value
> being used.
> For example, of the ~40 drivers, only tegra-hsp.c chose to (not had
> to) use active_req and it relied on the sentinel value, which will now
> be MBOX_NO_MSG.
> 
> Thanks
> Jassi

Thanks,
Joonwon Kang