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

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

[jassisinghbrar]

From: Jassi Brar <jassisinghbrar@gmail.com>

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@gmail.com>
---
 drivers/irqchip/irq-qcom-mpm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/irqchip/irq-qcom-mpm.c b/drivers/irqchip/irq-qcom-mpm.c
index 83f31ea657b7..181320528a47 100644
--- a/drivers/irqchip/irq-qcom-mpm.c
+++ b/drivers/irqchip/irq-qcom-mpm.c
@@ -306,6 +306,8 @@ static int mpm_pd_power_off(struct generic_pm_domain *genpd)
 	if (ret < 0)
 		return ret;
 
+	mbox_client_txdone(priv->mbox_chan, 0);
+
 	return 0;
 }
 
@@ -434,6 +436,7 @@ static int qcom_mpm_probe(struct platform_device *pdev, struct device_node *pare
 	}
 
 	priv->mbox_client.dev = dev;
+	priv->mbox_client.knows_txdone = true;
 	priv->mbox_chan = mbox_request_channel(&priv->mbox_client, 0);
 	if (IS_ERR(priv->mbox_chan)) {
 		ret = PTR_ERR(priv->mbox_chan);
-- 
2.43.0

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

[Doug Anderson]

Hi,

On Mon, Mar 9, 2026 at 7:23 PM <jassisinghbrar@gmail.com> wrote:
>
> From: Jassi Brar <jassisinghbrar@gmail.com>
>
> 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@gmail.com>
> ---
>  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.

-Doug

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

[Jassi Brar]

On Tue, Mar 10, 2026 at 10:22 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Mar 9, 2026 at 7:23 PM <jassisinghbrar@gmail.com> wrote:
> >
> > From: Jassi Brar <jassisinghbrar@gmail.com>
> >
> > 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@gmail.com>
> > ---
> >  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

Regards
Jassi

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@gmail.com> wrote:
>
> On Tue, Mar 10, 2026 at 10:22 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Mon, Mar 9, 2026 at 7:23 PM <jassisinghbrar@gmail.com> wrote:
> > >
> > > From: Jassi Brar <jassisinghbrar@gmail.com>
> > >
> > > 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@gmail.com>
> > > ---
> > >  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

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

[Doug Anderson]

Hi,

On Mon, Mar 9, 2026 at 7:23 PM <jassisinghbrar@gmail.com> wrote:
>
> From: Jassi Brar <jassisinghbrar@gmail.com>
>
> 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@gmail.com>
> ---
>  drivers/irqchip/irq-qcom-mpm.c | 3 +++
>  1 file changed, 3 insertions(+)

OK, so it sounds like the consensus [0] is that we're _not_ going with
my mbox_ring_doorbell() approach [1] and we're instead going with some
sort of patch that makes NULL messages just like all other messages.
Maybe something based on Jassi's patch [2], or maybe something based
on Joonwon's patch [3].

Given that, then I think we'll need ${SUBJECT} patch. Thus:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

get_maintainer says that this driver goes through Thomas Gleinxer's
tree, but I didn't see him CCed. I added him to the "To" list. Seems
like he'd either need to take it, or Ack it to go through the mailbox
tree?

[0] https://lore.kernel.org/r/20260310234616.334498-1-jassisinghbrar@gmail.com
[1] https://lore.kernel.org/r/20260208040240.1971442-1-dianders@chromium.org
[2] https://lore.kernel.org/r/20260310234616.334498-1-jassisinghbrar@gmail.com
[3] https://lore.kernel.org/r/20251126045926.2413532-1-joonwonkang@google.com/