Messages in this thread | | | Date | Tue, 17 Jan 2023 13:21:08 -0600 | From | Alex Elder <> | Subject | Re: [PATCH v8 07/28] mailbox: Allow direct registration to a channel |
| |
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
| |