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] [day] [month] [year] [list]

Message-ID: <20250928075641.GA29690@nxa18884-linux.ap.freescale.net>
Date: Sun, 28 Sep 2025 15:56:41 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Tanmay Shah <tanmay.shah@....com>
Cc: jassisinghbrar@...il.com, 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

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.

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

>
>> 
>> > +	res = (chan->msg_count == (MBOX_TX_QUEUE_LEN - 1));
>> > +	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.

Regards,
Peng

>
>
>> > +
>> > /**
>> >   * mbox_send_message -	For client to submit a message to be
>> >   *				sent to the remote.
>> 
>> Regards
>> Peng
>