Message-ID: <CABb+yY2=B2p5JhZiBE_mZLe3y16EUgVsUHK62LnRgaKa1-ptLg@mail.gmail.com>
Date:   Wed, 29 Mar 2023 11:04:37 -0500
From:   Jassi Brar <jassisinghbrar@...il.com>
To:     Hector Martin <marcan@...can.st>
Cc:     Sven Peter <sven@...npeter.dev>,
        Alyssa Rosenzweig <alyssa@...enzweig.io>,
        Janne Grunau <j@...nau.net>, linux-kernel@...r.kernel.org,
        asahi@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 0/5] mailbox: apple: Move driver into soc/apple and stop
 using the subsystem

On Tue, Mar 28, 2023 at 8:14 AM Hector Martin <marcan@...can.st> wrote:
>
> Once upon a time, Apple machines had some mailbox hardware, and we had
> to write a driver for it. And since it was a mailbox, it felt natural to
> use the Linux mailbox subsystem.
>
> More than a year later, it has become evident that was not the right
> decision.
>
> Linux driver class subsystems generally exist for a few purposes:
> 1. To allow mixing and matching generic producers and consumers.
> 2. To implement common code that is likely to be shared across drivers,
>    and do so correctly so correct code only has to be written once.
> 3. To force drivers into a "correct" design, such that driver authors
>    avoid common pitfalls.
>
The Mailbox subsystem is meant to serve (2) and possibly (3).
We never aimed for (1), we can't... because the firmware on the remote
end is also a part of "local hardware" -- keeping every bit of
hardware the same, if just the f/w changes you may have to change the
linux side driver.


> The mailbox subsystem does not do any of the above for us:
> 1. Mailbox hardware is not generic; "mailbox" is a vague idea, not a
>    standard for communication.
>
There _can not_ be a standard mailbox implementation without a
specification -- which doesn't exist.

>  Almost all mailbox consumers are tied to
>    one or a few producers. There is practically no mixing and matching
>    possible. We have one (1) consumer subsystem (RTKit) talking to one
>    (1) mailbox driver (apple-mailbox). We might have a second consumer
>    in the future (SEP), but there will still be no useful
>    combinatronics with other drivers.
>
Sorry I don't follow what you expect.

> 2. The mailbox subsystem implements a bunch of common code for queuing,
>    but we don't need that because our hardware has hardware queues. It
>    also implements a bunch of common code for supporting multiple
>    channels, but we don't need that because our hardware only has one
>    channel (it has "endpoints" but those are just tags that are part of
>    the message).
>
In note-1, you complain it is not standard/flexible enough, and here
its support for features, that you don't need, become a problem?


> On top of this, the mailbox subsystem makes design
>    decisions unsuitable for our use case. Its queuing implementation
>    has a fixed queue size and fails sends when full instead of pushing
>    back by blocking, which is completely unsuitable for high-traffic
>    mailboxes with hard reliability requirements, such as ours. We've
>    also run into multiple issues around using mailbox in an atomic
>    context (required for SMC reboot/shutdown requests).
>
I don't think you ever shared the issues you were struggling with.
Not to mean there can be no limitation but I knew a platform that ran
a virtual block-device and custom filesystem over the mailbox, and it
was good for a camera product that needs high bandwidth image
transfer.


> 3. Mailbox doesn't really do much for us as far as driver design.
>    If anything, it has been forcing us to add extra workarounds for the
>    impedance mismatches between the subsystem core and the hardware.
>    Other drivers already are inconsistent in how they use the mailbox
>    core, because the documentation is unclear on various details.
>
Again, would have helped to know the issues and details you feel missing.


> This series offers:
>
> - A modest reduction in overall code size (-27 net lines excluding #1).
> - Fixes a pile of bugs related to using the mailbox subsystem and its
>   quirks and limitations (race conditions when messages are already in
>   the queue on startup, atomic issues, the list goes on).
> - Adds runtime-PM support.
> - Adds support for the FIFOs in the mailbox hardware, improving
>   performance.
> - Simplifies code by removing extraneous memory allocations (the
>   mailbox subsystem requires consumers to pass pointers to messages,
>   instead of inlining them, even though messages are usually only one or
>   two registers in size) and the requisite cleanup/freeing in the
>   completion path.
>
> In addition, it paves the way for future Apple-specific mailbox
> optimizations, such as adding the ability to de-duplicate message sends
> if the same message is already known to be in the FIFO (which can be
> done by keeping a rolling history of recently sent messages). This is
> useful for doorbell-style messages, which are redundant to send more
> than once if not yet processed.
>
> Apple Silicon platforms use these mailboxes pervasively, including as
> part of the GPU submission hot path. On top of that, bad interactions
> with firmware coprocessors can cause immediate lockups or crashes with
> no recovery possible but a reboot. Our requirements for reliability and
> performance are probably much higher than the average mailbox user, and
> we'd much rather not have a bunch of common code getting in the way of
> performance profiling and future optimization. It doesn't make much
> sense for the mailbox subsystem either, since solving these issues would
> require major refactoring that is unlikely to provide significant
> benefit to most other users.
>
> So let's just call usage of the mailbox subsystem a failed experiment,
> and move the driver into soc/apple, where we can control the API and can
> add whatever peculiarities we need for our mailboxes. Farewell, mailbox.
>
> There are no changes to the DT bindings. This driver has been shipping
> in Asahi stable kernel packages for a week, with no regressions
> reported by any users.
>
> As an additional non-kernel-related benefit, this introduces a direct
> module dependency between consumers and the mailbox producer. This, in
> turn, is in the critical path for the NVMe driver on these platforms.
> Prior to this series, we had to have custom initramfs hooks to add
> apple-mailbox to distro initramfses, and accidentally removing these
> hooks would result in a completely unbootable system (there is no way
> for standard initramfs machinery to detect soft consumer/producer
> relationships like this, they usually just go looking for block device
> modules and their direct dependencies). We still need the initramfs
> hooks for other things, but with this change, completely removing all
> Apple-related initramfs hooks will at least result in a *bootable*
> system so you can fix the problem. This has already bit several users,
> and it also means many more distros have a chance of working out of the
> box (enough to let you install extra stuff) on these platforms, instead
> of having a hard requirement that *every single distro* fix up their
> initramfs generation in order to even boot/install on these platforms at
> all.
>
> Jassi: Ideally I'd like to get an ack on this and merge it all through
> asahi-soc, so we don't have to play things patch-by-patch across
> multiple merge cycles to avoid potentially broken intermediate states.
>
Instead of every platform implementing their own message queuing and
handling mechanism and parsing producer-comsumer links from the DT,
there is a reusable code in drivers/mailbox.   Which does seem
inadequate if one comes looking for a "standard/generic" mailbox
implementation (again, which can not exist).

And there is a reason the reduction in code with this patchset is
meager -- you anyway have to implement your platform specific stuff.
But if redoing mailbox overall saves you complexity, I am ok with it.

cheers.

Powered by blists - more mailing lists