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: <aNqbc5Q_tVStXkhI@p14s>
Date: Mon, 29 Sep 2025 08:45:07 -0600
From: Mathieu Poirier <mathieu.poirier@...aro.org>
To: Peng Fan <peng.fan@....nxp.com>
Cc: Tanmay Shah <tanmay.shah@....com>, 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 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
 
> >
> >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

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

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