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

[Tanmay Shah <tanmay.shah@amd.com>]

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