[PATCH v2 0/2] mailbox queue length check

[PATCH v2 0/2] mailbox queue length check

[Tanmay Shah]

Provide mechanism to check mailbox queue length before posting new
message to mailbox queue.

Changes in v2:
  - Split patchset in two patches for two different subsystems.
  - Change design and introduce new field per channel to track mbox
    queue length.
  - use this new field in xlnx remoteproc driver to prevent sending new
    messages if no slots available in the mbox tx channel queue.

Tanmay Shah (2):
  mailbox: check mailbox queue is full or not
  remoteproc: xlnx: do not kick if mbox queue is full

 drivers/mailbox/mailbox.c               | 3 +++
 drivers/remoteproc/xlnx_r5_remoteproc.c | 5 +++++
 include/linux/mailbox_controller.h      | 2 ++
 3 files changed, 10 insertions(+)


base-commit: 56d030ea3330ab737fe6c05f89d52f56208b07ac
-- 
2.34.1

[PATCH v2 1/2] mailbox: check mailbox queue is full or not

[Tanmay Shah]

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.1

Re: [PATCH v2 1/2] mailbox: check mailbox queue is full or not

[Jassi Brar]

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.

thanks
jassi

Re: [PATCH v2 1/2] mailbox: check mailbox queue is full or not

[Tanmay Shah]

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

Re: [PATCH v2 1/2] mailbox: check mailbox queue is full or not

[Tanmay Shah]

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

Re: [PATCH v2 1/2] mailbox: check mailbox queue is full or not

[Tanmay Shah]

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
> 

Re: [PATCH v2 1/2] mailbox: check mailbox queue is full or not

[Tanmay Shah]

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
>>
> 
> 

Re: [PATCH v2 1/2] mailbox: check mailbox queue is full or not

[Jassi Brar]

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

Re: [PATCH v2 1/2] mailbox: check mailbox queue is full or not

[Tanmay Shah]

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

Re: [PATCH v2 1/2] mailbox: check mailbox queue is full or not

[Jassi Brar]

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

Re: [PATCH v2 1/2] mailbox: check mailbox queue is full or not

[Tanmay Shah]

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

[PATCH v2 2/2] remoteproc: xlnx: do not kick if mbox queue is full

[Tanmay Shah]

If MBOX_TX_QUEUE_LEN number of kicks are pending, then no need to keep
doing more kicks because it will fail anyway. Preventing further kicks
is needed because it avoids printing false positive warning messages
from mailbox framework. Functionally nothing changes from RPMsg or
remoteproc point of view.

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---

v2:
  - Use msg_slot_ro to know if single slot is available in msg queue or not.
 drivers/remoteproc/xlnx_r5_remoteproc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
index 0b7b173d0d26..d589f31f45b9 100644
--- a/drivers/remoteproc/xlnx_r5_remoteproc.c
+++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
@@ -9,6 +9,7 @@
 #include <linux/firmware/xlnx-zynqmp.h>
 #include <linux/kernel.h>
 #include <linux/mailbox_client.h>
+#include <linux/mailbox_controller.h>
 #include <linux/mailbox/zynqmp-ipi-message.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
@@ -335,6 +336,10 @@ static void zynqmp_r5_rproc_kick(struct rproc *rproc, int vqid)
 	if (!ipi)
 		return;
 
+	/* Do not need new kick as already many kicks are pending. */
+	if (ipi->tx_chan->msg_slot_ro == 0)
+		return;
+
 	mb_msg = (struct zynqmp_ipi_message *)ipi->tx_mc_buf;
 	memcpy(mb_msg->data, &vqid, sizeof(vqid));
 	mb_msg->len = sizeof(vqid);
-- 
2.34.1