Message-ID: <CAK9=C2Ume2CmBYHYob7HSJHu=ZdfdWM+4JYPgFJ9Hir5Oi8cOg@mail.gmail.com>
Date: Thu, 3 Jul 2025 12:22:14 +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>, 
	Alexandre Ghiti <alex@...ti.fr>, 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-acpi@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 07/24] mailbox: Add RISC-V SBI message proxy (MPXY)
 based mailbox driver

On Wed, Jul 2, 2025 at 6:20 PM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>
> On Wed, Jul 02, 2025 at 10:43:28AM +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 <linux/cpu.h>
> > +#include <linux/errno.h>
> > +#include <linux/init.h>
> > +#include <linux/mailbox_controller.h>
> > +#include <linux/mailbox/riscv-rpmi-message.h>
>
> + minmax.h

Okay, I will add this.

>
> > +#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>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <asm/byteorder.h>
> > +#include <asm/sbi.h>
>
> ...
>
> > +static void mpxy_mbox_send_rpmi_data(struct mpxy_mbox_channel *mchan,
> > +                                  struct rpmi_mbox_message *msg)
> > +{
> > +     int rc = 0;
>
> Is it useful? I mean can you assign msg.error directly in each case?
> (Just asking)

I think it is more readable with a local variable for errors
but I don't mind directly assigning msg->error.

>
> > +     switch (msg->type) {
> > +     case RPMI_MBOX_MSG_TYPE_GET_ATTRIBUTE:
> > +             switch (msg->attr.id) {
> > +             case RPMI_MBOX_ATTR_SPEC_VERSION:
> > +                     msg->attr.value = mchan->attrs.msg_proto_version;
> > +                     break;
> > +             case RPMI_MBOX_ATTR_MAX_MSG_DATA_SIZE:
> > +                     msg->attr.value = mchan->max_xfer_len;
> > +                     break;
> > +             case RPMI_MBOX_ATTR_SERVICEGROUP_ID:
> > +                     msg->attr.value = mchan->rpmi_attrs.servicegroup_id;
> > +                     break;
> > +             case RPMI_MBOX_ATTR_SERVICEGROUP_VERSION:
> > +                     msg->attr.value = mchan->rpmi_attrs.servicegroup_version;
> > +                     break;
> > +             case RPMI_MBOX_ATTR_IMPL_ID:
> > +                     msg->attr.value = mchan->rpmi_attrs.impl_id;
> > +                     break;
> > +             case RPMI_MBOX_ATTR_IMPL_VERSION:
> > +                     msg->attr.value = mchan->rpmi_attrs.impl_version;
> > +                     break;
> > +             default:
> > +                     rc = -EOPNOTSUPP;
> > +                     break;
> > +             }
> > +             break;
> > +     case RPMI_MBOX_MSG_TYPE_SET_ATTRIBUTE:
> > +             /* None of the RPMI linux mailbox attributes are writeable */
> > +             rc = -EOPNOTSUPP;
> > +             break;
> > +     case RPMI_MBOX_MSG_TYPE_SEND_WITH_RESPONSE:
> > +             if ((!msg->data.request && msg->data.request_len) ||
> > +                 (msg->data.request &&
> > +                  msg->data.request_len > mchan->max_xfer_len) ||
> > +                 (!msg->data.response && msg->data.max_response_len)) {
> > +                     rc = -EINVAL;
> > +                     break;
> > +             }
> > +             if (!(mchan->attrs.capability & SBI_MPXY_CHAN_CAP_SEND_WITH_RESP)) {
> > +                     rc = -EIO;
> > +                     break;
> > +             }
> > +             rc = mpxy_send_message_with_resp(mchan->channel_id,
> > +                                              msg->data.service_id,
> > +                                              msg->data.request,
> > +                                              msg->data.request_len,
> > +                                              msg->data.response,
> > +                                              msg->data.max_response_len,
> > +                                              &msg->data.out_response_len);
> > +             break;
> > +     case RPMI_MBOX_MSG_TYPE_SEND_WITHOUT_RESPONSE:
> > +             if ((!msg->data.request && msg->data.request_len) ||
> > +                 (msg->data.request &&
> > +                  msg->data.request_len > mchan->max_xfer_len)) {
> > +                     rc = -EINVAL;
> > +                     break;
> > +             }
> > +             if (!(mchan->attrs.capability & SBI_MPXY_CHAN_CAP_SEND_WITHOUT_RESP)) {
> > +                     rc = -EIO;
> > +                     break;
> > +             }
> > +             rc = mpxy_send_message_without_resp(mchan->channel_id,
> > +                                                 msg->data.service_id,
> > +                                                 msg->data.request,
> > +                                                 msg->data.request_len);
> > +             break;
> > +     default:
> > +             rc = -EOPNOTSUPP;
> > +             break;
> > +     }
> > +
> > +     msg->error = rc;
> > +}
>
> ...
>
> > +static void mpxy_mbox_peek_rpmi_data(struct mbox_chan *chan,
> > +                                  struct mpxy_mbox_channel *mchan,
> > +                                  struct sbi_mpxy_notification_data *notif,
> > +                                  unsigned long events_data_len)
> > +{
> > +     struct rpmi_notification_event *event;
> > +     unsigned long pos = 0, event_size;
> > +     struct rpmi_mbox_message msg;
> > +
> > +     while ((pos < events_data_len) && !(pos & 0x3) &&
>
> 0x3 looks like a magic used for the non-aligned data.

This is paranoide check and can be dropped. I will update.

The RPMI spec clearly states that "An RPMI event may have associated
data whose size is specified in the EVENT_DATALEN field of the header
and this data size must be a multiple of 4-byte."

>
> > +            ((events_data_len - pos) <= sizeof(*event))) {
> > +             event = (struct rpmi_notification_event *)(notif->events_data + pos);
> > +
> > +             msg.type = RPMI_MBOX_MSG_TYPE_NOTIFICATION_EVENT;
> > +             msg.notif.event_datalen = le16_to_cpu(event->event_datalen);
> > +             msg.notif.event_id = event->event_id;
> > +             msg.notif.event_data = event->event_data;
> > +             msg.error = 0;
> > +
> > +             event_size = sizeof(*event) + msg.notif.event_datalen;
>
> Do you trust the source? This may wrap-around...
>
> > +             if (event_size > (events_data_len - pos)) {
> > +                     event_size = events_data_len - pos;
> > +                     goto skip_event;
> > +             }
> > +             if (event_size & 0x3)
> > +                     goto skip_event;
>
> ...and these checks be skipped. Is it a problem?

I agree these checks can be skipped.

>
> > +             mbox_chan_received_data(chan, &msg);
>
> > +skip_event:
>
> I think this may be replaced by a continue inside if you make a loop to be
> do {} while (). But it's just a thought, you can check if it gives a better
> readability after all.

I am more comfortable with "while ()" over here.

>
> > +             pos += event_size;
> > +     }
> > +}
>
> ...
>
> > +static int mpxy_mbox_probe(struct platform_device *pdev)
> > +{
> > +     u32 i, *channel_ids __free(kfree) = NULL;
>
> It's not recommended. Can you split channel_ids to the actual scope where it's
> used? Somewhere...

Let me create a separate function to populate channels
so that channel_ids will be used only in a limited scope.

>
> > +     struct device *dev = &pdev->dev;
> > +     struct mpxy_mbox_channel *mchan;
> > +     struct mpxy_mbox *mbox;
> > +     int msi_idx, rc;
> > +
> > +     /*
> > +      * 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 */
> > +     rc = mpxy_get_shmem_size(&mpxy_shmem_size);
> > +     if (rc)
> > +             return dev_err_probe(dev, rc, "failed to get MPXY shared memory size\n");
> > +
> > +     /*
> > +      * 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 = kcalloc(mbox->channel_count, sizeof(*channel_ids), GFP_KERNEL);
>
> ...here.
>
> > +     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 get 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;
> > +     }
> > +
> > +     /* 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 (dev_of_node(dev))
>
> Do you really need this check? What about ACPI?

Yes, this check is needed because of_msi_configure() only
works for OF does. For ACPI, we have a different way of
to re-configure MSI domain which is in a later patch.

In general, there is no generic fwnode API to find and
re-configure MSI domain in a bus independent way.

>
> > +                     of_msi_configure(dev, dev_of_node(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);
>
> Working driver doesn't need to issue a message.

The expected mailbox channels available to Linux will
vary based on platform. If there is a firmware bug then
Linux will not see all expected mailbox channels but the
probe of this driver will succeed.

>From the end user perspective (or user space), there
is no direct way of knowing the number of channels
details of each channel. In fact, the Linux mailbox
framework does not have common sysfs interface
to allow users know the mailbox controller instances
and their mailbox channels.

Regards,
Anup

Powered by blists - more mailing lists