Re: [PATCH v10 07/26] mailbox: Add Gunyah message queue mailbox

On 23/02/2023 00:15, Elliot Berman wrote:

On 2/20/2023 5:59 AM, Srinivas Kandagatla wrote:

quoted

On 14/02/2023 21:23, Elliot Berman wrote:

quoted

Gunyah message queues are a unidirectional inter-VM pipe for messages up
to 1024 bytes. This driver supports pairing a receiver message queue and
a transmitter message queue to expose a single mailbox channel.

Signed-off-by: Elliot Berman <redacted>
---
  Documentation/virt/gunyah/message-queue.rst |   8 +
  drivers/mailbox/Makefile                   |   2 +
  drivers/mailbox/gunyah-msgq.c               | 214 ++++++++++++++++++++
  include/linux/gunyah.h                      |  56 +++++
  4 files changed, 280 insertions(+)
  create mode 100644 drivers/mailbox/gunyah-msgq.c

diff --git a/Documentation/virt/gunyah/message-queue.rst 

b/Documentation/virt/gunyah/message-queue.rst
index 0667b3eb1ff9..082085e981e0 100644

--- a/Documentation/virt/gunyah/message-queue.rst
+++ b/Documentation/virt/gunyah/message-queue.rst

@@ -59,3 +59,11 @@ vIRQ: two TX message queues will have two vIRQs 

(and two capability IDs).
        |               |         |                 | |               |
        |               |         |                 | |               |
        +---------------+         +-----------------+ +---------------+
+
+Gunyah message queues are exposed as mailboxes. To create the 
mailbox, create
+a mbox_client and call `gh_msgq_init`. On receipt of the RX_READY 
interrupt,
+all messages in the RX message queue are read and pushed via the 
`rx_callback`
+of the registered mbox_client.
+
+.. kernel-doc:: drivers/mailbox/gunyah-msgq.c
+   :identifiers: gh_msgq_init

diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index fc9376117111..5f929bb55e9a 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile

@@ -55,6 +55,8 @@ obj-$(CONFIG_MTK_CMDQ_MBOX)    += mtk-cmdq-mailbox.o

  obj-$(CONFIG_ZYNQMP_IPI_MBOX)    += zynqmp-ipi-mailbox.o
+obj-$(CONFIG_GUNYAH)        += gunyah-msgq.o

Why are we reusing CONFIG_GUNYAH Kconfig symbol for mailbox, why not 
CONFIG_GUNYAH_MBOX?

There was some previous discussion about this:

https://lore.kernel.org/all/2a7bb5f2-1286-b661-659a-a5037150eae8@quicinc.com/ (local)

quoted

quoted

+
  obj-$(CONFIG_SUN6I_MSGBOX)    += sun6i-msgbox.o
  obj-$(CONFIG_SPRD_MBOX)       += sprd-mailbox.o

diff --git a/drivers/mailbox/gunyah-msgq.c 

b/drivers/mailbox/gunyah-msgq.c
new file mode 100644
index 000000000000..03ffaa30ce9b

--- /dev/null
+++ b/drivers/mailbox/gunyah-msgq.c

@@ -0,0 +1,214 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All 

rights reserved.
+ */
+
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/gunyah.h>
+#include <linux/printk.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/wait.h>

...

quoted

+/* Fired when message queue transitions from "full" to "space 
available" to send messages */
+static irqreturn_t gh_msgq_tx_irq_handler(int irq, void *data)
+{
+    struct gh_msgq *msgq = data;
+
+    mbox_chan_txdone(gh_msgq_chan(msgq), 0);
+
+    return IRQ_HANDLED;
+}
+
+/* Fired after sending message and hypercall told us there was more 
space available. */
+static void gh_msgq_txdone_tasklet(struct tasklet_struct *tasklet)

Tasklets have been long deprecated, consider using workqueues in this 
particular case.

Workqueues have higher latency and tasklets came as recommendation from 
Jassi. drivers/mailbox/imx-mailbox.c uses tasklets in the same way.

I did some quick unscientific measurements of ~1000x samples. The median 
latency for resource manager went from 25.5 us (tasklet) to 26 us 
(workqueue) (2% slower). The mean went from 28.7 us to 32.5 us (13% 
slower). Obviously, the outliers for workqueues were much more extreme.

TBH, this is expected because we are only testing resource manager, Note 
  the advantage that you will see shifting from tasket to workqueues is 
on overall system latencies and some drivers performance that need to 
react to events.

please take some time to read this nice article about this 
https://lwn.net/Articles/830964/


--srini

quoted

quoted

+{
+    struct gh_msgq *msgq = container_of(tasklet, struct gh_msgq, 
txdone_tasklet);
+
+    mbox_chan_txdone(gh_msgq_chan(msgq), msgq->last_ret);
+}
+
+static int gh_msgq_send_data(struct mbox_chan *chan, void *data)
+{

..

quoted

+    tasklet_schedule(&msgq->txdone_tasklet);
+
+    return 0;
+}
+
+static struct mbox_chan_ops gh_msgq_ops = {
+    .send_data = gh_msgq_send_data,
+};
+
+/**
+ * gh_msgq_init() - Initialize a Gunyah message queue with an 
mbox_client
+ * @parent: optional, device parent used for the mailbox controller
+ * @msgq: Pointer to the gh_msgq to initialize
+ * @cl: A mailbox client to bind to the mailbox channel that the 
message queue creates
+ * @tx_ghrsc: optional, the transmission side of the message queue
+ * @rx_ghrsc: optional, the receiving side of the message queue
+ *
+ * At least one of tx_ghrsc and rx_ghrsc should be not NULL. Most 
message queue use cases come with
+ * a pair of message queues to facilitate bidirectional 
communication. When tx_ghrsc is set,
+ * the client can send messages with 
mbox_send_message(gh_msgq_chan(msgq), msg). When rx_ghrsc
+ * is set, the mbox_client should register an .rx_callback() and the 
message queue driver will
+ * push all available messages upon receiving the RX ready 
interrupt. The messages should be
+ * consumed or copied by the client right away as the 
gh_msgq_rx_data will be replaced/destroyed
+ * after the callback.
+ *
+ * Returns - 0 on success, negative otherwise
+ */
+int gh_msgq_init(struct device *parent, struct gh_msgq *msgq, struct 
mbox_client *cl,
+             struct gunyah_resource *tx_ghrsc, struct 
gunyah_resource *rx_ghrsc)
+{
+    int ret;
+
+    /* Must have at least a tx_ghrsc or rx_ghrsc and that they are 
the right device types */
+    if ((!tx_ghrsc && !rx_ghrsc) ||
+        (tx_ghrsc && tx_ghrsc->type != GUNYAH_RESOURCE_TYPE_MSGQ_TX) ||
+        (rx_ghrsc && rx_ghrsc->type != GUNYAH_RESOURCE_TYPE_MSGQ_RX))
+        return -EINVAL;
+
+    if (gh_api_version() != GUNYAH_API_V1) {
+        pr_err("Unrecognized gunyah version: %u. Currently 
supported: %d\n",

dev_err(parent

would make this more useful

Done.

- Elliot

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel