Re: [PATCH] irqchip/qcom-mpm: Fix missing mailbox TX done acknowledgment

[Doug Anderson]

Hi,

On Tue, Mar 10, 2026 at 4:48 PM Jassi Brar <jassisinghbrar@xxxxxxxxx> wrote:
>
> On Tue, Mar 10, 2026 at 10:22 AM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > On Mon, Mar 9, 2026 at 7:23 PM <jassisinghbrar@xxxxxxxxx> wrote:
> > >
> > > From: Jassi Brar <jassisinghbrar@xxxxxxxxx>
> > >
> > > The mbox_client for qcom-mpm sends NULL doorbell messages via
> > > mbox_send_message() but never signals TX completion.
> > > Set knows_txdone=true and call mbox_client_txdone() after a
> > > successful send, matching the pattern used by other Qualcomm
> > > mailbox clients (smp2p, smsm, qcom_aoss etc) of similar controller.
> > >
> > > Fixes: a6199bb514d8a6 "irqchip: Add Qualcomm MPM controller driver"
> > > Signed-off-by: Jassi Brar <jassisinghbrar@xxxxxxxxx>
> > > ---
> > >  drivers/irqchip/irq-qcom-mpm.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> >
> > It's up to you, but according to all the research I did w/ NULL
> > messages, the mbox_client_txdone() didn't really do anything useful in
> > this case so we don't _really_ need to add it. The fact that it
> > historically did nothing is one reason why the new
> > mbox_ring_doorbell() series explicitly documents that you need not
> > (and, ideally, should not) call txdone() for doorbells.
> >
> > Specifically, mbox_client_txdone() will just call tx_tick(). That will
> > set `chan->active_req` to NULL (it already was). It will call
> > msg_submit() which likely doesn't do anything (since we don't queue
> > NULL messages in normal situations). It will notice that `mssg` is
> > NULL so it will return before calling tx_done() or signalling the
> > completion.
> >
> > If we make this change, then I'll need to spin my mbox_ring_doorbell()
> > series to delete the code. That's OK with me if that's what you want
> > to do, but I don't see a lot of benefit.
> >
> This is the only driver that doesn't "do the right thing" by missing
> mbox_client_txdone() while being one.
> I came across it while looking into if/how we can make
> mbox_send_message(NULL) work.
> Our root problem is active_req uses NULL as an 'idle' marker (early
> days when doorbell
> clients weren't known). If active_req used some other marker, NULL
> messages would work like any
> other message without a new api, even in blocking mode with optional
> tx-callbacks respected.
>
> For example, zynqmp-ipi-mailbox.c has 'txdone_poll = true' so
> zynqmp_power.c can not use this doorbell api -
> it needs to block on hrtimer based polling (which never happens when
> message is NULL). Such clients still need a solution.
>
> One option is to use the sentinel value ((void*) -1) internally.  The
> only catch is then it will no longer be
> a legitimate message, though I don't see any client using it (for
> example as a bitmask). I personally think that is a
> good 'tradeoff' for fixing the existing api without causing churn. I
> would appreciate your take on the
>  RFC  https://lkml.org/lkml/2026/3/10/2378

Hi! Yeah, I already responded there. Let's keep the discussion in
response to that patch to avoid fragmenting the discussion. I'll also
post a pointer from my cover letter in case any mailbox client driver
folks want to post their opinions.

-Doug