| Message ID | 20220502090225.26478-1-marcan@marcan.st (mailing list archive) |
|---|---|
| Headers | show |
| Series | mailbox: apple: peek_data cleanup and implementation | expand |
On 02/05/2022 18.02, Hector Martin wrote: > Cc: Anup Patel <anup.patel@broadcom.com> > Cc: Vinod Koul <vkoul@kernel.org> (maintainer:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM) > Cc: Sven Peter <sven@svenpeter.dev> (maintainer:ARM/APPLE MACHINE SUPPORT) > Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io> (reviewer:ARM/APPLE MACHINE SUPPORT) > To: Jassi Brar <jassisinghbrar@gmail.com> (maintainer:MAILBOX API) Sigh, why do I always screw up git send-emails... Jassi: this was supposed to be for you, but I had a spurious newline in the cover file and that dropped the to: (but not the cc:s). Let me know if you need the whole thing resent if you didn't get it via lists. My apologies.
From: Jassi Brar <jassisinghbrar@gmail.com> 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.
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...).