On Wed, Jul 2, 2025 at 6:20 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> 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.
