mailbox: Make mbox_send_message() return error code when tx fails
Subsystems (2)
PATCH RESEND 2/2 mailbox: Make mbox_send_message() return error code when tx fails
Previously, 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.
--- drivers/mailbox/mailbox.c | 17 +++++++++++++---- include/linux/mailbox_controller.h | 2 ++ 2 files changed, 15 insertions(+), 4 deletions(-)
drivers/mailbox/mailbox.c +13 -4
@@ -23,7 +23,8 @@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;
@@ -36,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 )
@@ -87,12 +89,14 @@ static void tx_tick(struct mbox_chan *chan, int r)int idx;void *mssg =NULL ;struct completion *tx_complete =NULL ; +int *tx_status =NULL ; scoped_guard(spinlock_irqsave, &chan->lock) { idx = chan->active_req;if (idx >=0 ) { mssg = chan->msg_data[idx].data; tx_complete = chan->msg_data[idx].tx_complete; + tx_status = chan->msg_data[idx].tx_status; chan->active_req =-1 ; } }
@@ -107,8 +111,10 @@ static void tx_tick(struct mbox_chan *chan, int r)if (chan->cl->tx_done) chan->cl->tx_done (chan->cl, mssg, r); -if (r != -ETIME && chan->cl->tx_block) +if (r != -ETIME && chan->cl->tx_block) { + *tx_status = r;complete (tx_complete); + } }static enum hrtimer_restart txdone_hrtimer (struct hrtimer *hrtimer)
@@ -253,15 +259,16 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) {int t;struct completion tx_complete; +int tx_status =0 ;if (!chan || !chan->cl)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 ) {
@@ -284,6 +291,8 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)if (ret ==0 ) { t = -ETIME ;tx_tick (chan, t); + }else if (tx_status <0 ) { + t = tx_status; } }
include/linux/mailbox_controller.h +2 -0
@@ -109,10 +109,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; };/**
Signature · 1 line
2.52.0.239.gd5f0c6e74e-goog
Jassi Brar
maintainerQuoted 7 lines · Joonwon Kang
Previously, 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 beforembox_send_message()returns.
A client submitted the message, and that client gets the actual status. mbox_send_message does not (can not) tell if the message was successfully sent or not. For example, consider non-blocking mode when mbox_send_message() immediately returns after simply placing the message in the fifo. It returns 0, but still the message transmission may fail when its turn comes. So I think it is fine as is. Thanks,
Jassi
Re: [PATCH 2/2 RESEND] mailbox: Make mbox_send_message() return error
Quoted 6 lines
Quoted 6 lines
Previously, 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 beforembox_send_message()returns.
Quoted 6 lines
A client submitted the message, and that client gets the actual status.
mbox_send_messagedoes not (can not) tell if the message was successfully sent or not. For example, consider non-blocking mode whenmbox_send_message()immediately returns after simply placing the message in the fifo. It returns 0, but still the message transmission may fail when its turn comes. So I think it is fine as is.
When it comes to non-blocking send function, the common expectation is that users of the function do not expect the final send result in its return value of the non-blocking function. And it makes sense for users to register a callback to collect the final send result. It is a common practice for non- blocking function.
For blocking send function, however, it will be quite unexpected behavior if the blocking send function returns no error code but it actually has failed. Also, it will be uncommon if users of the blocking send function have to register additional callback just to collect the final send result. If that is the case, the blocking function requires of users both blocking and callback, which is redundant.
Overall, I think it will be better if we return error code in blocking mode if it has actually failed transmiting the request. How do you think?