[PATCH] mailbox: check mailbox queue is full or not

[PATCH] 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. This API can help avoid false positive warning from mailbox
framework "Try increasing MBOX_TX_QUEUE_LEN".

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---
 drivers/mailbox/mailbox.c               | 24 ++++++++++++++++++++++++
 drivers/remoteproc/xlnx_r5_remoteproc.c |  4 ++++
 include/linux/mailbox_client.h          |  1 +
 3 files changed, 29 insertions(+)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 5cd8ae222073..7afdb2c9006d 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -217,6 +217,30 @@ bool mbox_client_peek_data(struct mbox_chan *chan)
 }
 EXPORT_SYMBOL_GPL(mbox_client_peek_data);
 
+/**
+ * mbox_queue_full - check if mailbox queue is full or not
+ * @chan: Mailbox channel assigned to this client.
+ *
+ * Clients can choose not to send new msg if mbox queue is full.
+ *
+ * Return: true if queue is full else false. < 0 for error
+ */
+int mbox_queue_full(struct mbox_chan *chan)
+{
+	unsigned long flags;
+	int res;
+
+	if (!chan)
+		return -EINVAL;
+
+	spin_lock_irqsave(&chan->lock, flags);
+	res = (chan->msg_count == (MBOX_TX_QUEUE_LEN - 1));
+	spin_unlock_irqrestore(&chan->lock, flags);
+
+	return res;
+}
+EXPORT_SYMBOL_GPL(mbox_queue_full);
+
 /**
  * mbox_send_message -	For client to submit a message to be
  *				sent to the remote.
diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
index 0b7b173d0d26..b3262de8a3ac 100644
--- a/drivers/remoteproc/xlnx_r5_remoteproc.c
+++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
@@ -335,6 +335,10 @@ static void zynqmp_r5_rproc_kick(struct rproc *rproc, int vqid)
 	if (!ipi)
 		return;
 
+	/* Do not need new kick as already many kick interrupts are pending. */
+	if (mbox_queue_full(ipi->tx_chan) == true)
+		return;
+
 	mb_msg = (struct zynqmp_ipi_message *)ipi->tx_mc_buf;
 	memcpy(mb_msg->data, &vqid, sizeof(vqid));
 	mb_msg->len = sizeof(vqid);
diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
index c6eea9afb943..4a19800af96f 100644
--- a/include/linux/mailbox_client.h
+++ b/include/linux/mailbox_client.h
@@ -46,5 +46,6 @@ int mbox_flush(struct mbox_chan *chan, unsigned long timeout);
 void mbox_client_txdone(struct mbox_chan *chan, int r); /* atomic */
 bool mbox_client_peek_data(struct mbox_chan *chan); /* atomic */
 void mbox_free_channel(struct mbox_chan *chan); /* may sleep */
+int mbox_queue_full(struct mbox_chan *chan);
 
 #endif /* __MAILBOX_CLIENT_H */

base-commit: 56d030ea3330ab737fe6c05f89d52f56208b07ac
-- 
2.34.1

Re: [PATCH] mailbox: check mailbox queue is full or not

[Peng Fan]

Hi Tanmay,

On Thu, Sep 25, 2025 at 11:50:44AM -0700, Tanmay Shah 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. This API can help avoid false positive warning from mailbox
>framework "Try increasing MBOX_TX_QUEUE_LEN".
>
>Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>---
> drivers/mailbox/mailbox.c               | 24 ++++++++++++++++++++++++
> drivers/remoteproc/xlnx_r5_remoteproc.c |  4 ++++
> include/linux/mailbox_client.h          |  1 +

The mailbox and remoteproc should be separated.

> 3 files changed, 29 insertions(+)
>
>diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>index 5cd8ae222073..7afdb2c9006d 100644
>--- a/drivers/mailbox/mailbox.c
>+++ b/drivers/mailbox/mailbox.c
>@@ -217,6 +217,30 @@ bool mbox_client_peek_data(struct mbox_chan *chan)
> }
> EXPORT_SYMBOL_GPL(mbox_client_peek_data);
> 
>+/**
>+ * mbox_queue_full - check if mailbox queue is full or not
>+ * @chan: Mailbox channel assigned to this client.
>+ *
>+ * Clients can choose not to send new msg if mbox queue is full.
>+ *
>+ * Return: true if queue is full else false. < 0 for error
>+ */
>+int mbox_queue_full(struct mbox_chan *chan)
>+{
>+	unsigned long flags;
>+	int res;
>+
>+	if (!chan)
>+		return -EINVAL;
>+
>+	spin_lock_irqsave(&chan->lock, flags);

Use scoped_guard.

>+	res = (chan->msg_count == (MBOX_TX_QUEUE_LEN - 1));
>+	spin_unlock_irqrestore(&chan->lock, flags);
>+
>+	return res;
>+}
>+EXPORT_SYMBOL_GPL(mbox_queue_full);

add_to_rbuf is able to return ENOBUFS when call mbox_send_message.
Does checking mbox_send_message return value works for you?

>+
> /**
>  * mbox_send_message -	For client to submit a message to be
>  *				sent to the remote.

Regards
Peng

Re: [PATCH] mailbox: check mailbox queue is full or not

[Tanmay Shah]

Hi Peng,

Thanks for reviews. Please find my comments below.

On 9/26/25 2:37 AM, Peng Fan wrote:
> Hi Tanmay,
> 
> On Thu, Sep 25, 2025 at 11:50:44AM -0700, Tanmay Shah 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. This API can help avoid false positive warning from mailbox
>> framework "Try increasing MBOX_TX_QUEUE_LEN".
>>
>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>> ---
>> drivers/mailbox/mailbox.c               | 24 ++++++++++++++++++++++++
>> drivers/remoteproc/xlnx_r5_remoteproc.c |  4 ++++
>> include/linux/mailbox_client.h          |  1 +
> 
> The mailbox and remoteproc should be separated.
> 

Mailbox framework is introducing new API. I wanted the use case to be in 
the same patch-set, otherwise we might see unused API warning.

Hence, both in the same patch makes more sense. If maintainers prefer, I 
will separate them.

>> 3 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>> index 5cd8ae222073..7afdb2c9006d 100644
>> --- a/drivers/mailbox/mailbox.c
>> +++ b/drivers/mailbox/mailbox.c
>> @@ -217,6 +217,30 @@ bool mbox_client_peek_data(struct mbox_chan *chan)
>> }
>> EXPORT_SYMBOL_GPL(mbox_client_peek_data);
>>
>> +/**
>> + * mbox_queue_full - check if mailbox queue is full or not
>> + * @chan: Mailbox channel assigned to this client.
>> + *
>> + * Clients can choose not to send new msg if mbox queue is full.
>> + *
>> + * Return: true if queue is full else false. < 0 for error
>> + */
>> +int mbox_queue_full(struct mbox_chan *chan)
>> +{
>> +	unsigned long flags;
>> +	int res;
>> +
>> +	if (!chan)
>> +		return -EINVAL;
>> +
>> +	spin_lock_irqsave(&chan->lock, flags);
> 
> Use scoped_guard.

Other APIs use spin_lock_irqsave. Probably scoped_guard should be 
introduced in a different patch for all APIs in the mailbox.

> 
>> +	res = (chan->msg_count == (MBOX_TX_QUEUE_LEN - 1));
>> +	spin_unlock_irqrestore(&chan->lock, flags);
>> +
>> +	return res;
>> +}
>> +EXPORT_SYMBOL_GPL(mbox_queue_full);
> 
> add_to_rbuf is able to return ENOBUFS when call mbox_send_message.
> Does checking mbox_send_message return value works for you?
> 

That is the problem. mbox_send_message uses add_to_rbuf and fails. But 
during failure, it prints warning message:

dev_err(chan->mbox->dev, "Try increasing MBOX_TX_QUEUE_LEN\n");

In some cases there are lot of such messages on terminal. Functionally 
nothing is wrong and everything is working but user keeps getting false 
positive warning about increasing mbox tx queue length. That is why we 
need API to check if mbox queue length is full or not before doing 
mbox_send_message. Not all clients need to use it, but some cane make 
use of it.


>> +
>> /**
>>   * mbox_send_message -	For client to submit a message to be
>>   *				sent to the remote.
> 
> Regards
> Peng

Re: [PATCH] mailbox: check mailbox queue is full or not

[Peng Fan]

Hi,

On Fri, Sep 26, 2025 at 10:40:09AM -0500, Tanmay Shah wrote:
>> > ---
>> > drivers/mailbox/mailbox.c               | 24 ++++++++++++++++++++++++
>> > drivers/remoteproc/xlnx_r5_remoteproc.c |  4 ++++
>> > include/linux/mailbox_client.h          |  1 +
>> 
>> The mailbox and remoteproc should be separated.
>> 
>
>Mailbox framework is introducing new API. I wanted the use case to be in the
>same patch-set, otherwise we might see unused API warning.

I mean two patches in one patchset.

>
>Hence, both in the same patch makes more sense. If maintainers prefer, I will
>separate them.
>
>> > 3 files changed, 29 insertions(+)
>> > 
>> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>> > index 5cd8ae222073..7afdb2c9006d 100644
>> > --- a/drivers/mailbox/mailbox.c
>> > +++ b/drivers/mailbox/mailbox.c
>> > @@ -217,6 +217,30 @@ bool mbox_client_peek_data(struct mbox_chan *chan)
>> > }
>> > EXPORT_SYMBOL_GPL(mbox_client_peek_data);
>> > 
>> > +/**
>> > + * mbox_queue_full - check if mailbox queue is full or not
>> > + * @chan: Mailbox channel assigned to this client.
>> > + *
>> > + * Clients can choose not to send new msg if mbox queue is full.
>> > + *
>> > + * Return: true if queue is full else false. < 0 for error
>> > + */
>> > +int mbox_queue_full(struct mbox_chan *chan)
>> > +{
>> > +	unsigned long flags;
>> > +	int res;
>> > +
>> > +	if (!chan)
>> > +		return -EINVAL;
>> > +
>> > +	spin_lock_irqsave(&chan->lock, flags);
>> 
>> Use scoped_guard.
>
>Other APIs use spin_lock_irqsave. Probably scoped_guard should be introduced
>in a different patch for all APIs in the mailbox.

Your code base seems not up to date.

>
>> 
>> > +	res = (chan->msg_count == (MBOX_TX_QUEUE_LEN - 1));
>> > +	spin_unlock_irqrestore(&chan->lock, flags);
>> > +
>> > +	return res;
>> > +}
>> > +EXPORT_SYMBOL_GPL(mbox_queue_full);
>> 
>> add_to_rbuf is able to return ENOBUFS when call mbox_send_message.
>> Does checking mbox_send_message return value works for you?
>> 
>
>That is the problem. mbox_send_message uses add_to_rbuf and fails. But during
>failure, it prints warning message:
>
>dev_err(chan->mbox->dev, "Try increasing MBOX_TX_QUEUE_LEN\n");
>
>In some cases there are lot of such messages on terminal. Functionally
>nothing is wrong and everything is working but user keeps getting false
>positive warning about increasing mbox tx queue length. That is why we need
>API to check if mbox queue length is full or not before doing
>mbox_send_message. Not all clients need to use it, but some cane make use of
>it.

I think check whether mbox_send_message returns -ENOBUFS or not should
work for you. If the "Try increasing MBOX_TX_QUEUE_LEN" message
bothers you, it could be update to dev_dbg per my understanding.

Regards,
Peng

>
>
>> > +
>> > /**
>> >   * mbox_send_message -	For client to submit a message to be
>> >   *				sent to the remote.
>> 
>> Regards
>> Peng
>

Re: [PATCH] mailbox: check mailbox queue is full or not

[Mathieu Poirier]

On Sun, Sep 28, 2025 at 03:56:41PM +0800, Peng Fan wrote:
> Hi,
> 
> On Fri, Sep 26, 2025 at 10:40:09AM -0500, Tanmay Shah wrote:
> >> > ---
> >> > drivers/mailbox/mailbox.c               | 24 ++++++++++++++++++++++++
> >> > drivers/remoteproc/xlnx_r5_remoteproc.c |  4 ++++
> >> > include/linux/mailbox_client.h          |  1 +
> >> 
> >> The mailbox and remoteproc should be separated.
> >> 
> >
> >Mailbox framework is introducing new API. I wanted the use case to be in the
> >same patch-set, otherwise we might see unused API warning.
> 
> I mean two patches in one patchset.
>

Agreed
 
> >
> >Hence, both in the same patch makes more sense. If maintainers prefer, I will
> >separate them.
> >
> >> > 3 files changed, 29 insertions(+)
> >> > 
> >> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> >> > index 5cd8ae222073..7afdb2c9006d 100644
> >> > --- a/drivers/mailbox/mailbox.c
> >> > +++ b/drivers/mailbox/mailbox.c
> >> > @@ -217,6 +217,30 @@ bool mbox_client_peek_data(struct mbox_chan *chan)
> >> > }
> >> > EXPORT_SYMBOL_GPL(mbox_client_peek_data);
> >> > 
> >> > +/**
> >> > + * mbox_queue_full - check if mailbox queue is full or not
> >> > + * @chan: Mailbox channel assigned to this client.
> >> > + *
> >> > + * Clients can choose not to send new msg if mbox queue is full.
> >> > + *
> >> > + * Return: true if queue is full else false. < 0 for error
> >> > + */
> >> > +int mbox_queue_full(struct mbox_chan *chan)
> >> > +{
> >> > +	unsigned long flags;
> >> > +	int res;
> >> > +
> >> > +	if (!chan)
> >> > +		return -EINVAL;
> >> > +
> >> > +	spin_lock_irqsave(&chan->lock, flags);
> >> 
> >> Use scoped_guard.
> >
> >Other APIs use spin_lock_irqsave. Probably scoped_guard should be introduced
> >in a different patch for all APIs in the mailbox.
> 
> Your code base seems not up to date.
> 

Agreed

> >
> >> 
> >> > +	res = (chan->msg_count == (MBOX_TX_QUEUE_LEN - 1));

Please have a look at this condition again - the implementation of
addr_to_rbuf() [1] is checking for space differently.

[1]. https://elixir.bootlin.com/linux/v6.17/source/drivers/mailbox/mailbox.c#L32

> >> > +	spin_unlock_irqrestore(&chan->lock, flags);
> >> > +
> >> > +	return res;
> >> > +}
> >> > +EXPORT_SYMBOL_GPL(mbox_queue_full);
> >> 
> >> add_to_rbuf is able to return ENOBUFS when call mbox_send_message.
> >> Does checking mbox_send_message return value works for you?
> >> 
> >
> >That is the problem. mbox_send_message uses add_to_rbuf and fails. But during
> >failure, it prints warning message:
> >
> >dev_err(chan->mbox->dev, "Try increasing MBOX_TX_QUEUE_LEN\n");
> >
> >In some cases there are lot of such messages on terminal. Functionally
> >nothing is wrong and everything is working but user keeps getting false
> >positive warning about increasing mbox tx queue length. That is why we need
> >API to check if mbox queue length is full or not before doing
> >mbox_send_message. Not all clients need to use it, but some cane make use of
> >it.
> 
> I think check whether mbox_send_message returns -ENOBUFS or not should
> work for you. If the "Try increasing MBOX_TX_QUEUE_LEN" message
> bothers you, it could be update to dev_dbg per my understanding.
> 

This new API is trying to avoid calling mbox_send_message(), no checking if it
succeeded or not.  Moving dev_err() nto dev_dbg() is also the wrong approach.

> Regards,
> Peng
> 
> >
> >
> >> > +
> >> > /**
> >> >   * mbox_send_message -	For client to submit a message to be
> >> >   *				sent to the remote.
> >> 
> >> Regards
> >> Peng
> >

Re: [PATCH] mailbox: check mailbox queue is full or not

[Tanmay Shah]

On 9/29/25 9:45 AM, Mathieu Poirier wrote:
> On Sun, Sep 28, 2025 at 03:56:41PM +0800, Peng Fan wrote:
>> Hi,
>>
>> On Fri, Sep 26, 2025 at 10:40:09AM -0500, Tanmay Shah wrote:
>>>>> ---
>>>>> drivers/mailbox/mailbox.c               | 24 ++++++++++++++++++++++++
>>>>> drivers/remoteproc/xlnx_r5_remoteproc.c |  4 ++++
>>>>> include/linux/mailbox_client.h          |  1 +
>>>>
>>>> The mailbox and remoteproc should be separated.
>>>>
>>>
>>> Mailbox framework is introducing new API. I wanted the use case to be in the
>>> same patch-set, otherwise we might see unused API warning.
>>
>> I mean two patches in one patchset.
>>
> 
> Agreed
>   

Okay.

>>>
>>> Hence, both in the same patch makes more sense. If maintainers prefer, I will
>>> separate them.
>>>
>>>>> 3 files changed, 29 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>>>>> index 5cd8ae222073..7afdb2c9006d 100644
>>>>> --- a/drivers/mailbox/mailbox.c
>>>>> +++ b/drivers/mailbox/mailbox.c
>>>>> @@ -217,6 +217,30 @@ bool mbox_client_peek_data(struct mbox_chan *chan)
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(mbox_client_peek_data);
>>>>>
>>>>> +/**
>>>>> + * mbox_queue_full - check if mailbox queue is full or not
>>>>> + * @chan: Mailbox channel assigned to this client.
>>>>> + *
>>>>> + * Clients can choose not to send new msg if mbox queue is full.
>>>>> + *
>>>>> + * Return: true if queue is full else false. < 0 for error
>>>>> + */
>>>>> +int mbox_queue_full(struct mbox_chan *chan)
>>>>> +{
>>>>> +	unsigned long flags;
>>>>> +	int res;
>>>>> +
>>>>> +	if (!chan)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	spin_lock_irqsave(&chan->lock, flags);
>>>>
>>>> Use scoped_guard.
>>>
>>> Other APIs use spin_lock_irqsave. Probably scoped_guard should be introduced
>>> in a different patch for all APIs in the mailbox.
>>
>> Your code base seems not up to date.
>>
> 
> Agreed
> 

Okay. I was referring to different branch when I referred other API in 
the mailbox framework.

Sure, I will use scoped_guard.

>>>
>>>>
>>>>> +	res = (chan->msg_count == (MBOX_TX_QUEUE_LEN - 1));
> 
> Please have a look at this condition again - the implementation of
> addr_to_rbuf() [1] is checking for space differently.
> 
> [1]. https://elixir.bootlin.com/linux/v6.17/source/drivers/mailbox/mailbox.c#L32
> 
>>>>> +	spin_unlock_irqrestore(&chan->lock, flags);
>>>>> +
>>>>> +	return res;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(mbox_queue_full);
>>>>
>>>> add_to_rbuf is able to return ENOBUFS when call mbox_send_message.
>>>> Does checking mbox_send_message return value works for you?
>>>>
>>>
>>> That is the problem. mbox_send_message uses add_to_rbuf and fails. But during
>>> failure, it prints warning message:
>>>
>>> dev_err(chan->mbox->dev, "Try increasing MBOX_TX_QUEUE_LEN\n");
>>>
>>> In some cases there are lot of such messages on terminal. Functionally
>>> nothing is wrong and everything is working but user keeps getting false
>>> positive warning about increasing mbox tx queue length. That is why we need
>>> API to check if mbox queue length is full or not before doing
>>> mbox_send_message. Not all clients need to use it, but some cane make use of
>>> it.
>>
>> I think check whether mbox_send_message returns -ENOBUFS or not should
>> work for you. If the "Try increasing MBOX_TX_QUEUE_LEN" message
>> bothers you, it could be update to dev_dbg per my understanding.
>>
> 
> This new API is trying to avoid calling mbox_send_message(), no checking if it
> succeeded or not.  Moving dev_err() nto dev_dbg() is also the wrong approach.
> 

Correct.

>> Regards,
>> Peng
>>
>>>
>>>
>>>>> +
>>>>> /**
>>>>>    * mbox_send_message -	For client to submit a message to be
>>>>>    *				sent to the remote.
>>>>
>>>> Regards
>>>> Peng
>>>

Re: [PATCH] mailbox: check mailbox queue is full or not

[Tanmay Shah]

On 9/29/25 2:26 PM, Tanmay Shah wrote:
> 
> 
> On 9/29/25 9:45 AM, Mathieu Poirier wrote:
>> On Sun, Sep 28, 2025 at 03:56:41PM +0800, Peng Fan wrote:
>>> Hi,
>>>
>>> On Fri, Sep 26, 2025 at 10:40:09AM -0500, Tanmay Shah wrote:
>>>>>> ---
>>>>>> drivers/mailbox/mailbox.c               | 24 ++++++++++++++++++++++++
>>>>>> drivers/remoteproc/xlnx_r5_remoteproc.c |  4 ++++
>>>>>> include/linux/mailbox_client.h          |  1 +
>>>>>
>>>>> The mailbox and remoteproc should be separated.
>>>>>
>>>>

[...]

> 
>>>>
>>>>>
>>>>>> +    res = (chan->msg_count == (MBOX_TX_QUEUE_LEN - 1));
>>
>> Please have a look at this condition again - the implementation of
>> addr_to_rbuf() [1] is checking for space differently.
>>
>> [1]. https://elixir.bootlin.com/linux/v6.17/source/drivers/mailbox/ 
>> mailbox.c#L32
>>

Here Ack as well. I think it should be same as what's there in add_to_rbuf.

>>>>>> +    spin_unlock_irqrestore(&chan->lock, flags);
>>>>>> +
>>>>>> +    return res;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(mbox_queue_full);
>>>>>
>>>>> add_to_rbuf is able to return ENOBUFS when call mbox_send_message.
>>>>> Does checking mbox_send_message return value works for you?
>>>>>
>>>>
>>>> That is the problem. mbox_send_message uses add_to_rbuf and fails. 
>>>> But during
>>>> failure, it prints warning message:
>>>>
>>>> dev_err(chan->mbox->dev, "Try increasing MBOX_TX_QUEUE_LEN\n");
>>>>
>>>> In some cases there are lot of such messages on terminal. Functionally
>>>> nothing is wrong and everything is working but user keeps getting false
>>>> positive warning about increasing mbox tx queue length. That is why 
>>>> we need
>>>> API to check if mbox queue length is full or not before doing
>>>> mbox_send_message. Not all clients need to use it, but some cane 
>>>> make use of
>>>> it.
>>>
>>> I think check whether mbox_send_message returns -ENOBUFS or not should
>>> work for you. If the "Try increasing MBOX_TX_QUEUE_LEN" message
>>> bothers you, it could be update to dev_dbg per my understanding.
>>>
>>
>> This new API is trying to avoid calling mbox_send_message(), no 
>> checking if it
>> succeeded or not.  Moving dev_err() nto dev_dbg() is also the wrong 
>> approach.
>>
> 
> Correct.
> 
>>> Regards,
>>> Peng
>>>
>>>>
>>>>
>>>>>> +
>>>>>> /**
>>>>>>    * mbox_send_message -    For client to submit a message to be
>>>>>>    *                sent to the remote.
>>>>>
>>>>> Regards
>>>>> Peng
>>>>
> 
> 

Re: [PATCH] mailbox: check mailbox queue is full or not

[Peng Fan]

On Mon, Sep 29, 2025 at 02:26:01PM -0500, Tanmay Shah wrote:
>> > > > 
>> > > > > +	res = (chan->msg_count == (MBOX_TX_QUEUE_LEN - 1));
>> 
>> Please have a look at this condition again - the implementation of
>> addr_to_rbuf() [1] is checking for space differently.

Tanmay,

May I know why you want to compare with "MBOX_TX_QUEUE_LEN - 1",
not "MBOX_TX_QUEUE_LEN"?

>> 
>> [1]. https://elixir.bootlin.com/linux/v6.17/source/drivers/mailbox/mailbox.c#L32
>> 
>> > > > > +	spin_unlock_irqrestore(&chan->lock, flags);
>> > > > > +
>> > > > > +	return res;
>> > > > > +}
>> > > > > +EXPORT_SYMBOL_GPL(mbox_queue_full);
>> > > > 
>> > > > add_to_rbuf is able to return ENOBUFS when call mbox_send_message.
>> > > > Does checking mbox_send_message return value works for you?
>> > > > 
>> > > 
>> > > That is the problem. mbox_send_message uses add_to_rbuf and fails. But during
>> > > failure, it prints warning message:
>> > > 
>> > > dev_err(chan->mbox->dev, "Try increasing MBOX_TX_QUEUE_LEN\n");
>> > > 
>> > > In some cases there are lot of such messages on terminal. Functionally
>> > > nothing is wrong and everything is working but user keeps getting false
>> > > positive warning about increasing mbox tx queue length. That is why we need
>> > > API to check if mbox queue length is full or not before doing
>> > > mbox_send_message. Not all clients need to use it, but some cane make use of
>> > > it.
>> > 
>> > I think check whether mbox_send_message returns -ENOBUFS or not should
>> > work for you. If the "Try increasing MBOX_TX_QUEUE_LEN" message
>> > bothers you, it could be update to dev_dbg per my understanding.
>> > 
>> 
>> This new API is trying to avoid calling mbox_send_message(), no checking if it
>> succeeded or not. 

I think it may not deserve to introduce a new API. add_to_rbuf is almost
the first function that mbox_send_message calls. But if Tanmay insists on
adding a new API, I am fine. Jassi may comment more. 

>> Moving dev_err() nto dev_dbg() is also the wrong approach.
>> 
>
>Correct.

The caller of mbox_send_message detect error value and choose
to add dev_err or not in caller driver, so I think dev_dbg is fine here.

I would appreciate if there is explaination on why dev_dbg is not correct :)

Thanks,
Peng

>
>> > Regards,
>> > Peng
>> > 
>> > > 
>> > > 
>> > > > > +
>> > > > > /**
>> > > > >    * mbox_send_message -	For client to submit a message to be
>> > > > >    *				sent to the remote.
>> > > > 
>> > > > Regards
>> > > > Peng
>> > > 
>

Re: [PATCH] mailbox: check mailbox queue is full or not

[Jassi Brar]

On Thu, Sep 25, 2025 at 1:51 PM 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. This API can help avoid false positive warning from mailbox
> framework "Try increasing MBOX_TX_QUEUE_LEN".
>
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> ---
>  drivers/mailbox/mailbox.c               | 24 ++++++++++++++++++++++++
>  drivers/remoteproc/xlnx_r5_remoteproc.c |  4 ++++
>  include/linux/mailbox_client.h          |  1 +
>  3 files changed, 29 insertions(+)
>
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 5cd8ae222073..7afdb2c9006d 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -217,6 +217,30 @@ bool mbox_client_peek_data(struct mbox_chan *chan)
>  }
>  EXPORT_SYMBOL_GPL(mbox_client_peek_data);
>
> +/**
> + * mbox_queue_full - check if mailbox queue is full or not
> + * @chan: Mailbox channel assigned to this client.
> + *
> + * Clients can choose not to send new msg if mbox queue is full.
> + *
> + * Return: true if queue is full else false. < 0 for error
> + */
> +int mbox_queue_full(struct mbox_chan *chan)
> +{
> +       unsigned long flags;
> +       int res;
> +
> +       if (!chan)
> +               return -EINVAL;
> +
> +       spin_lock_irqsave(&chan->lock, flags);
> +       res = (chan->msg_count == (MBOX_TX_QUEUE_LEN - 1));
>
1) If we really need this, it should be
        res = (chan->msg_count == MBOX_TX_QUEUE_LEN);

2) I am thinking instead, introduce a
       struct mbox_client.msg_slots_ro;
  Which is a read-only field for the client, denoting how many message
slots are currently available.
  The mailbox api will always adjust it when msg_count changes...
      chan->cl->msg_slots_ro = MBOX_TX_QUEUE_LEN - chan->msg_count;

-jassi

Re: [PATCH] mailbox: check mailbox queue is full or not

[Tanmay Shah]

Hi Jassi,

Please find my comments below.

On 9/30/25 9:11 AM, Jassi Brar wrote:
> On Thu, Sep 25, 2025 at 1:51 PM 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. This API can help avoid false positive warning from mailbox
>> framework "Try increasing MBOX_TX_QUEUE_LEN".
>>
>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>> ---
>>   drivers/mailbox/mailbox.c               | 24 ++++++++++++++++++++++++
>>   drivers/remoteproc/xlnx_r5_remoteproc.c |  4 ++++
>>   include/linux/mailbox_client.h          |  1 +
>>   3 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>> index 5cd8ae222073..7afdb2c9006d 100644
>> --- a/drivers/mailbox/mailbox.c
>> +++ b/drivers/mailbox/mailbox.c
>> @@ -217,6 +217,30 @@ bool mbox_client_peek_data(struct mbox_chan *chan)
>>   }
>>   EXPORT_SYMBOL_GPL(mbox_client_peek_data);
>>
>> +/**
>> + * mbox_queue_full - check if mailbox queue is full or not
>> + * @chan: Mailbox channel assigned to this client.
>> + *
>> + * Clients can choose not to send new msg if mbox queue is full.
>> + *
>> + * Return: true if queue is full else false. < 0 for error
>> + */
>> +int mbox_queue_full(struct mbox_chan *chan)
>> +{
>> +       unsigned long flags;
>> +       int res;
>> +
>> +       if (!chan)
>> +               return -EINVAL;
>> +
>> +       spin_lock_irqsave(&chan->lock, flags);
>> +       res = (chan->msg_count == (MBOX_TX_QUEUE_LEN - 1));
>>
> 1) If we really need this, it should be
>          res = (chan->msg_count == MBOX_TX_QUEUE_LEN);
> 

Ack here.

> 2) I am thinking instead, introduce a
>         struct mbox_client.msg_slots_ro;
>    Which is a read-only field for the client, denoting how many message
> slots are currently available.
>    The mailbox api will always adjust it when msg_count changes...
>        chan->cl->msg_slots_ro = MBOX_TX_QUEUE_LEN - chan->msg_count;
> 

It's not possible to make msg_slots_ro true Read-Only. Nothing prevents 
clients to write to that variable as far as I know. Also, we have 
msg_free variable that can be used to implement similar concept as 
msg_slots_ro. But that also has same problem, not possible to implement 
it in true read-only fashion.

I can instead implement API msg_queue_empty() and return msg_free 
variable via that API.


> -jassi

Re: [PATCH] mailbox: check mailbox queue is full or not

[Jassi Brar]

On Tue, Sep 30, 2025 at 11:52 AM Tanmay Shah <tanmay.shah@amd.com> wrote:
>
> Hi Jassi,
>
> Please find my comments below.
>
> On 9/30/25 9:11 AM, Jassi Brar wrote:
> > On Thu, Sep 25, 2025 at 1:51 PM 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. This API can help avoid false positive warning from mailbox
> >> framework "Try increasing MBOX_TX_QUEUE_LEN".
> >>
> >> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> >> ---
> >>   drivers/mailbox/mailbox.c               | 24 ++++++++++++++++++++++++
> >>   drivers/remoteproc/xlnx_r5_remoteproc.c |  4 ++++
> >>   include/linux/mailbox_client.h          |  1 +
> >>   3 files changed, 29 insertions(+)
> >>
> >> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> >> index 5cd8ae222073..7afdb2c9006d 100644
> >> --- a/drivers/mailbox/mailbox.c
> >> +++ b/drivers/mailbox/mailbox.c
> >> @@ -217,6 +217,30 @@ bool mbox_client_peek_data(struct mbox_chan *chan)
> >>   }
> >>   EXPORT_SYMBOL_GPL(mbox_client_peek_data);
> >>
> >> +/**
> >> + * mbox_queue_full - check if mailbox queue is full or not
> >> + * @chan: Mailbox channel assigned to this client.
> >> + *
> >> + * Clients can choose not to send new msg if mbox queue is full.
> >> + *
> >> + * Return: true if queue is full else false. < 0 for error
> >> + */
> >> +int mbox_queue_full(struct mbox_chan *chan)
> >> +{
> >> +       unsigned long flags;
> >> +       int res;
> >> +
> >> +       if (!chan)
> >> +               return -EINVAL;
> >> +
> >> +       spin_lock_irqsave(&chan->lock, flags);
> >> +       res = (chan->msg_count == (MBOX_TX_QUEUE_LEN - 1));
> >>
> > 1) If we really need this, it should be
> >          res = (chan->msg_count == MBOX_TX_QUEUE_LEN);
> >
>
> Ack here.
>
> > 2) I am thinking instead, introduce a
> >         struct mbox_client.msg_slots_ro;
> >    Which is a read-only field for the client, denoting how many message
> > slots are currently available.
> >    The mailbox api will always adjust it when msg_count changes...
> >        chan->cl->msg_slots_ro = MBOX_TX_QUEUE_LEN - chan->msg_count;
> >
>
> It's not possible to make msg_slots_ro true Read-Only. Nothing prevents
> clients to write to that variable as far as I know.
>
Correct, nothing prevents a client from changing 'msg_slots_ro', just
like nothing
prevents it from setting tx_done or rx_callback to 0xdeadbabe.
The '_ro' suffix is to tell the client developer to not mess with it.
I am slightly more inclined towards this approach because it avoids
adding another
convenience api and is more immediately available without needing a spinlock.

-jassi

Re: [PATCH] mailbox: check mailbox queue is full or not

[Tanmay Shah]

On 9/30/25 12:33 PM, Jassi Brar wrote:
> On Tue, Sep 30, 2025 at 11:52 AM Tanmay Shah <tanmay.shah@amd.com> wrote:
>>
>> Hi Jassi,
>>
>> Please find my comments below.
>>
>> On 9/30/25 9:11 AM, Jassi Brar wrote:
>>> On Thu, Sep 25, 2025 at 1:51 PM 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. This API can help avoid false positive warning from mailbox
>>>> framework "Try increasing MBOX_TX_QUEUE_LEN".
>>>>
>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>> ---
>>>>    drivers/mailbox/mailbox.c               | 24 ++++++++++++++++++++++++
>>>>    drivers/remoteproc/xlnx_r5_remoteproc.c |  4 ++++
>>>>    include/linux/mailbox_client.h          |  1 +
>>>>    3 files changed, 29 insertions(+)
>>>>
>>>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>>>> index 5cd8ae222073..7afdb2c9006d 100644
>>>> --- a/drivers/mailbox/mailbox.c
>>>> +++ b/drivers/mailbox/mailbox.c
>>>> @@ -217,6 +217,30 @@ bool mbox_client_peek_data(struct mbox_chan *chan)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(mbox_client_peek_data);
>>>>
>>>> +/**
>>>> + * mbox_queue_full - check if mailbox queue is full or not
>>>> + * @chan: Mailbox channel assigned to this client.
>>>> + *
>>>> + * Clients can choose not to send new msg if mbox queue is full.
>>>> + *
>>>> + * Return: true if queue is full else false. < 0 for error
>>>> + */
>>>> +int mbox_queue_full(struct mbox_chan *chan)
>>>> +{
>>>> +       unsigned long flags;
>>>> +       int res;
>>>> +
>>>> +       if (!chan)
>>>> +               return -EINVAL;
>>>> +
>>>> +       spin_lock_irqsave(&chan->lock, flags);
>>>> +       res = (chan->msg_count == (MBOX_TX_QUEUE_LEN - 1));
>>>>
>>> 1) If we really need this, it should be
>>>           res = (chan->msg_count == MBOX_TX_QUEUE_LEN);
>>>
>>
>> Ack here.
>>
>>> 2) I am thinking instead, introduce a
>>>          struct mbox_client.msg_slots_ro;
>>>     Which is a read-only field for the client, denoting how many message
>>> slots are currently available.
>>>     The mailbox api will always adjust it when msg_count changes...
>>>         chan->cl->msg_slots_ro = MBOX_TX_QUEUE_LEN - chan->msg_count;
>>>
>>
>> It's not possible to make msg_slots_ro true Read-Only. Nothing prevents
>> clients to write to that variable as far as I know.
>>
> Correct, nothing prevents a client from changing 'msg_slots_ro', just
> like nothing
> prevents it from setting tx_done or rx_callback to 0xdeadbabe.
> The '_ro' suffix is to tell the client developer to not mess with it.
> I am slightly more inclined towards this approach because it avoids
> adding another
> convenience api and is more immediately available without needing a spinlock.

To avoid spinlock, msg_slots_ro should be atomic right?

> 

Then in the remoteproc driver, before using mbox_send_message, I can 
simply check (chan->cl->msg_slots_ro == 0) then don't use mbox_send_message.


> -jassi

Re: [PATCH] mailbox: check mailbox queue is full or not

[Tanmay Shah]

On 9/30/25 12:58 PM, Tanmay Shah wrote:
> 
> 
> On 9/30/25 12:33 PM, Jassi Brar wrote:
>> On Tue, Sep 30, 2025 at 11:52 AM Tanmay Shah <tanmay.shah@amd.com> wrote:
>>>
>>> Hi Jassi,
>>>
>>> Please find my comments below.
>>>
>>> On 9/30/25 9:11 AM, Jassi Brar wrote:
>>>> On Thu, Sep 25, 2025 at 1:51 PM 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. This API can help avoid false positive warning from mailbox
>>>>> framework "Try increasing MBOX_TX_QUEUE_LEN".
>>>>>
>>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>>> ---
>>>>>    drivers/mailbox/mailbox.c               | 24 +++++++++++++++++++ 
>>>>> +++++
>>>>>    drivers/remoteproc/xlnx_r5_remoteproc.c |  4 ++++
>>>>>    include/linux/mailbox_client.h          |  1 +
>>>>>    3 files changed, 29 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>>>>> index 5cd8ae222073..7afdb2c9006d 100644
>>>>> --- a/drivers/mailbox/mailbox.c
>>>>> +++ b/drivers/mailbox/mailbox.c
>>>>> @@ -217,6 +217,30 @@ bool mbox_client_peek_data(struct mbox_chan 
>>>>> *chan)
>>>>>    }
>>>>>    EXPORT_SYMBOL_GPL(mbox_client_peek_data);
>>>>>
>>>>> +/**
>>>>> + * mbox_queue_full - check if mailbox queue is full or not
>>>>> + * @chan: Mailbox channel assigned to this client.
>>>>> + *
>>>>> + * Clients can choose not to send new msg if mbox queue is full.
>>>>> + *
>>>>> + * Return: true if queue is full else false. < 0 for error
>>>>> + */
>>>>> +int mbox_queue_full(struct mbox_chan *chan)
>>>>> +{
>>>>> +       unsigned long flags;
>>>>> +       int res;
>>>>> +
>>>>> +       if (!chan)
>>>>> +               return -EINVAL;
>>>>> +
>>>>> +       spin_lock_irqsave(&chan->lock, flags);
>>>>> +       res = (chan->msg_count == (MBOX_TX_QUEUE_LEN - 1));
>>>>>
>>>> 1) If we really need this, it should be
>>>>           res = (chan->msg_count == MBOX_TX_QUEUE_LEN);
>>>>
>>>
>>> Ack here.
>>>
>>>> 2) I am thinking instead, introduce a
>>>>          struct mbox_client.msg_slots_ro;
>>>>     Which is a read-only field for the client, denoting how many 
>>>> message
>>>> slots are currently available.
>>>>     The mailbox api will always adjust it when msg_count changes...
>>>>         chan->cl->msg_slots_ro = MBOX_TX_QUEUE_LEN - chan->msg_count;
>>>>
>>>
>>> It's not possible to make msg_slots_ro true Read-Only. Nothing prevents
>>> clients to write to that variable as far as I know.
>>>
>> Correct, nothing prevents a client from changing 'msg_slots_ro', just
>> like nothing
>> prevents it from setting tx_done or rx_callback to 0xdeadbabe.
>> The '_ro' suffix is to tell the client developer to not mess with it.
>> I am slightly more inclined towards this approach because it avoids
>> adding another
>> convenience api and is more immediately available without needing a 
>> spinlock.
> 
> To avoid spinlock, msg_slots_ro should be atomic right?

I take it back, it doesn't have to be atomic. I will send a new revision 
with the design we discussed here.

> 
>>
> 
> Then in the remoteproc driver, before using mbox_send_message, I can 
> simply check (chan->cl->msg_slots_ro == 0) then don't use 
> mbox_send_message.
> 
> 
>> -jassi
> 
>