[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <67b30fb5-8fd9-4fd5-b002-aec199b7e88a@amd.com>
Date: Tue, 30 Sep 2025 13:38:01 -0500
From: Tanmay Shah <tanmay.shah@....com>
To: Jassi Brar <jassisinghbrar@...il.com>
CC: <andersson@...nel.org>, <mathieu.poirier@...aro.org>,
<linux-kernel@...r.kernel.org>, <linux-remoteproc@...r.kernel.org>
Subject: Re: [PATCH] mailbox: check mailbox queue is full or not
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@....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@....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@....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
>
>
Powered by blists - more mailing lists