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

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

[Hector Martin]

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)
Cc: Mun Yew Tham <mun.yew.tham@intel.com> (maintainer:ALTERA MAILBOX DRIVER)
Cc: Chen-Yu Tsai <wens@csie.org> (maintainer:ARM/Allwinner sunXi SoC support)
Cc: Jernej Skrabec <jernej.skrabec@gmail.com> (maintainer:ARM/Allwinner sunXi SoC support)
Cc: Samuel Holland <samuel@sholland.org> (maintainer:ARM/Allwinner sunXi SoC support)
Cc: Michal Simek <michal.simek@xilinx.com> (supporter:ARM/ZYNQ ARCHITECTURE)
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-doc@vger.kernel.org (open list:DOCUMENTATION)
Cc: linux-kernel@vger.kernel.org (open list)
Cc: dmaengine@vger.kernel.org (open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM)
Cc: linux-arm-kernel@lists.infradead.org (moderated list:ARM/APPLE MACHINE SUPPORT)
Cc: linux-sunxi@lists.linux.dev (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

[PATCH 1/7] mailbox: zynq: Remove unused zynqmp_ipi_peek_data

[Hector Martin]

This op was ambiguously specified, and the way it was interpreted for
this implementation is not useful. It has no users, so remove it.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/mailbox/zynqmp-ipi-mailbox.c | 41 ----------------------------
 1 file changed, 41 deletions(-)

diff --git a/drivers/mailbox/zynqmp-ipi-mailbox.c b/drivers/mailbox/zynqmp-ipi-mailbox.c
index 31a0fa914274..7c3ef7c8078e 100644
--- a/drivers/mailbox/zynqmp-ipi-mailbox.c
+++ b/drivers/mailbox/zynqmp-ipi-mailbox.c
@@ -177,46 +177,6 @@ static irqreturn_t zynqmp_ipi_interrupt(int irq, void *data)
 	return IRQ_NONE;
 }
 
-/**
- * zynqmp_ipi_peek_data - Peek to see if there are any rx messages.
- *
- * @chan: Channel Pointer
- *
- * Return: 'true' if there is pending rx data, 'false' if there is none.
- */
-static bool zynqmp_ipi_peek_data(struct mbox_chan *chan)
-{
-	struct device *dev = chan->mbox->dev;
-	struct zynqmp_ipi_mbox *ipi_mbox = dev_get_drvdata(dev);
-	struct zynqmp_ipi_mchan *mchan = chan->con_priv;
-	int ret;
-	u64 arg0;
-	struct arm_smccc_res res;
-
-	if (WARN_ON(!ipi_mbox)) {
-		dev_err(dev, "no platform drv data??\n");
-		return false;
-	}
-
-	arg0 = SMC_IPI_MAILBOX_STATUS_ENQUIRY;
-	zynqmp_ipi_fw_call(ipi_mbox, arg0, 0, &res);
-	ret = (int)(res.a0 & 0xFFFFFFFF);
-
-	if (mchan->chan_type == IPI_MB_CHNL_TX) {
-		/* TX channel, check if the message has been acked
-		 * by the remote, if yes, response is available.
-		 */
-		if (ret < 0 || ret & IPI_MB_STATUS_SEND_PENDING)
-			return false;
-		else
-			return true;
-	} else if (ret > 0 && ret & IPI_MB_STATUS_RECV_PENDING) {
-		/* RX channel, check if there is message arrived. */
-		return true;
-	}
-	return false;
-}
-
 /**
  * zynqmp_ipi_last_tx_done - See if the last tx message is sent
  *
@@ -387,7 +347,6 @@ static void zynqmp_ipi_shutdown(struct mbox_chan *chan)
 static const struct mbox_chan_ops zynqmp_ipi_chan_ops = {
 	.startup = zynqmp_ipi_startup,
 	.shutdown = zynqmp_ipi_shutdown,
-	.peek_data = zynqmp_ipi_peek_data,
 	.last_tx_done = zynqmp_ipi_last_tx_done,
 	.send_data = zynqmp_ipi_send_data,
 };
-- 
2.35.1

[PATCH 2/7] mailbox: sun6i: Unexport unused sun6i_msgbox_peek_data

[Hector Martin]

This op was ambiguously specified, and the way it was interpreted for
this implementation is not useful. It has no users, so remove it. The
function is used internally by the driver, so just remove it from the
ops structure.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/mailbox/sun6i-msgbox.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/mailbox/sun6i-msgbox.c b/drivers/mailbox/sun6i-msgbox.c
index 7f8d931042d3..47aedda80f3f 100644
--- a/drivers/mailbox/sun6i-msgbox.c
+++ b/drivers/mailbox/sun6i-msgbox.c
@@ -189,7 +189,6 @@ static const struct mbox_chan_ops sun6i_msgbox_chan_ops = {
 	.startup      = sun6i_msgbox_startup,
 	.shutdown     = sun6i_msgbox_shutdown,
 	.last_tx_done = sun6i_msgbox_last_tx_done,
-	.peek_data    = sun6i_msgbox_peek_data,
 };
 
 static int sun6i_msgbox_probe(struct platform_device *pdev)
-- 
2.35.1

Re: [PATCH 2/7] mailbox: sun6i: Unexport unused sun6i_msgbox_peek_data

[Samuel Holland]

On 5/2/22 4:02 AM, Hector Martin wrote:
> This op was ambiguously specified, and the way it was interpreted for
> this implementation is not useful. It has no users, so remove it. The
> function is used internally by the driver, so just remove it from the
> ops structure.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>

Acked-by: Samuel Holland <samuel@sholland.org>

[PATCH 3/7] mailbox: ti-msgmgr Remove unused ti_msgmgr_queue_peek_data

[Hector Martin]

This op was ambiguously specified, and the way it was interpreted for
this implementation is not useful. It has no users, so remove it.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/mailbox/ti-msgmgr.c | 28 ----------------------------
 1 file changed, 28 deletions(-)

diff --git a/drivers/mailbox/ti-msgmgr.c b/drivers/mailbox/ti-msgmgr.c
index ddac423ac1a9..10881b812df2 100644
--- a/drivers/mailbox/ti-msgmgr.c
+++ b/drivers/mailbox/ti-msgmgr.c
@@ -309,33 +309,6 @@ static irqreturn_t ti_msgmgr_queue_rx_interrupt(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
-/**
- * ti_msgmgr_queue_peek_data() - Peek to see if there are any rx messages.
- * @chan:	Channel Pointer
- *
- * Return: 'true' if there is pending rx data, 'false' if there is none.
- */
-static bool ti_msgmgr_queue_peek_data(struct mbox_chan *chan)
-{
-	struct ti_queue_inst *qinst = chan->con_priv;
-	struct device *dev = chan->mbox->dev;
-	struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
-	const struct ti_msgmgr_desc *desc = inst->desc;
-	int msg_count;
-
-	if (qinst->is_tx)
-		return false;
-
-	if (ti_msgmgr_queue_is_error(desc, qinst)) {
-		dev_err(dev, "Error on channel %s\n", qinst->name);
-		return false;
-	}
-
-	msg_count = ti_msgmgr_queue_get_num_messages(desc, qinst);
-
-	return msg_count ? true : false;
-}
-
 /**
  * ti_msgmgr_last_tx_done() - See if all the tx messages are sent
  * @chan:	Channel pointer
@@ -744,7 +717,6 @@ static DEFINE_SIMPLE_DEV_PM_OPS(ti_msgmgr_pm_ops, ti_msgmgr_suspend, ti_msgmgr_r
 static const struct mbox_chan_ops ti_msgmgr_chan_ops = {
 	.startup = ti_msgmgr_queue_startup,
 	.shutdown = ti_msgmgr_queue_shutdown,
-	.peek_data = ti_msgmgr_queue_peek_data,
 	.last_tx_done = ti_msgmgr_last_tx_done,
 	.send_data = ti_msgmgr_send_data,
 };
-- 
2.35.1

[PATCH 4/7] mailbox: altera: Remove unused altera_mbox_peek_data

[Hector Martin]

This op was ambiguously specified, and the way it was interpreted for
this implementation is not useful. It has no users, so remove it.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/mailbox/mailbox-altera.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/mailbox/mailbox-altera.c b/drivers/mailbox/mailbox-altera.c
index afb320e9d69c..30de424df371 100644
--- a/drivers/mailbox/mailbox-altera.c
+++ b/drivers/mailbox/mailbox-altera.c
@@ -238,13 +238,6 @@ static bool altera_mbox_last_tx_done(struct mbox_chan *chan)
 	return altera_mbox_full(mbox) ? false : true;
 }
 
-static bool altera_mbox_peek_data(struct mbox_chan *chan)
-{
-	struct altera_mbox *mbox = mbox_chan_to_altera_mbox(chan);
-
-	return altera_mbox_pending(mbox) ? true : false;
-}
-
 static int altera_mbox_startup(struct mbox_chan *chan)
 {
 	struct altera_mbox *mbox = mbox_chan_to_altera_mbox(chan);
@@ -279,7 +272,6 @@ static const struct mbox_chan_ops altera_mbox_ops = {
 	.startup = altera_mbox_startup,
 	.shutdown = altera_mbox_shutdown,
 	.last_tx_done = altera_mbox_last_tx_done,
-	.peek_data = altera_mbox_peek_data,
 };
 
 static int altera_mbox_probe(struct platform_device *pdev)
-- 
2.35.1

[PATCH 5/7] mailbox: Rename peek_data to poll_data and fix documentation

[Hector Martin]

peek_data was poorly specified and the documentation implied it should
just check for whether there are messages available to be received.
The intent is for this function to be usable in atomic context. Just
checking for available data is not useful; the subsystem needs to
provide some way to poll for (and actually receive) data in atomic
context for this to make sense.

As one might expect, all existing drivers that implemented the "peek"
semantics had zero actual users, since that interpretation of the
feature doesn't lend itself to a useful purpose. There is only one
driver implementing peek_data as a proper poll: flexrm. That also
happens to be the only one with a user.

So, rename peek_data to poll_data and fix the documentation to actually
describe it properly as a synchronous poll for data. Previous patches
already removed the useless implementations, so this only has to fix the
bcm driver and client besides the subsystem.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 Documentation/driver-api/mailbox.rst |  2 +-
 drivers/dma/bcm-sba-raid.c           |  4 ++--
 drivers/mailbox/bcm-flexrm-mailbox.c |  4 ++--
 drivers/mailbox/mailbox.c            | 25 +++++++++++--------------
 include/linux/mailbox_client.h       |  2 +-
 include/linux/mailbox_controller.h   |  6 +++---
 6 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/Documentation/driver-api/mailbox.rst b/Documentation/driver-api/mailbox.rst
index 0ed95009cc30..e4ccafdb1e00 100644
--- a/Documentation/driver-api/mailbox.rst
+++ b/Documentation/driver-api/mailbox.rst
@@ -27,7 +27,7 @@ Controller Driver (See include/linux/mailbox_controller.h)
 
 
 Allocate mbox_controller and the array of mbox_chan.
-Populate mbox_chan_ops, except peek_data() all are mandatory.
+Populate mbox_chan_ops, except poll_data() all are mandatory.
 The controller driver might know a message has been consumed
 by the remote by getting an IRQ or polling some hardware flag
 or it can never know (the client knows by way of the protocol).
diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index 64239da02e74..841045ecc449 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -223,7 +223,7 @@ static struct sba_request *sba_alloc_request(struct sba_device *sba)
 		 * would have completed which will create more
 		 * room for new requests.
 		 */
-		mbox_client_peek_data(sba->mchan);
+		mbox_client_poll_data(sba->mchan);
 		return NULL;
 	}
 
@@ -555,7 +555,7 @@ static enum dma_status sba_tx_status(struct dma_chan *dchan,
 	if (ret == DMA_COMPLETE)
 		return ret;
 
-	mbox_client_peek_data(sba->mchan);
+	mbox_client_poll_data(sba->mchan);
 
 	return dma_cookie_status(dchan, cookie, txstate);
 }
diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c
index 22acb51531cb..9decb844eff8 100644
--- a/drivers/mailbox/bcm-flexrm-mailbox.c
+++ b/drivers/mailbox/bcm-flexrm-mailbox.c
@@ -1223,7 +1223,7 @@ static int flexrm_send_data(struct mbox_chan *chan, void *data)
 	return flexrm_new_request(ring, NULL, data);
 }
 
-static bool flexrm_peek_data(struct mbox_chan *chan)
+static bool flexrm_poll_data(struct mbox_chan *chan)
 {
 	int cnt = flexrm_process_completions(chan->con_priv);
 
@@ -1449,7 +1449,7 @@ static const struct mbox_chan_ops flexrm_mbox_chan_ops = {
 	.send_data	= flexrm_send_data,
 	.startup	= flexrm_startup,
 	.shutdown	= flexrm_shutdown,
-	.peek_data	= flexrm_peek_data,
+	.poll_data	= flexrm_poll_data,
 };
 
 static struct mbox_chan *flexrm_mbox_of_xlate(struct mbox_controller *cntlr,
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 3e7d4b20ab34..6b13a7047c64 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -200,28 +200,25 @@ void mbox_client_txdone(struct mbox_chan *chan, int r)
 EXPORT_SYMBOL_GPL(mbox_client_txdone);
 
 /**
- * mbox_client_peek_data - A way for client driver to pull data
- *			received from remote by the controller.
+ * mbox_client_poll_data - Poll for pending messages from the remote.
  * @chan: Mailbox channel assigned to this client.
  *
- * A poke to controller driver for any received data.
- * The data is actually passed onto client via the
- * mbox_chan_received_data()
- * The call can be made from atomic context, so the controller's
- * implementation of peek_data() must not sleep.
+ * Asks the controller driver to poll for any available data from the remote.
+ * The data will be synchronously delivered via mbox_chan_received_data().
+ * This call can be made from atomic context, so the controller's
+ * implementation of poll_data() must not sleep.
  *
- * Return: True, if controller has, and is going to push after this,
- *          some data.
- *         False, if controller doesn't have any data to be read.
+ * Return: True if the controller received and has processed some data.
+ *         False if controller didn't have any pending data to read.
  */
-bool mbox_client_peek_data(struct mbox_chan *chan)
+bool mbox_client_poll_data(struct mbox_chan *chan)
 {
-	if (chan->mbox->ops->peek_data)
-		return chan->mbox->ops->peek_data(chan);
+	if (chan->mbox->ops->poll_data)
+		return chan->mbox->ops->poll_data(chan);
 
 	return false;
 }
-EXPORT_SYMBOL_GPL(mbox_client_peek_data);
+EXPORT_SYMBOL_GPL(mbox_client_poll_data);
 
 /**
  * mbox_send_message -	For client to submit a message to be
diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
index 65229a45590f..ccfc81748295 100644
--- a/include/linux/mailbox_client.h
+++ b/include/linux/mailbox_client.h
@@ -43,7 +43,7 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index);
 int mbox_send_message(struct mbox_chan *chan, void *mssg);
 int mbox_flush(struct mbox_chan *chan, unsigned long timeout);
 void mbox_client_txdone(struct mbox_chan *chan, int r); /* atomic */
-bool mbox_client_peek_data(struct mbox_chan *chan); /* atomic */
+bool mbox_client_poll_data(struct mbox_chan *chan); /* atomic */
 void mbox_free_channel(struct mbox_chan *chan); /* may sleep */
 
 #endif /* __MAILBOX_CLIENT_H */
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index 36d6ce673503..70d766e623ac 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -40,8 +40,8 @@ struct mbox_chan;
  *		  mode 'send_data' is expected to return -EBUSY.
  *		  The controller may do stuff that need to sleep/block.
  *		  Used only if txdone_poll:=true && txdone_irq:=false
- * @peek_data: Atomic check for any received data. Return true if controller
- *		  has some data to push to the client. False otherwise.
+ * @poll_data:  Poll atomically for received data. Return true if there
+ *		was data available that has been processed. False otherwise.
  */
 struct mbox_chan_ops {
 	int (*send_data)(struct mbox_chan *chan, void *data);
@@ -49,7 +49,7 @@ struct mbox_chan_ops {
 	int (*startup)(struct mbox_chan *chan);
 	void (*shutdown)(struct mbox_chan *chan);
 	bool (*last_tx_done)(struct mbox_chan *chan);
-	bool (*peek_data)(struct mbox_chan *chan);
+	bool (*poll_data)(struct mbox_chan *chan);
 };
 
 /**
-- 
2.35.1

[PATCH 6/7] mailbox: apple: Implement flush() operation

[Hector Martin]

This allows clients to use the atomic-safe mailbox API style.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/mailbox/apple-mailbox.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/mailbox/apple-mailbox.c b/drivers/mailbox/apple-mailbox.c
index 496c4951ccb1..33e7acf71e3e 100644
--- a/drivers/mailbox/apple-mailbox.c
+++ b/drivers/mailbox/apple-mailbox.c
@@ -17,6 +17,7 @@
  */
 
 #include <linux/apple-mailbox.h>
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/gfp.h>
 #include <linux/interrupt.h>
@@ -112,6 +113,14 @@ static bool apple_mbox_hw_can_send(struct apple_mbox *apple_mbox)
 	return !(mbox_ctrl & apple_mbox->hw->control_full);
 }
 
+static bool apple_mbox_hw_send_empty(struct apple_mbox *apple_mbox)
+{
+	u32 mbox_ctrl =
+		readl_relaxed(apple_mbox->regs + apple_mbox->hw->a2i_control);
+
+	return mbox_ctrl & apple_mbox->hw->control_empty;
+}
+
 static int apple_mbox_hw_send(struct apple_mbox *apple_mbox,
 			      struct apple_mbox_msg *msg)
 {
@@ -219,6 +228,23 @@ static irqreturn_t apple_mbox_recv_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static int apple_mbox_chan_flush(struct mbox_chan *chan, unsigned long timeout)
+{
+	struct apple_mbox *apple_mbox = chan->con_priv;
+	unsigned long deadline = jiffies + msecs_to_jiffies(timeout);
+
+	while (time_before(jiffies, deadline)) {
+		if (apple_mbox_hw_send_empty(apple_mbox)) {
+			mbox_chan_txdone(&apple_mbox->chan, 0);
+			return 0;
+		}
+
+		udelay(1);
+	}
+
+	return -ETIME;
+}
+
 static int apple_mbox_chan_startup(struct mbox_chan *chan)
 {
 	struct apple_mbox *apple_mbox = chan->con_priv;
@@ -250,6 +276,7 @@ static void apple_mbox_chan_shutdown(struct mbox_chan *chan)
 
 static const struct mbox_chan_ops apple_mbox_ops = {
 	.send_data = apple_mbox_chan_send_data,
+	.flush = apple_mbox_chan_flush,
 	.startup = apple_mbox_chan_startup,
 	.shutdown = apple_mbox_chan_shutdown,
 };
-- 
2.35.1

[PATCH 7/7] mailbox: apple: Implement poll_data() operation

[Hector Martin]

This allows clients running in atomic context to poll for messages to
arrive.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/mailbox/apple-mailbox.c | 37 ++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/mailbox/apple-mailbox.c b/drivers/mailbox/apple-mailbox.c
index 33e7acf71e3e..e0c2bf7c6338 100644
--- a/drivers/mailbox/apple-mailbox.c
+++ b/drivers/mailbox/apple-mailbox.c
@@ -26,6 +26,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/spinlock.h>
 #include <linux/types.h>
 
 #define APPLE_ASC_MBOX_CONTROL_FULL  BIT(16)
@@ -101,6 +102,7 @@ struct apple_mbox {
 
 	struct device *dev;
 	struct mbox_controller controller;
+	spinlock_t rx_lock;
 };
 
 static const struct of_device_id apple_mbox_of_match[];
@@ -204,13 +206,16 @@ static irqreturn_t apple_mbox_send_empty_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t apple_mbox_recv_irq(int irq, void *data)
+static int apple_mbox_poll_data(struct apple_mbox *apple_mbox)
 {
-	struct apple_mbox *apple_mbox = data;
+
 	struct apple_mbox_msg msg;
+	int ret = 0;
 
-	while (apple_mbox_hw_recv(apple_mbox, &msg) == 0)
+	while (apple_mbox_hw_recv(apple_mbox, &msg) == 0) {
 		mbox_chan_received_data(&apple_mbox->chan, (void *)&msg);
+		ret++;
+	}
 
 	/*
 	 * The interrupt will keep firing even if there are no more messages
@@ -225,9 +230,33 @@ static irqreturn_t apple_mbox_recv_irq(int irq, void *data)
 			       apple_mbox->regs + apple_mbox->hw->irq_ack);
 	}
 
+	return ret;
+}
+
+static irqreturn_t apple_mbox_recv_irq(int irq, void *data)
+{
+	struct apple_mbox *apple_mbox = data;
+
+	spin_lock(&apple_mbox->rx_lock);
+	apple_mbox_poll_data(apple_mbox);
+	spin_unlock(&apple_mbox->rx_lock);
+
 	return IRQ_HANDLED;
 }
 
+static bool apple_mbox_chan_poll_data(struct mbox_chan *chan)
+{
+	struct apple_mbox *apple_mbox = chan->con_priv;
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&apple_mbox->rx_lock, flags);
+	ret = apple_mbox_poll_data(apple_mbox);
+	spin_unlock_irqrestore(&apple_mbox->rx_lock, flags);
+
+	return ret > 0;
+}
+
 static int apple_mbox_chan_flush(struct mbox_chan *chan, unsigned long timeout)
 {
 	struct apple_mbox *apple_mbox = chan->con_priv;
@@ -276,6 +305,7 @@ static void apple_mbox_chan_shutdown(struct mbox_chan *chan)
 
 static const struct mbox_chan_ops apple_mbox_ops = {
 	.send_data = apple_mbox_chan_send_data,
+	.poll_data = apple_mbox_chan_poll_data,
 	.flush = apple_mbox_chan_flush,
 	.startup = apple_mbox_chan_startup,
 	.shutdown = apple_mbox_chan_shutdown,
@@ -331,6 +361,7 @@ static int apple_mbox_probe(struct platform_device *pdev)
 	mbox->controller.txdone_irq = true;
 	mbox->controller.of_xlate = apple_mbox_of_xlate;
 	mbox->chan.con_priv = mbox;
+	spin_lock_init(&mbox->rx_lock);
 
 	irqname = devm_kasprintf(dev, GFP_KERNEL, "%s-recv", dev_name(dev));
 	if (!irqname)
-- 
2.35.1

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

[Hector Martin]

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.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

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

[jassisinghbrar]

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.

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

[Hector Martin]

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