Re: [PATCH v3 1/5] mbox: add polarfire soc system controller mailbox

["Jonathan Neuschäfer" <j.neuschaefer@gmx.net>]

[-- Attachment #1: Type: text/plain, Size: 8544 bytes --]

Hello,

I've added review comments below. Some of them might be more detailed
than necessary, and reflect my opinion rather than something that must
be fixed. Anyway, I hope my comments make sense.

On Wed, Dec 23, 2020 at 04:32:55PM +0000, conor.dooley@microchip.com wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> This driver adds support for the single mailbox channel of the MSS
> system controller on the Microchip PolarFire SoC.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
[...]
> +#define SERVICES_CR_OFFSET		0x50u
> +#define SERVICES_SR_OFFSET		0x54u
> +#define MAILBOX_REG_OFFSET		0x800u
> +#define MSS_SYS_BUSY			-EBUSY
> +#define MSS_SYS_PARAM_ERR		-EINVAL

Defining your own aliases for -Esomething is rather uncommon.

> +#define MSS_SYS_MAILBOX_DATA_OFFSET	0u
> +#define SCB_MASK_WIDTH			16u
> +
> +/* SCBCTRL service control register */
> +
> +#define SCB_CTRL_REQ (0)
> +#define SCB_CTRL_REQ_MASK BIT(SCB_CTRL_REQ)
> +
> +#define SCB_CTRL_BUSY (1)
> +#define SCB_CTRL_BUSY_MASK BIT(SCB_CTRL_BUSY)
> +
> +#define SCB_CTRL_ABORT (2)
> +#define SCB_CTRL_ABORT_MASK BIT(SCB_CTRL_ABORT)
> +
> +#define SCB_CTRL_NOTIFY (3)
> +#define SCB_CTRL_NOTIFY_MASK BIT(SCB_CTRL_NOTIFY)
> +
> +#define SCB_CTRL_POS (16)
> +#define SCB_CTRL_MASK GENMASK(SCB_CTRL_POS + SCB_MASK_WIDTH, SCB_CTRL_POS)
> +
> +/* SCBCTRL service status registers */

registers -> register ?
It seems to be only one status register.

> +
> +#define SCB_STATUS_REQ (0)
> +#define SCB_STATUS_REQ_MASK BIT(SCB_STATUS_REQ)
> +
> +#define SCB_STATUS_BUSY (1)
> +#define SCB_STATUS_BUSY_MASK BIT(SCB_STATUS_BUSY)
> +
> +#define SCB_STATUS_ABORT (2)
> +#define SCB_STATUS_ABORT_MASK BIT(SCB_STATUS_ABORT)
> +
> +#define SCB_STATUS_NOTIFY (3)
> +#define SCB_STATUS_NOTIFY_MASK BIT(SCB_STATUS_NOTIFY)
> +
> +#define SCB_STATUS_POS (16)
> +#define SCB_STATUS_MASK GENMASK(SCB_STATUS_POS + SCB_MASK_WIDTH, SCB_STATUS_POS)
> +
> +struct mpfs_mbox {
> +	struct mbox_controller controller;
> +	struct device *dev;
> +	int irq;
> +	void __iomem *mailbox_base;
> +	void __iomem *int_reg;
> +	struct mbox_chan *chan;
> +	u16 response_size;
> +	u16 response_offset;
> +};
> +
> +static bool mpfs_mbox_busy(struct mpfs_mbox *mbox)
> +{
> +	u32 status;
> +
> +	status = readl_relaxed(mbox->mailbox_base + SERVICES_SR_OFFSET);
> +
> +	return status & SCB_STATUS_BUSY_MASK;
> +}
> +
> +static struct mpfs_mbox *mbox_chan_to_mpfs_mbox(struct mbox_chan *chan)
> +{
> +	if (!chan)
> +		return NULL;
> +
> +	return (struct mpfs_mbox *)chan->con_priv;
> +}
> +
> +static int mpfs_mbox_send_data(struct mbox_chan *chan, void *data)
> +{
> +	u32 index;
> +	u32 *word_buf;
> +	u8 *byte_buf;
> +	u8 byte_off;
> +	u8 extra_bits;
> +	u8 i;
> +	u32 mailbox_val = 0u;
> +	u16 mbox_offset;
> +	u16 mbox_options_select;
> +	u32 mbox_tx_trigger;

the mbox_ prefix seems unnecessary because the whole driver deals with
a mailbox.

Some of the variables are only used in if/for scopes below. I'd move
their declaration down into these scopes, to make the outer scope of the
function a little easier to understand.

> +	struct mpfs_mss_msg *msg = data;
> +	struct mpfs_mbox *mbox = mbox_chan_to_mpfs_mbox(chan);
> +
> +	mbox_offset = msg->mailbox_offset;

mbox_offset is only used once. The indirection of storing the value in
another variable makes the code slightly slower to read, IMO.

> +	mbox->response_size = msg->response_size;
> +	mbox->response_offset = msg->response_offset;
> +
> +	if (mpfs_mbox_busy(mbox))
> +		return MSS_SYS_BUSY;
> +
> +	mbox_options_select = ((mbox_offset << 7u) | (msg->cmd_opcode & 0x7fu));
> +	mbox_tx_trigger = (((mbox_options_select << SCB_CTRL_POS) &
> +		SCB_CTRL_MASK) | SCB_CTRL_REQ_MASK | SCB_STATUS_NOTIFY_MASK);

Slightly reformatted, you could save a few parentheses:

	mbox_options_select = (mbox_offset << 7u) | (msg->cmd_opcode & 0x7fu);
	mbox_tx_trigger = (mbox_options_select << SCB_CTRL_POS) & SCB_CTRL_MASK;
	mbox_tx_trigger |= SCB_CTRL_REQ_MASK | SCB_STATUS_NOTIFY_MASK;


> +	/* Code for MSS_SYS_PARAM_ERR is not implemented with this version of driver. */

What is the "code for MSS_SYS_PARAM_ERR" semantically? Input validation?

> +	writel_relaxed(0, mbox->int_reg);

What does a write to mbox->int_reg do? Does it acknowledge an interrupt?
It would be interesting to have an explaining comment here.

> +
> +	if (msg->cmd_data_size) {
> +		byte_buf = msg->cmd_data;
> +
> +		for (index = 0; index < (msg->cmd_data_size / 4); index++)
> +			writel_relaxed(word_buf[index],
> +				       mbox->mailbox_base + MAILBOX_REG_OFFSET + index);

word_buf is uninitialized.

In mpfs_mbox_rx_data, you access the registers at mbox->mailbox_base +
MAILBOX_REG_OFFSET in steps of four bytes, here you increment the offset
in steps of one byte, because the index isn't scaled. This seems wrong.

> +		extra_bits = msg->cmd_data_size & 3;
> +		if (extra_bits) {
> +			byte_off = ALIGN_DOWN(msg->cmd_data_size, 4);
> +			byte_buf = msg->cmd_data + byte_off;
> +
> +			mailbox_val = readl_relaxed(mbox->mailbox_base +
> +						    MAILBOX_REG_OFFSET + index);
> +
> +			for (i = 0u; i < extra_bits; i++) {
> +				mailbox_val &= ~(0xffu << (i * 8u));
> +				mailbox_val |= (byte_buf[i] << (i * 8u));
> +			}
> +
> +			writel_relaxed(mailbox_val,
> +				       mbox->mailbox_base + MAILBOX_REG_OFFSET + index);
> +		}
> +	}
> +

I'd move the calculation of mbox_tx_trigger down here, right before its
use, because it's not necessary for the code above (if I'm reading it
correctly).

> +	writel_relaxed(mbox_tx_trigger, mbox->mailbox_base + SERVICES_CR_OFFSET);
> +
> +	return 0;
> +}
> +
> +static inline bool mpfs_mbox_pending(struct mpfs_mbox *mbox)
> +{
> +	u32 status;
> +
> +	status = readl_relaxed(mbox->mailbox_base + SERVICES_SR_OFFSET);
> +
> +	return !(status & SCB_STATUS_BUSY_MASK);
> +}
> +
> +static void mpfs_mbox_rx_data(struct mbox_chan *chan)
> +{
> +	struct mpfs_mbox *mbox = mbox_chan_to_mpfs_mbox(chan);
> +	u32 *data;
> +	u32 i;
> +	u32 response_limit;
> +
> +	data = devm_kzalloc(mbox->dev, sizeof(*data) * mbox->response_size, GFP_ATOMIC);

The use of devm_ seems bogus here, because the allocated data does not
have the same lifetime as the device; it is allocated and freed for
every RX event.

However, please use kcalloc instead of kzalloc, so you don't have to do
the multiplication manually. kcalloc checks for overflows.

> +	if (!data)
> +		dev_err(mbox->dev, "failed to assign memory for response\n", -ENOMEM);

There is no format specifier for parameter -ENOMEM.

In the !data case, the code will still run into the loop below, and
probably cause a crash.

> +
> +	response_limit = mbox->response_size + mbox->response_offset;
> +	if (mpfs_mbox_pending(mbox) && mbox->response_size > 0U) {
> +		for (i = mbox->response_offset; i < response_limit; i++) {
> +			data[i - mbox->response_offset] =
> +				readl_relaxed(mbox->mailbox_base + MAILBOX_REG_OFFSET + i * 0x4);
> +		}
> +	}

It seems slightly easier to me to let the loop run from zero to
mbox->response_size-1. Most importantly, it becomes easy to see that
data is not accessed out of bounds.  (But it's your choice)

	if (mpfs_mbox_pending(mbox)) {
		for (i = 0; i < mbox->response_size; i++) {
			data[i] = readl_relaxed(mbox->mailbox_base + MAILBOX_REG_OFFSET +
			                        (i + mbox->response_offset) * 0x4);
		}
	}


> +
> +	mbox_chan_received_data(chan, (void *)data);
> +	devm_kfree(mbox->dev, data);
> +}
> +
> +static irqreturn_t mpfs_mbox_inbox_isr(int irq, void *data)
> +{
> +	struct mbox_chan *chan = (struct mbox_chan *)data;

This cast and the one at the end of mpfs_mbox_rx_data are somewhat
uncessary, because C allows implicit conversion of void pointers to and
from other pointer types.

> +	struct mpfs_mbox *mbox = mbox_chan_to_mpfs_mbox(chan);
> +
> +	writel_relaxed(0, mbox->int_reg);
> +
> +	mpfs_mbox_rx_data(chan);
> +
> +	mbox_chan_txdone(chan, 0);
> +	return IRQ_HANDLED;
> +}
[...]
> +++ b/include/soc/microchip/mpfs.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + *
> + * Microchip PolarFire SoC (MPFS) mailbox

This is mpfs.h, but the comment implies it's only for mailbox-related
code, not for all of the Microchip PolarFire SoC. Is this intentional?


Best regards,
Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]