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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8cb065f6-eee3-49f4-b657-1f4c74f1b324@amd.com>
Date: Mon, 29 Sep 2025 14:26:01 -0500
From: Tanmay Shah <tanmay.shah@....com>
To: Mathieu Poirier <mathieu.poirier@...aro.org>, Peng Fan
	<peng.fan@....nxp.com>
CC: <jassisinghbrar@...il.com>, <andersson@...nel.org>,
	<linux-kernel@...r.kernel.org>, <linux-remoteproc@...r.kernel.org>
Subject: Re: [PATCH] mailbox: check mailbox queue is full or not



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