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

[Alex Elder]

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@xxxxxxxxxxx>
> > > ---
> > >   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