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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Returning -EAGAIN on format error desynchronizes the RX FIFO and causes an infinite interrupt storm.
- [High] Out-of-bounds write in the RX path when receiving Classical CAN frames with anomalous payload lengths.
- [High] TOCTOU races and incorrect clearing of Write-1-to-Clear (W1C) interrupt status bits cause permanent loss of interrupts.
- [Medium] Failure to use the correct DLC mapping for Classic CAN frames with DLC > 8.
- [Medium] Queue handling race in ndo_start_xmit causes false NETDEV_TX_BUSY returns and severe console spam.
- [Medium] Bit timing logic fails to redistribute oversized prop_seg, silently misconfiguring hardware timings.
- [Medium] Incorrect initial operational state set in loongson_canfd_chip_start.
- [Low] Unreachable dead code loop intended to drain the RX FIFO.
--

commit e517c4643a4070a6b8fcde3c6d3ac1988016c84b
Author: Binbin Zhou <zhoubinbin@loongson.cn>

can: Add Loongson CAN-FD controller driver

This commit adds a new driver for the Loongson CAN-FD controller.
It supports both CAN 2.0 and CAN-FD protocols, implements NAPI-based
receive paths, TX echo support, error reporting, and various common CAN
controller modes.

> diff --git a/drivers/net/can/loongson_canfd.c b/drivers/net/can/loongson_canfd.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/net/can/loongson_canfd.c
> @@ -0,0 +1,1623 @@
[ ... ]
> +static void loongson_canfd_set_btr(struct net_device *ndev, struct can_bittiming *bt, bool nominal)
> +{
> +	struct loongson_canfd_priv *priv = netdev_priv(ndev);
> +	u32 ph1_max = FIELD_MAX(REG_BTR_PH1);
> +	u32 prop = bt->prop_seg, phase1 = bt->phase_seg1;
> +	u32 btr = 0;
> +
> +	/* Hardware PROP is 7 bits, PH1 is 6 bits; redistribute if PH1 exceeds max */
> +	if (phase1 > ph1_max) {
> +		prop += phase1 - ph1_max;
> +		phase1 = ph1_max;
> +	}
> +
> +	if (nominal) {
> +		btr = FIELD_PREP(REG_BTR_PROP, prop) |

[Severity: Medium]
Does this logic fail to redistribute the values if prop exceeds its 7-bit
maximum?

If the user specifies prop_seg = 180 and phase_seg1 = 10, prop will exceed
the maximum and its upper bits will be silently bitwise-truncated by the
FIELD_PREP(REG_BTR_PROP, prop) macro, resulting in invalid hardware timings.
Should it also shift the excess from prop to phase1?

[ ... ]
> +static int loongson_canfd_chip_start(struct net_device *ndev)
> +{
[ ... ]
> +	int_msk = FIELD_GET(REG_INT_MASK_SET, ~int_ena);
> +
> +	regmap_write(priv->regmap, LOONGSON_CANFD_INT_MASK, int_msk);
> +	regmap_write(priv->regmap, LOONGSON_CANFD_INT_ENA, int_ena);
> +
> +	priv->can.state = CAN_STATE_STOPPED;

[Severity: Medium]
Should the initial CAN state be set to CAN_STATE_ERROR_ACTIVE here instead of
CAN_STATE_STOPPED?

The CAN subsystem conventions typically require transitioning the state to
CAN_STATE_ERROR_ACTIVE when the hardware is enabled and traffic can flow.

> +
> +	regmap_update_bits(priv->regmap, LOONGSON_CANFD_CONF, REG_CONF_ENA, REG_CONF_ENA);
> +
> +	return 0;
> +}

[ ... ]
> +static bool loongson_canfd_insert_frame(struct loongson_canfd_priv *priv,
> +					const struct canfd_frame *cf, u8 buf, bool isfdf)
> +{
[ ... ]
> +	if (isfdf) {
> +		frame_hdr1 = REG_FRAME_META1_FDF;
> +		if (cf->flags & CANFD_BRS)
> +			frame_hdr1 |= REG_FRAME_META1_BRS;
> +	}
> +
> +	frame_hdr1 |= FIELD_PREP(REG_FRAME_META1_DLC, can_fd_len2dlc(cf->len));

[Severity: Medium]
For Classic CAN frames requesting a DLC greater than 8, will
can_fd_len2dlc(cf->len) result in an incorrect clamped DLC being transmitted?

The driver advertises CAN_CTRLMODE_CC_LEN8_DLC support, meaning a Classic CAN
frame may have cf->len = 8 but a user-requested cf->len8_dlc > 8.
Should can_get_cc_dlc() be used here for Classic CAN frames?

[ ... ]
> +static netdev_tx_t loongson_canfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
[ ... ]
> +	for_each_clear_bit(i, &tx_brp, LOONGSON_CANFD_TXBUF_NUM) {
> +		u32 transmission_complete_mask = 0x3 << (i * 2);
> +
> +		if (!(tx_bs & transmission_complete_mask)) {
> +			buf_id = i;
> +			break;
> +		}
> +	}
> +
> +	if (buf_id == LOONGSON_CANFD_TXBUF_NUM) {
> +		netif_stop_queue(ndev);
> +		netdev_err(ndev, "No writable TX buffer, stopping queue\n");
> +		return NETDEV_TX_BUSY;
> +	}

[Severity: Medium]
Could this condition trigger erroneously during a normal transient race window?

A buffer whose pending bit REG_TX_STAT_BRP is 0 might have its software
status BS set to TX_BS_VALID if the hardware completed transmission but the
ISR hasn't cleaned it yet. In that case, no buf_id will be found, resulting
in console spam and a false queue stoppage.

[ ... ]
> +static int loongson_canfd_rx(struct net_device *ndev)
> +{
[ ... ]
> +	regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, &frame_hdr0);
> +	regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, &frame_hdr1);
> +
> +	rwcnt = FIELD_GET(REG_FRAME_META1_RWCNT, frame_hdr1);
> +	if (rwcnt < 2)
> +		return -EAGAIN;
> +
> +	dbcnt = (rwcnt - 2) * LOONGSON_CANFD_DW_BYTE;
> +	if (dbcnt > CANFD_MAX_DLEN)
> +		return -EAGAIN;

[Severity: High]
By returning -EAGAIN early here without draining the remaining unread payload
words from the hardware FIFO, will this permanently desynchronize the RX path?

It appears this would abort the NAPI poll while leaving data in the buffer.
When NAPI completes, the RX Not Empty interrupt is unmasked and immediately
fires again, causing an infinite interrupt storm.

> +
> +	is_can_fd = !!(frame_hdr1 & REG_FRAME_META1_FDF);
> +	if (is_can_fd) {
> +		skb = alloc_canfd_skb(ndev, &cfd);
> +	} else {
> +		skb = alloc_can_skb(ndev, &ccf);
> +		cfd = (struct canfd_frame *)ccf;
> +	}
[ ... ]
> +	for (i = 0; i < dbcnt; i += LOONGSON_CANFD_DW_BYTE) {
> +		regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, &data);
> +		put_unaligned_le32(data, cfd->data + i);
> +	}

[Severity: High]
Does this loop allow out-of-bounds writes if a Classical CAN frame has an
anomalous payload length?

If the hardware returns a frame where FDF=0 but with rwcnt up
to 18 (dbcnt = 64 bytes), the length check dbcnt > CANFD_MAX_DLEN allows it.
The alloc_can_skb() function only provides an 8-byte data array for Classic
CAN frames, so this loop would write up to 64 bytes, overflowing the buffer
and corrupting the skb_shared_info structure.

> +
> +	while (unlikely(i < dbcnt)) {
> +		regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, &data);
> +		i += LOONGSON_CANFD_DW_BYTE;
> +	}

[Severity: Low]
Is this loop unreachable dead code?

Since dbcnt is mathematically guaranteed to be a multiple of 4, the prior
for loop exactly terminates when i == dbcnt, making the i < dbcnt condition
strictly false here.

[ ... ]
> +static void loongson_canfd_err_interrupt(struct net_device *ndev, u32 isr)
> +{
[ ... ]
> +	if (skb)
> +		netif_rx(skb);
> +
> +	regmap_write(priv->regmap, LOONGSON_CANFD_INT_STAT, mask);

[Severity: High]
Does double-clearing the Write-1-to-Clear (W1C) error interrupt bits here
cause a TOCTOU race?

The main ISR loongson_canfd_interrupt() appears to have already cleared these
bits, so writing to them again could wipe out newly arrived interrupts.

> +	regmap_write(priv->regmap, LOONGSON_CANFD_INT_MASK, mask << 16);
> +}
> +
> +static int loongson_canfd_rx_napi(struct napi_struct *napi, int quota)
> +{
[ ... ]
> +	if (no_more_data) {
> +		if (napi_complete_done(napi, work_done)) {
> +			regmap_write(priv->regmap, LOONGSON_CANFD_INT_STAT, REG_INT_STAT_RBNEI);

[Severity: High]
Is it safe to clear the REG_INT_STAT_RBNEI interrupt flag at the end of the
NAPI poll?

This could create a race condition where any RX interrupts that arrived after
the empty check are dropped.

> +			regmap_write(priv->regmap, LOONGSON_CANFD_INT_MASK,
> +				     REG_INT_STAT_RBNEI << 16);
> +		}
> +	}
> +
> +	return work_done;
> +}
> +
> +static void loongson_canfd_tx_interrupt(struct net_device *ndev)
> +{
[ ... ]
> +		loongson_canfd_set_txbuf_cmd(ndev, TXT_CMD_SR_CLEAN, buf_id);
> +	}
> +
> +	regmap_write(priv->regmap, LOONGSON_CANFD_INT_STAT, REG_INT_STAT_TXBHCI);

[Severity: High]
Similar to the above, does clearing the aggregate TX completion interrupt flag
after checking the queues create a race window where missed completions could
stall the TX queue?

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