Re: [PATCH v8 07/28] mailbox: Allow direct registration to a channel

From: Alex Elder <elder@linaro.org>
To: Elliot Berman <quic_eberman@quicinc.com>,
	Bjorn Andersson <quic_bjorande@quicinc.com>,
	Jassi Brar <jassisinghbrar@gmail.com>,
	Sudeep Holla <sudeep.holla@arm.com>
Cc: Murali Nalajala <quic_mnalajal@quicinc.com>,
	Trilok Soni <quic_tsoni@quicinc.com>,
	Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>,
	Carl van Schaik <quic_cvanscha@quicinc.com>,
	Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Jonathan Corbet <corbet@lwn.net>,
	Bagas Sanjaya <bagasdotme@gmail.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org
Subject: Re: [PATCH v8 07/28] mailbox: Allow direct registration to a channel
Date: Mon, 9 Jan 2023 15:34:33 -0600	[thread overview]
Message-ID: <942f20cc-87d3-d014-1527-8d3d3e1ae44b@linaro.org> (raw)
In-Reply-To: <20221219225850.2397345-8-quic_eberman@quicinc.com>

On 12/19/22 4:58 PM, Elliot Berman wrote:
> Support virtual mailbox controllers and clients which are not platform
> devices or come from the devicetree by allowing them to match client to
> channel via some other mechanism.

The new function behaves very much like mbox_request_channel()
did before.

The new function differs from omap_mbox_request_channel() in that
it can change the if chan->txdone_method is TXDONE_BY_POLL, it
is changed to TXDONE_BY_ACK if the client's knows_txdone field is
set.  Is this OK?  Why?

It also assumes chan->mbox->ops->startup us non-null (though that
isn't really a problem).

> 
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>   drivers/mailbox/mailbox.c      | 96 ++++++++++++++++++++++++----------
>   drivers/mailbox/omap-mailbox.c | 18 ++-----
>   drivers/mailbox/pcc.c          | 18 ++-----
>   include/linux/mailbox_client.h |  1 +
>   4 files changed, 76 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 4229b9b5da98..adf36c05fa43 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -317,6 +317,71 @@ int mbox_flush(struct mbox_chan *chan, unsigned long timeout)
>   }
>   EXPORT_SYMBOL_GPL(mbox_flush);
>   
> +static int __mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl)

There should be an unbind_client() call.  At a minimum, you are
calling try_module_get(), and the matching module_put() call
would belong there.  And even though one might just call
module_put() elsewhere for this, it would be cleaner to have
a function that similarly encapsulates the shutdown call
as well.

					-Alex

> +{
> +	struct device *dev = cl->dev;
> +	unsigned long flags;
> +	int ret;
> +
> +	if (chan->cl || !try_module_get(chan->mbox->dev->driver->owner)) {
> +		dev_dbg(dev, "%s: mailbox not free\n", __func__);
> +		return -EBUSY;
> +	}
> +
> +	spin_lock_irqsave(&chan->lock, flags);
> +	chan->msg_free = 0;
> +	chan->msg_count = 0;
> +	chan->active_req = NULL;
> +	chan->cl = cl;
> +	init_completion(&chan->tx_complete);
> +
> +	if (chan->txdone_method	== TXDONE_BY_POLL && cl->knows_txdone)
> +		chan->txdone_method = TXDONE_BY_ACK;
> +
> +	spin_unlock_irqrestore(&chan->lock, flags);
> +
> +	if (chan->mbox->ops->startup) {
> +		ret = chan->mbox->ops->startup(chan);
> +
> +		if (ret) {
> +			dev_err(dev, "Unable to startup the chan (%d)\n", ret);
> +			mbox_free_channel(chan);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * mbox_bind_client - Request a mailbox channel.
> + * @chan: The mailbox channel to bind the client to.
> + * @cl: Identity of the client requesting the channel.
> + *
> + * The Client specifies its requirements and capabilities while asking for
> + * a mailbox channel. It can't be called from atomic context.
> + * The channel is exclusively allocated and can't be used by another
> + * client before the owner calls mbox_free_channel.
> + * After assignment, any packet received on this channel will be
> + * handed over to the client via the 'rx_callback'.
> + * The framework holds reference to the client, so the mbox_client
> + * structure shouldn't be modified until the mbox_free_channel returns.
> + *
> + * Return: 0 if the channel was assigned to the client successfully.
> + *         <0 for request failure.
> + */
> +int mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl)
> +{
> +	int ret;
> +
> +	mutex_lock(&con_mutex);
> +	ret = __mbox_bind_client(chan, cl);
> +	mutex_unlock(&con_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(mbox_bind_client);
> +
>   /**
>    * mbox_request_channel - Request a mailbox channel.
>    * @cl: Identity of the client requesting the channel.
> @@ -340,7 +405,6 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
>   	struct mbox_controller *mbox;
>   	struct of_phandle_args spec;
>   	struct mbox_chan *chan;
> -	unsigned long flags;
>   	int ret;
>   
>   	if (!dev || !dev->of_node) {
> @@ -372,33 +436,9 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
>   		return chan;
>   	}
>   
> -	if (chan->cl || !try_module_get(mbox->dev->driver->owner)) {
> -		dev_dbg(dev, "%s: mailbox not free\n", __func__);
> -		mutex_unlock(&con_mutex);
> -		return ERR_PTR(-EBUSY);
> -	}
> -
> -	spin_lock_irqsave(&chan->lock, flags);
> -	chan->msg_free = 0;
> -	chan->msg_count = 0;
> -	chan->active_req = NULL;
> -	chan->cl = cl;
> -	init_completion(&chan->tx_complete);
> -
> -	if (chan->txdone_method	== TXDONE_BY_POLL && cl->knows_txdone)
> -		chan->txdone_method = TXDONE_BY_ACK;
> -
> -	spin_unlock_irqrestore(&chan->lock, flags);
> -
> -	if (chan->mbox->ops->startup) {
> -		ret = chan->mbox->ops->startup(chan);
> -
> -		if (ret) {
> -			dev_err(dev, "Unable to startup the chan (%d)\n", ret);
> -			mbox_free_channel(chan);
> -			chan = ERR_PTR(ret);
> -		}
> -	}
> +	ret = __mbox_bind_client(chan, cl);
> +	if (ret)
> +		chan = ERR_PTR(ret);
>   
>   	mutex_unlock(&con_mutex);
>   	return chan;
> diff --git a/drivers/mailbox/omap-mailbox.c b/drivers/mailbox/omap-mailbox.c
> index 098c82d87137..711a56ec8592 100644
> --- a/drivers/mailbox/omap-mailbox.c
> +++ b/drivers/mailbox/omap-mailbox.c
> @@ -441,21 +441,9 @@ struct mbox_chan *omap_mbox_request_channel(struct mbox_client *cl,
>   	if (!mbox || !mbox->chan)
>   		return ERR_PTR(-ENOENT);
>   
> -	chan = mbox->chan;
> -	spin_lock_irqsave(&chan->lock, flags);
> -	chan->msg_free = 0;
> -	chan->msg_count = 0;
> -	chan->active_req = NULL;
> -	chan->cl = cl;
> -	init_completion(&chan->tx_complete);
> -	spin_unlock_irqrestore(&chan->lock, flags);
> -
> -	ret = chan->mbox->ops->startup(chan);
> -	if (ret) {
> -		pr_err("Unable to startup the chan (%d)\n", ret);
> -		mbox_free_channel(chan);
> -		chan = ERR_PTR(ret);
> -	}
> +	ret = mbox_bind_client(mbox->chan, cl);
> +	if (ret)
> +		return ERR_PTR(ret);
>   
>   	return chan;
>   }
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 3c2bc0ca454c..f655be083369 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -283,7 +283,7 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
>   	struct pcc_chan_info *pchan;
>   	struct mbox_chan *chan;
>   	struct device *dev;
> -	unsigned long flags;
> +	int rc;
>   
>   	if (subspace_id < 0 || subspace_id >= pcc_chan_count)
>   		return ERR_PTR(-ENOENT);
> @@ -296,21 +296,11 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
>   	}
>   	dev = chan->mbox->dev;
>   
> -	spin_lock_irqsave(&chan->lock, flags);
> -	chan->msg_free = 0;
> -	chan->msg_count = 0;
> -	chan->active_req = NULL;
> -	chan->cl = cl;
> -	init_completion(&chan->tx_complete);
> -
> -	if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
> -		chan->txdone_method = TXDONE_BY_ACK;
> -
> -	spin_unlock_irqrestore(&chan->lock, flags);
> +	rc = mbox_bind_client(chan, cl);
> +	if (rc)
> +		return ERR_PTR(rc);
>   
>   	if (pchan->plat_irq > 0) {
> -		int rc;
> -
>   		rc = devm_request_irq(dev, pchan->plat_irq, pcc_mbox_irq, 0,
>   				      MBOX_IRQ_NAME, chan);
>   		if (unlikely(rc)) {
> diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
> index 65229a45590f..734694912ef7 100644
> --- a/include/linux/mailbox_client.h
> +++ b/include/linux/mailbox_client.h
> @@ -37,6 +37,7 @@ struct mbox_client {
>   	void (*tx_done)(struct mbox_client *cl, void *mssg, int r);
>   };
>   
> +int mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl);
>   struct mbox_chan *mbox_request_channel_byname(struct mbox_client *cl,
>   					      const char *name);
>   struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index);