[PATCH v1] net: can: mpfs_can: add Polarfire SoC MSS CAN

[PATCH v1] net: can: mpfs_can: add Polarfire SoC MSS CAN

[Conor Dooley]

From: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com>

The Microchip Polarfire SoC has two instances of CAN devices inside the
MSS block. Add driver to support this MSS CAN.

Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---

Hey Marc,
I've finally got around to sending this driver.

Unfortunately the author left microchip (which is part of why sending it
took so long). I've done some mechanical cleanup, but left it mostly
functionally unchanged mainly because I lack a setup to test it
thoroughly were I to do so.
I'm hoping that what's here is by and large okay on that front, and
maybe that sasiko will point some stuff out if there's stuff I missed.
I did remove/modify some code, so hopefully I caught all the obviously
problematic stuff.

The IP that we are using seems to lack an internal reset, so the reset
for the whole peripheral is used instead, and I wonder if it is
overeager? Doing ip link set xxx up causes a double reset, for example.

There's no binding of course, because that's been upstream for quite a
while!

CC: Marc Kleine-Budde <mkl@pengutronix.de>
CC: Vincent Mailhol <mailhol@kernel.org>
CC: Conor Dooley <conor.dooley@microchip.com>
CC: Daire McNamara <daire.mcnamara@microchip.com>
CC: linux-kernel@vger.kernel.org
CC: linux-can@vger.kernel.org
CC: linux-riscv@lists.infradead.org
CC: Valentina.FernandezAlanis@microchip.com
---
 MAINTAINERS                |   1 +
 drivers/net/can/Kconfig    |   9 +
 drivers/net/can/Makefile   |   1 +
 drivers/net/can/mpfs_can.c | 882 +++++++++++++++++++++++++++++++++++++
 4 files changed, 893 insertions(+)
 create mode 100644 drivers/net/can/mpfs_can.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 15011f5752a9..bc47aad34b0d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23258,6 +23258,7 @@ F:	drivers/firmware/microchip/mpfs-auto-update.c
 F:	drivers/gpio/gpio-mpfs.c
 F:	drivers/i2c/busses/i2c-microchip-corei2c.c
 F:	drivers/mailbox/mailbox-mpfs.c
+F:	drivers/net/can/mpfs_can.c
 F:	drivers/pci/controller/plda/pcie-microchip-host.c
 F:	drivers/pinctrl/microchip/pinctrl-mpfs-iomux0.c
 F:	drivers/pinctrl/microchip/pinctrl-mpfs-mssio.c
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index e4058708ae68..3e70a2716726 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -189,6 +189,15 @@ config CAN_KVASER_PCIEFD
 	    Kvaser M.2 PCIe 4xCAN
 	    Kvaser PCIe 8xCAN
 
+config CAN_POLARFIRE_SOC
+	tristate "Microchip Polarfire SoC CAN"
+	depends on ARCH_MICROCHIP || COMPILE_TEST
+	help
+	  Driver for the CAN controllers on Microchip Polarfire SoC.
+
+	  This driver can be built as a module. If so, the module will be
+	  called mpfs_can.
+
 config CAN_SLCAN
 	tristate "Serial / USB serial CAN Adaptors (slcan)"
 	depends on TTY
diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index 4010d17f8583..4dd1fa1104ff 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_CAN_KVASER_PCIEFD)	+= kvaser_pciefd/
 obj-$(CONFIG_CAN_MSCAN)		+= mscan/
 obj-$(CONFIG_CAN_M_CAN)		+= m_can/
 obj-$(CONFIG_CAN_PEAK_PCIEFD)	+= peak_canfd/
+obj-$(CONFIG_CAN_POLARFIRE_SOC) += mpfs_can.o
 obj-$(CONFIG_CAN_SJA1000)	+= sja1000/
 obj-$(CONFIG_CAN_SUN4I)		+= sun4i_can.o
 obj-$(CONFIG_CAN_TI_HECC)	+= ti_hecc.o
diff --git a/drivers/net/can/mpfs_can.c b/drivers/net/can/mpfs_can.c
new file mode 100644
index 000000000000..3a82726492ec
--- /dev/null
+++ b/drivers/net/can/mpfs_can.c
@@ -0,0 +1,882 @@
+// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+/*
+ * Microchip Polarfire SoC MSS CAN controller driver
+ *
+ * Copyright (C) 2023 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com>
+ *
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/can/dev.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+
+#define MPFS_CAN_ISR_OFFSET		0x00
+#define MPFS_CAN_IER_OFFSET		0x04
+#define MPFS_CAN_RXBUF_STAT_OFFSET	0x08
+#define MPFS_CAN_TXBUF_STAT_OFFSET	0x0c
+#define MPFS_CAN_ESR_OFFSET		0x10
+#define MPFS_CAN_COMMAND_OFFSET		0x14
+#define MPFS_CAN_CONFIG_OFFSET		0x18
+#define MPFS_CAN_TX_MSG_CTRL_CMD_BASE	0x20
+#define MPFS_CAN_TX_MSG_ID_BASE		0x24
+#define MPFS_CAN_TX_MSG_DATAL_BASE	0x28
+#define MPFS_CAN_TX_MSG_DATAH_BASE	0x2c
+#define MPFS_CAN_RX_MSG_CTRL_CMD_BASE	0x220
+#define MPFS_CAN_RX_MSG_ID_BASE		0x224
+#define MPFS_CAN_RX_MSG_DATAL_BASE	0x228
+#define MPFS_CAN_RX_MSG_DATAH_BASE	0x22c
+#define MPFS_CAN_RX_MSG_AMR_BASE	0x230
+#define MPFS_CAN_RX_MSG_ACR_BASE	0x234
+#define MPFS_CAN_RX_MSG_AMR_DATA_BASE	0x238
+#define MPFS_CAN_RX_MSG_ACR_DATA_BASE	0x23c
+
+#define MPFS_CAN_ISR_ARB_LOSS_MASK	BIT(2)
+#define MPFS_CAN_ISR_OVR_LOAD_MASK	BIT(3)
+#define MPFS_CAN_ISR_BIT_ERR_MASK	BIT(4)
+#define MPFS_CAN_ISR_STUFF_ERR_MASK	BIT(5)
+#define MPFS_CAN_ISR_ACK_ERR_MASK	BIT(6)
+#define MPFS_CAN_ISR_FORM_ERR_MASK	BIT(7)
+#define MPFS_CAN_ISR_CRC_ERR_MASK	BIT(8)
+#define MPFS_CAN_ISR_BUS_OFF_MASK	BIT(9)
+#define MPFS_CAN_ISR_TXMSG_SNT_MASK	BIT(11)
+#define MPFS_CAN_ISR_RXMSG_SNT_MASK	BIT(12)
+
+#define MPFS_CAN_ERR_MASK	(MPFS_CAN_ISR_ARB_LOSS_MASK | \
+				 MPFS_CAN_ISR_BIT_ERR_MASK | \
+				 MPFS_CAN_ISR_STUFF_ERR_MASK | \
+				 MPFS_CAN_ISR_ACK_ERR_MASK | \
+				 MPFS_CAN_ISR_FORM_ERR_MASK | \
+				 MPFS_CAN_ISR_CRC_ERR_MASK | \
+				 MPFS_CAN_ISR_OVR_LOAD_MASK | \
+				 MPFS_CAN_ISR_BUS_OFF_MASK)
+
+#define MPFS_CAN_IER_GLOBAL_MASK	BIT(0)
+#define MPFS_CAN_IER_ARB_LOSS_MASK	BIT(2)
+#define MPFS_CAN_IER_OVR_LOAD_MASK	BIT(3)
+#define MPFS_CAN_IER_BIT_ERR_MASK	BIT(4)
+#define MPFS_CAN_IER_STUFF_ERR_MASK	BIT(5)
+#define MPFS_CAN_IER_ACK_ERR_MASK	BIT(6)
+#define MPFS_CAN_IER_FORM_ERR_MASK	BIT(7)
+#define MPFS_CAN_IER_CRC_ERR_MASK	BIT(8)
+#define MPFS_CAN_IER_BUS_OFF_MASK	BIT(9)
+#define MPFS_CAN_IER_RXMSG_LOS_MASK	BIT(10)
+#define MPFS_CAN_IER_TXMSG_SNT_MASK	BIT(11)
+#define MPFS_CAN_IER_RXMSG_SNT_MASK	BIT(12)
+#define MPFS_CAN_IER_RTR_SNT_MASK	BIT(13)
+
+#define MPFS_CAN_IER_MASK	(MPFS_CAN_IER_ARB_LOSS_MASK | \
+				 MPFS_CAN_IER_BIT_ERR_MASK | \
+				 MPFS_CAN_IER_STUFF_ERR_MASK | \
+				 MPFS_CAN_IER_ACK_ERR_MASK | \
+				 MPFS_CAN_IER_FORM_ERR_MASK | \
+				 MPFS_CAN_IER_CRC_ERR_MASK | \
+				 MPFS_CAN_IER_BUS_OFF_MASK | \
+				 MPFS_CAN_IER_RXMSG_LOS_MASK | \
+				 MPFS_CAN_IER_TXMSG_SNT_MASK | \
+				 MPFS_CAN_IER_RXMSG_SNT_MASK | \
+				 MPFS_CAN_IER_RTR_SNT_MASK | \
+				 MPFS_CAN_IER_OVR_LOAD_MASK | \
+				 MPFS_CAN_IER_GLOBAL_MASK)
+
+#define MPFS_CAN_ERR_TXCNT_MASK		GENMASK(7, 0)
+#define MPFS_CAN_ERR_RXCNT_MASK		GENMASK(15, 8)
+#define MPFS_CAN_ERR_PASSIVE_MASK	BIT(16)
+#define MPFS_CAN_ERR_BUS_OFF_MASK	BIT(17)
+#define MPFS_CAN_ERR_TX96_MASK		BIT(18)
+#define MPFS_CAN_ERR_RX96_MASK		BIT(19)
+#define MPFS_CAN_ERR_RXCNT_SHIFT	8
+
+#define MPFS_CAN_CMD_NORMAL_MASK	BIT(0)
+#define MPFS_CAN_CMD_LOOP_MASK		GENMASK(2, 0)
+
+#define MPFS_CAN_CONFIG_CFG_SJW_MASK	GENMASK(3, 2)
+#define MPFS_CAN_CONFIG_TSEG2_MASK	GENMASK(7, 5)
+#define MPFS_CAN_CONFIG_TSEG1_MASK	GENMASK(11, 8)
+#define MPFS_CAN_CONFIG_BITR_MASK	GENMASK(30, 16)
+
+#define MPFS_CAN_TXMSG_CTRL_CMD_TXREQ_MASK	BIT(0)
+#define MPFS_CAN_TXMSG_CTRL_CMD_TXINTEN_MASK	BIT(2)
+#define MPFS_CAN_TXMSG_CTRL_CMD_WPNA_MASK	BIT(3)
+#define MPFS_CAN_TXMSG_CTRL_CMD_DLC_MASK	GENMASK(19, 16)
+#define MPFS_CAN_TXMSG_CTRL_CMD_IDE_MASK	BIT(20)
+#define MPFS_CAN_TXMSG_CTRL_CMD_RTR_MASK	BIT(21)
+#define MPFS_CAN_TXMSG_CTRL_CMD_WPNB_MASK	BIT(23)
+
+#define MPFS_CAN_RXMSG_CTRL_CMD_AVAIL_MASK	BIT(0)
+#define MPFS_CAN_RXMSG_CTRL_CMD_BUFEN_MASK	BIT(3)
+#define MPFS_CAN_RXMSG_CTRL_CMD_RXINTEN_MASK	BIT(5)
+#define MPFS_CAN_RXMSG_CTRL_CMD_LF_MASK		BIT(6)
+#define MPFS_CAN_RXMSG_CTRL_CMD_WPNL_MASK	BIT(7)
+#define MPFS_CAN_RXMSG_CTRL_CMD_DLC_MASK	GENMASK(19, 16)
+#define MPFS_CAN_RXMSG_CTRL_CMD_IDE_MASK	BIT(20)
+#define MPFS_CAN_RXMSG_CTRL_CMD_RTR_MASK	BIT(21)
+#define MPFS_CAN_RXMSG_CTRL_CMD_WPNH_MASK	BIT(23)
+
+#define MPFS_CAN_BTR_SJW_MASK	GENMASK(3, 2)
+#define MPFS_CAN_BTR_MASK	GENMASK(30, 16)
+#define MPFS_CAN_DLC_MASK	GENMASK(19, 16)
+#define MPFS_CAN_TS1_MASK	GENMASK(11, 8)
+#define MPFS_CAN_TS2_MASK	GENMASK(7, 5)
+#define MPFS_CAN_STDID_SHIFT	21
+#define MPFS_CAN_EXTID_SHIFT	3
+
+#define MPFS_CAN_TX_REG_LEN	16
+#define MPFS_CAN_RX_REG_LEN	32
+
+#define MPFS_CAN_TX_MSG_BASE(base, fifo)	((base) + ((fifo) * MPFS_CAN_TX_REG_LEN))
+#define MPFS_CAN_RX_MSG_BASE(base, fifo)	((base) + ((fifo) * MPFS_CAN_RX_REG_LEN))
+
+#define MPFS_CAN_TX_MSG_CTR_OFFSET(fifo)	MPFS_CAN_TX_MSG_BASE(MPFS_CAN_TX_MSG_CTRL_CMD_BASE, fifo)
+#define MPFS_CAN_TX_MSG_ID_OFFSET(fifo)		MPFS_CAN_TX_MSG_BASE(MPFS_CAN_TX_MSG_ID_BASE, fifo)
+#define MPFS_CAN_TX_DATAL_OFFSET(fifo)		MPFS_CAN_TX_MSG_BASE(MPFS_CAN_TX_MSG_DATAL_BASE, fifo)
+#define MPFS_CAN_TX_DATAH_OFFSET(fifo)		MPFS_CAN_TX_MSG_BASE(MPFS_CAN_TX_MSG_DATAH_BASE, fifo)
+
+#define MPFS_CAN_RX_MSG_CTR_OFFSET(fifo)	MPFS_CAN_RX_MSG_BASE(MPFS_CAN_RX_MSG_CTRL_CMD_BASE, fifo)
+#define MPFS_CAN_RX_MSG_ID_OFFSET(fifo)		MPFS_CAN_RX_MSG_BASE(MPFS_CAN_RX_MSG_ID_BASE, fifo)
+#define MPFS_CAN_RX_DATAL_OFFSET(fifo)		MPFS_CAN_RX_MSG_BASE(MPFS_CAN_RX_MSG_DATAL_BASE, fifo)
+#define MPFS_CAN_RX_DATAH_OFFSET(fifo)		MPFS_CAN_RX_MSG_BASE(MPFS_CAN_RX_MSG_DATAH_BASE, fifo)
+#define MPFS_CAN_AMR_OFFSET(fifo)		MPFS_CAN_RX_MSG_BASE(MPFS_CAN_RX_MSG_AMR_BASE, fifo)
+#define MPFS_CAN_ACR_OFFSET(fifo)		MPFS_CAN_RX_MSG_BASE(MPFS_CAN_RX_MSG_ACR_BASE, fifo)
+#define MPFS_CAN_AMR_DATA_OFFSET(fifo)		MPFS_CAN_RX_MSG_BASE(MPFS_CAN_RX_MSG_AMR_DATA_BASE, fifo)
+#define MPFS_CAN_ACR_DATA_OFFSET(fifo)		MPFS_CAN_RX_MSG_BASE(MPFS_CAN_RX_MSG_ACR_DATA_BASE, fifo)
+
+#define MPFS_CAN_TX_BUFFERS	32
+#define MPFS_CAN_RX_BUFFERS	32
+#define MPFS_CAN_AMR_MASK	GENMASK(31, 0)
+#define MPFS_CAN_AMR_DATA_MASK	GENMASK(31, 0)
+
+struct mpfs_can_priv {
+	struct can_priv can;
+	struct napi_struct napi;
+	struct device *dev;
+	void __iomem *base;
+	struct clk_bulk_data *clks;
+	const struct can_bittiming_const *bittiming_const;
+	spinlock_t tx_lock;
+	unsigned long irq_flags;
+	unsigned int tx_head;
+	unsigned int tx_tail;
+	unsigned int tx_max;
+	int num_clocks;
+};
+
+static const struct can_bittiming_const mpfs_can_bittiming_const = {
+	.name = "mpfs_can",
+	.tseg1_min = 2,
+	.tseg1_max = 16,
+	.tseg2_min = 1,
+	.tseg2_max = 8,
+	.sjw_max = 4,
+	.brp_min = 0,
+	.brp_max = 32767,
+	.brp_inc = 1,
+};
+
+static int mpfs_can_reset(struct net_device *ndev)
+{
+	struct mpfs_can_priv *priv = netdev_priv(ndev);
+	int ret;
+
+	/* There is no separate register in the MSS CAN to do the soft reset
+	 * hence do the full device reset and this is needed to bring back the
+	 * bus to normal state in error cases.
+	 */
+	ret = device_reset(priv->dev);
+	if (ret)
+		return ret;
+
+	priv->tx_head = 0;
+	priv->tx_tail = 0;
+
+	return 0;
+}
+
+static int mpfs_can_set_bittiming(struct net_device *ndev)
+{
+	struct mpfs_can_priv *priv = netdev_priv(ndev);
+	struct can_bittiming *bt = &priv->can.bittiming;
+	u32 config, can_started;
+
+	can_started = readl(priv->base + MPFS_CAN_COMMAND_OFFSET) & MPFS_CAN_CMD_NORMAL_MASK;
+	if (can_started) {
+		netdev_alert(ndev,
+			     "BUG! Cannot set bittiming - CAN is already running\n");
+		return -EPERM;
+	}
+
+	/* The TS1, TS2 and buad rate values written in the Config register are
+	 * added to 1 to compute the prescalar.
+	 * TS1 configuration for the mpfs can includes both propagation seg and phase seg1
+	 */
+	config = FIELD_PREP(MPFS_CAN_BTR_MASK, bt->brp - 1) |
+		 FIELD_PREP(MPFS_CAN_TS1_MASK, bt->prop_seg + bt->phase_seg1 - 1) |
+		 FIELD_PREP(MPFS_CAN_TS2_MASK, bt->phase_seg2 - 1) |
+		 FIELD_PREP(MPFS_CAN_BTR_SJW_MASK, bt->sjw - 1);
+
+	writel(config, priv->base + MPFS_CAN_CONFIG_OFFSET);
+
+	return 0;
+}
+
+static int mpfs_can_start(struct net_device *ndev)
+{
+	struct mpfs_can_priv *priv = netdev_priv(ndev);
+	int ret, buf;
+	u32 val;
+
+	//TODO this means that the device is reset twice on setting the device
+	//"up" with ip link. Is that excessive? Could we get away with only
+	//resetting in mpfs_can_open() instead of mpfs_can_start()?
+	ret = mpfs_can_reset(ndev);
+	if (ret)
+		return ret;
+
+	ret = mpfs_can_set_bittiming(ndev);
+	if (ret < 0)
+		return ret;
+
+	/* Clear pending interrupts */
+	writel(0x0, priv->base + MPFS_CAN_ISR_OFFSET);
+
+	/* Enable interrupts */
+	writel(MPFS_CAN_IER_MASK, priv->base + MPFS_CAN_IER_OFFSET);
+
+	if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
+		val = MPFS_CAN_CMD_LOOP_MASK;
+	else
+		val = MPFS_CAN_CMD_NORMAL_MASK;
+
+	writel(val, priv->base + MPFS_CAN_COMMAND_OFFSET);
+
+	for (buf = 0; buf < MPFS_CAN_RX_BUFFERS; buf++)	{
+		writel(MPFS_CAN_AMR_MASK, priv->base + MPFS_CAN_AMR_OFFSET(buf));
+		writel(0x0, priv->base + MPFS_CAN_ACR_OFFSET(buf));
+
+		writel(MPFS_CAN_AMR_DATA_MASK, priv->base + MPFS_CAN_AMR_DATA_OFFSET(buf));
+		writel(0x0, priv->base + MPFS_CAN_ACR_DATA_OFFSET(buf));
+
+		/* Enable the link flag for the buffers, so that the receive data will store
+		 * in sequential order, except for the last buffer.
+		 */
+		val = MPFS_CAN_RXMSG_CTRL_CMD_WPNL_MASK | MPFS_CAN_RXMSG_CTRL_CMD_WPNH_MASK |
+		      MPFS_CAN_RXMSG_CTRL_CMD_BUFEN_MASK | MPFS_CAN_RXMSG_CTRL_CMD_RXINTEN_MASK;
+
+		if (buf != (MPFS_CAN_RX_BUFFERS - 1))
+			val |= MPFS_CAN_RXMSG_CTRL_CMD_LF_MASK;
+
+		writel(val, priv->base + MPFS_CAN_RX_MSG_CTR_OFFSET(buf));
+	}
+
+	priv->can.state = CAN_STATE_ERROR_ACTIVE;
+
+	return 0;
+}
+
+static int mpfs_can_do_set_mode(struct net_device *ndev, enum can_mode mode)
+{
+	int ret;
+
+	switch (mode) {
+	case CAN_MODE_START:
+
+		ret = mpfs_can_start(ndev);
+		if (ret < 0) {
+			netdev_err(ndev, "mpfs_can_start() failed!\n");
+			return ret;
+		}
+
+		netif_wake_queue(ndev);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static void mpfs_can_tx(struct net_device *ndev, struct sk_buff *skb,
+			int buf_off)
+{
+	struct can_frame *frame = (struct can_frame *)skb->data;
+	struct mpfs_can_priv *priv = netdev_priv(ndev);
+	u32 id = 0, data[2] = {0, 0}, ctlr = 0;
+
+	if (frame->can_id & CAN_EFF_FLAG) {
+		ctlr = MPFS_CAN_TXMSG_CTRL_CMD_IDE_MASK;
+		id = (frame->can_id << MPFS_CAN_EXTID_SHIFT);
+	} else {
+		id = (frame->can_id << MPFS_CAN_STDID_SHIFT);
+	}
+
+	if (frame->can_id & CAN_RTR_FLAG)
+		ctlr |= MPFS_CAN_TXMSG_CTRL_CMD_RTR_MASK;
+
+	ctlr |= FIELD_PREP(MPFS_CAN_DLC_MASK, frame->len) |
+		MPFS_CAN_TXMSG_CTRL_CMD_WPNA_MASK | MPFS_CAN_TXMSG_CTRL_CMD_WPNB_MASK |
+		MPFS_CAN_TXMSG_CTRL_CMD_TXREQ_MASK | MPFS_CAN_TXMSG_CTRL_CMD_TXINTEN_MASK;
+	can_put_echo_skb(skb, ndev, priv->tx_head % priv->tx_max, 0);
+
+	priv->tx_head++;
+	writel(id, priv->base + MPFS_CAN_TX_MSG_ID_OFFSET(buf_off));
+
+	if (frame->len > 0)
+		data[0] = be32_to_cpup((__be32 *)(frame->data + 0));
+
+	if (frame->len > 4)
+		data[1] = be32_to_cpup((__be32 *)(frame->data + 4));
+
+	if (!(frame->can_id & CAN_RTR_FLAG)) {
+		writel(data[0], priv->base + MPFS_CAN_TX_DATAL_OFFSET(buf_off));
+		writel(data[1], priv->base + MPFS_CAN_TX_DATAH_OFFSET(buf_off));
+	}
+
+	writel(ctlr, priv->base + MPFS_CAN_TX_MSG_CTR_OFFSET(buf_off));
+}
+
+static int mpfs_can_get_txmsg_index(struct mpfs_can_priv *priv)
+{
+	for (int buf_index = 0; buf_index < MPFS_CAN_TX_BUFFERS; buf_index++) {
+		int available_txreq;
+
+		available_txreq = readl(priv->base + MPFS_CAN_TX_MSG_CTR_OFFSET(buf_index));
+
+		if (available_txreq & MPFS_CAN_TXMSG_CTRL_CMD_TXREQ_MASK)
+			continue;
+
+		return buf_index;
+	}
+
+	return -ENOBUFS;
+}
+
+static int mpfs_can_get_rxmsg_index(struct mpfs_can_priv *priv)
+{
+	for (int buf_index = 0; buf_index < MPFS_CAN_RX_BUFFERS; buf_index++) {
+		int available_rxreq;
+
+		available_rxreq = readl(priv->base + MPFS_CAN_RX_MSG_CTR_OFFSET(buf_index));
+
+		if (!(available_rxreq & MPFS_CAN_RXMSG_CTRL_CMD_AVAIL_MASK))
+			continue;
+
+		return buf_index;
+	}
+
+	return -ENOBUFS;
+}
+
+static netdev_tx_t mpfs_can_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct mpfs_can_priv *priv = netdev_priv(ndev);
+	int buf_off;
+	unsigned long flags;
+
+	if (can_dropped_invalid_skb(ndev, skb))
+		return NETDEV_TX_OK;
+
+	buf_off = mpfs_can_get_txmsg_index(priv);
+	if (buf_off < 0) {
+		netdev_err(ndev, "BUG!, TX full when queue awake!\n");
+		netif_stop_queue(ndev);
+		return NETDEV_TX_BUSY;
+	}
+
+	spin_lock_irqsave(&priv->tx_lock, flags);
+	mpfs_can_tx(ndev, skb, buf_off);
+
+	if ((priv->tx_head - priv->tx_tail) == priv->tx_max)
+		netif_stop_queue(ndev);
+
+	netif_stop_queue(ndev);
+
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
+
+	return NETDEV_TX_OK;
+}
+
+static int mpfs_can_rx(struct net_device *ndev, int buf_off)
+{
+	struct mpfs_can_priv *priv = netdev_priv(ndev);
+	struct net_device_stats *stats = &ndev->stats;
+	struct can_frame *frame;
+	struct sk_buff *skb;
+	u32 data[2] = {0, 0};
+	u32 id, ctrl;
+
+	skb = alloc_can_skb(ndev, &frame);
+	if (unlikely(!skb)) {
+		stats->rx_dropped++;
+		return 0;
+	}
+
+	id = readl(priv->base + MPFS_CAN_RX_MSG_ID_OFFSET(buf_off));
+	ctrl = readl(priv->base + MPFS_CAN_RX_MSG_CTR_OFFSET(buf_off));
+
+	if (ctrl & MPFS_CAN_RXMSG_CTRL_CMD_IDE_MASK) {
+		frame->can_id = CAN_EFF_FLAG;
+		frame->can_id |= (id >> MPFS_CAN_EXTID_SHIFT);
+	} else {
+		frame->can_id = (id >> MPFS_CAN_STDID_SHIFT);
+	}
+
+	if (ctrl & MPFS_CAN_RXMSG_CTRL_CMD_RTR_MASK)
+		frame->can_id |= CAN_RTR_FLAG;
+
+	frame->len = (ctrl & MPFS_CAN_DLC_MASK) >> 16;
+
+	data[0] = readl(priv->base + MPFS_CAN_RX_DATAL_OFFSET(buf_off));
+	data[1] = readl(priv->base + MPFS_CAN_RX_DATAH_OFFSET(buf_off));
+
+	if (!(frame->can_id & CAN_RTR_FLAG)) {
+		if (frame->len > 0)
+			*(__be32 *)(frame->data) = cpu_to_be32(data[0]);
+
+		if (frame->len > 4)
+			*(__be32 *)(frame->data + 4) = cpu_to_be32(data[1]);
+	}
+
+	/* Notify the controller that the rx message is read by writing the
+	 * message available bit.
+	 */
+	ctrl = readl(priv->base + MPFS_CAN_RX_MSG_CTR_OFFSET(buf_off));
+	ctrl |= MPFS_CAN_RXMSG_CTRL_CMD_AVAIL_MASK;
+	writel(ctrl, priv->base + MPFS_CAN_RX_MSG_CTR_OFFSET(buf_off));
+
+	stats->rx_bytes += frame->len;
+	stats->rx_packets++;
+	netif_receive_skb(skb);
+
+	return 1;
+}
+
+static enum can_state mpfs_can_current_error_state(struct net_device *ndev)
+{
+	struct mpfs_can_priv *priv = netdev_priv(ndev);
+	u32 status;
+
+	status = readl(priv->base + MPFS_CAN_ESR_OFFSET);
+
+	if (status & MPFS_CAN_ERR_BUS_OFF_MASK)
+		return CAN_STATE_BUS_OFF;
+	else if (status & MPFS_CAN_ERR_PASSIVE_MASK)
+		return CAN_STATE_ERROR_PASSIVE;
+	else if (status & (MPFS_CAN_ERR_TX96_MASK | MPFS_CAN_ERR_RX96_MASK))
+		return CAN_STATE_ERROR_WARNING;
+	else
+		return CAN_STATE_ERROR_ACTIVE;
+}
+
+static void mpfs_can_set_error_state(struct net_device *ndev, enum can_state new_state,
+				     struct can_frame *frame)
+{
+	struct mpfs_can_priv *priv = netdev_priv(ndev);
+	enum can_state tx_state, rx_state;
+	u32 ecr, txerr, rxerr;
+
+	ecr = readl(priv->base + MPFS_CAN_ESR_OFFSET);
+	txerr = ecr & MPFS_CAN_ERR_TXCNT_MASK;
+	rxerr = (ecr & MPFS_CAN_ERR_RXCNT_MASK) >> MPFS_CAN_ERR_RXCNT_SHIFT;
+
+	tx_state = txerr >= rxerr ? new_state : 0;
+	rx_state = txerr <= rxerr ? new_state : 0;
+
+	if (new_state > CAN_STATE_ERROR_PASSIVE) {
+		frame->can_id |= CAN_ERR_CRTL;
+		frame->data[1] = (txerr > rxerr) ?
+				  CAN_ERR_CRTL_TX_PASSIVE : CAN_ERR_CRTL_RX_PASSIVE;
+	} else {
+		can_change_state(ndev, frame, tx_state, rx_state);
+
+		if (frame) {
+			frame->data[6] = txerr;
+			frame->data[7] = rxerr;
+		}
+	}
+}
+
+static void mpfs_can_update_error_state(struct net_device *ndev)
+{
+	struct mpfs_can_priv *priv = netdev_priv(ndev);
+	enum can_state old_state = priv->can.state;
+	enum can_state new_state;
+
+	if (old_state != CAN_STATE_ERROR_WARNING &&
+	    old_state != CAN_STATE_ERROR_PASSIVE)
+		return;
+
+	new_state = mpfs_can_current_error_state(ndev);
+
+	if (new_state != old_state) {
+		struct sk_buff *skb;
+		struct can_frame *cf;
+
+		skb = alloc_can_err_skb(ndev, &cf);
+
+		mpfs_can_set_error_state(ndev, new_state, skb ? cf : NULL);
+
+		if (skb) {
+			struct net_device_stats *stats = &ndev->stats;
+
+			stats->rx_packets++;
+			stats->rx_bytes += cf->len;
+			netif_rx(skb);
+		}
+	}
+}
+
+static void mpfs_can_err_interrupt(struct net_device *ndev, u32 int_status)
+{
+	struct mpfs_can_priv *priv = netdev_priv(ndev);
+	struct net_device_stats *stats = &ndev->stats;
+	struct can_frame cf = { };
+	u32 err_status;
+
+	err_status = readl(priv->base + MPFS_CAN_ESR_OFFSET);
+
+	if (int_status & MPFS_CAN_ISR_BUS_OFF_MASK) {
+		priv->can.state = CAN_STATE_BUS_OFF;
+		priv->can.can_stats.bus_off++;
+		can_bus_off(ndev);
+		cf.can_id |= CAN_ERR_BUSOFF;
+	} else {
+		enum can_state new_state = mpfs_can_current_error_state(ndev);
+
+		if (new_state != priv->can.state)
+			mpfs_can_set_error_state(ndev, new_state, &cf);
+	}
+
+	if (int_status & MPFS_CAN_ISR_ARB_LOSS_MASK) {
+		priv->can.can_stats.arbitration_lost++;
+		cf.can_id |= CAN_ERR_LOSTARB;
+		cf.data[0] = CAN_ERR_LOSTARB_UNSPEC;
+	}
+
+	if (int_status & MPFS_CAN_ERR_MASK) {
+		priv->can.can_stats.bus_error++;
+		cf.can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+
+		if (int_status & MPFS_CAN_ISR_ACK_ERR_MASK) {
+			stats->tx_errors++;
+			cf.can_id |= CAN_ERR_ACK;
+			cf.data[3] = CAN_ERR_PROT_LOC_ACK;
+		}
+
+		if (int_status & MPFS_CAN_ISR_BIT_ERR_MASK) {
+			stats->tx_errors++;
+			cf.can_id |= CAN_ERR_PROT;
+			cf.data[2] |= CAN_ERR_PROT_BIT;
+		}
+
+		if (int_status & MPFS_CAN_ISR_STUFF_ERR_MASK) {
+			stats->rx_errors++;
+			cf.can_id |= CAN_ERR_PROT;
+			cf.data[2] |= CAN_ERR_PROT_STUFF;
+		}
+
+		if (err_status & MPFS_CAN_ISR_FORM_ERR_MASK) {
+			stats->rx_errors++;
+			cf.can_id |= CAN_ERR_PROT;
+			cf.data[2] |= CAN_ERR_PROT_FORM;
+		}
+
+		if (err_status & MPFS_CAN_ISR_CRC_ERR_MASK) {
+			stats->rx_errors++;
+			cf.can_id |= CAN_ERR_PROT;
+			cf.data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ;
+		}
+
+		if (err_status & MPFS_CAN_ISR_OVR_LOAD_MASK) {
+			stats->rx_over_errors++;
+			stats->rx_errors++;
+			cf.can_id |= CAN_ERR_PROT;
+			cf.data[3] |= CAN_ERR_PROT_OVERLOAD;
+		}
+	}
+
+	if (cf.can_id) {
+		struct can_frame *frame;
+		struct sk_buff *skb = alloc_can_err_skb(ndev, &frame);
+
+		if (skb) {
+			frame->can_id |= cf.can_id;
+			memcpy(frame->data, cf.data, CAN_ERR_DLC);
+			stats->rx_packets++;
+			stats->rx_bytes += CAN_ERR_DLC;
+			netif_rx(skb);
+		}
+	}
+
+	netdev_dbg(ndev, "error status register:0x%x\n", err_status);
+}
+
+static int mpfs_can_rx_poll(struct napi_struct *napi, int quota)
+{
+	struct net_device *ndev = napi->dev;
+	struct mpfs_can_priv *priv = netdev_priv(ndev);
+	int processed = 0, buf_off;
+	u32 int_enable;
+
+	while ((buf_off = mpfs_can_get_rxmsg_index(priv)) >= 0 && (processed < quota))
+		processed += mpfs_can_rx(ndev, buf_off);
+
+	if (processed)
+		mpfs_can_update_error_state(ndev);
+
+	if (processed < quota) {
+		if (napi_complete_done(napi, processed)) {
+			int_enable = readl(priv->base + MPFS_CAN_IER_OFFSET);
+			int_enable |= (MPFS_CAN_IER_RXMSG_SNT_MASK);
+			writel(int_enable, priv->base + MPFS_CAN_IER_OFFSET);
+		}
+	}
+
+	return processed;
+}
+
+static void mpfs_can_tx_interrupt(struct net_device *ndev)
+{
+	struct mpfs_can_priv *priv = netdev_priv(ndev);
+	struct net_device_stats *stats = &ndev->stats;
+	unsigned int frames_in_fifo;
+	unsigned long flags;
+	int bytes = 0;
+
+	spin_lock_irqsave(&priv->tx_lock, flags);
+	frames_in_fifo = priv->tx_head - priv->tx_tail;
+
+	while (frames_in_fifo--) {
+		bytes = can_get_echo_skb(ndev, priv->tx_tail % priv->tx_max, NULL);
+		stats->tx_bytes += bytes;
+		priv->tx_tail++;
+		stats->tx_packets++;
+	}
+
+	netif_wake_queue(ndev);
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
+	mpfs_can_update_error_state(ndev);
+}
+
+static irqreturn_t mpfs_can_interrupt(int irq, void *dev_id)
+{
+	struct net_device *ndev = (struct net_device *)dev_id;
+	struct mpfs_can_priv *priv = netdev_priv(ndev);
+	u32 isr, ier;
+
+	isr = readl(priv->base + MPFS_CAN_ISR_OFFSET);
+	if (!isr)
+		return IRQ_NONE;
+
+	writel(isr, priv->base + MPFS_CAN_ISR_OFFSET);
+
+	if (isr & MPFS_CAN_ISR_TXMSG_SNT_MASK)
+		mpfs_can_tx_interrupt(ndev);
+
+	if (isr & MPFS_CAN_ERR_MASK)
+		mpfs_can_err_interrupt(ndev, isr);
+
+	if (isr & MPFS_CAN_ISR_RXMSG_SNT_MASK) {
+		ier = readl(priv->base + MPFS_CAN_IER_OFFSET);
+		ier &= ~(MPFS_CAN_IER_RXMSG_SNT_MASK | MPFS_CAN_IER_RTR_SNT_MASK);
+		writel(ier, priv->base + MPFS_CAN_IER_OFFSET);
+		napi_schedule(&priv->napi);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int mpfs_can_chip_stop(struct net_device *ndev)
+{
+	struct mpfs_can_priv *priv = netdev_priv(ndev);
+	int ret;
+
+	ret = mpfs_can_reset(ndev);
+	if (ret)
+		return ret;
+
+	priv->can.state = CAN_STATE_STOPPED;
+
+	return 0;
+}
+
+static int mpfs_can_open(struct net_device *ndev)
+{
+	struct mpfs_can_priv *priv = netdev_priv(ndev);
+	int ret;
+
+	ret = request_irq(ndev->irq, mpfs_can_interrupt, priv->irq_flags,
+			  ndev->name, ndev);
+	if (ret < 0) {
+		netdev_err(ndev, "irq allocation for CAN failed\n");
+		return ret;
+	}
+
+	ret = mpfs_can_reset(ndev);
+	if (ret)
+		goto err_irq;
+
+	ret = open_candev(ndev);
+	if (ret)
+		goto err_irq;
+
+	ret = mpfs_can_start(ndev);
+	if (ret < 0) {
+		netdev_err(ndev, "mpfs_can_start() failed!\n");
+		goto err_candev;
+	}
+
+	napi_enable(&priv->napi);
+	netif_start_queue(ndev);
+
+	return 0;
+
+err_candev:
+	close_candev(ndev);
+err_irq:
+	free_irq(ndev->irq, ndev);
+
+	return ret;
+}
+
+static int mpfs_can_close(struct net_device *ndev)
+{
+	struct mpfs_can_priv *priv = netdev_priv(ndev);
+	int ret;
+
+	netif_stop_queue(ndev);
+	napi_disable(&priv->napi);
+	ret = mpfs_can_chip_stop(ndev);
+	free_irq(ndev->irq, ndev);
+	close_candev(ndev);
+
+	return ret;
+}
+
+static int mpfs_can_get_berr_counter(const struct net_device *ndev,
+				     struct can_berr_counter *bec)
+{
+	struct mpfs_can_priv *priv = netdev_priv(ndev);
+	int err_cnt;
+
+	err_cnt = readl(priv->base + MPFS_CAN_ESR_OFFSET);
+	bec->txerr = err_cnt & MPFS_CAN_ERR_TXCNT_MASK;
+	bec->rxerr = (err_cnt & MPFS_CAN_ERR_RXCNT_MASK) >> MPFS_CAN_ERR_RXCNT_SHIFT;
+
+	return 0;
+}
+
+static const struct net_device_ops mpfs_can_netdev_ops = {
+	.ndo_open = mpfs_can_open,
+	.ndo_stop = mpfs_can_close,
+	.ndo_start_xmit	= mpfs_can_start_xmit,
+};
+
+static int mpfs_can_probe(struct platform_device *pdev)
+{
+	struct net_device *ndev;
+	struct mpfs_can_priv *priv;
+	struct resource *res;
+	int ret;
+
+	ndev = alloc_candev(sizeof(struct mpfs_can_priv), MPFS_CAN_TX_BUFFERS);
+	if (!ndev)
+		return -ENOMEM;
+
+	priv = netdev_priv(ndev);
+	priv->dev = &pdev->dev;
+	priv->can.bittiming_const = &mpfs_can_bittiming_const;
+	priv->can.do_set_mode = mpfs_can_do_set_mode;
+	priv->can.do_get_berr_counter = mpfs_can_get_berr_counter;
+	priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_BERR_REPORTING;
+	priv->tx_max = MPFS_CAN_TX_BUFFERS;
+
+	priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(priv->base)) {
+		ret = PTR_ERR(priv->base);
+		goto err_free_candev;
+	}
+
+	spin_lock_init(&priv->tx_lock);
+
+	ndev->irq = platform_get_irq(pdev, 0);
+	ndev->flags |= IFF_ECHO;
+	ndev->netdev_ops = &mpfs_can_netdev_ops;
+	platform_set_drvdata(pdev, ndev);
+	SET_NETDEV_DEV(ndev, &pdev->dev);
+
+	ret = clk_bulk_get_all(&pdev->dev, &priv->clks);
+	if (ret < 2)
+		goto err_free_candev;
+
+	priv->num_clocks = ret;
+	ret = clk_bulk_prepare_enable(priv->num_clocks, priv->clks);
+	if (ret)
+		goto err_free_candev;
+
+	/* Retrieve the CAN clock rate */
+	priv->can.clock.freq = clk_get_rate(priv->clks[1].clk);
+
+	netif_napi_add_weight(ndev, &priv->napi, mpfs_can_rx_poll, MPFS_CAN_RX_BUFFERS);
+
+	ret = register_candev(ndev);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register can device\n");
+		goto err_del_napi;
+	}
+
+	netdev_dbg(ndev, "reg=0x%p irq=%d clock=%d, max tx buffers %d\n",
+		   priv->base, ndev->irq, priv->can.clock.freq,
+		   priv->tx_max);
+
+	return 0;
+
+err_del_napi:
+	netif_napi_del(&priv->napi);
+err_free_candev:
+	free_candev(ndev);
+
+	return ret;
+}
+
+static void mpfs_can_remove(struct platform_device *pdev)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct mpfs_can_priv *priv = netdev_priv(ndev);
+
+	unregister_candev(ndev);
+	netif_napi_del(&priv->napi);
+	free_candev(ndev);
+}
+
+static const struct of_device_id mpfs_can_of_match[] = {
+	{ .compatible = "microchip,mpfs-can", .data = NULL },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(of, mpfs_can_of_match);
+
+static struct platform_driver mpfs_can_driver = {
+	.probe = mpfs_can_probe,
+	.remove	= mpfs_can_remove,
+	.driver	= {
+		.name = "mpfs_can",
+		.of_match_table	= mpfs_can_of_match,
+	},
+};
+
+module_platform_driver(mpfs_can_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Microchip Polarfire SoC MSS CAN controller driver");
+MODULE_AUTHOR("Naga Sureshkumar Relli <nagasuresh.relli@microchip.com");
-- 
2.53.0

Re: [PATCH v1] net: can: mpfs_can: add Polarfire SoC MSS CAN

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 16 potential issue(s) to consider:
- [High] TX completion handler blindly echoes frames in FIFO order without checking hardware buffer status.
- [High] Infinite loop in NAPI RX poll upon SKB allocation failure (Hard CPU lockup).
- [High] Unvalidated DLC leads to invalid CAN frame length and out-of-bounds memory reads.
- [High] Clock resources and memory are leaked on probe error paths and on device removal.
- [High] request_irq called before napi_enable introduces a potential kernel crash.
- [High] Probe error handling returns success for 0 or 1 clocks, leading to Use-After-Free on removal.
- [High] `brp_min = 0` in bit timing constants is mathematically invalid and causes divide-by-zero kernel panics.
- [High] NULL pointer dereference in `mpfs_can_set_error_state()` if `alloc_can_err_skb()` fails.
- [High] `mpfs_can_set_error_state()` mishandles `CAN_STATE_BUS_OFF` and fails to update software state.
- [High] Unprotected concurrent modifications of CAN state and network statistics across softirq and hardirq contexts.
- [Medium] Unconditional `netif_stop_queue()` in `mpfs_can_start_xmit()` reduces the TX queue depth to 1.
- [Medium] Failure to clear pending interrupts in device start due to W1C misuse.
- [Medium] Permanent disablement of RTR interrupts.
- [Medium] Unchecked `platform_get_irq()` breaks deferred probing.
- [Medium] Fragile reliance on clock array index from clk_bulk_get_all.
- [Medium] Failure to flush echo SKBs during bus-off recovery causes memory leaks.
--

commit 57e6dc05d07385bee78a46cd8d38b75b30a88ebe
Author: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com>
Subject: net: can: mpfs_can: add Polarfire SoC MSS CAN

The Microchip Polarfire SoC has two instances of CAN devices inside the MSS
block. Add driver to support this MSS CAN.

> diff --git a/drivers/net/can/mpfs_can.c b/drivers/net/can/mpfs_can.c
> new file mode 100644
> index 0000000000000..3a82726492ec6
> --- /dev/null
> +++ b/drivers/net/can/mpfs_can.c
> @@ -0,0 +1,882 @@

[ ... ]

> +static const struct can_bittiming_const mpfs_can_bittiming_const = {
> +	.name = "mpfs_can",
> +	.tseg1_min = 2,
> +	.tseg1_max = 16,
> +	.tseg2_min = 1,
> +	.tseg2_max = 8,
> +	.sjw_max = 4,
> +	.brp_min = 0,

[Severity: High]
Is it intentional to set brp_min to 0? The hardware prescaler represents
division, and allowing brp_min = 0 can cause the core CAN bit timing
calculator (can_calc_bittiming) to start its calculation loop with brp = 0,
which would lead to an immediate divide-by-zero kernel exception.

> +	.brp_max = 32767,
> +	.brp_inc = 1,
> +};
> +
> +static int mpfs_can_reset(struct net_device *ndev)
> +{
> +	struct mpfs_can_priv *priv = netdev_priv(ndev);
> +	int ret;
> +
> +	/* There is no separate register in the MSS CAN to do the soft reset
> +	 * hence do the full device reset and this is needed to bring back the
> +	 * bus to normal state in error cases.
> +	 */
> +	ret = device_reset(priv->dev);
> +	if (ret)
> +		return ret;
> +
> +	priv->tx_head = 0;
> +	priv->tx_tail = 0;

[Severity: Medium]
Will this leak memory during bus-off recovery? When handling a reset via
mpfs_can_reset(), the driver zeroes priv->tx_head and priv->tx_tail, but it
fails to call can_free_echo_skb() to clear out frames currently in the echo
ring. These trapped frames will be silently overwritten and leaked when
transmissions resume.

> +
> +	return 0;
> +}

[ ... ]

> +static int mpfs_can_start(struct net_device *ndev)
> +{
> +	struct mpfs_can_priv *priv = netdev_priv(ndev);
> +	int ret, buf;
> +	u32 val;
> +
> +	//TODO this means that the device is reset twice on setting the device
> +	//"up" with ip link. Is that excessive? Could we get away with only
> +	//resetting in mpfs_can_open() instead of mpfs_can_start()?
> +	ret = mpfs_can_reset(ndev);
> +	if (ret)
> +		return ret;
> +
> +	ret = mpfs_can_set_bittiming(ndev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Clear pending interrupts */
> +	writel(0x0, priv->base + MPFS_CAN_ISR_OFFSET);

[Severity: Medium]
Does writing 0x0 actually clear the interrupts? The ISR register appears to
be a Write-1-to-Clear register based on mpfs_can_interrupt(). Writing 0x0
would be a no-op, leaving stale interrupts that will fire immediately upon
unmasking.

> +
> +	/* Enable interrupts */
> +	writel(MPFS_CAN_IER_MASK, priv->base + MPFS_CAN_IER_OFFSET);

[ ... ]

> +static netdev_tx_t mpfs_can_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	struct mpfs_can_priv *priv = netdev_priv(ndev);
> +	int buf_off;
> +	unsigned long flags;
> +
> +	if (can_dropped_invalid_skb(ndev, skb))
> +		return NETDEV_TX_OK;
> +
> +	buf_off = mpfs_can_get_txmsg_index(priv);
> +	if (buf_off < 0) {
> +		netdev_err(ndev, "BUG!, TX full when queue awake!\n");
> +		netif_stop_queue(ndev);
> +		return NETDEV_TX_BUSY;
> +	}
> +
> +	spin_lock_irqsave(&priv->tx_lock, flags);
> +	mpfs_can_tx(ndev, skb, buf_off);
> +
> +	if ((priv->tx_head - priv->tx_tail) == priv->tx_max)
> +		netif_stop_queue(ndev);
> +
> +	netif_stop_queue(ndev);

[Severity: Medium]
Why is netif_stop_queue() called unconditionally here? This will stop
the network queue after every single packet transmission, capping the active
transmission queue at 1 and significantly reducing the throughput, regardless
of the available capacity in the hardware.

> +
> +	spin_unlock_irqrestore(&priv->tx_lock, flags);
> +
> +	return NETDEV_TX_OK;
> +}
> +
> +static int mpfs_can_rx(struct net_device *ndev, int buf_off)
> +{
> +	struct mpfs_can_priv *priv = netdev_priv(ndev);
> +	struct net_device_stats *stats = &ndev->stats;
> +	struct can_frame *frame;
> +	struct sk_buff *skb;
> +	u32 data[2] = {0, 0};
> +	u32 id, ctrl;
> +
> +	skb = alloc_can_skb(ndev, &frame);
> +	if (unlikely(!skb)) {
> +		stats->rx_dropped++;
> +		return 0;
> +	}

[Severity: High]
Can this cause a system lockup? If alloc_can_skb() fails, this returns 0
without clearing the pending hardware buffer status flag
(MPFS_CAN_RXMSG_CTRL_CMD_AVAIL_MASK). Back in mpfs_can_rx_poll(), the
processed count does not increase, but mpfs_can_get_rxmsg_index() will
continue to return the exact same buffer index. This could cause the NAPI
while-loop to spin infinitely in softirq context.

> +
> +	id = readl(priv->base + MPFS_CAN_RX_MSG_ID_OFFSET(buf_off));
> +	ctrl = readl(priv->base + MPFS_CAN_RX_MSG_CTR_OFFSET(buf_off));
> +
> +	if (ctrl & MPFS_CAN_RXMSG_CTRL_CMD_IDE_MASK) {
> +		frame->can_id = CAN_EFF_FLAG;
> +		frame->can_id |= (id >> MPFS_CAN_EXTID_SHIFT);
> +	} else {
> +		frame->can_id = (id >> MPFS_CAN_STDID_SHIFT);
> +	}
> +
> +	if (ctrl & MPFS_CAN_RXMSG_CTRL_CMD_RTR_MASK)
> +		frame->can_id |= CAN_RTR_FLAG;
> +
> +	frame->len = (ctrl & MPFS_CAN_DLC_MASK) >> 16;

[Severity: High]
Does this field need to be validated via can_cc_dlc2len()? By directly
assigning the hardware DLC field (which can be up to 15) without sanitizing
it, frame->len can exceed the 8-byte limit for Classical CAN. This violates
the can_frame UAPI constraint and could cause SocketCAN applications iterating
over frame->len to perform out-of-bounds reads on the 8-byte data payload array.

> +
> +	data[0] = readl(priv->base + MPFS_CAN_RX_DATAL_OFFSET(buf_off));
> +	data[1] = readl(priv->base + MPFS_CAN_RX_DATAH_OFFSET(buf_off));

[ ... ]

> +static void mpfs_can_set_error_state(struct net_device *ndev, enum can_state new_state,
> +				     struct can_frame *frame)
> +{
> +	struct mpfs_can_priv *priv = netdev_priv(ndev);
> +	enum can_state tx_state, rx_state;
> +	u32 ecr, txerr, rxerr;
> +
> +	ecr = readl(priv->base + MPFS_CAN_ESR_OFFSET);
> +	txerr = ecr & MPFS_CAN_ERR_TXCNT_MASK;
> +	rxerr = (ecr & MPFS_CAN_ERR_RXCNT_MASK) >> MPFS_CAN_ERR_RXCNT_SHIFT;
> +
> +	tx_state = txerr >= rxerr ? new_state : 0;
> +	rx_state = txerr <= rxerr ? new_state : 0;
> +
> +	if (new_state > CAN_STATE_ERROR_PASSIVE) {
> +		frame->can_id |= CAN_ERR_CRTL;

[Severity: High]
Could this result in a NULL pointer dereference? In
mpfs_can_update_error_state(), if alloc_can_err_skb() fails, it passes NULL
as the frame parameter to mpfs_can_set_error_state(). If the controller
enters the bus-off state (new_state > CAN_STATE_ERROR_PASSIVE), the function
unconditionally dereferences the frame pointer here.

Additionally, this if-branch entirely bypasses the else block containing
can_change_state(). As a result, the internal software state (priv->can.state)
is never updated to CAN_STATE_BUS_OFF and the can_bus_off() recovery helper
is never invoked, leaving the device locked up.

> +		frame->data[1] = (txerr > rxerr) ?
> +				  CAN_ERR_CRTL_TX_PASSIVE : CAN_ERR_CRTL_RX_PASSIVE;
> +	} else {
> +		can_change_state(ndev, frame, tx_state, rx_state);
> +
> +		if (frame) {
> +			frame->data[6] = txerr;
> +			frame->data[7] = rxerr;
> +		}
> +	}
> +}

[ ... ]

> +static void mpfs_can_err_interrupt(struct net_device *ndev, u32 int_status)
> +{
> +	struct mpfs_can_priv *priv = netdev_priv(ndev);
> +	struct net_device_stats *stats = &ndev->stats;
> +	struct can_frame cf = { };
> +	u32 err_status;
> +
> +	err_status = readl(priv->base + MPFS_CAN_ESR_OFFSET);

[ ... ]

> +		if (err_status & MPFS_CAN_ISR_OVR_LOAD_MASK) {
> +			stats->rx_over_errors++;
> +			stats->rx_errors++;
> +			cf.can_id |= CAN_ERR_PROT;
> +			cf.data[3] |= CAN_ERR_PROT_OVERLOAD;
> +		}
> +	}
> +
> +	if (cf.can_id) {
> +		struct can_frame *frame;
> +		struct sk_buff *skb = alloc_can_err_skb(ndev, &frame);
> +
> +		if (skb) {
> +			frame->can_id |= cf.can_id;
> +			memcpy(frame->data, cf.data, CAN_ERR_DLC);
> +			stats->rx_packets++;
> +			stats->rx_bytes += CAN_ERR_DLC;

[Severity: High]
Are there unprotected concurrent modifications of network statistics here?
The NAPI poll routine mpfs_can_rx() runs in softirq context and updates
stats->rx_packets and stats->rx_bytes. Concurrently, mpfs_can_err_interrupt()
runs in hardirq context and updates the exact same variables on the same
structure without any locking. An interrupt firing during the softirq
read-modify-write operation can corrupt the network statistics.

> +			netif_rx(skb);
> +		}
> +	}
> +
> +	netdev_dbg(ndev, "error status register:0x%x\n", err_status);
> +}
> +
> +static int mpfs_can_rx_poll(struct napi_struct *napi, int quota)
> +{
> +	struct net_device *ndev = napi->dev;
> +	struct mpfs_can_priv *priv = netdev_priv(ndev);
> +	int processed = 0, buf_off;
> +	u32 int_enable;
> +
> +	while ((buf_off = mpfs_can_get_rxmsg_index(priv)) >= 0 && (processed < quota))
> +		processed += mpfs_can_rx(ndev, buf_off);
> +
> +	if (processed)
> +		mpfs_can_update_error_state(ndev);
> +
> +	if (processed < quota) {
> +		if (napi_complete_done(napi, processed)) {
> +			int_enable = readl(priv->base + MPFS_CAN_IER_OFFSET);
> +			int_enable |= (MPFS_CAN_IER_RXMSG_SNT_MASK);

[Severity: Medium]
When an RX or RTR interrupt fires, mpfs_can_interrupt() masks both
MPFS_CAN_IER_RXMSG_SNT_MASK and MPFS_CAN_IER_RTR_SNT_MASK in the IER register.
However, when NAPI polling completes here, only MPFS_CAN_IER_RXMSG_SNT_MASK
is unmasked. Will this cause Remote Transmission Request (RTR) interrupts to
be permanently disabled after the first receive event?

> +			writel(int_enable, priv->base + MPFS_CAN_IER_OFFSET);
> +		}
> +	}
> +
> +	return processed;
> +}
> +
> +static void mpfs_can_tx_interrupt(struct net_device *ndev)
> +{
> +	struct mpfs_can_priv *priv = netdev_priv(ndev);
> +	struct net_device_stats *stats = &ndev->stats;
> +	unsigned int frames_in_fifo;
> +	unsigned long flags;
> +	int bytes = 0;
> +
> +	spin_lock_irqsave(&priv->tx_lock, flags);
> +	frames_in_fifo = priv->tx_head - priv->tx_tail;
> +
> +	while (frames_in_fifo--) {
> +		bytes = can_get_echo_skb(ndev, priv->tx_tail % priv->tx_max, NULL);

[Severity: High]
Does this blindly echo frames in FIFO order without checking hardware buffer
status? CAN arbitration means multiple queued frames can be transmitted out
of order based on priority. The TX interrupt handler loops over
tx_head - tx_tail and echoes all pending frames unconditionally. If two frames
are queued and only the higher-priority one transmits, a single TX interrupt
will fire, and the driver will erroneously echo both frames and free them
before the second one is actually sent to the bus.

> +		stats->tx_bytes += bytes;
> +		priv->tx_tail++;
> +		stats->tx_packets++;
> +	}
> +
> +	netif_wake_queue(ndev);
> +	spin_unlock_irqrestore(&priv->tx_lock, flags);
> +	mpfs_can_update_error_state(ndev);
> +}

[ ... ]

> +static int mpfs_can_open(struct net_device *ndev)
> +{
> +	struct mpfs_can_priv *priv = netdev_priv(ndev);
> +	int ret;
> +
> +	ret = request_irq(ndev->irq, mpfs_can_interrupt, priv->irq_flags,
> +			  ndev->name, ndev);

[Severity: High]
Could this cause a kernel crash by requesting the IRQ before enabling NAPI?
request_irq() is called here before napi_enable(). Furthermore,
mpfs_can_start(), which unmasks hardware interrupts, is also called before
napi_enable(). If a pending interrupt fires immediately,
mpfs_can_interrupt() will invoke napi_schedule() on a NAPI instance that has
not yet been initialized or enabled, leading to a kernel panic.

> +	if (ret < 0) {
> +		netdev_err(ndev, "irq allocation for CAN failed\n");
> +		return ret;
> +	}
> +
> +	ret = mpfs_can_reset(ndev);
> +	if (ret)
> +		goto err_irq;
> +
> +	ret = open_candev(ndev);
> +	if (ret)
> +		goto err_irq;
> +
> +	ret = mpfs_can_start(ndev);
> +	if (ret < 0) {
> +		netdev_err(ndev, "mpfs_can_start() failed!\n");
> +		goto err_candev;
> +	}
> +
> +	napi_enable(&priv->napi);
> +	netif_start_queue(ndev);
> +
> +	return 0;

[ ... ]

> +static int mpfs_can_probe(struct platform_device *pdev)
> +{
> +	struct net_device *ndev;
> +	struct mpfs_can_priv *priv;
> +	struct resource *res;
> +	int ret;
> +
> +	ndev = alloc_candev(sizeof(struct mpfs_can_priv), MPFS_CAN_TX_BUFFERS);
> +	if (!ndev)
> +		return -ENOMEM;
> +
> +	priv = netdev_priv(ndev);
> +	priv->dev = &pdev->dev;
> +	priv->can.bittiming_const = &mpfs_can_bittiming_const;
> +	priv->can.do_set_mode = mpfs_can_do_set_mode;
> +	priv->can.do_get_berr_counter = mpfs_can_get_berr_counter;
> +	priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_BERR_REPORTING;
> +	priv->tx_max = MPFS_CAN_TX_BUFFERS;
> +
> +	priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(priv->base)) {
> +		ret = PTR_ERR(priv->base);
> +		goto err_free_candev;
> +	}
> +
> +	spin_lock_init(&priv->tx_lock);
> +
> +	ndev->irq = platform_get_irq(pdev, 0);
> +	ndev->flags |= IFF_ECHO;

[Severity: Medium]
Is it safe to proceed without checking the return value of platform_get_irq()?
The driver assigns the result directly to ndev->irq without verifying if the
result is a negative error code (e.g., -EPROBE_DEFER). The probe would
complete successfully, but a subsequent request_irq() during device open
will fail with the negative irq number.

> +	ndev->netdev_ops = &mpfs_can_netdev_ops;
> +	platform_set_drvdata(pdev, ndev);
> +	SET_NETDEV_DEV(ndev, &pdev->dev);
> +
> +	ret = clk_bulk_get_all(&pdev->dev, &priv->clks);
> +	if (ret < 2)
> +		goto err_free_candev;

[Severity: High]
Does this return success on error? clk_bulk_get_all() returns the number of
clocks found (>= 0). If ret < 2 (0 or 1), the driver jumps to
err_free_candev which frees the ndev structure but returns ret (which is 0
or 1, not a negative error code). The driver core will interpret this as a
successful probe. When the device is later unbound, mpfs_can_remove() will
dereference the already freed ndev, causing a Use-After-Free.

> +
> +	priv->num_clocks = ret;
> +	ret = clk_bulk_prepare_enable(priv->num_clocks, priv->clks);
> +	if (ret)
> +		goto err_free_candev;
> +
> +	/* Retrieve the CAN clock rate */
> +	priv->can.clock.freq = clk_get_rate(priv->clks[1].clk);

[Severity: Medium]
Is it safe to rely on a hardcoded array index to fetch the CAN core
frequency clock? clk_bulk_get_all() returns clocks based on arbitrary Device
Tree property order. Relying on priv->clks[1].clk makes the driver extremely
fragile against DT variations and will silently miscalculate baud rates if
the order changes.

[Severity: High]
The driver acquires and enables clocks using clk_bulk_get_all() and
clk_bulk_prepare_enable() but lacks corresponding
clk_bulk_disable_unprepare() and clk_bulk_put_all() calls on the probe
failure paths (e.g., if register_candev() fails). Will this leak both memory
and active clock references indefinitely?

> +
> +	netif_napi_add_weight(ndev, &priv->napi, mpfs_can_rx_poll, MPFS_CAN_RX_BUFFERS);
> +
> +	ret = register_candev(ndev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register can device\n");
> +		goto err_del_napi;
> +	}
> +
> +	netdev_dbg(ndev, "reg=0x%p irq=%d clock=%d, max tx buffers %d\n",
> +		   priv->base, ndev->irq, priv->can.clock.freq,
> +		   priv->tx_max);
> +
> +	return 0;
> +
> +err_del_napi:
> +	netif_napi_del(&priv->napi);
> +err_free_candev:
> +	free_candev(ndev);
> +
> +	return ret;
> +}
> +
> +static void mpfs_can_remove(struct platform_device *pdev)
> +{
> +	struct net_device *ndev = platform_get_drvdata(pdev);
> +	struct mpfs_can_priv *priv = netdev_priv(ndev);
> +
> +	unregister_candev(ndev);
> +	netif_napi_del(&priv->napi);
> +	free_candev(ndev);

[Severity: High]
This device removal function seems to entirely lack clock cleanup. Should
there be a clk_bulk_disable_unprepare() and clk_bulk_put_all() here to avoid
leaking memory and leaving clocks enabled indefinitely upon module unload?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-lens-constrict-6dc7d287c647@spud?part=1

Re: [PATCH v1] net: can: mpfs_can: add Polarfire SoC MSS CAN

[Conor Dooley]

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

On Thu, Jul 02, 2026 at 03:34:58PM +0100, Conor Dooley wrote:
> From: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com>
> 
> The Microchip Polarfire SoC has two instances of CAN devices inside the
> MSS block. Add driver to support this MSS CAN.
> 
> Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> 
> Hey Marc,
> I've finally got around to sending this driver.
> 
> Unfortunately the author left microchip (which is part of why sending it
> took so long). I've done some mechanical cleanup, but left it mostly
> functionally unchanged mainly because I lack a setup to test it
> thoroughly were I to do so.
> I'm hoping that what's here is by and large okay on that front, and
> maybe that sasiko will point some stuff out if there's stuff I missed.
> I did remove/modify some code, so hopefully I caught all the obviously
> problematic stuff.

Apparently there's a lot that I did not catch, given there's a wad of
complaints from sashiko. Certainly feel free to ignore this version
until I resolve the dozen or so things that it has raised.

Thanks,
Conor.

> 
> The IP that we are using seems to lack an internal reset, so the reset
> for the whole peripheral is used instead, and I wonder if it is
> overeager? Doing ip link set xxx up causes a double reset, for example.
> 
> There's no binding of course, because that's been upstream for quite a
> while!

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