Re: [PATCH v7 07/24] mailbox: Add RISC-V SBI message proxy (MPXY) based mailbox driver

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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

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

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

> +	       ((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?

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

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

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

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

> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko