On Wed, Nov 28, 2018 at 09:43:29AM +0000, Jon Hunter wrote:
> 
> On 12/11/2018 15:18, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > The mailbox framework supports blocking transfers via completions for
> > clients that can sleep. In order to support blocking transfers in cases
> > where the transmission is not permitted to sleep, add a new ->flush()
> > callback that controller drivers can implement to busy loop until the
> > transmission has been completed. This will automatically be called when
> > available and interrupts are disabled for clients that request blocking
> > transfers.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/mailbox/mailbox.c          | 8 ++++++++
> >  include/linux/mailbox_controller.h | 4 ++++
> >  2 files changed, 12 insertions(+)
> > 
> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> > index 674b35f402f5..0eaf21259874 100644
> > --- a/drivers/mailbox/mailbox.c
> > +++ b/drivers/mailbox/mailbox.c
> > @@ -267,6 +267,14 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
> >  		unsigned long wait;
> >  		int ret;
> >  
> > +		if (irqs_disabled() && chan->mbox->ops->flush) {
> > +			ret = chan->mbox->ops->flush(chan, chan->cl->tx_tout);
> > +			if (ret < 0)
> > +				tx_tick(chan, ret);
> > +
> > +			return ret;
> > +		}
> 
> It seems to me that if mbox_send_message() is called from an atomic
> context AND tx_block is true, then if 'flush' is not populated this
> should be an error condition as we do not wish to call
> wait_for_completion from an atomic context.
> 
> I understand that there is some debate about adding such flush support,
> but irrespective of the above change, it seems to me that if the
> mbox_send_message() can be called from an atomic context (which it
> appears to), then it should be detecting if someone is trying to do so
> with 'tx_block' set as this should be an error.
> 
> Furthermore, if the underlying mailbox driver can support sending a
> message from an atomic context and busy wait until it is done, surely
> the mailbox framework should provide a means to support this?
> 
> Now it could be possible for the underlying mailbox driver to detect we
> are in an atomic context and if the 'tx_block' is set do the right thing
> by busy waiting until the message is sent. However, the problem with
> that is, that for the mbox_send_message() to ensure the right thing is
> done, it needs to check that 'tx_done' is set in the case of a blocking
> transfer in an atomic context. At that point you may as well add the
> flush operator as I think it is more implicit/clear.

Heh, interesting timing, I just sent out v3 of this series with a
slightly different solution. Basically what v3 implements is explicit
flushing of the mailbox via a new function called mbox_flush().

I originally liked the idea of "hiding" the flush operation in the
mbox_send_message() function, but the more I look at the new explicit
flush solution, the more I prefer it. One reason why I prefer it is
because it no longer has the slightly odd "irqs_disabled()" check that
always made me a bit uneasy. The other advantage of the explicit flush
is that it becomes completely opt-in for both mailbox providers and
consumers. Furthermore the consumer should always know if it can be
called in atomic context or not, so none of these code paths should
have to be "clever" and check at runtime whether or not interrupts are
enabled. If the consumer knows that it will be called in atomic context
it can just unconditionally flush the mailbox.

Also, it nicely addresses the concern about erroring out if the provider
doesn't support flushing. The new mbox_flush() API returns -ENOTSUPP if
called on a mailbox channel provided by a driver that doesn't implement
->flush().

Thierry