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

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

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