Re: [PATCH v4 1/2] can: Add Loongson CAN-FD controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [High] Lockless queue stop in `start_xmit` races with queue wake in the TX interrupt, risking a permanent deadlock.
- [High] Failing to allocate an SKB in the RX path causes early return without draining the hardware FIFO, permanently desynchronizing the RX stream and hanging NAPI.
- [High] Clearing the TX Hardware Command Interrupt flag at the end of `loongson_canfd_tx_interrupt()` creates a race condition leading to lost TX interrupts.
- [High] The driver immediately clears hardware error counters upon entering BUS_OFF, bypassing the mandatory software restart delay.
- [High] Overwriting the `isr` variable in `loongson_canfd_err_interrupt()` drops concurrent error interrupts and leaves them permanently masked.
- [High] Using the logical `cfd->len` to pop payload data from the RX FIFO permanently corrupts the hardware FIFO state for CAN 2.0 RTR frames.
- [High] Incorrect constraints and silent truncation for CAN FD data phase timing lead to corrupted bit-timing configurations.
- [High] Missing volatile register definition for `LOONGSON_CANFD_TRV_DLY` causes stale cached TDCV readings.
- [Medium] The queue space check incorrectly evaluates if ALL transmit buffers are free instead of AT LEAST ONE, severely throttling transmission.
- [Medium] The driver passes a hardcoded 0 frame length to `can_put_echo_skb()`, preventing the `tx_bytes` statistic from ever incrementing.
- [Medium] Synchronous polling of hardware status in TX interrupt causes runaway double counting of the `tx_packets` statistic.
--

--- Patch [1]: [PATCH v4 1/2] can: Add Loongson CAN-FD controller driver ---
commit ecb7c7be40459261656c18cd36d4c01fa4e29b1d
Author: Binbin Zhou <zhoubinbin@loongson.cn>
Subject: can: Add Loongson CAN-FD controller driver

This commit adds a new driver for the Loongson CAN-FD controller integrated
into Loongson-2K series SoCs. It implements the core CAN device operations,
including a NAPI-based receive path, TX echo support, error reporting, and
various CAN controller modes.

> diff --git a/drivers/net/can/loongson_canfd.c b/drivers/net/can/loongson_canfd.c
> new file mode 100644
> index 0000000000000..83aa620c9d587
> --- /dev/null
> +++ b/drivers/net/can/loongson_canfd.c
> @@ -0,0 +1,1688 @@
[ ... ]
> +static const struct can_bittiming_const loongson_canfd_bit_timing_data = {
> +	.name		= DEV_NAME,
> +	.tseg1_min	= 2,
> +	.tseg1_max	= 190,
> +	.tseg2_min	= 2,
> +	.tseg2_max	= 63,

[Severity: High]
Are these data phase bounds correct for the FD hardware? The struct
loongson_canfd_bit_timing_data advertises tseg1_max as 190 and tseg2_max
as 63. However, looking at loongson_canfd_set_btr(), the phase segments are
passed to FIELD_PREP(REG_BTR_FD_PH1...) which is bounded by a 5-bit mask
(GENMASK(11, 7)). Will values greater than 31 be silently truncated here?

> +	.sjw_max	= 31,
> +	.brp_min	= 1,
> +	.brp_max	= 255,
> +	.brp_inc	= 1,
> +};
[ ... ]
> +static bool loongson_canfd_txbuf_freed(struct loongson_canfd_priv *priv)
> +{
> +	return !regmap_test_bits(priv->regmap, LOONGSON_CANFD_TX_STAT, REG_TX_STAT_BRP);
> +}

[Severity: Medium]
Does this check properly evaluate if a transmit buffer is free? The use
of !regmap_test_bits() requires all 8 Buffer Request Pending bits to be 0
for the function to return true. In loongson_canfd_start_xmit(), this leads
to the queue being stopped immediately after a single frame is queued,
effectively reducing the 8-buffer queue down to a depth of 1.

[ ... ]
> +static netdev_tx_t loongson_canfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
[ ... ]
> +	if (unlikely(!loongson_canfd_txbuf_freed(priv))) {
> +		netif_stop_queue(ndev);
> +		netdev_err(ndev, "BUG!, no TXB free when queue awake!\n");
> +		return NETDEV_TX_BUSY;
> +	}

[Severity: High]
Is this check safe from a race condition with the TX interrupt? The queue
status is read locklessly outside of tx_lock. If this detects a full queue,
but gets preempted before netif_stop_queue() is called, a concurrent TX
interrupt could free a buffer and call netif_wake_queue().

When this thread resumes, it would stop the queue despite free space being
available, potentially resulting in a permanent stall.

> +
> +	guard(spinlock_irqsave)(&priv->tx_lock);
[ ... ]
> +	can_put_echo_skb(skb, ndev, buf_id, 0);

[Severity: Medium]
Is it intentional to pass 0 as the frame length to can_put_echo_skb()?
When the TX interrupt later fires, can_get_echo_skb() will return this
length of 0, meaning stats->tx_bytes will never increment.

> +
> +	loongson_canfd_set_txbuf_cmd(ndev, TXT_CMD_ADD, buf_id);
> +
> +	/* Check if all TX buffers are full */
> +	if (!loongson_canfd_txbuf_freed(priv))
> +		netif_stop_queue(ndev);
> +
> +	return NETDEV_TX_OK;
> +}
[ ... ]
> +static int loongson_canfd_rx(struct net_device *ndev)
> +{
[ ... ]
> +	/* Flexible Data-rate Format */
> +	if (is_can_fd) {
> +		skb = alloc_canfd_skb(ndev, &cfd);
> +	} else {
> +		skb = alloc_can_skb(ndev, &ccf);
> +		cfd = (struct canfd_frame *)ccf;
> +	}
> +
> +	if (unlikely(!skb))
> +		return 0;

[Severity: High]
Can returning 0 here when skb allocation fails corrupt the hardware FIFO
state? This early return skips popping the remaining payload words from
the FIFO. The next read operation might then mistakenly interpret payload
data as frame headers.

Additionally, returning 0 breaks the NAPI polling loop in
loongson_canfd_rx_napi(), which will then exit without calling
napi_complete_done() when ret == 0, potentially hanging the receive path.

> +
> +	/* Extended Identifier Type */
[ ... ]
> +	/* Data Length Code */
> +	dlc = FIELD_GET(REG_FRAME_META1_DLC, meta1);
> +	if (is_can_fd)
> +		cfd->len = can_fd_dlc2len(dlc);
> +	else
> +		can_frame_set_cc_len(ccf, dlc, priv->can.ctrlmode);
> +
> +	/* Data */
> +	for (unsigned int i = 0; i < cfd->len; i += LOONGSON_CANFD_DW_BYTE)
> +		regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, (u32 *)(cfd->data + i));

[Severity: High]
Does iterating based on cfd->len safely handle CAN 2.0 RTR frames? For
Remote Transmission Request frames, cfd->len specifies the requested
length (up to 8) even though there is no physical payload on the wire.
Reading from the FIFO based on this length could consume data belonging
to the next frame, breaking stream synchronization. Would it be safer to
read exactly REG_FRAME_META1_RWCNT words?

> +
> +	if (!(cfd->can_id & CAN_RTR_FLAG))
> +		stats->rx_bytes += cfd->len;
> +	stats->rx_packets++;
> +	netif_receive_skb(skb);
> +
> +	return 1;
> +}
[ ... ]
> +static void loongson_canfd_err_interrupt(struct net_device *ndev, u32 isr)
> +{
[ ... ]
> +	/*
> +	 * EWLI: error warning limit condition met
> +	 * FCSI: fault confinement state changed
> +	 * ALI:  arbitration lost (just informative)
> +	 * BEI:  bus error interrupt
> +	 */
> +	if ((isr & REG_INT_STAT_FCSI) || (isr & REG_INT_STAT_EWLI)) {
> +		netdev_info(ndev, "state changes from %s to %s\n",
> +			    can_get_state_str(priv->can.state), can_get_state_str(state));
> +
> +		if (priv->can.state == state)
> +			netdev_warn(ndev, "cur and pre state is the same!(miss intr?)\n");
> +
> +		isr = REG_INT_STAT_FCSI | REG_INT_STAT_EWLI;

[Severity: High]
Does overwriting the isr variable here drop concurrent error interrupts? If
multiple errors occur simultaneously, this assignment destroys the other
flags (like REG_INT_STAT_ALI).

Because this modified isr variable is later used to write back to
LOONGSON_CANFD_INT_MASK at the end of the function, the dropped interrupts
could remain permanently masked in the hardware.

> +		priv->can.state = state;
> +		switch (state) {
> +		case CAN_STATE_BUS_OFF:
> +			priv->can.can_stats.bus_off++;
> +			if (priv->can.restart_ms)
> +				regmap_write(priv->regmap, LOONGSON_CANFD_CMD,
> +					     REG_CMD_ERCRST);

[Severity: High]
Does clearing the error counters here bypass the software recovery
protocol? The REG_CMD_ERCRST command instantly zeroes the hardware TEC/REC
counters, which could cause the controller to immediately rejoin the bus,
bypassing the mandatory CAN subsystem recovery delay specified by
priv->can.restart_ms.

> +
> +			can_bus_off(ndev);
[ ... ]
> +static void loongson_canfd_tx_interrupt(struct net_device *ndev)
> +{
[ ... ]
> +	while ((buf_id = loongson_canfd_get_txbuf_id(priv, &sts)) < LOONGSON_CANFD_TXBUF_NUM) {
> +		switch (sts) {
> +		case TX_BS_VALID:
> +			stats->tx_bytes += can_get_echo_skb(ndev, buf_id, NULL);
> +			stats->tx_packets++;
> +			break;
> +		case TX_BS_FAIL:
> +			u32 cnt;
[ ... ]
> +			stats->tx_dropped++;
> +			break;
> +		default:
> +			break;
> +		}
> +
> +		loongson_canfd_set_txbuf_cmd(ndev, TXT_CMD_SR_CLEAN, buf_id);
> +	}

[Severity: Medium]
Can this loop sequence artificially inflate the tx_packets statistic? The
code issues a TXT_CMD_SR_CLEAN command and immediately continues the loop
without verifying the status cleared.

Since hardware register updates take time, the very next regmap_read() in
loongson_canfd_get_txbuf_id() could yield the same TX_BS_VALID status. The
buffer would then be processed again, safely returning 0 from
can_get_echo_skb(), but unconditionally incrementing stats->tx_packets
over and over.

> +
> +	/*
> +	 * Clear the interrupt again. We do not want to receive again interrupt
> +	 * for the buffer already handled. If it is the last finished one then
> +	 * it would cause log of spurious interrupt.
> +	 */
> +	regmap_write(priv->regmap, LOONGSON_CANFD_INT_STAT, REG_INT_STAT_TXBHCI);

[Severity: High]
Is it safe to clear the hardware interrupt flag after processing the buffers
instead of before? If a new buffer finishes transmission between the end of
the loop and this regmap_write, the newly set interrupt flag would be
cleared without getting processed, potentially causing a lost interrupt.

> +
> +	/* Check if at least one TX buffer is free */
> +	if (loongson_canfd_txbuf_freed(priv))
> +		netif_wake_queue(ndev);
> +}
[ ... ]
> +static bool loongson_canfd_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case LOONGSON_CANFD_MODE:
> +	case LOONGSON_CANFD_CONF:

[Severity: High]
Should LOONGSON_CANFD_TRV_DLY be included in this volatile register list?
Since regmap uses REGCACHE_MAPLE, omitting this register causes the first
read in loongson_canfd_get_auto_tdcv() to be cached indefinitely. This
would continuously feed stale TDCV readings into the auto-TDC logic.

> +	case LOONGSON_CANFD_STAT:
> +	case LOONGSON_CANFD_INT_STAT:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783391759.git.zhoubinbin@loongson.cn?part=1