Cc: Anup Patel <[email protected]>
Cc: Vinod Koul <[email protected]> (maintainer:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM)
Cc: Sven Peter <[email protected]> (maintainer:ARM/APPLE MACHINE SUPPORT)
Cc: Alyssa Rosenzweig <[email protected]> (reviewer:ARM/APPLE MACHINE SUPPORT)
To: Jassi Brar <[email protected]> (maintainer:MAILBOX API)
Cc: Mun Yew Tham <[email protected]> (maintainer:ALTERA MAILBOX DRIVER)
Cc: Chen-Yu Tsai <[email protected]> (maintainer:ARM/Allwinner sunXi SoC support)
Cc: Jernej Skrabec <[email protected]> (maintainer:ARM/Allwinner sunXi SoC support)
Cc: Samuel Holland <[email protected]> (maintainer:ARM/Allwinner sunXi SoC support)
Cc: Michal Simek <[email protected]> (supporter:ARM/ZYNQ ARCHITECTURE)
Cc: Arnd Bergmann <[email protected]>
Cc: [email protected] (open list:DOCUMENTATION)
Cc: [email protected] (open list)
Cc: [email protected] (open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM)
Cc: [email protected] (moderated list:ARM/APPLE MACHINE SUPPORT)
Cc: [email protected] (open list:ARM/Allwinner sunXi SoC support)
Hi all,
We had to implement atomic mailbox operations for apple-mailbox, and
along the way we ran into a mailbox API issue. This series attempts
to clean up the problem first, and then adds the apple implementation.
The mailbox API has a `peek_data` operation. Its intent and
documentation is rather ambiguous; at first glance and based on the
name, it seems like it should only check for whether data is currently
pending in the controller, without actually delivering it to the
consumer. However, this interpretation is not useful for anything: the
function can be called from atomic context, but without a way to
actually *poll* for data from atomic context, there is no use in just
checking for whether data is available.
A more useful operation would be one that actually *polls* for incoming
data and delivers it to the consumer, synchronously and from atomic
context. This is what we need for apple-mailbox (in particular because
the upcoming SMC driver needs to be able to talk to the mailbox from
atomic context, for reboot/shutdown requests and possibly panic stuff).
Over time, various drivers have implemented this with "peek"
semantics... and none of them have any users. Which isn't surprising,
given how these sematics aren't terribly useful :-)
There is, however, one driver that has instead interpreted this as a
poll operation: bcm-flexrm-mailbox. And, in fact, that is the only
mailbox with a consumer that actually uses the peek_data op.
So, it seems pretty clear that we should rename this to poll_data and
fix the documentation. Since the existing "peek" semantics
implementations are unused, we can just remove them. That leaves just
bcm-flexrm-mailbox (producer) and bcm-sba-raid (consumer) to fix up
along with the rename. This series does that, then implements the
missing ops for apple-mailbox.
Merge notes: it would be helpful if we could merge this via the SoC
tree, or otherwise I can provide a git branch so you can pull the
changes directly, and then we can merge it into SoC as well.
The upcoming SMC driver needs poll_data, and that will allow us to
merge that with the proper dependencies without waiting for a merge
cycle in between.
Hector Martin (7):
mailbox: zynq: Remove unused zynqmp_ipi_peek_data
mailbox: sun6i: Unexport unused sun6i_msgbox_peek_data
mailbox: ti-msgmgr Remove unused ti_msgmgr_queue_peek_data
mailbox: altera: Remove unused altera_mbox_peek_data
mailbox: Rename peek_data to poll_data and fix documentation
mailbox: apple: Implement flush() operation
mailbox: apple: Implement poll_data() operation
Documentation/driver-api/mailbox.rst | 2 +-
drivers/dma/bcm-sba-raid.c | 4 +-
drivers/mailbox/apple-mailbox.c | 64 ++++++++++++++++++++++++++--
drivers/mailbox/bcm-flexrm-mailbox.c | 4 +-
drivers/mailbox/mailbox-altera.c | 8 ----
drivers/mailbox/mailbox.c | 25 +++++------
drivers/mailbox/sun6i-msgbox.c | 1 -
drivers/mailbox/ti-msgmgr.c | 28 ------------
drivers/mailbox/zynqmp-ipi-mailbox.c | 41 ------------------
include/linux/mailbox_client.h | 2 +-
include/linux/mailbox_controller.h | 6 +--
11 files changed, 81 insertions(+), 104 deletions(-)
--
2.35.1
On 24/05/2022 23.55, [email protected] wrote:
> From: Jassi Brar <[email protected]>
>> The mailbox API has a `peek_data` operation. Its intent and
>> documentation is rather ambiguous; at first glance and based on the
>> name, it seems like it should only check for whether data is currently
>> pending in the controller, without actually delivering it to the
>> consumer. However, this interpretation is not useful for anything: the
>> function can be called from atomic context, but without a way to
>> actually *poll* for data from atomic context, there is no use in just
>> checking for whether data is available.
>>
> Not exactly... the 'peek_data' is a means for client driver to hint the
> controller driver that some data might have arrived (for controllers that
> don't have anything like RX-Irq). The controller is then expected to dispatch
> data after "not necessarily atomic" read.
If that was the intent, there are no in-kernel users with the "hint"
intent... I am having a hard time imagining a use case for those semantics.
Are there any controllers without an RX IRQ? What do they do, poll
constantly? Or just assume all requests are req/response and have
drivers poll via this function when a request is pending? And in that
case wouldn't reading be atomic too anyway?
> For example, a quick look at some bit may tell there is data available,
> but actually reading the data from buffer may be non-atomic.
Are there any examples of mailbox drivers that have this constraint?
> In your case, you could already implement the patch-7/7 by simply calling it
> peek_data() instead of poll_data(). Its ok to call mbox_chan_received_data()
> from peek_data() because your data-read can be atomic.
So some mailboxes may implement peek_data in a way that guarantees
atomic/synchronous data arrival, and some may not, and consumers are
expected to just know how their particular mailbox behaves?
That doesn't sound like a very good API design...
> Also some platforms may not have users of peek_data upstream (yet), so
> simply weeding them out may not be right.
That's why everyone involved is CCed :)
I'm going to be honest though: I'm finding the entire mailbox
abstraction to be very frustrating. It's trying to cater to a bunch of
rather disparate hardware used as a low-level channel for very tightly
coupled drivers and, in the end, fails to be a useful abstraction since
it can't abstract those differences away. It would've taken us less code
to open-code the mailbox part of our driver into its only consumer,
would've saved a bunch of debugging and headaches, and would perform
better, and wouldn't lose any generality since we only have one consumer
anyway (and if we had more it'd still take less code to roll our own API
rather than using mailbox...).
--
Hector Martin ([email protected])
Public Key: https://mrcn.st/pub
From: Jassi Brar <[email protected]>
Hi,
> The mailbox API has a `peek_data` operation. Its intent and
> documentation is rather ambiguous; at first glance and based on the
> name, it seems like it should only check for whether data is currently
> pending in the controller, without actually delivering it to the
> consumer. However, this interpretation is not useful for anything: the
> function can be called from atomic context, but without a way to
> actually *poll* for data from atomic context, there is no use in just
> checking for whether data is available.
>
Not exactly... the 'peek_data' is a means for client driver to hint the
controller driver that some data might have arrived (for controllers that
don't have anything like RX-Irq). The controller is then expected to dispatch
data after "not necessarily atomic" read.
For example, a quick look at some bit may tell there is data available,
but actually reading the data from buffer may be non-atomic.
In your case, you could already implement the patch-7/7 by simply calling it
peek_data() instead of poll_data(). Its ok to call mbox_chan_received_data()
from peek_data() because your data-read can be atomic.
Also some platforms may not have users of peek_data upstream (yet), so
simply weeding them out may not be right.
thanks.