| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAK9=C2Whow_XeG9xYrWScvqT-G93jGJRUF8XJgb=9zsVPmkq+g@mail.gmail.com>
Date: Thu, 26 Dec 2024 17:21:10 +0530
From: Anup Patel <apatel@...tanamicro.com>
To: Leyfoon Tan <leyfoon.tan@...rfivetech.com>
Cc: Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Jassi Brar <jassisinghbrar@...il.com>, Palmer Dabbelt <palmer@...belt.com>,
Paul Walmsley <paul.walmsley@...ive.com>, Sunil V L <sunilvl@...tanamicro.com>,
Rahul Pathak <rpathak@...tanamicro.com>, Atish Patra <atishp@...shpatra.org>,
Andrew Jones <ajones@...tanamicro.com>, Anup Patel <anup@...infault.org>,
"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 6/8] mailbox: Add RISC-V SBI message proxy (MPXY)
based mailbox driver
On Tue, Dec 24, 2024 at 8:50 AM Leyfoon Tan
<leyfoon.tan@...rfivetech.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Anup Patel <apatel@...tanamicro.com>
> > Sent: Monday, December 16, 2024 4:48 PM
> > To: Michael Turquette <mturquette@...libre.com>; Stephen Boyd
> > <sboyd@...nel.org>; Rob Herring <robh@...nel.org>; Krzysztof Kozlowski
> > <krzk+dt@...nel.org>; Conor Dooley <conor+dt@...nel.org>; Jassi Brar
> > <jassisinghbrar@...il.com>
> > Cc: Palmer Dabbelt <palmer@...belt.com>; Paul Walmsley
> > <paul.walmsley@...ive.com>; Sunil V L <sunilvl@...tanamicro.com>; Rahul
> > Pathak <rpathak@...tanamicro.com>; Leyfoon Tan
> > <leyfoon.tan@...rfivetech.com>; Atish Patra <atishp@...shpatra.org>;
> > Andrew Jones <ajones@...tanamicro.com>; Anup Patel
> > <anup@...infault.org>; linux-clk@...r.kernel.org;
> > devicetree@...r.kernel.org; linux-riscv@...ts.infradead.org; linux-
> > kernel@...r.kernel.org; Anup Patel <apatel@...tanamicro.com>
> > Subject: [RFC PATCH 6/8] mailbox: Add RISC-V SBI message proxy (MPXY)
> > based mailbox driver
> >
> > Add a mailbox controller driver for the new SBI message proxy extension
> > which is part of the SBI v3.0 specification.
> >
> > Co-developed-by: Rahul Pathak <rpathak@...tanamicro.com>
> > Signed-off-by: Rahul Pathak <rpathak@...tanamicro.com>
> > Signed-off-by: Anup Patel <apatel@...tanamicro.com>
> > ---
> > drivers/mailbox/Kconfig | 11 +
> > drivers/mailbox/Makefile | 2 +
> > drivers/mailbox/riscv-sbi-mpxy-mbox.c | 979
> > ++++++++++++++++++++++++++
> > 3 files changed, 992 insertions(+)
> > create mode 100644 drivers/mailbox/riscv-sbi-mpxy-mbox.c
> >
>
> [...]
>
> > sbi-mpxy-mbox.c
> > new file mode 100644
> > index 000000000000..0592df3028f9
> > --- /dev/null
> > +++ b/drivers/mailbox/riscv-sbi-mpxy-mbox.c
> > @@ -0,0 +1,979 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * RISC-V SBI Message Proxy (MPXY) mailbox controller driver
> > + *
> > + * Copyright (C) 2024 Ventana Micro Systems Inc.
> > + */
> > +
> > +#include <asm/sbi.h>
> > +#include <linux/cpu.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/jump_label.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mailbox_controller.h>
> > +#include <linux/mailbox/riscv-rpmi-message.h>
>
> > +#include <linux/mm.h>
> > +#include <linux/msi.h>
> > +#include <linux/module.h>
> > +#include <linux/of_irq.h>
> Sorting include header files based on alphanumeric.
Okay, I will update.
>
> > +#include <linux/percpu.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/smp.h>
> > +
> > +/* ====== SBI MPXY extension data structures ====== */
> > +
> > +/* SBI MPXY MSI related channel attributes */ struct sbi_mpxy_msi_info
> > +{
> > + /* Lower 32-bits of the MSI target address */
> > + u32 msi_addr_lo;
> > + /* Upper 32-bits of the MSI target address */
> > + u32 msi_addr_hi;
> > + /* MSI data value */
> > + u32 msi_data;
> > +};
> > +
> > +/*
> > + * SBI MPXY standard channel attributes.
> > + *
> > + * NOTE: The sequence of attribute fields are as-per the
> > + * defined sequence in the attribute table in spec (or
> > + * as-per the enum sbi_mpxy_attribute_id).
> > + */
> > +struct sbi_mpxy_channel_attrs {
> > + /* Message protocol ID */
> > + u32 msg_proto_id;
> > + /* Message protocol Version */
> Don't need capital letter for "version" .
Okay, I will update.
>
> > + u32 msg_proto_version;
> > + /* Message protocol maximum message length */
> > + u32 msg_max_len;
> > + /* Message protocol message send timeout in microseconds */
> > + u32 msg_send_timeout;
> > + /* Message protocol message completion timeout in microseconds */
> > + u32 msg_completion_timeout;
> > + /* Bit array for channel capabilities */
> > + u32 capability;
> > + /* SSE Event Id */
> Same for 'event'.
Okay, I will update.
> > + u32 sse_event_id;
> > + /* MSI enable/disable control knob */
> > + u32 msi_control;
> > + /* Channel MSI info */
> > + struct sbi_mpxy_msi_info msi_info;
> > + /* Events State Control */
> Same here
Okay, I will update.
>
> > + u32 events_state_ctrl;
> > +};
> > +
> > +/*
>
>
> [...]
>
> > +
> > +static int mpxy_send_message_with_resp(u32 channel_id, u32 msg_id,
> > + void *tx, unsigned long tx_len,
> > + void *rx, unsigned long max_rx_len,
> > + unsigned long *rx_len)
> > +{
> > + struct mpxy_local *mpxy = this_cpu_ptr(&mpxy_local);
> > + unsigned long rx_bytes;
> > + struct sbiret sret;
> > +
> > + if (!mpxy->shmem_active)
> > + return -ENODEV;
> > + if (!tx && tx_len)
> > + return -EINVAL;
> > +
> > + get_cpu();
> > +
> > + /* Message protocols allowed to have no data in messages */
> > + if (tx_len)
> > + memcpy(mpxy->shmem, tx, tx_len);
> > +
> > + sret = sbi_ecall(SBI_EXT_MPXY,
> > SBI_EXT_MPXY_SEND_MSG_WITH_RESP,
> > + channel_id, msg_id, tx_len, 0, 0, 0);
> > + if (rx && !sret.error) {
> > + rx_bytes = sret.value;
> > + rx_bytes = min(max_rx_len, rx_bytes);
>
> Caller should know if the rx_bytes is larger than max_rx_len?
Good catch.
It is better to just fail when rx_bytes > max_rx_len so that
caller can deal with an undersized rx buffer.
I will update accordingly.
>
> > + memcpy(rx, mpxy->shmem, rx_bytes);
> > + if (rx_len)
> > + *rx_len = rx_bytes;
> > + }
> > +
> > + put_cpu();
> > + return sbi_err_map_linux_errno(sret.error);
> > +}
> > +
>
> [...]
>
> > +
> > +static int mpxy_mbox_setup_msi(struct mbox_chan *chan,
> > + struct mpxy_mbox_channel *mchan) {
> > + struct device *dev = mchan->mbox->dev;
> > + int rc;
> > +
> > + /* Do nothing if MSI not supported */
> > + if (mchan->msi_irq == U32_MAX)
> > + return 0;
> > +
> > + /* Request channel MSI handler */
> > + rc = request_threaded_irq(mchan->msi_irq,
> > + mpxy_mbox_irq_event,
> > + mpxy_mbox_irq_thread,
> > + 0, dev_name(dev), chan);
> > + if (rc) {
> > + dev_err(dev, "failed to request MPXY channel 0x%x IRQ\n",
> > + mchan->channel_id);
> > + return rc;
> > + }
> > +
> > + /* Enable channel MSI control */
> > + mchan->attrs.msi_control = 1;
> > + rc = mpxy_write_attrs(mchan->channel_id,
> > SBI_MPXY_ATTR_MSI_CONTROL,
> > + 1, &mchan->attrs.msi_control);
> > + if (rc) {
> > + dev_err(dev, "enable MSI control failed for MPXY channel
> > 0x%x\n",
> > + mchan->channel_id);
> > + free_irq(mchan->msi_irq, chan);
>
> Set mchan->attrs.msi_control = 0 if failed?
Okay, I will update.
>
> > + return rc;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void mpxy_mbox_cleanup_msi(struct mbox_chan *chan,
> > + struct mpxy_mbox_channel *mchan)
> > +{
> > + struct device *dev = mchan->mbox->dev;
> > + int rc;
> > +
> > + /* Do nothing if MSI not supported */
> > + if (mchan->msi_irq == U32_MAX)
>
>
> Should check if(!mchan->attrs.msi_control) instead of mchan->msi_irq?
Actually, we should check both over here as well as in
mpxy_mbox_setup_msi() to avoid inappropriate calls
to these functions.
>
>
> > + return;
> > +
> > + /* Disable channel MSI control */
> > + mchan->attrs.msi_control = 0;
> > + rc = mpxy_write_attrs(mchan->channel_id,
> > SBI_MPXY_ATTR_MSI_CONTROL,
> > + 1, &mchan->attrs.msi_control);
> > + if (rc) {
> > + dev_err(dev, "disable MSI control failed for MPXY channel
> > 0x%x\n",
> > + mchan->channel_id);
> > + }
> > +
> > + /* Free channel MSI handler */
> > + free_irq(mchan->msi_irq, chan);
> > +}
> > +
> > +static int mpxy_mbox_setup_events(struct mpxy_mbox_channel *mchan) {
> > + struct device *dev = mchan->mbox->dev;
> > + int rc;
> > +
> > + /* Do nothing if events state not supported */
> > + if (!mchan->have_events_state)
> > + return 0;
> > +
> > + /* Enable channel events state */
> > + mchan->attrs.events_state_ctrl = 1;
> > + rc = mpxy_write_attrs(mchan->channel_id,
> > SBI_MPXY_ATTR_EVENTS_STATE_CONTROL,
> > + 1, &mchan->attrs.events_state_ctrl);
> > + if (rc) {
> > + dev_err(dev, "enable events state failed for MPXY channel
> > 0x%x\n",
> > + mchan->channel_id);
>
> Should set mchan->attrs.events_state_ctrl = 0; ?
Okay, I will update.
>
> > + return rc;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void mpxy_mbox_cleanup_events(struct mpxy_mbox_channel
> > *mchan) {
> > + struct device *dev = mchan->mbox->dev;
> > + int rc;
> > +
> > + /* Do nothing if events state not supported */
> > + if (!mchan->have_events_state)
> Check also if (!mchan->attrs.events_state_ctrl)?
Okay, I will update.
>
> > + return;
> > +
> > + /* Disable channel events state */
> > + mchan->attrs.events_state_ctrl = 0;
> > + rc = mpxy_write_attrs(mchan->channel_id,
> > SBI_MPXY_ATTR_EVENTS_STATE_CONTROL,
> > + 1, &mchan->attrs.events_state_ctrl);
> > + if (rc) {
> > + dev_err(dev, "disbable events state failed for MPXY channel
>
> Typo ' disbable'.
Okay, I will update.
>
> > 0x%x\n",
> > + mchan->channel_id);
> > + }
> > +}
> > +
>
>
> [...]
>
>
> > +
> > +static int mpxy_mbox_probe(struct platform_device *pdev) {
> > + struct device *dev = &pdev->dev;
> > + struct mpxy_mbox_channel *mchan;
> > + struct mpxy_mbox *mbox;
> > + int i, msi_idx, rc;
> > + u32 *channel_ids;
> > +
> > + /*
> > + * Initialize MPXY shared memory only once. This also ensures
> > + * that SBI MPXY mailbox is probed only once.
> > + */
> > + if (mpxy_shmem_init_done) {
> > + dev_err(dev, "SBI MPXY mailbox already initialized\n");
> > + return -EALREADY;
> > + }
> > +
> > + /* Probe for SBI MPXY extension */
> > + if (sbi_spec_version < sbi_mk_version(1, 0) ||
> > + sbi_probe_extension(SBI_EXT_MPXY) <= 0) {
> > + dev_info(dev, "SBI MPXY extension not available\n");
> > + return -ENODEV;
> > + }
> > +
> > + /* Setup cpuhp notifier for per-CPU MPXY shared memory */
> > + cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "riscv/sbi-mpxy-
> > shmem",
> > + mpxy_setup_shmem, mpxy_cleanup_shmem);
> > +
> > + /* Mark as MPXY shared memory initialization done */
> > + mpxy_shmem_init_done = true;
> > +
> > + /* Allocate mailbox instance */
> > + mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> > + if (!mbox)
> > + return -ENOMEM;
> > + mbox->dev = dev;
> > + platform_set_drvdata(pdev, mbox);
> > +
> > + /* Find-out of number of channels */
> > + rc = mpxy_get_channel_count(&mbox->channel_count);
> > + if (rc) {
> > + dev_err(dev, "failed to get number of MPXY channels\n");
> Suggest print 'rc' value when error. Same for other error messages below.
Okay, I will update.
>
> > + return rc;
> > + }
> > + if (!mbox->channel_count) {
> > + dev_err(dev, "no MPXY channels available\n");
> > + return -ENODEV;
> > + }
> > +
> > + /* Allocate and fetch all channel IDs */
> > + channel_ids = devm_kcalloc(dev, mbox->channel_count,
> > + sizeof(*channel_ids), GFP_KERNEL);
> > + if (!channel_ids)
> > + return -ENOMEM;
> > + rc = mpxy_get_channel_ids(mbox->channel_count, channel_ids);
> > + if (rc) {
> > + dev_err(dev, "failed to get number of MPXY channels\n");
> > + return rc;
> > + }
> > +
>
> [...]
>
> Regards
> Ley Foon
Regards,
Anup
Powered by blists - more mailing lists