Sometimes clients need to know if mailbox queue is full or not before
posting new message via mailbox. If mailbox queue is full clients can
choose not to post new message. This doesn't mean current queue length
should be increased, but clients may want to wait till previous Tx is
done. Introduce variable per channel to track available msg slots.
Clients can check this variable and decide not to send new message if
it is 0. This can help avoid false positive warning from mailbox
framework "Try increasing MBOX_TX_QUEUE_LEN".
Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---
v2:
- Separate patch for remoteproc subsystem.
- Change design and introduce msg_slot_ro field for each channel
instead of API. Clients can use this variable directly.
- Remove mbox_queue_full API
drivers/mailbox/mailbox.c | 3 +++
include/linux/mailbox_controller.h | 2 ++
2 files changed, 5 insertions(+)
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 5cd8ae222073..c2e187aa5d22 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -35,6 +35,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
idx = chan->msg_free;
chan->msg_data[idx] = mssg;
chan->msg_count++;
+ chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN - chan->msg_count);
if (idx == MBOX_TX_QUEUE_LEN - 1)
chan->msg_free = 0;
@@ -70,6 +71,7 @@ static void msg_submit(struct mbox_chan *chan)
if (!err) {
chan->active_req = data;
chan->msg_count--;
+ chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN - chan->msg_count);
}
}
@@ -318,6 +320,7 @@ 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->msg_slot_ro = MBOX_TX_QUEUE_LEN;
chan->active_req = NULL;
chan->cl = cl;
init_completion(&chan->tx_complete);
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index ad01c4082358..514f10fbcd42 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -112,6 +112,7 @@ struct mbox_controller {
* @msg_count: No. of mssg currently queued
* @msg_free: Index of next available mssg slot
* @msg_data: Hook for data packet
+ * @msg_slot_ro: remaining message slots in the queue.
* @lock: Serialise access to the channel
* @con_priv: Hook for controller driver to attach private data
*/
@@ -123,6 +124,7 @@ struct mbox_chan {
void *active_req;
unsigned msg_count, msg_free;
void *msg_data[MBOX_TX_QUEUE_LEN];
+ unsigned int msg_slot_ro;
spinlock_t lock; /* Serialise access to the channel */
void *con_priv;
};
--
2.34.1On Tue, Oct 7, 2025 at 10:19 AM Tanmay Shah <tanmay.shah@amd.com> wrote:
>
> Sometimes clients need to know if mailbox queue is full or not before
> posting new message via mailbox. If mailbox queue is full clients can
> choose not to post new message. This doesn't mean current queue length
> should be increased, but clients may want to wait till previous Tx is
> done. Introduce variable per channel to track available msg slots.
> Clients can check this variable and decide not to send new message if
> it is 0. This can help avoid false positive warning from mailbox
> framework "Try increasing MBOX_TX_QUEUE_LEN".
>
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> ---
>
> v2:
> - Separate patch for remoteproc subsystem.
> - Change design and introduce msg_slot_ro field for each channel
> instead of API. Clients can use this variable directly.
> - Remove mbox_queue_full API
>
> drivers/mailbox/mailbox.c | 3 +++
> include/linux/mailbox_controller.h | 2 ++
> 2 files changed, 5 insertions(+)
>
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 5cd8ae222073..c2e187aa5d22 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -35,6 +35,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
> idx = chan->msg_free;
> chan->msg_data[idx] = mssg;
> chan->msg_count++;
> + chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN - chan->msg_count);
>
> if (idx == MBOX_TX_QUEUE_LEN - 1)
> chan->msg_free = 0;
> @@ -70,6 +71,7 @@ static void msg_submit(struct mbox_chan *chan)
> if (!err) {
> chan->active_req = data;
> chan->msg_count--;
> + chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN - chan->msg_count);
>
No, I had suggested adding this info in client structure.
There is no point in carrying msg_count and msg_slot_ro in mbox_chan.
The client needs this info but can/should not access the chan internals.
thanks
jassi
On 10/7/25 2:58 PM, Jassi Brar wrote:
> On Tue, Oct 7, 2025 at 10:19 AM Tanmay Shah <tanmay.shah@amd.com> wrote:
>>
>> Sometimes clients need to know if mailbox queue is full or not before
>> posting new message via mailbox. If mailbox queue is full clients can
>> choose not to post new message. This doesn't mean current queue length
>> should be increased, but clients may want to wait till previous Tx is
>> done. Introduce variable per channel to track available msg slots.
>> Clients can check this variable and decide not to send new message if
>> it is 0. This can help avoid false positive warning from mailbox
>> framework "Try increasing MBOX_TX_QUEUE_LEN".
>>
>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>> ---
>>
>> v2:
>> - Separate patch for remoteproc subsystem.
>> - Change design and introduce msg_slot_ro field for each channel
>> instead of API. Clients can use this variable directly.
>> - Remove mbox_queue_full API
>>
>> drivers/mailbox/mailbox.c | 3 +++
>> include/linux/mailbox_controller.h | 2 ++
>> 2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>> index 5cd8ae222073..c2e187aa5d22 100644
>> --- a/drivers/mailbox/mailbox.c
>> +++ b/drivers/mailbox/mailbox.c
>> @@ -35,6 +35,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
>> idx = chan->msg_free;
>> chan->msg_data[idx] = mssg;
>> chan->msg_count++;
>> + chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN - chan->msg_count);
>>
>> if (idx == MBOX_TX_QUEUE_LEN - 1)
>> chan->msg_free = 0;
>> @@ -70,6 +71,7 @@ static void msg_submit(struct mbox_chan *chan)
>> if (!err) {
>> chan->active_req = data;
>> chan->msg_count--;
>> + chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN - chan->msg_count);
>>
> No, I had suggested adding this info in client structure.
> There is no point in carrying msg_count and msg_slot_ro in mbox_chan.
> The client needs this info but can/should not access the chan internals.
>
Hi Jassi,
If I move msg_slot_ro to mbox_client that means, each channel needs its
own client structure. But it is possible to map single client to
multiple channels.
How about if I rename msg_slot_ro to msg_slot_tx_ro -> that says this
field should be used only for "tx" channel.
Or is it must to map unique client data structure to each channel? If
so, can I update mbox_client structure documentation ?
Thanks,
Tanmay.
> thanks
> jassi
On 10/8/25 11:49 AM, Tanmay Shah wrote:
>
>
> On 10/7/25 2:58 PM, Jassi Brar wrote:
>> On Tue, Oct 7, 2025 at 10:19 AM Tanmay Shah <tanmay.shah@amd.com> wrote:
>>>
>>> Sometimes clients need to know if mailbox queue is full or not before
>>> posting new message via mailbox. If mailbox queue is full clients can
>>> choose not to post new message. This doesn't mean current queue length
>>> should be increased, but clients may want to wait till previous Tx is
>>> done. Introduce variable per channel to track available msg slots.
>>> Clients can check this variable and decide not to send new message if
>>> it is 0. This can help avoid false positive warning from mailbox
>>> framework "Try increasing MBOX_TX_QUEUE_LEN".
>>>
>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>> ---
>>>
>>> v2:
>>> - Separate patch for remoteproc subsystem.
>>> - Change design and introduce msg_slot_ro field for each channel
>>> instead of API. Clients can use this variable directly.
>>> - Remove mbox_queue_full API
>>>
>>> drivers/mailbox/mailbox.c | 3 +++
>>> include/linux/mailbox_controller.h | 2 ++
>>> 2 files changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>>> index 5cd8ae222073..c2e187aa5d22 100644
>>> --- a/drivers/mailbox/mailbox.c
>>> +++ b/drivers/mailbox/mailbox.c
>>> @@ -35,6 +35,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void
>>> *mssg)
>>> idx = chan->msg_free;
>>> chan->msg_data[idx] = mssg;
>>> chan->msg_count++;
>>> + chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN - chan->msg_count);
>>>
>>> if (idx == MBOX_TX_QUEUE_LEN - 1)
>>> chan->msg_free = 0;
>>> @@ -70,6 +71,7 @@ static void msg_submit(struct mbox_chan *chan)
>>> if (!err) {
>>> chan->active_req = data;
>>> chan->msg_count--;
>>> + chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN -
>>> chan->msg_count);
>>>
>> No, I had suggested adding this info in client structure.
>> There is no point in carrying msg_count and msg_slot_ro in mbox_chan.
>> The client needs this info but can/should not access the chan internals.
>>
>
> Hi Jassi,
>
> If I move msg_slot_ro to mbox_client that means, each channel needs its
> own client structure. But it is possible to map single client to
> multiple channels.
>
> How about if I rename msg_slot_ro to msg_slot_tx_ro -> that says this
> field should be used only for "tx" channel.
>
> Or is it must to map unique client data structure to each channel? If
> so, can I update mbox_client structure documentation ?
>
Hi Jassi,
I looked into this further. Looks like it's not possible to introduce
msg_slot_ro in the client data structure as of today. Because multiple
drivers are sharing same client for "tx" and "rx" both channels.
[references: 1, 2, 3]
so, msg_slot_ro won't be calculated correctly I believe.
I can change architecture for xlnx driver i.e. assign separate clients
for each channel, but still it won't work for other platforms. Please
let me know how to proceed further.
Can we provide API that does (MBOX_TX_QUEUE_LEN - chan->msg_count) == 0?
I am not sure if I should attempt to change the architecture of other
platform's drivers.
References:
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/drivers/remoteproc/imx_rproc.c?h=for-next#n768
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/drivers/remoteproc/xlnx_r5_remoteproc.c?h=for-next#n280
[3]
https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/drivers/remoteproc/st_remoteproc.c?h=for-next#n386
Thank you.
> Thanks,
> Tanmay.
>
>> thanks
>> jassi
>
On Wed, Oct 15, 2025 at 10:27 AM Tanmay Shah <tanmay.shah@amd.com> wrote:
> >>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> >>> index 5cd8ae222073..c2e187aa5d22 100644
> >>> --- a/drivers/mailbox/mailbox.c
> >>> +++ b/drivers/mailbox/mailbox.c
> >>> @@ -35,6 +35,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void
> >>> *mssg)
> >>> idx = chan->msg_free;
> >>> chan->msg_data[idx] = mssg;
> >>> chan->msg_count++;
> >>> + chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN - chan->msg_count);
> >>>
> >>> if (idx == MBOX_TX_QUEUE_LEN - 1)
> >>> chan->msg_free = 0;
> >>> @@ -70,6 +71,7 @@ static void msg_submit(struct mbox_chan *chan)
> >>> if (!err) {
> >>> chan->active_req = data;
> >>> chan->msg_count--;
> >>> + chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN -
> >>> chan->msg_count);
> >>>
> >> No, I had suggested adding this info in client structure.
> >> There is no point in carrying msg_count and msg_slot_ro in mbox_chan.
> >> The client needs this info but can/should not access the chan internals.
> >>
> >
> > Hi Jassi,
> >
> > If I move msg_slot_ro to mbox_client that means, each channel needs its
> > own client structure. But it is possible to map single client to
> > multiple channels.
> >
> > How about if I rename msg_slot_ro to msg_slot_tx_ro -> that says this
> > field should be used only for "tx" channel.
> >
> > Or is it must to map unique client data structure to each channel? If
> > so, can I update mbox_client structure documentation ?
> >
>
> Hi Jassi,
>
> I looked into this further. Looks like it's not possible to introduce
> msg_slot_ro in the client data structure as of today. Because multiple
> drivers are sharing same client for "tx" and "rx" both channels.
> [references: 1, 2, 3]
>
> so, msg_slot_ro won't be calculated correctly I believe.
>
I don't see it. Can you please explain how the calculated value could be wrong?
thnx
-jassi
On 11/15/25 11:38 AM, Jassi Brar wrote:
> On Wed, Oct 15, 2025 at 10:27 AM Tanmay Shah <tanmay.shah@amd.com> wrote:
>>>>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>>>>> index 5cd8ae222073..c2e187aa5d22 100644
>>>>> --- a/drivers/mailbox/mailbox.c
>>>>> +++ b/drivers/mailbox/mailbox.c
>>>>> @@ -35,6 +35,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void
>>>>> *mssg)
>>>>> idx = chan->msg_free;
>>>>> chan->msg_data[idx] = mssg;
>>>>> chan->msg_count++;
>>>>> + chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN - chan->msg_count);
>>>>>
>>>>> if (idx == MBOX_TX_QUEUE_LEN - 1)
>>>>> chan->msg_free = 0;
>>>>> @@ -70,6 +71,7 @@ static void msg_submit(struct mbox_chan *chan)
>>>>> if (!err) {
>>>>> chan->active_req = data;
>>>>> chan->msg_count--;
>>>>> + chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN -
>>>>> chan->msg_count);
>>>>>
>>>> No, I had suggested adding this info in client structure.
>>>> There is no point in carrying msg_count and msg_slot_ro in mbox_chan.
>>>> The client needs this info but can/should not access the chan internals.
>>>>
>>>
>>> Hi Jassi,
>>>
>>> If I move msg_slot_ro to mbox_client that means, each channel needs its
>>> own client structure. But it is possible to map single client to
>>> multiple channels.
>>>
>>> How about if I rename msg_slot_ro to msg_slot_tx_ro -> that says this
>>> field should be used only for "tx" channel.
>>>
>>> Or is it must to map unique client data structure to each channel? If
>>> so, can I update mbox_client structure documentation ?
>>>
>>
>> Hi Jassi,
>>
>> I looked into this further. Looks like it's not possible to introduce
>> msg_slot_ro in the client data structure as of today. Because multiple
>> drivers are sharing same client for "tx" and "rx" both channels.
>> [references: 1, 2, 3]
>>
>> so, msg_slot_ro won't be calculated correctly I believe.
>>
> I don't see it. Can you please explain how the calculated value could be wrong?
>
Hi Jassi,
so on my platform I introduced some extra logs:
[ 80.827479] mbox chan Rx send message
[ 80.827485] add_to_rbuf: &chan->cl->msg_slot_ro = 00000000ab5fa771,
msg slot ro = 19
[ 80.827494] msg_submit: &chan->cl->msg_slot_ro = 00000000ab5fa771,
msg slot ro = 20
[ 80.833064] mbox chan Tx send message
[ 80.833070] add_to_rbuf: &chan->cl->msg_slot_ro = 00000000ab5fa771,
msg slot ro = 19
[ 80.833079] msg_submit: &chan->cl->msg_slot_ro = 00000000ab5fa771,
msg slot ro = 20
[ 80.837486] mbox chan Rx send message
[ 80.837492] add_to_rbuf: &chan->cl->msg_slot_ro = 00000000ab5fa771,
msg slot ro = 19
[ 80.837501] msg_submit: &chan->cl->msg_slot_ro = 00000000ab5fa771,
msg slot ro = 20
Tx and Rx both channels are updating same address of msg_slot_ro. If
user wants to check msg_slot_ro for Tx channel, then it is possible that
it was updated by Rx channel instead. Ideally there should be two copies
of msg_slot_ro, one for Tx and one for Rx channel.
This happens because Tx and Rx both channels shares same client data
structure.
Same can happen on other platforms as well.
Thanks,
Tanmay
> thnx
> -jassi
On Tue, Nov 18, 2025 at 12:51 PM Tanmay Shah <tanmay.shah@amd.com> wrote:
> On 11/15/25 11:38 AM, Jassi Brar wrote:
> > On Wed, Oct 15, 2025 at 10:27 AM Tanmay Shah <tanmay.shah@amd.com> wrote:
> >>>>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> >>>>> index 5cd8ae222073..c2e187aa5d22 100644
> >>>>> --- a/drivers/mailbox/mailbox.c
> >>>>> +++ b/drivers/mailbox/mailbox.c
> >>>>> @@ -35,6 +35,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void
> >>>>> *mssg)
> >>>>> idx = chan->msg_free;
> >>>>> chan->msg_data[idx] = mssg;
> >>>>> chan->msg_count++;
> >>>>> + chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN - chan->msg_count);
> >>>>>
> >>>>> if (idx == MBOX_TX_QUEUE_LEN - 1)
> >>>>> chan->msg_free = 0;
> >>>>> @@ -70,6 +71,7 @@ static void msg_submit(struct mbox_chan *chan)
> >>>>> if (!err) {
> >>>>> chan->active_req = data;
> >>>>> chan->msg_count--;
> >>>>> + chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN -
> >>>>> chan->msg_count);
> >>>>>
> >>>> No, I had suggested adding this info in client structure.
> >>>> There is no point in carrying msg_count and msg_slot_ro in mbox_chan.
> >>>> The client needs this info but can/should not access the chan internals.
> >>>>
> >>>
> >>> Hi Jassi,
> >>>
> >>> If I move msg_slot_ro to mbox_client that means, each channel needs its
> >>> own client structure. But it is possible to map single client to
> >>> multiple channels.
> >>>
> >>> How about if I rename msg_slot_ro to msg_slot_tx_ro -> that says this
> >>> field should be used only for "tx" channel.
> >>>
> >>> Or is it must to map unique client data structure to each channel? If
> >>> so, can I update mbox_client structure documentation ?
> >>>
> >>
> >> Hi Jassi,
> >>
> >> I looked into this further. Looks like it's not possible to introduce
> >> msg_slot_ro in the client data structure as of today. Because multiple
> >> drivers are sharing same client for "tx" and "rx" both channels.
> >> [references: 1, 2, 3]
> >>
> >> so, msg_slot_ro won't be calculated correctly I believe.
> >>
> > I don't see it. Can you please explain how the calculated value could be wrong?
> >
>
> Hi Jassi,
>
> so on my platform I introduced some extra logs:
>
> [ 80.827479] mbox chan Rx send message
> [ 80.827485] add_to_rbuf: &chan->cl->msg_slot_ro = 00000000ab5fa771,
> msg slot ro = 19
> [ 80.827494] msg_submit: &chan->cl->msg_slot_ro = 00000000ab5fa771,
> msg slot ro = 20
> [ 80.833064] mbox chan Tx send message
> [ 80.833070] add_to_rbuf: &chan->cl->msg_slot_ro = 00000000ab5fa771,
> msg slot ro = 19
> [ 80.833079] msg_submit: &chan->cl->msg_slot_ro = 00000000ab5fa771,
> msg slot ro = 20
> [ 80.837486] mbox chan Rx send message
> [ 80.837492] add_to_rbuf: &chan->cl->msg_slot_ro = 00000000ab5fa771,
> msg slot ro = 19
> [ 80.837501] msg_submit: &chan->cl->msg_slot_ro = 00000000ab5fa771,
> msg slot ro = 20
>
> Tx and Rx both channels are updating same address of msg_slot_ro. If
> user wants to check msg_slot_ro for Tx channel, then it is possible that
> it was updated by Rx channel instead. Ideally there should be two copies
> of msg_slot_ro, one for Tx and one for Rx channel.
>
> This happens because Tx and Rx both channels shares same client data
> structure.
>
> Same can happen on other platforms as well.
>
The queue is only for TX.
The received data is directly handed to the client. So RX path should
not be modifying that queue availability variable.
thanks
On 12/3/25 8:13 PM, Jassi Brar wrote:
> On Tue, Nov 18, 2025 at 12:51 PM Tanmay Shah <tanmay.shah@amd.com> wrote:
>> On 11/15/25 11:38 AM, Jassi Brar wrote:
>>> On Wed, Oct 15, 2025 at 10:27 AM Tanmay Shah <tanmay.shah@amd.com> wrote:
>>>>>>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>>>>>>> index 5cd8ae222073..c2e187aa5d22 100644
>>>>>>> --- a/drivers/mailbox/mailbox.c
>>>>>>> +++ b/drivers/mailbox/mailbox.c
>>>>>>> @@ -35,6 +35,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void
>>>>>>> *mssg)
>>>>>>> idx = chan->msg_free;
>>>>>>> chan->msg_data[idx] = mssg;
>>>>>>> chan->msg_count++;
>>>>>>> + chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN - chan->msg_count);
>>>>>>>
>>>>>>> if (idx == MBOX_TX_QUEUE_LEN - 1)
>>>>>>> chan->msg_free = 0;
>>>>>>> @@ -70,6 +71,7 @@ static void msg_submit(struct mbox_chan *chan)
>>>>>>> if (!err) {
>>>>>>> chan->active_req = data;
>>>>>>> chan->msg_count--;
>>>>>>> + chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN -
>>>>>>> chan->msg_count);
>>>>>>>
>>>>>> No, I had suggested adding this info in client structure.
>>>>>> There is no point in carrying msg_count and msg_slot_ro in mbox_chan.
>>>>>> The client needs this info but can/should not access the chan internals.
>>>>>>
>>>>>
>>>>> Hi Jassi,
>>>>>
>>>>> If I move msg_slot_ro to mbox_client that means, each channel needs its
>>>>> own client structure. But it is possible to map single client to
>>>>> multiple channels.
>>>>>
>>>>> How about if I rename msg_slot_ro to msg_slot_tx_ro -> that says this
>>>>> field should be used only for "tx" channel.
>>>>>
>>>>> Or is it must to map unique client data structure to each channel? If
>>>>> so, can I update mbox_client structure documentation ?
>>>>>
>>>>
>>>> Hi Jassi,
>>>>
>>>> I looked into this further. Looks like it's not possible to introduce
>>>> msg_slot_ro in the client data structure as of today. Because multiple
>>>> drivers are sharing same client for "tx" and "rx" both channels.
>>>> [references: 1, 2, 3]
>>>>
>>>> so, msg_slot_ro won't be calculated correctly I believe.
>>>>
>>> I don't see it. Can you please explain how the calculated value could be wrong?
>>>
>>
>> Hi Jassi,
>>
>> so on my platform I introduced some extra logs:
>>
>> [ 80.827479] mbox chan Rx send message
>> [ 80.827485] add_to_rbuf: &chan->cl->msg_slot_ro = 00000000ab5fa771,
>> msg slot ro = 19
>> [ 80.827494] msg_submit: &chan->cl->msg_slot_ro = 00000000ab5fa771,
>> msg slot ro = 20
>> [ 80.833064] mbox chan Tx send message
>> [ 80.833070] add_to_rbuf: &chan->cl->msg_slot_ro = 00000000ab5fa771,
>> msg slot ro = 19
>> [ 80.833079] msg_submit: &chan->cl->msg_slot_ro = 00000000ab5fa771,
>> msg slot ro = 20
>> [ 80.837486] mbox chan Rx send message
>> [ 80.837492] add_to_rbuf: &chan->cl->msg_slot_ro = 00000000ab5fa771,
>> msg slot ro = 19
>> [ 80.837501] msg_submit: &chan->cl->msg_slot_ro = 00000000ab5fa771,
>> msg slot ro = 20
>>
>> Tx and Rx both channels are updating same address of msg_slot_ro. If
>> user wants to check msg_slot_ro for Tx channel, then it is possible that
>> it was updated by Rx channel instead. Ideally there should be two copies
>> of msg_slot_ro, one for Tx and one for Rx channel.
>>
>> This happens because Tx and Rx both channels shares same client data
>> structure.
>>
>> Same can happen on other platforms as well.
>>
> The queue is only for TX.
> The received data is directly handed to the client. So RX path should
> not be modifying that queue availability variable.
>
Hi Jassi,
Thank you for this clarification. The xlnx mbox driver is designed to send and
receive on both "tx" and "rx" channels. The "rx" channel receives data, and ack
back on the same "rx" channel.
I guess the best solution would be to have separate mbox_client data structure
for each channel in the remoteproc xlnx platform driver, and then implement the
rest of the solution as you had suggested.
I will send v3 after testing.
Thanks,
Tanmay
> thanks
On 10/15/25 10:26 AM, Tanmay Shah wrote:
>
>
> On 10/8/25 11:49 AM, Tanmay Shah wrote:
>>
>>
>> On 10/7/25 2:58 PM, Jassi Brar wrote:
>>> On Tue, Oct 7, 2025 at 10:19 AM Tanmay Shah <tanmay.shah@amd.com> wrote:
>>>>
>>>> Sometimes clients need to know if mailbox queue is full or not before
>>>> posting new message via mailbox. If mailbox queue is full clients can
>>>> choose not to post new message. This doesn't mean current queue length
>>>> should be increased, but clients may want to wait till previous Tx is
>>>> done. Introduce variable per channel to track available msg slots.
>>>> Clients can check this variable and decide not to send new message if
>>>> it is 0. This can help avoid false positive warning from mailbox
>>>> framework "Try increasing MBOX_TX_QUEUE_LEN".
>>>>
>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>> ---
>>>>
>>>> v2:
>>>> - Separate patch for remoteproc subsystem.
>>>> - Change design and introduce msg_slot_ro field for each channel
>>>> instead of API. Clients can use this variable directly.
>>>> - Remove mbox_queue_full API
>>>>
>>>> drivers/mailbox/mailbox.c | 3 +++
>>>> include/linux/mailbox_controller.h | 2 ++
>>>> 2 files changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>>>> index 5cd8ae222073..c2e187aa5d22 100644
>>>> --- a/drivers/mailbox/mailbox.c
>>>> +++ b/drivers/mailbox/mailbox.c
>>>> @@ -35,6 +35,7 @@ static int add_to_rbuf(struct mbox_chan *chan,
>>>> void *mssg)
>>>> idx = chan->msg_free;
>>>> chan->msg_data[idx] = mssg;
>>>> chan->msg_count++;
>>>> + chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN - chan->msg_count);
>>>>
>>>> if (idx == MBOX_TX_QUEUE_LEN - 1)
>>>> chan->msg_free = 0;
>>>> @@ -70,6 +71,7 @@ static void msg_submit(struct mbox_chan *chan)
>>>> if (!err) {
>>>> chan->active_req = data;
>>>> chan->msg_count--;
>>>> + chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN -
>>>> chan->msg_count);
>>>>
>>> No, I had suggested adding this info in client structure.
>>> There is no point in carrying msg_count and msg_slot_ro in mbox_chan.
>>> The client needs this info but can/should not access the chan internals.
>>>
>>
>> Hi Jassi,
>>
>> If I move msg_slot_ro to mbox_client that means, each channel needs
>> its own client structure. But it is possible to map single client to
>> multiple channels.
>>
>> How about if I rename msg_slot_ro to msg_slot_tx_ro -> that says this
>> field should be used only for "tx" channel.
>>
>> Or is it must to map unique client data structure to each channel? If
>> so, can I update mbox_client structure documentation ?
>>
>
> Hi Jassi,
>
> I looked into this further. Looks like it's not possible to introduce
> msg_slot_ro in the client data structure as of today. Because multiple
> drivers are sharing same client for "tx" and "rx" both channels.
> [references: 1, 2, 3]
>
> so, msg_slot_ro won't be calculated correctly I believe.
>
> I can change architecture for xlnx driver i.e. assign separate clients
> for each channel, but still it won't work for other platforms. Please
> let me know how to proceed further.
>
> Can we provide API that does (MBOX_TX_QUEUE_LEN - chan->msg_count) == 0?
>
> I am not sure if I should attempt to change the architecture of other
> platform's drivers.
>
>
> References:
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/
> linux.git/tree/drivers/remoteproc/imx_rproc.c?h=for-next#n768
>
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/
> linux.git/tree/drivers/remoteproc/xlnx_r5_remoteproc.c?h=for-next#n280
>
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/
> linux.git/tree/drivers/remoteproc/st_remoteproc.c?h=for-next#n386
>
> Thank you.
>
>
Hello Jassi,
Gentle reminder on this matter. I am waiting for your input on above.
Thank You,
Tanmay
>> Thanks,
>> Tanmay.
>>
>>> thanks
>>> jassi
>>
>
>
On 10/7/25 2:58 PM, Jassi Brar wrote:
> On Tue, Oct 7, 2025 at 10:19 AM Tanmay Shah <tanmay.shah@amd.com> wrote:
>>
>> Sometimes clients need to know if mailbox queue is full or not before
>> posting new message via mailbox. If mailbox queue is full clients can
>> choose not to post new message. This doesn't mean current queue length
>> should be increased, but clients may want to wait till previous Tx is
>> done. Introduce variable per channel to track available msg slots.
>> Clients can check this variable and decide not to send new message if
>> it is 0. This can help avoid false positive warning from mailbox
>> framework "Try increasing MBOX_TX_QUEUE_LEN".
>>
>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>> ---
>>
>> v2:
>> - Separate patch for remoteproc subsystem.
>> - Change design and introduce msg_slot_ro field for each channel
>> instead of API. Clients can use this variable directly.
>> - Remove mbox_queue_full API
>>
>> drivers/mailbox/mailbox.c | 3 +++
>> include/linux/mailbox_controller.h | 2 ++
>> 2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>> index 5cd8ae222073..c2e187aa5d22 100644
>> --- a/drivers/mailbox/mailbox.c
>> +++ b/drivers/mailbox/mailbox.c
>> @@ -35,6 +35,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
>> idx = chan->msg_free;
>> chan->msg_data[idx] = mssg;
>> chan->msg_count++;
>> + chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN - chan->msg_count);
>>
>> if (idx == MBOX_TX_QUEUE_LEN - 1)
>> chan->msg_free = 0;
>> @@ -70,6 +71,7 @@ static void msg_submit(struct mbox_chan *chan)
>> if (!err) {
>> chan->active_req = data;
>> chan->msg_count--;
>> + chan->msg_slot_ro = (MBOX_TX_QUEUE_LEN - chan->msg_count);
>>
> No, I had suggested adding this info in client structure.
> There is no point in carrying msg_count and msg_slot_ro in mbox_chan.
> The client needs this info but can/should not access the chan internals.
>
Sure, that makes sense. Will change in v3.
> thanks
> jassi
© 2016 - 2026 Red Hat, Inc.