Skip to contentSkip to conversation

mailbox: Use per-thread completion to fix wrong completion order

PATCH Superseded Joonwon Kang 2 participants 4 messages Jun 18, 2025 – Jul 22, 2025

Subsystems (2)
Tegra architecture support Mailbox API

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Joonwon Kang

author
#0 lore raw

PATCH 1/2 mailbox: Use per-thread completion to fix wrong completion order

Previously, a sender thread in mbox_send_message() could be woken up at a wrong time in blocking mode. It is because there was only a single completion for a channel whereas messages from multiple threads could be sent in any order; since the shared completion could be signalled in any order, it could wake up a wrong sender thread.

This commit resolves the false wake-up issue with the following changes:
- Completions are created as many as the number of concurrent sender threads
- A completion is created in a sender thread's stack
- Each slot of the message queue, i.e. msg_data, contains a pointer to its target completion
- tx_tick() signals the completion of the currently active slot of the message queue

Link https://lore.kernel.org/all/[email protected] open here
Signed-off-by Joonwon Kang <[email protected]>
---
 drivers/mailbox/mailbox.c          | 43 +++++++++++++++++++-----------
 drivers/mailbox/tegra-hsp.c        |  2 +-
 include/linux/mailbox_controller.h | 20 +++++++++-----
 3 files changed, 43 insertions(+), 22 deletions(-)
drivers/mailbox/mailbox.c +28 -15
index 5cd8ae222073..80cd310964a8 100644
@@ -22,7 +22,7 @@
 static LIST_HEAD(mbox_cons);
 static DEFINE_MUTEX(con_mutex);
 
-static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
+static int add_to_rbuf(struct mbox_chan *chan, void *mssg, struct completion *tx_complete)
 {
 	int idx;
 

@@ -33,7 +33,8 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
 		return -ENOBUFS;
 
 	idx = chan->msg_free;
-	chan->msg_data[idx] = mssg;
+	chan->msg_data[idx].data = mssg;
+	chan->msg_data[idx].tx_complete = tx_complete;
 	chan->msg_count++;
 
 	if (idx == MBOX_TX_QUEUE_LEN - 1)

@@ -51,7 +52,7 @@ static void msg_submit(struct mbox_chan *chan)
 	int err = -EBUSY;
 
 	scoped_guard(spinlock_irqsave, &chan->lock) {
-		if (!chan->msg_count || chan->active_req)
+		if (!chan->msg_count || chan->active_req >= 0)
 			break;
 
 		count = chan->msg_count;

@@ -61,14 +62,14 @@ static void msg_submit(struct mbox_chan *chan)
 		else
 			idx += MBOX_TX_QUEUE_LEN - count;
 
-		data = chan->msg_data[idx];
+		data = chan->msg_data[idx].data;
 
 		if (chan->cl->tx_prepare)
 			chan->cl->tx_prepare(chan->cl, data);
 		/* Try to submit a message to the MBOX controller */
 		err = chan->mbox->ops->send_data(chan, data);
 		if (!err) {
-			chan->active_req = data;
+			chan->active_req = idx;
 			chan->msg_count--;
 		}
 	}

@@ -82,11 +83,17 @@ static void msg_submit(struct mbox_chan *chan)
 
 static void tx_tick(struct mbox_chan *chan, int r)
 {
-	void *mssg;
+	int idx;
+	void *mssg = NULL;
+	struct completion *tx_complete = NULL;
 
 	scoped_guard(spinlock_irqsave, &chan->lock) {
-		mssg = chan->active_req;
-		chan->active_req = NULL;
+		idx = chan->active_req;
+		if (idx >= 0) {
+			mssg = chan->msg_data[idx].data;
+			tx_complete = chan->msg_data[idx].tx_complete;
+			chan->active_req = -1;
+		}
 	}
 
 	/* Submit next message */

@@ -100,7 +107,7 @@ static void tx_tick(struct mbox_chan *chan, int r)
 		chan->cl->tx_done(chan->cl, mssg, r);
 
 	if (r != -ETIME && chan->cl->tx_block)
-		complete(&chan->tx_complete);
+		complete(tx_complete);
 }
 
 static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)

@@ -113,7 +120,7 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
 	for (i = 0; i < mbox->num_chans; i++) {
 		struct mbox_chan *chan = &mbox->chans[i];
 
-		if (chan->active_req && chan->cl) {
+		if (chan->active_req >= 0 && chan->cl) {
 			txdone = chan->mbox->ops->last_tx_done(chan);
 			if (txdone)
 				tx_tick(chan, 0);

@@ -244,11 +251,18 @@ EXPORT_SYMBOL_GPL(mbox_client_peek_data);
 int mbox_send_message(struct mbox_chan *chan, void *mssg)
 {
 	int t;
+	struct completion tx_complete;
 
 	if (!chan || !chan->cl)
 		return -EINVAL;
 
-	t = add_to_rbuf(chan, mssg);
+	if (chan->cl->tx_block) {
+		init_completion(&tx_complete);
+		t = add_to_rbuf(chan, mssg, &tx_complete);
+	} else {
+		t = add_to_rbuf(chan, mssg, NULL);
+	}
+
 	if (t < 0) {
 		dev_err(chan->mbox->dev, "Try increasing MBOX_TX_QUEUE_LEN\n");
 		return t;

@@ -265,7 +279,7 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
 		else
 			wait = msecs_to_jiffies(chan->cl->tx_tout);
 
-		ret = wait_for_completion_timeout(&chan->tx_complete, wait);
+		ret = wait_for_completion_timeout(&tx_complete, wait);
 		if (ret == 0) {
 			t = -ETIME;
 			tx_tick(chan, t);

@@ -318,9 +332,8 @@ static int __mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl)
 	scoped_guard(spinlock_irqsave, &chan->lock) {
 		chan->msg_free = 0;
 		chan->msg_count = 0;
-		chan->active_req = NULL;
+		chan->active_req = -1;
 		chan->cl = cl;
-		init_completion(&chan->tx_complete);
 
 		if (chan->txdone_method	== TXDONE_BY_POLL && cl->knows_txdone)
 			chan->txdone_method = TXDONE_BY_ACK;

@@ -461,7 +474,7 @@ void mbox_free_channel(struct mbox_chan *chan)
 	/* The queued TX requests are simply aborted, no callbacks are made */
 	scoped_guard(spinlock_irqsave, &chan->lock) {
 		chan->cl = NULL;
-		chan->active_req = NULL;
+		chan->active_req = -1;
 		if (chan->txdone_method == TXDONE_BY_ACK)
 			chan->txdone_method = TXDONE_BY_POLL;
 	}
drivers/mailbox/tegra-hsp.c +1 -1
index ed9a0bb2bcd8..de7494ce0a9f 100644
@@ -497,7 +497,7 @@ static int tegra_hsp_mailbox_flush(struct mbox_chan *chan,
 			mbox_chan_txdone(chan, 0);
 
 			/* Wait until channel is empty */
-			if (chan->active_req != NULL)
+			if (chan->active_req >= 0)
 				continue;
 
 			return 0;
include/linux/mailbox_controller.h +14 -6
index ad01c4082358..ae29fba3bc46 100644
@@ -102,16 +102,25 @@ struct mbox_controller {
  */
 #define MBOX_TX_QUEUE_LEN	20
 
+/**
+ * struct mbox_message - Internal representation of a mailbox message
+ * @data:		Data packet
+ * @tx_complete:	Pointer to the transmission completion
+ */
+struct mbox_message {
+	void *data;
+	struct completion *tx_complete;
+};
+
 /**
  * struct mbox_chan - s/w representation of a communication chan
  * @mbox:		Pointer to the parent/provider of this channel
  * @txdone_method:	Way to detect TXDone chosen by the API
  * @cl:			Pointer to the current owner of this channel
- * @tx_complete:	Transmission completion
- * @active_req:		Currently active request hook
+ * @active_req:		Index of the currently active slot in the queue
  * @msg_count:		No. of mssg currently queued
  * @msg_free:		Index of next available mssg slot
- * @msg_data:		Hook for data packet
+ * @msg_data:		Queue of data packets
  * @lock:		Serialise access to the channel
  * @con_priv:		Hook for controller driver to attach private data
  */

@@ -119,10 +128,9 @@ struct mbox_chan {
 	struct mbox_controller *mbox;
 	unsigned txdone_method;
 	struct mbox_client *cl;
-	struct completion tx_complete;
-	void *active_req;
+	int active_req;
 	unsigned msg_count, msg_free;
-	void *msg_data[MBOX_TX_QUEUE_LEN];
+	struct mbox_message msg_data[MBOX_TX_QUEUE_LEN];
 	spinlock_t lock; /* Serialise access to the channel */
 	void *con_priv;
 };
Signature · 1 line
2.50.0.rc2.696.g1fc2a0284f-goog
maintainer
#1 lore raw
Quoted 16 lines · Joonwon Kang

Previously, a sender thread in mbox_send_message() could be woken up at a wrong time in blocking mode. It is because there was only a single completion for a channel whereas messages from multiple threads could be sent in any order; since the shared completion could be signalled in any order, it could wake up a wrong sender thread.

This commit resolves the false wake-up issue with the following changes:
- Completions are created as many as the number of concurrent sender threads
- A completion is created in a sender thread's stack
- Each slot of the message queue, i.e. msg_data, contains a pointer to its target completion
- tx_tick() signals the completion of the currently active slot of the message queue

Quoted 1 line
Link https://lore.kernel.org/all/[email protected] open here

Is your issue different from what is described in the Link?

thanks

author
#2 lore raw

Quoted 16 lines · On Wed, Jun 18, 2025 at 3:04=E2=80=AFAM Joonwon Kang <[email protected]=…
Quoted 16 lines

Previously, a sender thread in mbox_send_message() could be woken up at a wrong time in blocking mode. It is because there was only a single completion for a channel whereas messages from multiple threads could be sent in any order; since the shared completion could be signalled in any order, it could wake up a wrong sender thread.

This commit resolves the false wake-up issue with the following changes:
- Completions are created as many as the number of concurrent sender threads
- A completion is created in a sender thread's stack
- Each slot of the message queue, i.e. msg_data, contains a pointer to its target completion
- tx_tick() signals the completion of the currently active slot of the message queue

Quoted 6 lines
Quoted 1 line
Link https://lore.kernel.org/all/1490809381-28869-1-git-send-email-jaswi=

[email protected]

Is your issue different from what is described in the Link?

thanks

The issue is the same, but the solution is different.

The previous solution in the Link tried to have per-message completion; each completion belongs to each slot of the message queue. However, the solution in this patch tries to have per-thread completion; each completion belongs to each sender thread and each slot of the message queue has a pointer to that completion; struct mbox_message has the "pointer" field
struct completion *tx_complete which points to the completion which is created in the stack of the sender instead of owning the completion by having the non-pointer field struct completion tx_complete. This way, we could avoid the window where the same completion is reused by different sender threads which could lead to a wrong completion order, IMHO.

author
#3 lore raw
In reply to Jassi Brar
Quoted 16 lines · Joonwon Kang
Quoted 16 lines

Previously, a sender thread in mbox_send_message() could be woken up at a wrong time in blocking mode. It is because there was only a single completion for a channel whereas messages from multiple threads could be sent in any order; since the shared completion could be signalled in any order, it could wake up a wrong sender thread.

This commit resolves the false wake-up issue with the following changes:
- Completions are created as many as the number of concurrent sender threads
- A completion is created in a sender thread's stack
- Each slot of the message queue, i.e. msg_data, contains a pointer to its target completion
- tx_tick() signals the completion of the currently active slot of the message queue

Quoted 5 lines
Quoted 1 line
Link https://lore.kernel.org/all/[email protected] open here

Is your issue different from what is described in the Link?

thanks

The issue is the same, but the solution is different.

The previous solution in the Link tried to have per-message completion; each completion belongs to each slot of the message queue. However, the solution in this patch tries to have per-thread completion; each completion belongs to each sender thread and each slot of the message queue has a pointer to that completion; struct mbox_message has the "pointer" field
struct completion *tx_complete which points to the completion which is created in the stack of the sender instead of owning the completion by having the non-pointer field struct completion tx_complete. This way, we could avoid the window where the same completion is reused by different sender threads which could lead to a wrong completion order, IMHO.