On Wed, May 28, 2025 at 4:23 PM Andy Shevchenko
[off-list ref] wrote:

On Sun, May 25, 2025 at 02:16:54PM +0530, Anup Patel wrote:

quoted

Add a mailbox controller driver for the new SBI message proxy extension
which is part of the SBI v3.0 specification.

...

quoted

+#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.

quoted

+#include <linux/cpu.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/jump_label.h>

quoted

+#include <linux/kernel.h>

What for?

quoted

+#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.

...

quoted

+
+     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.

quoted

+             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);
+}

...

quoted

+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.

quoted

+     put_cpu();
+     return sbi_err_map_linux_errno(sret.error);
+}

quoted

+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.

quoted

+     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);
+}

...

quoted

+     msg->error = rc;
+     return 0;

What's the point of having int and not void function?

Okay, I will update.

...

quoted

+                            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().

...

quoted

+     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.

quoted

+             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;
+     }

...

quoted

+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);

quoted

+     if (rc) {
+             dev_err(dev, "disable events state failed for MPXY channel 0x%x\n",
+                     mchan->channel_id);
+     }

Redundant {}.

Okay, I will update.

quoted

+}

...

quoted

+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.

quoted

+     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;
+     }

quoted

+     /* 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[].

quoted

+     /* 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;
+}

...

quoted

+     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.

quoted

+             platform_device_msi_free_irqs_all(mbox->dev);
+}

...

quoted

+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