| 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=C2XJwgsC5AK-eVOHQqN1tPxtrsTjVoKdHgALbREv=sb8zQ@mail.gmail.com>
Date: Mon, 9 Jun 2025 17:59:40 +0530
From: Anup Patel <apatel@...tanamicro.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.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>, Thomas Gleixner <tglx@...utronix.de>,
"Rafael J . Wysocki" <rafael@...nel.org>, Mika Westerberg <mika.westerberg@...ux.intel.com>,
Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>,
Uwe Kleine-König <ukleinek@...nel.org>,
Palmer Dabbelt <palmer@...belt.com>, Paul Walmsley <paul.walmsley@...ive.com>,
Len Brown <lenb@...nel.org>, Sunil V L <sunilvl@...tanamicro.com>,
Rahul Pathak <rpathak@...tanamicro.com>, Leyfoon Tan <leyfoon.tan@...rfivetech.com>,
Atish Patra <atish.patra@...ux.dev>, Andrew Jones <ajones@...tanamicro.com>,
Samuel Holland <samuel.holland@...ive.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
Subject: Re: [PATCH v4 07/23] mailbox: Add RISC-V SBI message proxy (MPXY)
based mailbox driver
On Wed, May 28, 2025 at 4:23 PM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>
> On Sun, May 25, 2025 at 02:16:54PM +0530, Anup Patel wrote:
> > Add a mailbox controller driver for the new SBI message proxy extension
> > which is part of the SBI v3.0 specification.
>
> ...
>
> > +#include <asm/sbi.h>
>
> asm/* usually goes after generic linux/* ones. Why here?
I am not aware of any such convention but I will update anyway.
>
> > +#include <linux/cpu.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/jump_label.h>
>
> > +#include <linux/kernel.h>
>
> What for?
>
> > +#include <linux/mailbox_controller.h>
> > +#include <linux/mailbox/riscv-rpmi-message.h>
> > +#include <linux/mm.h>
> > +#include <linux/module.h>
> > +#include <linux/msi.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/percpu.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/smp.h>
>
> Please, double check that you follow IWYU (Include What You Use) principle and
> have no "proxy" headers.
>
> (E.g., types.h is missing here, but it got included via one of the above, like
> kernel.h)
>
Okay, I will update.
> ...
>
> > +
> > + get_cpu();
> > +
> > + /* Get the remaining and returned fields to calculate total */
> > + sret = sbi_ecall(SBI_EXT_MPXY, SBI_EXT_MPXY_GET_CHANNEL_IDS,
> > + 0, 0, 0, 0, 0, 0);
> > + if (!sret.error) {
>
> Can it be standard pattern?
>
> if (error)
> goto err_put_cpu;
>
> Ditto for other similar cases.
Okay, I will update.
>
> > + remaining = le32_to_cpu(sdata->remaining);
> > + returned = le32_to_cpu(sdata->returned);
> > + *channel_count = remaining + returned;
> > + }
> > +
> > + put_cpu();
> > + return sbi_err_map_linux_errno(sret.error);
> > +}
>
> ...
>
> > +static int mpxy_get_channel_ids(u32 channel_count, u32 *channel_ids)
> > +{
> > + u32 remaining, returned, sidx, start_index = 0, cidx = 0;
> > + struct mpxy_local *mpxy = this_cpu_ptr(&mpxy_local);
> > + struct sbi_mpxy_channel_ids_data *sdata = mpxy->shmem;
> > + struct sbiret sret;
> > +
> > + if (!mpxy->shmem_active)
> > + return -ENODEV;
> > + if (!channel_count || !channel_ids)
> > + return -EINVAL;
> > +
> > + get_cpu();
> > +
> > + do {
> > + sret = sbi_ecall(SBI_EXT_MPXY, SBI_EXT_MPXY_GET_CHANNEL_IDS,
> > + start_index, 0, 0, 0, 0, 0);
> > + if (sret.error)
> > + goto done;
> > +
> > + remaining = le32_to_cpu(sdata->remaining);
> > + returned = le32_to_cpu(sdata->returned);
> > +
> > + for (sidx = 0; sidx < returned && cidx < channel_count; sidx++) {
> > + channel_ids[cidx] = le32_to_cpu(sdata->channel_array[sidx]);
> > + cidx += 1;
> > + }
> > +
> > + start_index = cidx;
> > +
> > + } while (remaining);
> > +
> > +done:
>
> It sounds to me like an 'err_put_cpu' for the name.
Okay, I will update.
>
> > + put_cpu();
> > + return sbi_err_map_linux_errno(sret.error);
> > +}
>
> > +static int mpxy_write_attrs(u32 channel_id, u32 base_attrid, u32 attr_count,
> > + u32 *attrs_buf)
> > +{
> > + struct mpxy_local *mpxy = this_cpu_ptr(&mpxy_local);
> > + struct sbiret sret;
> > + u32 i;
> > +
> > + if (!mpxy->shmem_active)
> > + return -ENODEV;
> > + if (!attr_count || !attrs_buf)
> > + return -EINVAL;
> > +
> > + get_cpu();
> > +
> > + for (i = 0; i < attr_count; i++)
> > + ((__le32 *)mpxy->shmem)[i] = cpu_to_le32(attrs_buf[i]);
>
> Don't we have helpers for this? They are suffixed with _array.
> https://elixir.bootlin.com/linux/v6.15-rc6/source/include/linux/byteorder/generic.h#L168
> Don't forget to have asm/byteorder.h being included.
>
> Ditto for the similar case(s).
The cpu_to_le32_array() and le32_to_cpu_array() helpers update data
in-place but over here we have separate source and destination.
>
> > + sret = sbi_ecall(SBI_EXT_MPXY, SBI_EXT_MPXY_WRITE_ATTRS,
> > + channel_id, base_attrid, attr_count, 0, 0, 0);
> > +
> > + put_cpu();
> > + return sbi_err_map_linux_errno(sret.error);
> > +}
>
> ...
>
> > + msg->error = rc;
> > + return 0;
>
> What's the point of having int and not void function?
Okay, I will update.
>
> ...
>
> > + sizeof(mchan->rpmi_attrs) / sizeof(u32),
> > + (u32 *)&mchan->rpmi_attrs);
>
> Why casting? What about alignment?
The RPMI attributes (aka struct sbi_mpxy_rpmi_channel_attrs) are
a collection of u32 attributes hence we can also treat rpmi_attrs
as a u32 array. Further, the rpmi_attrs is XLEN aligned within the
struct mpxy_mbox_channel so no alignment issue with the casting
on both RV32 and RV64.
If we want to avoid the casting then we will have to use a temporary
u32 array plus additional memcpy().
>
> ...
>
> > + while (1) {
>
> Do it as do {} while, it will give an idea that the loop will run at least once
> (without looking into condition), ideally it shouldn't be infinite loop, but it
> might be harder to realise.
Okay, I will update.
>
> > + rc = mpxy_get_notifications(mchan->channel_id, notif, &data_len);
> > + if (rc || !data_len)
> > + break;
> > +
> > + if (mchan->attrs.msg_proto_id == SBI_MPXY_MSGPROTO_RPMI_ID)
> > + mpxy_mbox_peek_rpmi_data(chan, mchan, notif, data_len);
> > +
> > + have_notifications = true;
> > + }
>
> ...
>
> > +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)
> > + return;
> > +
> > + /* Do nothing if events state already disabled */
> > + if (!mchan->attrs.events_state_ctrl)
> > + 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, "disable events state failed for MPXY channel 0x%x\n",
> > + mchan->channel_id);
> > + }
>
> Redundant {}.
Okay, I will update.
>
> > +}
>
> ...
>
> > +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;
>
> Why is 'i' signed? Also make sure either they use POD (like unsigned int) or
> exact data type the upper limit does or API where you pass it in.
'i' is used to iterate over channels where channel_count and channel ids
are u32 so better to use the u32 data type.
>
> > + 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;
> > + }
> > +
> > + /* Find-out shared memory size */
> > + mpxy_shmem_size = mpxy_get_shmem_size();
> > +
> > + /*
> > + * Setup MPXY shared memory on each CPU
> > + *
> > + * Note: Don't cleanup MPXY shared memory upon CPU power-down
> > + * because the RPMI System MSI irqchip driver needs it to be
> > + * available when migrating IRQs in CPU power-down path.
> > + */
> > + cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "riscv/sbi-mpxy-shmem",
> > + mpxy_setup_shmem, NULL);
> > +
> > + /* 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)
> > + return dev_err_probe(dev, rc, "failed to get number of MPXY channels\n");
> > + if (!mbox->channel_count)
> > + dev_err_probe(dev, -ENODEV, "no MPXY channels available\n");
> > +
> > + /* 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)
> > + return dev_err_probe(dev, rc, "failed to MPXY channel IDs\n");
> > +
> > + /* Populate all channels */
> > + mbox->channels = devm_kcalloc(dev, mbox->channel_count,
> > + sizeof(*mbox->channels), GFP_KERNEL);
> > + if (!mbox->channels)
> > + return -ENOMEM;
> > + for (i = 0; i < mbox->channel_count; i++) {
> > + mchan = &mbox->channels[i];
> > + mchan->mbox = mbox;
> > + mchan->channel_id = channel_ids[i];
> > +
> > + rc = mpxy_read_attrs(mchan->channel_id, SBI_MPXY_ATTR_MSG_PROT_ID,
> > + sizeof(mchan->attrs) / sizeof(u32),
> > + (u32 *)&mchan->attrs);
> > + if (rc) {
> > + return dev_err_probe(dev, rc,
> > + "MPXY channel 0x%x read attrs failed\n",
> > + mchan->channel_id);
> > + }
> > +
> > + if (mchan->attrs.msg_proto_id == SBI_MPXY_MSGPROTO_RPMI_ID) {
> > + rc = mpxy_mbox_read_rpmi_attrs(mchan);
> > + if (rc) {
> > + return dev_err_probe(dev, rc,
> > + "MPXY channel 0x%x read RPMI attrs failed\n",
> > + mchan->channel_id);
> > + }
> > + }
> > +
> > + mchan->notif = devm_kzalloc(dev, mpxy_shmem_size, GFP_KERNEL);
> > + if (!mchan->notif)
> > + return -ENOMEM;
> > +
> > + mchan->max_xfer_len = min(mpxy_shmem_size, mchan->attrs.msg_max_len);
> > +
> > + if ((mchan->attrs.capability & SBI_MPXY_CHAN_CAP_GET_NOTIFICATIONS) &&
> > + (mchan->attrs.capability & SBI_MPXY_CHAN_CAP_EVENTS_STATE))
> > + mchan->have_events_state = true;
> > +
> > + if ((mchan->attrs.capability & SBI_MPXY_CHAN_CAP_GET_NOTIFICATIONS) &&
> > + (mchan->attrs.capability & SBI_MPXY_CHAN_CAP_MSI))
> > + mchan->msi_index = mbox->msi_count++;
> > + else
> > + mchan->msi_index = U32_MAX;
> > + mchan->msi_irq = U32_MAX;
> > + }
>
> > + /* Free-up channel IDs */
> > + devm_kfree(dev, channel_ids);
>
> Just no. This is most likely (99.9%) wrong use of managed resources. This is
> the biggest issue with this patch so far.
Okay, I will use __free(kfree) for channel_ids[].
>
> > + /* Initialize mailbox controller */
> > + mbox->controller.txdone_irq = false;
> > + mbox->controller.txdone_poll = false;
> > + mbox->controller.ops = &mpxy_mbox_ops;
> > + mbox->controller.dev = dev;
> > + mbox->controller.num_chans = mbox->channel_count;
> > + mbox->controller.fw_xlate = mpxy_mbox_fw_xlate;
> > + mbox->controller.chans = devm_kcalloc(dev, mbox->channel_count,
> > + sizeof(*mbox->controller.chans),
> > + GFP_KERNEL);
> > + if (!mbox->controller.chans)
> > + return -ENOMEM;
> > + for (i = 0; i < mbox->channel_count; i++)
> > + mbox->controller.chans[i].con_priv = &mbox->channels[i];
> > +
> > + /* Set the MSI domain if not available */
> > + if (!dev_get_msi_domain(dev)) {
> > + /*
> > + * The device MSI domain for OF devices is only set at the
> > + * time of populating/creating OF device. If the device MSI
> > + * domain is discovered later after the OF device is created
> > + * then we need to set it explicitly before using any platform
> > + * MSI functions.
> > + */
> > + if (is_of_node(dev_fwnode(dev)))
> > + of_msi_configure(dev, to_of_node(dev_fwnode(dev)));
> > + }
> > +
> > + /* Setup MSIs for mailbox (if required) */
> > + if (mbox->msi_count) {
> > + mbox->msi_index_to_channel = devm_kcalloc(dev, mbox->msi_count,
> > + sizeof(*mbox->msi_index_to_channel),
> > + GFP_KERNEL);
> > + if (!mbox->msi_index_to_channel)
> > + return -ENOMEM;
> > +
> > + for (msi_idx = 0; msi_idx < mbox->msi_count; msi_idx++) {
> > + for (i = 0; i < mbox->channel_count; i++) {
> > + mchan = &mbox->channels[i];
> > + if (mchan->msi_index == msi_idx) {
> > + mbox->msi_index_to_channel[msi_idx] = mchan;
> > + break;
> > + }
> > + }
> > + }
> > +
> > + rc = platform_device_msi_init_and_alloc_irqs(dev, mbox->msi_count,
> > + mpxy_mbox_msi_write);
> > + if (rc) {
> > + return dev_err_probe(dev, rc, "Failed to allocate %d MSIs\n",
> > + mbox->msi_count);
> > + }
> > +
> > + for (i = 0; i < mbox->channel_count; i++) {
> > + mchan = &mbox->channels[i];
> > + if (mchan->msi_index == U32_MAX)
> > + continue;
> > + mchan->msi_irq = msi_get_virq(dev, mchan->msi_index);
> > + }
> > + }
> > +
> > + /* Register mailbox controller */
> > + rc = devm_mbox_controller_register(dev, &mbox->controller);
> > + if (rc) {
> > + dev_err_probe(dev, rc, "Registering SBI MPXY mailbox failed\n");
> > + if (mbox->msi_count)
> > + platform_device_msi_free_irqs_all(dev);
> > + return rc;
> > + }
> > +
> > + dev_info(dev, "mailbox registered with %d channels\n",
> > + mbox->channel_count);
> > + return 0;
> > +}
>
> ...
>
> > + if (mbox->msi_count)
>
> Is this check really needed?
MSIs are optional for the SBI MPXY mailbox so we should only use
platform_device_msi_xyz() APIs only when MSIs are available.
>
> > + platform_device_msi_free_irqs_all(mbox->dev);
> > +}
>
> ...
>
> > +static const struct of_device_id mpxy_mbox_of_match[] = {
> > + {.compatible = "riscv,sbi-mpxy-mbox", },
> > + {},
>
> No comma for the terminator entry.
> You should apply the reviewers' comments on all of your code, not just where it
> was commented.
Okay, I will update.
Regards,
Anup
Powered by blists - more mailing lists