Re: [PATCH 0/7] mailbox: apple: peek_data cleanup and implementation

[Hector Martin <marcan@marcan.st>]

On 24/05/2022 23.55, jassisinghbrar@gmail.com wrote:
> From: Jassi Brar <jassisinghbrar@gmail.com>
>> 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 (marcan@marcan.st)
Public Key: https://mrcn.st/pub