Re: [PATCH v2 01/10] mailbox: Support blocking transfers in atomic context

Jassi Brar
2018-12-10T21:32:20Z

On Tue, Dec 11, 2018 at 2:15 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Tue, Dec 11, 2018 at 02:00:48AM +0530, Jassi Brar wrote:
> > On Mon, Dec 10, 2018 at 3:22 PM Thierry Reding <thierry.reding@gmail.com> wrote:

> > > >
> > > > The 'flush' api is going to have no use other than implement
> > > > busy-waits. I am afraid people are simply going to take it easy and
> > > > copy the busy-waits from the test/verification codes.
> > > > "discouraging" seldom works because the developer comes with the
> > > > failproof excuse "the h/w doesn't provide any other way". Frankly I
> > > > would like to push back on such designs.
> > >
> > > I certainly approve of that stance.
> > >
> > > However, I'd like to reiterate that the TCU design does in fact support
> > > "another way". If you look at the mailbox driver implementation (in the
> > > drivers/mailbox/tegra-hsp.c driver), you'll see that it does support an
> > > interrupt driven mode. After you had reviewed Mikko's earliest proposal
> > > that was indeed implementing busy looping exclusively we both set out
> > > to properly implement interrupt support. This took quite a while to do
> > > because of some gaps in the documentation, but we eventually got it to
> > > work properly. And then it was discovered that it was all a waste of
> > > effort because the console driver couldn't make use of it anyway. Well,
> > > I should say "waste of effort" because I'm still happy that the proper
> > > implementation exists and we can use it under the right circumstances.
> > >
> > > So, at least in this particular case, it is not the hardware or firmware
> > > design that's flawed or was taking any shortcuts. It's really just the
> > > particular use-case of the console that doesn't allow us to make use of
> > > the right implementation, which is why we have to resort to the inferior
> > > method of busy-looping.
> > >
> > I am not complaining about the hardware, but the firmware.
> > It is essential we dump logs during early boot but the platform(Linux)
> > doesn't have access to serial port. All the firmware allows is 24bits
> > per transfer!! We could do better.
>
> Hardware UARTs don't usually have much more FIFO space than that either.
>
> > A smarter option could have been Linux simply placing characters in
> > the shmem ring-buffer, while the remote consuming (and then poisoning)
> > the ring buffer upon 'hint' from Linux.
>
> I don't think that would've been much smarter, especially not in this
> case. As we discussed earlier, no matter how large you make the ring-
> buffer you can always run into situations where you overflow it. The
> ring-buffer implementation is also a lot more complicated and error-
> prone.
>
Please think about it again.
The ring buffer becomes the effective h/w fifo. And you don't have to
wait at all for the mailbox register to clear .... you could simply
overwrite it when Linux puts some data in the ring-buffer (the data
written will just be a 'hint' command for remote to go look into the
ring-buffer for new data).

> Plus there is the fact that in this particular case we actually
> don't want buffering because the buffer may hide important information
> in case of a crash.
>
Even if the Linux crashes, whatever data is placed in the ring-buffer
will eventually be printed by the still-alive remote.

Anyways once we have the flush api, I don't really care how broken your f/w is.