[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9934e6d0-8f38-4a8e-ae0f-fb86b24cd44c@amd.com>
Date: Tue, 30 Sep 2025 12:58:21 -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: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?
>
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