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

[sashiko-bot@kernel.org]

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