From: Elliot Berman <quic_eberman@quicinc.com>
To: Pavan Kondeti <quic_pkondeti@quicinc.com>
Cc: Bjorn Andersson <quic_bjorande@quicinc.com>,
Jassi Brar <jassisinghbrar@gmail.com>,
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>,
Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>,
Andy Gross <agross@kernel.org>,
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
<linux-arm-kernel@lists.infradead.org>,
Mark Rutland <mark.rutland@arm.com>,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
Sudeep Holla <sudeep.holla@arm.com>,
Marc Zyngier <maz@kernel.org>, Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Jonathan Corbet <corbet@lwn.net>, "Will Deacon" <will@kernel.org>,
Catalin Marinas <catalin.marinas@arm.com>,
"Arnd Bergmann" <arnd@arndb.de>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
Amol Maheshwari <amahesh@qti.qualcomm.com>,
Kalle Valo <kvalo@kernel.org>, <devicetree@vger.kernel.org>,
<linux-doc@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 09/21] mailbox: Add Gunyah message queue mailbox
Date: Tue, 1 Nov 2022 10:44:59 -0700 [thread overview]
Message-ID: <cc9f6d43-0655-482e-384d-e75230e951cf@quicinc.com> (raw)
In-Reply-To: <20221027135510.GA29032@hu-pkondeti-hyd.qualcomm.com>
On 10/27/2022 6:55 AM, Pavan Kondeti wrote:
> Hi Elliot,
>
> On Wed, Oct 26, 2022 at 11:58:34AM -0700, Elliot Berman wrote:
>> 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 <quic_eberman@quicinc.com>
>
> <snip>
>
>> +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;
>> +}
>> +
>> +static void gh_msgq_txdone_tasklet(unsigned long data)
>> +{
>> + struct gh_msgq *msgq = (struct gh_msgq *)data;
>> +
>> + mbox_chan_txdone(gh_msgq_chan(msgq), msgq->last_status);
>> +}
>> +
>> +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;
>> + unsigned long ret;
>> + bool ready;
>> +
>> + if (msgq_data->push)
>> + tx_flags |= GH_HYPERCALL_MSGQ_TX_FLAGS_PUSH;
>> +
>> + ret = gh_hypercall_msgq_send(msgq->tx_ghrsc->capid, msgq_data->length,
>> + (uintptr_t)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(ret == GH_ERROR_MSGQUEUE_FULL))
>> + return -EAGAIN;
>> +
>> + /**
>> + * 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_status = gh_remap_error(ret);
>> +
>> + /**
>> + * This message was successfully sent, but message queue isn't ready to
>> + * receive more messages because it's now full. Mailbox framework
>> + * requires that we only report that message was transmitted only 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 (ret == GH_ERROR_OK && !ready)
>> + return 0;
>> +
>> + /**
>> + * We can send more messages. Mailbox framework requires that tx done
>> + * happens asynchronously to sending the message. 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.
>> + */
>> + tasklet_schedule(&msgq->txdone_tasklet);
>> +
>
> Nice comments.
>
> irq_work would be a better choice.
>
>> + return 0;
>> +}
>> +
>> +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 facilitiate 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;
>> +
>> + msgq->tx_ghrsc = tx_ghrsc;
>> + msgq->rx_ghrsc = rx_ghrsc;
>> +
>> + msgq->mbox.dev = parent;
>> + msgq->mbox.ops = &gh_msgq_ops;
>> + msgq->mbox.chans = kcalloc(1, sizeof(*msgq->mbox.chans), GFP_KERNEL);
>
> Error handling missing.
>
> minor nit pick:
>
> If you initialize num_chans to 1 before, then you can use that as the first
> argument to kcalloc() which makes it more readable since you opted for kcalloc()
> instead of kzalloc() there.
>
>> + msgq->mbox.num_chans = 1;
>> + msgq->mbox.txdone_irq = true;
>> +
>> + if (gh_msgq_has_tx(msgq)) {
>> + ret = request_irq(msgq->tx_ghrsc->irq, gh_msgq_tx_irq_handler, 0, "gh_msgq_tx",
>> + msgq);
>> + if (ret)
>> + goto err_chans;
>> + }
>> +
>> + if (gh_msgq_has_rx(msgq)) {
>> + ret = request_threaded_irq(msgq->rx_ghrsc->irq, NULL, gh_msgq_rx_irq_handler,
>> + IRQF_ONESHOT, "gh_msgq_rx", msgq);
>> + if (ret)
>> + goto err_tx_irq;
>> + }
>> +
>> + tasklet_init(&msgq->txdone_tasklet, gh_msgq_txdone_tasklet, (unsigned long)msgq);
>
> If you wish to use tasklets, use tasklet_setup().
>
>> +
>> + ret = mbox_controller_register(&msgq->mbox);
>> + if (ret)
>> + goto err_rx_irq;
>> +
>> + ret = mbox_bind_client(gh_msgq_chan(msgq), cl);
>> + if (ret)
>> + goto err_mbox;
>> +
>> + return 0;
>> +err_mbox:
>> + mbox_controller_unregister(&msgq->mbox);
>> +err_rx_irq:
>> + if (gh_msgq_has_rx(msgq))
>> + free_irq(msgq->rx_ghrsc->irq, msgq);
>> +err_tx_irq:
>> + if (gh_msgq_has_tx(msgq))
>> + free_irq(msgq->tx_ghrsc->irq, msgq);
>> +err_chans:
>> + kfree(msgq->mbox.chans);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(gh_msgq_init);
>> +
>> +void gh_msgq_remove(struct gh_msgq *msgq)
>> +{
>> + if (gh_msgq_has_rx(msgq))
>> + free_irq(msgq->rx_ghrsc->irq, msgq);
>> +
>> + if (gh_msgq_has_tx(msgq))
>> + free_irq(msgq->tx_ghrsc->irq, msgq);
>> +
>> + kfree(msgq->mbox.chans);
>> +}
>> +EXPORT_SYMBOL_GPL(gh_msgq_remove);
>> +
>
> Is gh_msgq_remove() supposed to undo every thing done in gh_msgq_init()?
> ex: mbox controller and channel are not unregistered.
>
Ah thanks for catching, updated this as well as rest of your comments.
next prev parent reply other threads:[~2022-11-01 17:45 UTC|newest]
Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-26 18:58 [PATCH v6 00/21] Drivers for gunyah hypervisor Elliot Berman
2022-10-26 18:58 ` [PATCH v6 01/21] docs: gunyah: Introduce Gunyah Hypervisor Elliot Berman
2022-11-02 12:35 ` Bagas Sanjaya
2022-10-26 18:58 ` [PATCH v6 02/21] dt-bindings: Add binding for gunyah hypervisor Elliot Berman
2022-10-27 19:57 ` Krzysztof Kozlowski
2022-10-28 2:33 ` Jassi Brar
2022-11-01 3:19 ` Elliot Berman
2022-11-01 16:23 ` Jassi Brar
2022-11-01 20:35 ` Elliot Berman
2022-11-01 21:58 ` Jassi Brar
2022-11-02 0:12 ` Elliot Berman
2022-11-02 2:01 ` Jassi Brar
2022-11-02 18:05 ` Elliot Berman
2022-11-02 18:24 ` Jassi Brar
2022-11-02 23:23 ` Elliot Berman
2022-11-03 3:21 ` Jassi Brar
2022-11-03 19:45 ` Elliot Berman
2022-10-26 18:58 ` [PATCH v6 03/21] gunyah: Common types and error codes for Gunyah hypercalls Elliot Berman
2022-10-26 19:47 ` Dmitry Baryshkov
2022-10-26 18:58 ` [PATCH v6 04/21] arm64: smccc: Include alternative-macros.h Elliot Berman
2022-10-26 19:46 ` Dmitry Baryshkov
2022-10-26 20:23 ` Elliot Berman
2022-10-26 20:39 ` Dmitry Baryshkov
2022-10-26 18:58 ` [PATCH v6 05/21] virt: gunyah: Add hypercalls to identify Gunyah Elliot Berman
2022-10-26 18:58 ` [PATCH v6 06/21] virt: gunyah: Identify hypervisor version Elliot Berman
2022-10-26 18:58 ` [PATCH v6 07/21] mailbox: Allow direct registration to a channel Elliot Berman
2022-10-26 18:58 ` [PATCH v6 08/21] virt: gunyah: msgq: Add hypercalls to send and receive messages Elliot Berman
2022-10-26 18:58 ` [PATCH v6 09/21] mailbox: Add Gunyah message queue mailbox Elliot Berman
2022-10-27 13:55 ` Pavan Kondeti
2022-11-01 17:44 ` Elliot Berman [this message]
2022-10-26 18:58 ` [PATCH v6 10/21] gunyah: rsc_mgr: Add resource manager RPC core Elliot Berman
2022-11-01 18:02 ` Greg Kroah-Hartman
2022-11-02 0:12 ` Elliot Berman
2022-11-02 2:53 ` Greg Kroah-Hartman
2022-11-02 18:04 ` Elliot Berman
2022-11-03 0:22 ` Greg Kroah-Hartman
2022-11-03 22:07 ` Elliot Berman
2022-11-03 22:09 ` Elliot Berman
2022-10-26 18:58 ` [PATCH v6 11/21] gunyah: rsc_mgr: Add subdevices bus Elliot Berman
2022-10-26 18:58 ` [PATCH v6 12/21] gunyah: rsc_mgr: Add VM lifecycle RPC Elliot Berman
2022-10-26 18:58 ` [PATCH v6 13/21] gunyah: vm_mgr: Introduce basic VM Manager Elliot Berman
2022-11-02 5:14 ` Greg Kroah-Hartman
2022-11-02 18:45 ` Elliot Berman
2022-11-03 0:24 ` Greg Kroah-Hartman
2022-11-04 0:11 ` Elliot Berman
2022-11-04 8:10 ` Arnd Bergmann
2022-11-04 22:38 ` Elliot Berman
2022-11-05 4:19 ` Trilok Soni
2022-11-11 0:03 ` Elliot Berman
2022-11-11 6:24 ` Greg Kroah-Hartman
2022-11-11 17:08 ` Elliot Berman
2022-11-02 7:31 ` Arnd Bergmann
2022-11-02 18:44 ` Elliot Berman
2022-11-03 0:20 ` Greg Kroah-Hartman
2022-11-03 22:33 ` Elliot Berman
2022-11-03 9:39 ` Arnd Bergmann
2022-11-03 22:10 ` Elliot Berman
2022-10-26 18:58 ` [PATCH v6 14/21] gunyah: rsc_mgr: Add RPC for sharing memory Elliot Berman
2022-10-26 18:58 ` [PATCH v6 15/21] gunyah: vm_mgr: Add/remove user memory regions Elliot Berman
2022-10-26 18:58 ` [PATCH v6 16/21] gunyah: vm_mgr: Add ioctls to support basic non-proxy VM boot Elliot Berman
2022-10-26 18:58 ` [PATCH v6 17/21] samples: Add sample userspace Gunyah VM Manager Elliot Berman
2022-10-26 18:58 ` [PATCH v6 18/21] gunyah: rsc_mgr: Add platform ops on mem_lend/mem_reclaim Elliot Berman
2022-10-26 18:58 ` [PATCH v6 19/21] firmware: qcom_scm: Use fixed width src vm bitmap Elliot Berman
2022-10-26 18:58 ` [PATCH v6 20/21] firmware: qcom_scm: Register Gunyah platform ops Elliot Berman
2022-10-26 21:32 ` kernel test robot
2022-10-26 18:58 ` [PATCH v6 21/21] docs: gunyah: Document Gunyah VM Manager Elliot Berman
2022-11-02 13:05 ` Bagas Sanjaya
2022-11-02 18:04 ` Elliot Berman
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=cc9f6d43-0655-482e-384d-e75230e951cf@quicinc.com \
--to=quic_eberman@quicinc.com \
--cc=agross@kernel.org \
--cc=amahesh@qti.qualcomm.com \
--cc=arnd@arndb.de \
--cc=catalin.marinas@arm.com \
--cc=corbet@lwn.net \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.baryshkov@linaro.org \
--cc=gregkh@linuxfoundation.org \
--cc=jassisinghbrar@gmail.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=kvalo@kernel.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=lorenzo.pieralisi@arm.com \
--cc=mark.rutland@arm.com \
--cc=maz@kernel.org \
--cc=quic_bjorande@quicinc.com \
--cc=quic_cvanscha@quicinc.com \
--cc=quic_mnalajal@quicinc.com \
--cc=quic_pheragu@quicinc.com \
--cc=quic_pkondeti@quicinc.com \
--cc=quic_svaddagi@quicinc.com \
--cc=quic_tsoni@quicinc.com \
--cc=robh+dt@kernel.org \
--cc=srinivas.kandagatla@linaro.org \
--cc=sudeep.holla@arm.com \
--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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).