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

  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.

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

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

On 1/9/2023 1:34 PM, Alex Elder wrote:
> 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?

Both of the current drivers that use mbox_bind_client use TXDONE_BY_IRQ, 
so this doesn't cause issue for checking whether the client has 
txdone_method.

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

The function for this is "mbox_free_channel".

Thanks,
Elliot
  

On 1/10/23 11:57 AM, Elliot Berman wrote:
> 
> 
> On 1/9/2023 1:34 PM, Alex Elder wrote:
>> 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?

Sorry, reading that now, I see I placed an "if" in the wrong spot.

> Both of the current drivers that use mbox_bind_client use TXDONE_BY_IRQ, 
> so this doesn't cause issue for checking whether the client has 
> txdone_method.

I'm not so sure, but it's on you to make sure you don't break
anything...  I see only two spots where TXDONE_BY_IRQ is set,
and TXDONE_BY_IRQ seems to be set when channels are freed.

I spent (too much) time trying to track this back but I'm
giving up.  If you're sure it's correct, I accept that...

>>
>> 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.
> n
> The function for this is "mbox_free_channel".

My point is about the way you are abstracting the "bind" operation
as a (now encapsulated) part of requesting the channel.  Yes, when
mbox_free_channel() is called, it effectively "unbinds" the channel.
But you're creating a "bind" abstraction, where it's not explicit
that you're requesting the channel.  I'm suggesting you also create
an "unbind" operation to reverse that.

This is more important for the mbox_bind_client() call than 
mbox_request_channel().  (And by the way, it looks like
pcc_mbox_free_channel() doesn't call pcc_mbox_free_channel()
as it should, but this unfamiliar code...)

And... it's weird to me that gh_rm_drv_probe() calls gh_msgq_init()
(to initialize the Gunhah message queue code), but then has to
call mbox_free_channel() *separate* from gh_msgq_remove(); the
free channel (or better, unbind client) should happen in the
message queue code.

It's not a critically important point, but now at least I hope
you understand what I'm trying to say.

					-Alex
> 
> Thanks,
> Elliot