From: Jassi Brar <jassisinghbrar@gmail.com> To: Elliot Berman <quic_eberman@quicinc.com> Cc: Alex Elder <elder@linaro.org>, Srinivas Kandagatla <srinivas.kandagatla@linaro.org>, Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>, Jonathan Corbet <corbet@lwn.net>, Murali Nalajala <quic_mnalajal@quicinc.com>, Trilok Soni <quic_tsoni@quicinc.com>, Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>, Carl van Schaik <quic_cvanscha@quicinc.com>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Arnd Bergmann <arnd@arndb.de>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, Bagas Sanjaya <bagasdotme@gmail.com>, Will Deacon <will@kernel.org>, Andy Gross <agross@kernel.org>, Catalin Marinas <catalin.marinas@arm.com>, linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v14 06/25] mailbox: Add Gunyah message queue mailbox Date: Wed, 2 Aug 2023 19:33:02 -0500 [thread overview] Message-ID: <CABb+yY3LGxDTQDiz8Q5yMwzFDm-ejF+WnQsb0zJKizfxPkx3nQ@mail.gmail.com> (raw) In-Reply-To: <20230613172054.3959700-7-quic_eberman@quicinc.com> On Tue, Jun 13, 2023 at 12:21 PM Elliot Berman <quic_eberman@quicinc.com> wrote: ...... > diff --git a/drivers/mailbox/gunyah-msgq.c b/drivers/mailbox/gunyah-msgq.c > new file mode 100644 > index 0000000000000..7f777339278eb > --- /dev/null > +++ b/drivers/mailbox/gunyah-msgq.c > @@ -0,0 +1,219 @@ > +// 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> > + I believe some includes could be removed without issue. > > +#define mbox_chan_to_msgq(chan) (container_of(chan->mbox, struct gh_msgq, mbox)) > + container_of need not be in brackets. > +static irqreturn_t gh_msgq_rx_irq_handler(int irq, void *data) > +{ > + struct gh_msgq *msgq = data; > + struct gh_msgq_rx_data rx_data; > + enum gh_error gh_error; > + bool ready = true; > + Please limit the scope of rx_data and gh_error by moving them inside the while() body. > + while (ready) { > + gh_error = gh_hypercall_msgq_recv(msgq->rx_ghrsc->capid, > + &rx_data.data, sizeof(rx_data.data), > + &rx_data.length, &ready); > + if (gh_error != GH_ERROR_OK) { > + if (gh_error != GH_ERROR_MSGQUEUE_EMPTY) > + dev_warn(msgq->mbox.dev, "Failed to receive data: %d\n", gh_error); > + break; > + } > + if (likely(gh_msgq_chan(msgq)->cl)) > + mbox_chan_received_data(gh_msgq_chan(msgq), &rx_data); > + } > + > + return IRQ_HANDLED; > +} > + > +static int gh_msgq_send_data(struct mbox_chan *chan, void *data) > +{ > + struct gh_msgq *msgq = mbox_chan_to_msgq(chan); > + struct gh_msgq_tx_data *msgq_data = data; > + u64 tx_flags = 0; > + enum gh_error gh_error; > + bool ready; > + > + if (!msgq->tx_ghrsc) > + return -EOPNOTSUPP; > + If we hit this error, the fix will still be in the upper layer. So please drop the check and, if needed, add one in the client driver. > + if (msgq_data->push) > + tx_flags |= GH_HYPERCALL_MSGQ_TX_FLAGS_PUSH; > + > + gh_error = gh_hypercall_msgq_send(msgq->tx_ghrsc->capid, msgq_data->length, msgq_data->data, > + tx_flags, &ready); > + > + /** > + * unlikely because Linux tracks state of msgq and should not try to > + * send message when msgq is full. > + */ > + if (unlikely(gh_error == GH_ERROR_MSGQUEUE_FULL)) > + return -EAGAIN; > + If it is not expected to hit, please remove the check. If there can be a 'race' like situation, still remove this and try to find an appropriate place to avoid the race. > + /** > + * Propagate all other errors to client. If we return error to mailbox > + * framework, then no other messages can be sent and nobody will know > + * to retry this message. > + */ > + msgq->last_ret = gh_error_remap(gh_error); > + > + /** > + * This message was successfully sent, but message queue isn't ready to > + * accept more messages because it's now full. Mailbox framework > + * requires that we only report that message was transmitted when > + * we're ready to transmit another message. We'll get that in the form > + * of tx IRQ once the other side starts to drain the msgq. > + */ > + if (gh_error == GH_ERROR_OK) { > + if (!ready) > + return 0; > + } else { > + dev_err(msgq->mbox.dev, "Failed to send data: %d (%d)\n", gh_error, msgq->last_ret); > + } > + > + /** > + * We can send more messages. > ... until we can not (when the platform specific queue is full). > Mailbox framework requires that tx done > + * happens asynchronously to sending the message. > hence the mailbox api needs to track each transfer's stage. > Gunyah message queues > + * tell us right away on the hypercall return whether we can send more > + * messages. To work around this, defer the txdone to a tasklet. > + */ > If not here, you would still have to put the 'defer' somewhere in the upper layer. So it is not exactly a "workaround". > + tasklet_schedule(&msgq->txdone_tasklet); > + > + return 0; > +} > + > diff --git a/include/linux/gunyah.h b/include/linux/gunyah.h > index 01a6f202d037e..982e27d10d57f 100644 > --- a/include/linux/gunyah.h > +++ b/include/linux/gunyah.h > @@ -8,11 +8,68 @@ > > #include <linux/bitfield.h> > #include <linux/errno.h> > +#include <linux/interrupt.h> > #include <linux/limits.h> > +#include <linux/mailbox_controller.h> > +#include <linux/mailbox_client.h> > #include <linux/types.h> > controller.h and client.h aren't supposed to be in the same header. The Client and the Controller driver could include them respectively before this header. Cheers.
WARNING: multiple messages have this Message-ID (diff)
From: Jassi Brar <jassisinghbrar@gmail.com> To: Elliot Berman <quic_eberman@quicinc.com> Cc: Alex Elder <elder@linaro.org>, Srinivas Kandagatla <srinivas.kandagatla@linaro.org>, Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>, Jonathan Corbet <corbet@lwn.net>, Murali Nalajala <quic_mnalajal@quicinc.com>, Trilok Soni <quic_tsoni@quicinc.com>, Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>, Carl van Schaik <quic_cvanscha@quicinc.com>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Arnd Bergmann <arnd@arndb.de>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, Bagas Sanjaya <bagasdotme@gmail.com>, Will Deacon <will@kernel.org>, Andy Gross <agross@kernel.org>, Catalin Marinas <catalin.marinas@arm.com>, linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v14 06/25] mailbox: Add Gunyah message queue mailbox Date: Wed, 2 Aug 2023 19:33:02 -0500 [thread overview] Message-ID: <CABb+yY3LGxDTQDiz8Q5yMwzFDm-ejF+WnQsb0zJKizfxPkx3nQ@mail.gmail.com> (raw) In-Reply-To: <20230613172054.3959700-7-quic_eberman@quicinc.com> On Tue, Jun 13, 2023 at 12:21 PM Elliot Berman <quic_eberman@quicinc.com> wrote: ...... > diff --git a/drivers/mailbox/gunyah-msgq.c b/drivers/mailbox/gunyah-msgq.c > new file mode 100644 > index 0000000000000..7f777339278eb > --- /dev/null > +++ b/drivers/mailbox/gunyah-msgq.c > @@ -0,0 +1,219 @@ > +// 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> > + I believe some includes could be removed without issue. > > +#define mbox_chan_to_msgq(chan) (container_of(chan->mbox, struct gh_msgq, mbox)) > + container_of need not be in brackets. > +static irqreturn_t gh_msgq_rx_irq_handler(int irq, void *data) > +{ > + struct gh_msgq *msgq = data; > + struct gh_msgq_rx_data rx_data; > + enum gh_error gh_error; > + bool ready = true; > + Please limit the scope of rx_data and gh_error by moving them inside the while() body. > + while (ready) { > + gh_error = gh_hypercall_msgq_recv(msgq->rx_ghrsc->capid, > + &rx_data.data, sizeof(rx_data.data), > + &rx_data.length, &ready); > + if (gh_error != GH_ERROR_OK) { > + if (gh_error != GH_ERROR_MSGQUEUE_EMPTY) > + dev_warn(msgq->mbox.dev, "Failed to receive data: %d\n", gh_error); > + break; > + } > + if (likely(gh_msgq_chan(msgq)->cl)) > + mbox_chan_received_data(gh_msgq_chan(msgq), &rx_data); > + } > + > + return IRQ_HANDLED; > +} > + > +static int gh_msgq_send_data(struct mbox_chan *chan, void *data) > +{ > + struct gh_msgq *msgq = mbox_chan_to_msgq(chan); > + struct gh_msgq_tx_data *msgq_data = data; > + u64 tx_flags = 0; > + enum gh_error gh_error; > + bool ready; > + > + if (!msgq->tx_ghrsc) > + return -EOPNOTSUPP; > + If we hit this error, the fix will still be in the upper layer. So please drop the check and, if needed, add one in the client driver. > + if (msgq_data->push) > + tx_flags |= GH_HYPERCALL_MSGQ_TX_FLAGS_PUSH; > + > + gh_error = gh_hypercall_msgq_send(msgq->tx_ghrsc->capid, msgq_data->length, msgq_data->data, > + tx_flags, &ready); > + > + /** > + * unlikely because Linux tracks state of msgq and should not try to > + * send message when msgq is full. > + */ > + if (unlikely(gh_error == GH_ERROR_MSGQUEUE_FULL)) > + return -EAGAIN; > + If it is not expected to hit, please remove the check. If there can be a 'race' like situation, still remove this and try to find an appropriate place to avoid the race. > + /** > + * Propagate all other errors to client. If we return error to mailbox > + * framework, then no other messages can be sent and nobody will know > + * to retry this message. > + */ > + msgq->last_ret = gh_error_remap(gh_error); > + > + /** > + * This message was successfully sent, but message queue isn't ready to > + * accept more messages because it's now full. Mailbox framework > + * requires that we only report that message was transmitted when > + * we're ready to transmit another message. We'll get that in the form > + * of tx IRQ once the other side starts to drain the msgq. > + */ > + if (gh_error == GH_ERROR_OK) { > + if (!ready) > + return 0; > + } else { > + dev_err(msgq->mbox.dev, "Failed to send data: %d (%d)\n", gh_error, msgq->last_ret); > + } > + > + /** > + * We can send more messages. > ... until we can not (when the platform specific queue is full). > Mailbox framework requires that tx done > + * happens asynchronously to sending the message. > hence the mailbox api needs to track each transfer's stage. > Gunyah message queues > + * tell us right away on the hypercall return whether we can send more > + * messages. To work around this, defer the txdone to a tasklet. > + */ > If not here, you would still have to put the 'defer' somewhere in the upper layer. So it is not exactly a "workaround". > + tasklet_schedule(&msgq->txdone_tasklet); > + > + return 0; > +} > + > diff --git a/include/linux/gunyah.h b/include/linux/gunyah.h > index 01a6f202d037e..982e27d10d57f 100644 > --- a/include/linux/gunyah.h > +++ b/include/linux/gunyah.h > @@ -8,11 +8,68 @@ > > #include <linux/bitfield.h> > #include <linux/errno.h> > +#include <linux/interrupt.h> > #include <linux/limits.h> > +#include <linux/mailbox_controller.h> > +#include <linux/mailbox_client.h> > #include <linux/types.h> > controller.h and client.h aren't supposed to be in the same header. The Client and the Controller driver could include them respectively before this header. Cheers. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-08-03 0:33 UTC|newest] Thread overview: 102+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-06-13 17:20 [PATCH v14 00/25] Drivers for Gunyah hypervisor Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-06-13 17:20 ` [PATCH v14 01/25] docs: gunyah: Introduce Gunyah Hypervisor Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-06-16 16:32 ` Alex Elder 2023-06-16 16:32 ` Alex Elder 2023-07-03 22:41 ` Elliot Berman 2023-07-03 22:41 ` Elliot Berman 2023-07-14 13:35 ` Alex Elder 2023-07-14 13:35 ` Alex Elder 2023-08-05 17:07 ` Bjorn Andersson 2023-08-05 17:07 ` Bjorn Andersson 2023-06-13 17:20 ` [PATCH v14 02/25] dt-bindings: Add binding for gunyah hypervisor Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-06-16 16:32 ` Alex Elder 2023-06-16 16:32 ` Alex Elder 2023-08-05 3:34 ` Bjorn Andersson 2023-08-05 3:34 ` Bjorn Andersson 2023-06-13 17:20 ` [PATCH v14 03/25] gunyah: Common types and error codes for Gunyah hypercalls Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-08-05 3:39 ` Bjorn Andersson 2023-08-05 3:39 ` Bjorn Andersson 2023-06-13 17:20 ` [PATCH v14 04/25] virt: gunyah: Add hypercalls to identify Gunyah Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-06-13 17:20 ` [PATCH v14 05/25] virt: gunyah: msgq: Add hypercalls to send and receive messages Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-08-05 3:51 ` Bjorn Andersson 2023-08-05 3:51 ` Bjorn Andersson 2023-06-13 17:20 ` [PATCH v14 06/25] mailbox: Add Gunyah message queue mailbox Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-06-16 16:33 ` Alex Elder 2023-06-16 16:33 ` Alex Elder 2023-08-03 0:33 ` Jassi Brar [this message] 2023-08-03 0:33 ` Jassi Brar 2023-08-05 4:16 ` Bjorn Andersson 2023-08-05 4:16 ` Bjorn Andersson 2023-06-13 17:20 ` [PATCH v14 07/25] gunyah: rsc_mgr: Add resource manager RPC core Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-08-05 5:02 ` Bjorn Andersson 2023-08-05 5:02 ` Bjorn Andersson 2023-06-13 17:20 ` [PATCH v14 08/25] gunyah: rsc_mgr: Add VM lifecycle RPC Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-08-05 17:18 ` Bjorn Andersson 2023-08-05 17:18 ` Bjorn Andersson 2023-06-13 17:20 ` [PATCH v14 09/25] gunyah: vm_mgr: Introduce basic VM Manager Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-08-05 17:26 ` Bjorn Andersson 2023-08-05 17:26 ` Bjorn Andersson 2023-06-13 17:20 ` [PATCH v14 10/25] gunyah: rsc_mgr: Add RPC for sharing memory Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-08-05 17:55 ` Bjorn Andersson 2023-08-05 17:55 ` Bjorn Andersson 2023-06-13 17:20 ` [PATCH v14 11/25] gunyah: vm_mgr: Add/remove user memory regions Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-06-16 16:33 ` Alex Elder 2023-06-16 16:33 ` Alex Elder 2023-06-13 17:20 ` [PATCH v14 12/25] gunyah: vm_mgr: Add ioctls to support basic non-proxy VM boot Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-06-13 17:20 ` [PATCH v14 13/25] samples: Add sample userspace Gunyah VM Manager Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-08-05 18:03 ` Bjorn Andersson 2023-08-05 18:03 ` Bjorn Andersson 2023-06-13 17:20 ` [PATCH v14 14/25] gunyah: rsc_mgr: Add platform ops on mem_lend/mem_reclaim Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-08-05 18:07 ` Bjorn Andersson 2023-08-05 18:07 ` Bjorn Andersson 2023-06-13 17:20 ` [PATCH v14 15/25] virt: gunyah: Add Qualcomm Gunyah platform ops Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-08-05 18:11 ` Bjorn Andersson 2023-08-05 18:11 ` Bjorn Andersson 2023-06-13 17:20 ` [PATCH v14 16/25] docs: gunyah: Document Gunyah VM Manager Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-06-16 16:34 ` Alex Elder 2023-06-16 16:34 ` Alex Elder 2023-06-13 17:20 ` [PATCH v14 17/25] virt: gunyah: Translate gh_rm_hyp_resource into gunyah_resource Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-08-05 18:15 ` Bjorn Andersson 2023-08-05 18:15 ` Bjorn Andersson 2023-06-13 17:20 ` [PATCH v14 18/25] gunyah: vm_mgr: Add framework for VM Functions Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-06-16 16:34 ` Alex Elder 2023-06-16 16:34 ` Alex Elder 2023-06-13 17:20 ` [PATCH v14 19/25] virt: gunyah: Add resource tickets Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-06-13 17:20 ` [PATCH v14 20/25] virt: gunyah: Add IO handlers Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-06-13 17:20 ` [PATCH v14 21/25] virt: gunyah: Add proxy-scheduled vCPUs Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-06-13 17:20 ` [PATCH v14 22/25] virt: gunyah: Add hypercalls for sending doorbell Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-06-13 17:20 ` [PATCH v14 23/25] virt: gunyah: Add irqfd interface Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-06-16 16:34 ` Alex Elder 2023-06-16 16:34 ` Alex Elder 2023-06-13 17:20 ` [PATCH v14 24/25] virt: gunyah: Add ioeventfd Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-06-13 17:20 ` [PATCH v14 25/25] MAINTAINERS: Add Gunyah hypervisor drivers section Elliot Berman 2023-06-13 17:20 ` Elliot Berman 2023-08-05 18:19 ` Bjorn Andersson 2023-08-05 18:19 ` Bjorn Andersson 2023-06-16 16:35 ` [PATCH v14 00/25] Drivers for Gunyah hypervisor Alex Elder 2023-06-16 16:35 ` Alex Elder
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CABb+yY3LGxDTQDiz8Q5yMwzFDm-ejF+WnQsb0zJKizfxPkx3nQ@mail.gmail.com \ --to=jassisinghbrar@gmail.com \ --cc=agross@kernel.org \ --cc=andersson@kernel.org \ --cc=arnd@arndb.de \ --cc=bagasdotme@gmail.com \ --cc=catalin.marinas@arm.com \ --cc=conor+dt@kernel.org \ --cc=corbet@lwn.net \ --cc=devicetree@vger.kernel.org \ --cc=dmitry.baryshkov@linaro.org \ --cc=elder@linaro.org \ --cc=gregkh@linuxfoundation.org \ --cc=konrad.dybcio@linaro.org \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=quic_cvanscha@quicinc.com \ --cc=quic_eberman@quicinc.com \ --cc=quic_mnalajal@quicinc.com \ --cc=quic_pheragu@quicinc.com \ --cc=quic_svaddagi@quicinc.com \ --cc=quic_tsoni@quicinc.com \ --cc=robh+dt@kernel.org \ --cc=srinivas.kandagatla@linaro.org \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.