Add Loongson CAN-FD controller driver
Subsystems (2)
v3 cover Add Loongson CAN-FD controller driver
Hi all:
This patchset adds support for the CAN-FD controller found on Loongson CPUs.
Patch 1 introduces the basic CAN-FD controller driver with support for classic CAN and CAN FD, including bit timing, error handling, NAPI-based RX, and multiple TX buffers.
Patch 2 adds optional DMA support for RX path using the Loongson APB CMC DMA engine, which significantly reduces CPU load under high receive throughput.
The driver has been tested on Loongson-2K3000 platforms with various CAN/CAN FD traffic patterns.
Finally, I'd like to thank Bingxiong, the original author of this driver, for his efforts in working on the patch.
Thanks.
Binbin
=========
V3:
Patch (1/2):
- Replace the switch-case statement with a calculated mask in loongson_canfd_get_txbuf_sts();
- Change loongson_canfd_reset() return type to void;
- Drop old code in loongson_canfd_set_btr();
- Rename loongson_canfd_set_mode() to loongson_canfd_set_conf_mode();
- Drop cf->__res* usage;
- Drop unnecessary macro definitions;
- Add netif_napi_del() in probe error route path;
- Replace ternary operator with if-else;
- Drop stats->rx_packets and stats->rx_bytes count in CAN error frames;
- Add ethtool_ops;
- Add CAN_CTRLMODE_CC_LEN8_DLC;
Patch (2/2):
- Fix W=2 warning;
- Rename get_rx_frc() -> get_rxbuf_empty();
- Put loongson_canfd_rxdma_remove() before free_candev();
- Adjust the RXDMA probe process.
Link to V2:
https://lore.kernel.org/all/[email protected]/
V2:
Patch (1/2):
- Put all code into one file;
- Add COMPILE_TEST Kconfig option;
- Rewrite Kconfig description;
- Use regmap_test_bits() to simplify bit field checks;
- Drop odd FIELD_GET() usage;
- Don't use FIELD_GET() for a single bit;
- Use an if/else instead of the ternary operator;
- Use the CAN TDC framework to get the SSP value;
- Use guard(spinlock)/scoped_guard(spinlock);
- Use netdev_debug() to be less verboss;
- Check for memory allocation failure;
- Add CAN_ERR_CNT flag;
- Drop unused REG_DATA_xx_yy_W_DATA_yy definition;
- Add more function comment;
Link to V1:
https://lore.kernel.org/all/[email protected]/
Binbin Zhou (2):
can: Add Loongson CAN-FD controller driver
can: loongson_canfd: Add RXDMA support
MAINTAINERS | 7 + drivers/net/can/Kconfig | 11 + drivers/net/can/Makefile | 1 + drivers/net/can/loongson_canfd.c | 1859 ++++++++++++++++++++++++++++++ 4 files changed, 1878 insertions(+) create mode 100644 drivers/net/can/loongson_canfd.c
base-commit: b266bacba796ff5c4dcd2ae2fc08aacf7ab39153
Signature · 1 line
2.52.0
PATCH v3 1/2 can: Add Loongson CAN-FD controller driver
Add support for the CAN-FD controller integrated into Loongson-2K series SoCs. The controller supports both Classic CAN and CAN FD, with up to 8 transmit buffers, hardware timestamping, error counters, and various control modes (loopback, listen-only, one-shot, non-ISO FD, etc.).
The driver implements:
- NAPI for RX path
- TX buffer management with echo skb support
- Bus error reporting and fault confinement state handling
- Bit timing configuration for nominal and data phase
- Secondary Sample Point (SSP) configuration for high bitrates
--- MAINTAINERS | 7 + drivers/net/can/Kconfig | 11 + drivers/net/can/Makefile | 1 + drivers/net/can/loongson_canfd.c | 1693 ++++++++++++++++++++++++++++++ 4 files changed, 1712 insertions(+) create mode 100644 drivers/net/can/loongson_canfd.c
MAINTAINERS +7 -0
@@ -15042,6 +15042,13 @@ F: arch/loongarch/ F: drivers/*/*loongarch* F: drivers/cpufreq/loongson3_cpufreq.c +LOONGSON CAN FD DRIVER +M: Bingxiong Li <[email protected]> +M: Binbin Zhou <[email protected]> +L: [email protected] +S: Maintained +F: drivers/net/can/loongson_canfd/ + LOONGSON GPIO DRIVER M: Yinbo Zhu <[email protected]> L: [email protected]
drivers/net/can/Kconfig +11 -0
@@ -189,6 +189,17 @@ config CAN_KVASER_PCIEFD Kvaser M.2 PCIe 4xCAN Kvaser PCIe 8xCAN +config CAN_LOONGSON_CANFD + tristate "Loongson CAN-FD controller" + depends on HAS_IOMEM + select REGMAP_MMIO + help + This is a canfd driver switch for the Loongson platform, + integrated with the Loongson-2K series SoCs. + + To compile as a module, choose M here: the module will be + called loongson_canfd. + config CAN_SLCAN tristate "Serial / USB serial CAN Adaptors (slcan)" depends on TTY
drivers/net/can/Makefile +1 -0
@@ -27,6 +27,7 @@ obj-$(CONFIG_CAN_GRCAN) += grcan.o obj-$(CONFIG_CAN_IFI_CANFD) += ifi_canfd/ obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o obj-$(CONFIG_CAN_KVASER_PCIEFD) += kvaser_pciefd/ +obj-$(CONFIG_CAN_LOONGSON_CANFD) += loongson_canfd.o obj-$(CONFIG_CAN_MSCAN) += mscan/ obj-$(CONFIG_CAN_M_CAN) += m_can/ obj-$(CONFIG_CAN_PEAK_PCIEFD) += peak_canfd/
drivers/net/can/loongson_canfd.c new file +1693 -0
1,693 lines not on this page · raw message
Signature · 1 line
2.52.0
[email protected]
AI reviewRe: [PATCH v3 1/2] can: Add Loongson CAN-FD controller driver
Thank you for your contribution! Sashiko AI review found 18 potential issue(s) to consider: - [High] Missing `skb` check in `CAN_STATE_ERROR_ACTIVE` case leads to a NULL pointer dereference.…
Thank you for your contribution! Sashiko AI review found 18 potential issue(s) to consider:
- [High] Missing skb check in CAN_STATE_ERROR_ACTIVE case leads to a NULL pointer dereference.
- [High] SKB allocation failure or invalid frame in RX path permanently stalls NAPI and corrupts RX FIFO.
- [High] NAPI poll list corruption due to calling napi_complete_done while returning quota.
- [High] Missing napi_disable() on error paths in loongson_canfd_open() leads to a kernel panic on subsequent open attempts.
- [High] Race condition in loongson_canfd_start_xmit causes unexpected packet drops due to un-reaped TX buffers.
- [High] Missing bounds check on the hardware-provided word count REG_FRAME_META1_RWCNT causes heap buffer overflow.
- [High] Unchecked return value of device_property_read_u32 leaves clk_rate uninitialized.
- [High] Premature hardware restart on Bus-Off when auto-restart is configured.
- [High] Bitwise NOT on a 16-bit variable unintentionally unmasks all interrupts due to integer promotion.
- [High] Overwriting of isr variable causes lost error interrupts and permanent interrupt masking.
- [High] Using regmap_update_bits on command-style SET/CLR registers causes conflicting hardware commands.
- [High] Calling kfree_skb() with hardware interrupts disabled.
- [High] Potential infinite loop in TX interrupt handler due to repeated status reads of posted writes.
- [Medium] The driver advertises 'hardware timestamping' in commit message but does not implement it.
- [Medium] ndev->stats.rx_errors is modified concurrently from both hardirq and NAPI softirq contexts without locking.
- [Medium] Incorrect check for RTR frames causes invalid RX byte statistics.
- [Medium] Missing cf->len8_dlc assignment for Classic CAN frames.
- [Low] Syntax error due to variable declarations immediately following a switch case label.
--
commit e0d474f047900e47a73eaa207fc07dd8c35084bf
Author: Binbin Zhou <[email protected]>
Subject: can: Add Loongson CAN-FD controller driver
This commit adds support for the CAN-FD controller integrated into Loongson-2K series SoCs. It implements NAPI for the RX path, TX buffer management, bus error reporting, and various timing configurations.
[Severity: Medium]
The commit message states the driver supports hardware timestamping, but is this functionality actually implemented? Looking at the driver code, it sets ethtool_op_get_ts_info but does not seem to extract the timestamp field from the hardware or write to skb_hwtstamps in the receive path.
Quoted 5 lines
drivers/net/can/loongson_canfd.c new file +0
-0
[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -891,37 +891,24 @@ static int loongson_canfd_chip_start(struct net_device *ndev)
[ ... ]
drivers/net/can/loongson_canfd.c line 778
Quoted 10 lines
drivers/net/can/loongson_canfd.c new file +10
-0+/* Configure interrupts */ + int_ena =REG_INT_STAT_RBNEI |REG_INT_STAT_TXBHCI | +REG_INT_STAT_EWLI |REG_INT_STAT_FCSI ; + +/* Bus error reporting */ +if (priv->can.ctrlmode &CAN_CTRLMODE_BERR_REPORTING ) + int_ena |=REG_INT_STAT_ALI |REG_INT_STAT_BEI ; + +/* It's after reset, so there is no need to clear anything */ +regmap_write (priv->regmap,LOONGSON_CANFD_INT_MASK , ~int_ena);
[Severity: High]
Will this bitwise NOT operation unmask all interrupts on the controller? Because int_ena is a 16-bit unsigned integer, the bitwise NOT causes integer promotion to a 32-bit signed int, filling the upper 16 bits with 1s. Since the upper bits of this register clear the mask, does this unintentionally enable every possible interrupt?
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +1
-0+regmap_write (priv->regmap,LOONGSON_CANFD_INT_ENA , int_ena);
[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1048,46 +1048,22 @@ static netdev_tx_t loongson_canfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
[ ... ]
drivers/net/can/loongson_canfd.c line 938
Quoted 12 lines
drivers/net/can/loongson_canfd.c new file +12
-0+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 (!loongson_canfd_insert_frame (priv, cf, buf_id,can_is_canfd_skb (skb))) { +netdev_err (ndev,"TXNF set but cannot insert frame into TXTB! HW Bug?" ); +kfree_skb (skb);
[Severity: High]
Could a race condition here cause silent packet drops? The hardware clears tx_brp immediately on completion, but tx_bs is cleared later in the ISR. If all inactive buffers are awaiting a reap, the loop finishes without finding a buffer, leaving buf_id at 0. This would cause the insert check to fail, dropping the packet while incorrectly returning NETDEV_TX_OK.
[Severity: High]
Is it safe to call kfree_skb here? The tx_lock is held with spinlock_irqsave which disables local interrupts. Calling kfree_skb with hard interrupts disabled can lead to kernel warnings and potential deadlocks. Should this use dev_kfree_skb_any instead?
Quoted 3 lines
drivers/net/can/loongson_canfd.c new file +3
-0+ ndev->stats.tx_dropped++; +return NETDEV_TX_OK ; + }
[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1105,37 +1105,25 @@ static void loongson_canfd_read_rx_frame(struct loongson_canfd_priv *priv, struct canfd_frame *cf,
[ ... ]
drivers/net/can/loongson_canfd.c line 996
Quoted 10 lines
drivers/net/can/loongson_canfd.c new file +10
-0+/* Data Length Code */ + len =FIELD_GET (REG_FRAME_META1_DLC , meta1); +if (len >8 ) { +if (meta1 ®_FRAME_META1_FDF ) + len = wc <<2 ; +else + len =8 ; + } + + cf->len = len;
[Severity: Medium]
Does this properly support the CAN_CTRLMODE_CC_LEN8_DLC feature? For Classic CAN frames with a DLC greater than 8, the length is appropriately capped to 8, but the original DLC is not saved to cf->len8_dlc. This might break userspace protocols that rely on the original DLC data.
drivers/net/can/loongson_canfd.c line 1002
Quoted 6 lines
drivers/net/can/loongson_canfd.c new file +6
-0+if (unlikely (len > wc *LOONGSON_CANFD_DW_BYTE )) + len = wc *LOONGSON_CANFD_DW_BYTE ; + +/* Data */ +for (i =0 ; i < len; i +=LOONGSON_CANFD_DW_BYTE ) +regmap_read (priv->regmap,LOONGSON_CANFD_RX_DATA , (u32 *)(cf->data + i));
[Severity: High]
Can a malformed hardware word count cause a heap buffer overflow? The len variable is checked against the hardware-provided word count, but not against CANFD_MAX_DLEN. If the hardware yields an invalid read word count, the length could exceed the 64-byte payload limit, causing out of bounds memory writes to the skb data.
[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1164,28 +1164,22 @@ static int loongson_canfd_rx(struct net_device *ndev)
[ ... ]
drivers/net/can/loongson_canfd.c line 1041
Quoted 7 lines
drivers/net/can/loongson_canfd.c new file +7
-0+/* Flexible Data-rate Format */ +if (meta1 ®_FRAME_META1_FDF ) + skb =alloc_canfd_skb (ndev, &cf); +else + skb =alloc_can_skb (ndev, (struct can_frame **)&cf); +if (unlikely (!skb)) +return 0 ;
[Severity: High]
If the skb allocation fails, does returning 0 early permanently stall the NAPI loop? Since the rest of the data payload is not read from the hardware FIFO, the RX stream might desynchronize. Also, returning 0 here causes the caller to exit its loop without completing NAPI, potentially leaving it permanently in a scheduled state.
drivers/net/can/loongson_canfd.c line 1046
Quoted 5 lines
drivers/net/can/loongson_canfd.c new file +5
-0+ +loongson_canfd_read_rx_frame (priv, cf, meta0, meta1); + +if (cf->can_id !=CAN_RTR_FLAG ) + stats->rx_bytes += cf->len;
[Severity: Medium]
Is this RTR check accurate? The can_id field contains both the CAN ID and the flags, so a direct inequality comparison to CAN_RTR_FLAG might evaluate true for almost all RTR frames, erroneously inflating the RX byte statistics for frames with no actual payload.
[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1203,32 +1203,26 @@ static enum can_state loongson_canfd_read_fault_state(struct net_device *ndev)
[ ... ]
drivers/net/can/loongson_canfd.c line 1069
Quoted 3 lines
drivers/net/can/loongson_canfd.c new file +3
-0+switch (FIELD_GET (REG_FSTAT_MASK , fstat)) { +case REG_FSTAT_ERA : +u32 ewl, erl, rec_tec, max_tec;
[Severity: Low]
Will this compile on strict C11 compilers? The C11 standard does not allow variable declarations immediately following a switch case label.
[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1278,82 +1278,28 @@ static void loongson_canfd_err_interrupt(struct net_device *ndev, u32 isr)
[ ... ]
drivers/net/can/loongson_canfd.c line 1172
Quoted 8 lines
drivers/net/can/loongson_canfd.c new file +8
-0+if ((isr ®_INT_STAT_FCSI ) || (isr ®_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 permanently disable other interrupts? If arbitration lost or bus error flags are pending, this assignment destroys them. Later in the function, isr is shifted to unmask interrupts, meaning the erased flags will never be unmasked again.
drivers/net/can/loongson_canfd.c line 1179
Quoted 7 lines
drivers/net/can/loongson_canfd.c new file +7
-0+ 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 this prematurely restart the hardware upon a bus-off event? Instantly writing the error reset command clears the hardware error state and rejoins the bus immediately, ignoring the restart delay contract enforced by the core can_bus_off timer.
[ ... ]
drivers/net/can/loongson_canfd.c line 1212
Quoted 4 lines
drivers/net/can/loongson_canfd.c new file +4
-0+case CAN_STATE_ERROR_ACTIVE : + cf->can_id |=CAN_ERR_CNT ; + cf->data[1 ] =CAN_ERR_CRTL_ACTIVE ; + cf->data[6 ] = bec.txerr;
[Severity: High]
Could this lead to a null pointer dereference? The alloc_can_err_skb call earlier in the function can fail and leave cf uninitialized. While the other switch cases check if skb is valid before modifying cf data, this case does not, which would crash if memory pressure prevents allocation.
[ ... ]
drivers/net/can/loongson_canfd.c line 1238
Quoted 6 lines
drivers/net/can/loongson_canfd.c new file +6
-0+/* Check for Bus Error interrupt */ +if (isr ®_INT_STAT_BEI ) { + isr =REG_INT_STAT_BEI ; +netdev_err (ndev,"Bus Error interrupt\n" ); + priv->can.can_stats.bus_error++; + stats->rx_errors++;
[Severity: Medium]
Is it safe to increment stats->rx_errors without locking here? This counter is modified from hardirq context here and concurrently from softirq context in the NAPI poll routine, which could cause lost increments due to data races on 32-bit systems.
[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1391,37 +1391,25 @@ static int loongson_canfd_rx_napi(struct napi_struct *napi, int quota)
[ ... ]
drivers/net/can/loongson_canfd.c line 1278
Quoted 10 lines
drivers/net/can/loongson_canfd.c new file +10
-0+/* Check for RX FIFO Overflow */ +regmap_read (priv->regmap,LOONGSON_CANFD_STAT , &sts); +if (sts ®_STAT_DOR ) { +struct net_device_stats *stats = &ndev->stats; +struct can_frame *cf; +struct sk_buff *skb; + +netdev_info (ndev,"Loongson canfd RX overflow\n" ); + stats->rx_over_errors++; + stats->rx_errors++;
[Severity: Medium]
As noted above, stats->rx_errors is updated concurrently in both softirq and hardirq contexts without locking.
[ ... ]
drivers/net/can/loongson_canfd.c line 1292
Quoted 2 lines
drivers/net/can/loongson_canfd.c new file +2
-0+if (rxbuf_is_empty && ret !=0 ) { +if (napi_complete_done (napi, work_done)) {
[Severity: High]
Should this check if work_done is less than quota? If exactly quota frames are processed and the hardware buffer becomes empty, this logic calls napi_complete_done and then returns quota to the networking core. Returning the full quota instructs the core to reschedule the instance, but the state has already been manipulated, which can corrupt the NAPI poll list.
[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1425,33 +1425,18 @@ static void loongson_canfd_tx_interrupt(struct net_device *ndev)
[ ... ]
drivers/net/can/loongson_canfd.c line 1320
Quoted 6 lines
drivers/net/can/loongson_canfd.c new file +6
-0+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 ;
[Severity: High]
Could this result in an inescapable infinite loop? When the driver writes a clear command to the posted MMIO register at the end of the loop, it immediately reads the status register back in the while condition. Because the hardware takes cycles to clear the bit, the old status is read, causing the driver to repeatedly process the same buffer and spin in the ISR.
drivers/net/can/loongson_canfd.c line 1322
Quoted 2 lines
drivers/net/can/loongson_canfd.c new file +2
-0+case TX_BS_FAIL : +u32 cnt;
[Severity: Low]
Similar to the fault state read function, this violates C11 syntax by declaring a variable immediately following a switch case label.
[ ... ]
drivers/net/can/loongson_canfd.c static void loongson_canfd_chip_stop(struct net_device *ndev) · line 1590
Quoted 8 lines
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1584,20 +1584,14 @@ static void loongson_canfd_chip_stop(struct net_device *ndev) {struct loongson_canfd_priv *priv =netdev_priv (ndev);/* Disable interrupts and disable CAN */ regmap_update_bits (priv->regmap,LOONGSON_CANFD_INT_ENA ,REG_INT_ENA_CLR ,REG_INT_ENA_CLR );regmap_update_bits (priv->regmap,LOONGSON_CANFD_INT_MASK ,REG_INT_MASK_SET ,REG_INT_MASK_SET );
[Severity: High]
Are these regmap_update_bits calls safe for W1S/W1C command registers? This function reads the current 1s from the lower half of INT_ENA and writes them back along with the clear mask in the upper half, which commands the hardware to simultaneously set and clear the exact same interrupts.
[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1604,45 +1604,22 @@ static int loongson_canfd_open(struct net_device *ndev)
[ ... ]
drivers/net/can/loongson_canfd.c line 1496
Quoted 6 lines
drivers/net/can/loongson_canfd.c new file +6
-0+err_chip_start: +free_irq (ndev->irq, ndev); +err_irq: +close_candev (ndev); +return ret; +}
[Severity: High]
Are we missing a napi_disable call on these error paths? If the driver fails to open, the candev is closed but NAPI is left enabled. A subsequent attempt to bring the interface up will call napi_enable on an already enabled instance, triggering a BUG_ON in the core.
[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1690,46 +1690,24 @@ static int loongson_canfd_probe(struct platform_device *pdev)
[ ... ]
drivers/net/can/loongson_canfd.c line 1611
Quoted 7 lines
drivers/net/can/loongson_canfd.c new file +7
-0+ regmap =devm_regmap_init_mmio (dev, base, &loongson_cangfd_regmap); +if (IS_ERR (regmap)) +return PTR_ERR (regmap); + +device_property_read_u32 (dev,"clock-frequency" , &clk_rate); + + irq =platform_get_irq (pdev,0 );
[Severity: High]
Should the return value of device_property_read_u32 be checked? If the property is missing, clk_rate retains uninitialized stack garbage which is then assigned to priv->can.clock.freq, leading to unpredictable hardware timing.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Vincent Mailhol
maintainerRe: [PATCH v3 1/2] can: Add Loongson CAN-FD controller driver
Hi Binbin,
Thanks for the v3. Please first have a look at the sashiko bot findings. Some of these are legit.
I will continue my review after you address the relevant findings.
Quoted 315 lines · On 30/06/2026 at 05:16, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 18 potential issue(s) to consider:
- [High] Missingskbcheck inCAN_STATE_ERROR_ACTIVEcase leads to a NULL pointer dereference.
- [High] SKB allocation failure or invalid frame in RX path permanently stalls NAPI and corrupts RX FIFO.
- [High] NAPI poll list corruption due to callingnapi_complete_donewhile returningquota.
- [High] Missingnapi_disable()on error paths inloongson_canfd_open()leads to a kernel panic on subsequent open attempts.
- [High] Race condition inloongson_canfd_start_xmitcauses unexpected packet drops due to un-reaped TX buffers.
- [High] Missing bounds check on the hardware-provided word countREG_FRAME_META1_RWCNTcauses heap buffer overflow.
- [High] Unchecked return value ofdevice_property_read_u32leavesclk_rateuninitialized.
- [High] Premature hardware restart on Bus-Off when auto-restart is configured.
- [High] Bitwise NOT on a 16-bit variable unintentionally unmasks all interrupts due to integer promotion.
- [High] Overwriting ofisrvariable causes lost error interrupts and permanent interrupt masking.
- [High] Usingregmap_update_bitson command-style SET/CLR registers causes conflicting hardware commands.
- [High] Callingkfree_skb()with hardware interrupts disabled.
- [High] Potential infinite loop in TX interrupt handler due to repeated status reads of posted writes.
- [Medium] The driver advertises 'hardware timestamping' in commit message but does not implement it.
- [Medium]ndev->stats.rx_errorsis modified concurrently from both hardirq and NAPI softirq contexts without locking.
- [Medium] Incorrect check for RTR frames causes invalid RX byte statistics.
- [Medium] Missingcf->len8_dlcassignment for Classic CAN frames.
- [Low] Syntax error due to variable declarations immediately following a switch case label.
--commit
e0d474f047900e47a73eaa207fc07dd8c35084bf
Author: Binbin Zhou <[email protected]>
Subject: can: Add Loongson CAN-FD controller driverThis commit adds support for the CAN-FD controller integrated into Loongson-2K series SoCs. It implements NAPI for the RX path, TX buffer management, bus error reporting, and various timing configurations.
[Severity: Medium]
The commit message states the driver supports hardware timestamping, but is this functionality actually implemented? Looking at the driver code, it setsethtool_op_get_ts_infobut does not seem to extract the timestamp field from the hardware or write toskb_hwtstampsin the receive path.Quoted 5 lines
drivers/net/can/loongson_canfd.c new file +0
-0[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -891,37 +891,24 @@ static int loongson_canfd_chip_start(struct net_device *ndev)[ ... ]
drivers/net/can/loongson_canfd.c line 778
Quoted 10 lines
drivers/net/can/loongson_canfd.c new file +10
-0+/* Configure interrupts */ + int_ena =REG_INT_STAT_RBNEI |REG_INT_STAT_TXBHCI | +REG_INT_STAT_EWLI |REG_INT_STAT_FCSI ; + +/* Bus error reporting */ +if (priv->can.ctrlmode &CAN_CTRLMODE_BERR_REPORTING ) + int_ena |=REG_INT_STAT_ALI |REG_INT_STAT_BEI ; + +/* It's after reset, so there is no need to clear anything */ +regmap_write (priv->regmap,LOONGSON_CANFD_INT_MASK , ~int_ena);[Severity: High]
Will this bitwise NOT operation unmask all interrupts on the controller? Becauseint_enais a 16-bit unsigned integer, the bitwise NOT causes integer promotion to a 32-bit signed int, filling the upper 16 bits with 1s. Since the upper bits of this register clear the mask, does this unintentionally enable every possible interrupt?Quoted 1 line
drivers/net/can/loongson_canfd.c new file +1
-0+regmap_write (priv->regmap,LOONGSON_CANFD_INT_ENA , int_ena);[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1048,46 +1048,22 @@ static netdev_tx_t loongson_canfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)[ ... ]
drivers/net/can/loongson_canfd.c line 938
Quoted 12 lines
drivers/net/can/loongson_canfd.c new file +12
-0+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 (!loongson_canfd_insert_frame (priv, cf, buf_id,can_is_canfd_skb (skb))) { +netdev_err (ndev,"TXNF set but cannot insert frame into TXTB! HW Bug?" ); +kfree_skb (skb);[Severity: High]
Could a race condition here cause silent packet drops? The hardware clearstx_brpimmediately on completion, buttx_bsis cleared later in the ISR. If all inactive buffers are awaiting a reap, the loop finishes without finding a buffer, leavingbuf_idat 0. This would cause the insert check to fail, dropping the packet while incorrectly returningNETDEV_TX_OK.[Severity: High]
Is it safe to callkfree_skbhere? Thetx_lockis held withspinlock_irqsavewhich disables local interrupts. Callingkfree_skbwith hard interrupts disabled can lead to kernel warnings and potential deadlocks. Should this usedev_kfree_skb_anyinstead?Quoted 3 lines
drivers/net/can/loongson_canfd.c new file +3
-0+ ndev->stats.tx_dropped++; +return NETDEV_TX_OK ; + }[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1105,37 +1105,25 @@ static void loongson_canfd_read_rx_frame(struct loongson_canfd_priv *priv, struct canfd_frame *cf,[ ... ]
drivers/net/can/loongson_canfd.c line 996
Quoted 10 lines
drivers/net/can/loongson_canfd.c new file +10
-0+/* Data Length Code */ + len =FIELD_GET (REG_FRAME_META1_DLC , meta1); +if (len >8 ) { +if (meta1 ®_FRAME_META1_FDF ) + len = wc <<2 ; +else + len =8 ; + } + + cf->len = len;[Severity: Medium]
Does this properly support theCAN_CTRLMODE_CC_LEN8_DLCfeature? For Classic CAN frames with a DLC greater than 8, the length is appropriately capped to 8, but the original DLC is not saved tocf->len8_dlc. This might break userspace protocols that rely on the original DLC data.drivers/net/can/loongson_canfd.c line 1002
Quoted 6 lines
drivers/net/can/loongson_canfd.c new file +6
-0+if (unlikely (len > wc *LOONGSON_CANFD_DW_BYTE )) + len = wc *LOONGSON_CANFD_DW_BYTE ; + +/* Data */ +for (i =0 ; i < len; i +=LOONGSON_CANFD_DW_BYTE ) +regmap_read (priv->regmap,LOONGSON_CANFD_RX_DATA , (u32 *)(cf->data + i));[Severity: High]
Can a malformed hardware word count cause a heap buffer overflow? The len variable is checked against the hardware-provided word count, but not againstCANFD_MAX_DLEN. If the hardware yields an invalid read word count, the length could exceed the 64-byte payload limit, causing out of bounds memory writes to the skb data.[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1164,28 +1164,22 @@ static int loongson_canfd_rx(struct net_device *ndev)[ ... ]
drivers/net/can/loongson_canfd.c line 1041
Quoted 7 lines
drivers/net/can/loongson_canfd.c new file +7
-0+/* Flexible Data-rate Format */ +if (meta1 ®_FRAME_META1_FDF ) + skb =alloc_canfd_skb (ndev, &cf); +else + skb =alloc_can_skb (ndev, (struct can_frame **)&cf); +if (unlikely (!skb)) +return 0 ;[Severity: High]
If the skb allocation fails, does returning 0 early permanently stall the NAPI loop? Since the rest of the data payload is not read from the hardware FIFO, the RX stream might desynchronize. Also, returning 0 here causes the caller to exit its loop without completing NAPI, potentially leaving it permanently in a scheduled state.drivers/net/can/loongson_canfd.c line 1046
Quoted 5 lines
drivers/net/can/loongson_canfd.c new file +5
-0+ +loongson_canfd_read_rx_frame (priv, cf, meta0, meta1); + +if (cf->can_id !=CAN_RTR_FLAG ) + stats->rx_bytes += cf->len;[Severity: Medium]
Is this RTR check accurate? Thecan_idfield contains both the CAN ID and the flags, so a direct inequality comparison toCAN_RTR_FLAGmight evaluate true for almost all RTR frames, erroneously inflating the RX byte statistics for frames with no actual payload.[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1203,32 +1203,26 @@ static enum can_state loongson_canfd_read_fault_state(struct net_device *ndev)[ ... ]
drivers/net/can/loongson_canfd.c line 1069
Quoted 3 lines
drivers/net/can/loongson_canfd.c new file +3
-0+switch (FIELD_GET (REG_FSTAT_MASK , fstat)) { +case REG_FSTAT_ERA : +u32 ewl, erl, rec_tec, max_tec;[Severity: Low]
Will this compile on strict C11 compilers? The C11 standard does not allow variable declarations immediately following a switch case label.[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1278,82 +1278,28 @@ static void loongson_canfd_err_interrupt(struct net_device *ndev, u32 isr)[ ... ]
drivers/net/can/loongson_canfd.c line 1172
Quoted 8 lines
drivers/net/can/loongson_canfd.c new file +8
-0+if ((isr ®_INT_STAT_FCSI ) || (isr ®_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 permanently disable other interrupts? If arbitration lost or bus error flags are pending, this assignment destroys them. Later in the function, isr is shifted to unmask interrupts, meaning the erased flags will never be unmasked again.drivers/net/can/loongson_canfd.c line 1179
Quoted 7 lines
drivers/net/can/loongson_canfd.c new file +7
-0+ 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 this prematurely restart the hardware upon a bus-off event? Instantly writing the error reset command clears the hardware error state and rejoins the bus immediately, ignoring the restart delay contract enforced by the corecan_bus_offtimer.[ ... ]
drivers/net/can/loongson_canfd.c line 1212
Quoted 4 lines
drivers/net/can/loongson_canfd.c new file +4
-0+case CAN_STATE_ERROR_ACTIVE : + cf->can_id |=CAN_ERR_CNT ; + cf->data[1 ] =CAN_ERR_CRTL_ACTIVE ; + cf->data[6 ] = bec.txerr;[Severity: High]
Could this lead to a null pointer dereference? Thealloc_can_err_skbcall earlier in the function can fail and leave cf uninitialized. While the other switch cases check if skb is valid before modifying cf data, this case does not, which would crash if memory pressure prevents allocation.[ ... ]
drivers/net/can/loongson_canfd.c line 1238
Quoted 6 lines
drivers/net/can/loongson_canfd.c new file +6
-0+/* Check for Bus Error interrupt */ +if (isr ®_INT_STAT_BEI ) { + isr =REG_INT_STAT_BEI ; +netdev_err (ndev,"Bus Error interrupt\n" ); + priv->can.can_stats.bus_error++; + stats->rx_errors++;[Severity: Medium]
Is it safe to incrementstats->rx_errorswithout locking here? This counter is modified from hardirq context here and concurrently from softirq context in the NAPI poll routine, which could cause lost increments due to data races on 32-bit systems.[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1391,37 +1391,25 @@ static int loongson_canfd_rx_napi(struct napi_struct *napi, int quota)[ ... ]
drivers/net/can/loongson_canfd.c line 1278
Quoted 10 lines
drivers/net/can/loongson_canfd.c new file +10
-0+/* Check for RX FIFO Overflow */ +regmap_read (priv->regmap,LOONGSON_CANFD_STAT , &sts); +if (sts ®_STAT_DOR ) { +struct net_device_stats *stats = &ndev->stats; +struct can_frame *cf; +struct sk_buff *skb; + +netdev_info (ndev,"Loongson canfd RX overflow\n" ); + stats->rx_over_errors++; + stats->rx_errors++;[Severity: Medium]
As noted above,stats->rx_errorsis updated concurrently in both softirq and hardirq contexts without locking.[ ... ]
drivers/net/can/loongson_canfd.c line 1292
Quoted 2 lines
drivers/net/can/loongson_canfd.c new file +2
-0+if (rxbuf_is_empty && ret !=0 ) { +if (napi_complete_done (napi, work_done)) {[Severity: High]
Should this check ifwork_doneis less than quota? If exactly quota frames are processed and the hardware buffer becomes empty, this logic callsnapi_complete_doneand then returns quota to the networking core. Returning the full quota instructs the core to reschedule the instance, but the state has already been manipulated, which can corrupt the NAPI poll list.[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1425,33 +1425,18 @@ static void loongson_canfd_tx_interrupt(struct net_device *ndev)[ ... ]
drivers/net/can/loongson_canfd.c line 1320
Quoted 6 lines
drivers/net/can/loongson_canfd.c new file +6
-0+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 ;[Severity: High]
Could this result in an inescapable infinite loop? When the driver writes a clear command to the posted MMIO register at the end of the loop, it immediately reads the status register back in the while condition. Because the hardware takes cycles to clear the bit, the old status is read, causing the driver to repeatedly process the same buffer and spin in the ISR.drivers/net/can/loongson_canfd.c line 1322
Quoted 2 lines
drivers/net/can/loongson_canfd.c new file +2
-0+case TX_BS_FAIL : +u32 cnt;[Severity: Low]
Similar to the fault state read function, this violates C11 syntax by declaring a variable immediately following a switch case label.[ ... ]
drivers/net/can/loongson_canfd.c static void loongson_canfd_chip_stop(struct net_device *ndev) · line 1590
Quoted 8 lines
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1584,20 +1584,14 @@ static void loongson_canfd_chip_stop(struct net_device *ndev) {struct loongson_canfd_priv *priv =netdev_priv (ndev);/* Disable interrupts and disable CAN */ regmap_update_bits (priv->regmap,LOONGSON_CANFD_INT_ENA ,REG_INT_ENA_CLR ,REG_INT_ENA_CLR );regmap_update_bits (priv->regmap,LOONGSON_CANFD_INT_MASK ,REG_INT_MASK_SET ,REG_INT_MASK_SET );[Severity: High]
Are theseregmap_update_bitscalls safe for W1S/W1C command registers? This function reads the current 1s from the lower half ofINT_ENAand writes them back along with the clear mask in the upper half, which commands the hardware to simultaneously set and clear the exact same interrupts.[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1604,45 +1604,22 @@ static int loongson_canfd_open(struct net_device *ndev)[ ... ]
drivers/net/can/loongson_canfd.c line 1496
Quoted 6 lines
drivers/net/can/loongson_canfd.c new file +6
-0+err_chip_start: +free_irq (ndev->irq, ndev); +err_irq: +close_candev (ndev); +return ret; +}[Severity: High]
Are we missing anapi_disablecall on these error paths? If the driver fails to open, the candev is closed but NAPI is left enabled. A subsequent attempt to bring the interface up will callnapi_enableon an already enabled instance, triggering aBUG_ONin the core.[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1690,46 +1690,24 @@ static int loongson_canfd_probe(struct platform_device *pdev)[ ... ]
Quoted 13 lines
drivers/net/can/loongson_canfd.c line 1611
Quoted 7 lines
drivers/net/can/loongson_canfd.c new file +7
-0+ regmap =devm_regmap_init_mmio (dev, base, &loongson_cangfd_regmap); +if (IS_ERR (regmap)) +return PTR_ERR (regmap); + +device_property_read_u32 (dev,"clock-frequency" , &clk_rate); + + irq =platform_get_irq (pdev,0 );[Severity: High]
Should the return value ofdevice_property_read_u32be checked? If the property is missing,clk_rateretains uninitialized stack garbage which is then assigned topriv->can.clock.freq, leading to unpredictable hardware timing.
Yours sincerely,
Vincent Mailhol
Re: [PATCH v3 1/2] can: Add Loongson CAN-FD controller driver
In reply to [email protected]Hi Vincent:
I've checked these issues and tried to provide solutions. Please check to see if they are correct.
Quoted 29 lines · On 2026/6/30 11:16, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 18 potential issue(s) to consider:
- [High] Missingskbcheck inCAN_STATE_ERROR_ACTIVEcase leads to a NULL pointer dereference.
- [High] SKB allocation failure or invalid frame in RX path permanently stalls NAPI and corrupts RX FIFO.
- [High] NAPI poll list corruption due to callingnapi_complete_donewhile returningquota.
- [High] Missingnapi_disable()on error paths inloongson_canfd_open()leads to a kernel panic on subsequent open attempts.
- [High] Race condition inloongson_canfd_start_xmitcauses unexpected packet drops due to un-reaped TX buffers.
- [High] Missing bounds check on the hardware-provided word countREG_FRAME_META1_RWCNTcauses heap buffer overflow.
- [High] Unchecked return value ofdevice_property_read_u32leavesclk_rateuninitialized.
- [High] Premature hardware restart on Bus-Off when auto-restart is configured.
- [High] Bitwise NOT on a 16-bit variable unintentionally unmasks all interrupts due to integer promotion.
- [High] Overwriting ofisrvariable causes lost error interrupts and permanent interrupt masking.
- [High] Usingregmap_update_bitson command-style SET/CLR registers causes conflicting hardware commands.
- [High] Callingkfree_skb()with hardware interrupts disabled.
- [High] Potential infinite loop in TX interrupt handler due to repeated status reads of posted writes.
- [Medium] The driver advertises 'hardware timestamping' in commit message but does not implement it.
- [Medium]ndev->stats.rx_errorsis modified concurrently from both hardirq and NAPI softirq contexts without locking.
- [Medium] Incorrect check for RTR frames causes invalid RX byte statistics.
- [Medium] Missingcf->len8_dlcassignment for Classic CAN frames.
- [Low] Syntax error due to variable declarations immediately following a switch case label.
--commit
e0d474f047900e47a73eaa207fc07dd8c35084bf
Author: Binbin Zhou <[email protected]>
Subject: can: Add Loongson CAN-FD controller driverThis commit adds support for the CAN-FD controller integrated into Loongson-2K series SoCs. It implements NAPI for the RX path, TX buffer management, bus error reporting, and various timing configurations.
Quoted 5 lines
[Severity: Medium]
The commit message states the driver supports hardware timestamping, but is this functionality actually implemented? Looking at the driver code, it setsethtool_op_get_ts_infobut does not seem to extract the timestamp field from the hardware or write toskb_hwtstampsin the receive path.
Hardware timestamps are not supported at this time; I will rewrite the commit.
Quoted 18 lines
Quoted 6 lines
drivers/net/can/loongson_canfd.c new file +0
-0[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -891,37 +891,24 @@ static int loongson_canfd_chip_start(struct net_device *ndev)[ ... ]
Quoted 9 lines
drivers/net/can/loongson_canfd.c new file +9
-0+/* Configure interrupts */ + int_ena =REG_INT_STAT_RBNEI |REG_INT_STAT_TXBHCI | +REG_INT_STAT_EWLI |REG_INT_STAT_FCSI ; + +/* Bus error reporting */ +if (priv->can.ctrlmode &CAN_CTRLMODE_BERR_REPORTING ) + int_ena |=REG_INT_STAT_ALI |REG_INT_STAT_BEI ; + +/* It's after reset, so there is no need to clear anything */
Quoted 8 lines
drivers/net/can/loongson_canfd.c line 778
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +1
-0+regmap_write (priv->regmap,LOONGSON_CANFD_INT_MASK , ~int_ena);[Severity: High]
Will this bitwise NOT operation unmask all interrupts on the controller? Becauseint_enais a 16-bit unsigned integer, the bitwise NOT causes integer promotion to a 32-bit signed int, filling the upper 16 bits with 1s. Since the upper bits of this register clear the mask, does this unintentionally enable every possible interrupt?
Use FIELD_GET(REG_INT_MASK_SET, ~int_ena) instsed.
Quoted 37 lines
Quoted 2 lines
drivers/net/can/loongson_canfd.c new file +1
-0+regmap_write (priv->regmap,LOONGSON_CANFD_INT_ENA , int_ena);[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1048,46 +1048,22 @@ static netdev_tx_t loongson_canfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)[ ... ]
drivers/net/can/loongson_canfd.c line 938
Quoted 12 lines
drivers/net/can/loongson_canfd.c new file +12
-0+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 (!loongson_canfd_insert_frame (priv, cf, buf_id,can_is_canfd_skb (skb))) { +netdev_err (ndev,"TXNF set but cannot insert frame into TXTB! HW Bug?" ); +kfree_skb (skb);[Severity: High]
Could a race condition here cause silent packet drops? The hardware clearstx_brpimmediately on completion, buttx_bsis cleared later in the ISR. If all inactive buffers are awaiting a reap, the loop finishes without finding a buffer, leavingbuf_idat 0. This would cause the insert check to fail, dropping the packet while incorrectly returningNETDEV_TX_OK.[Severity: High]
Is it safe to callkfree_skbhere? Thetx_lockis held withspinlock_irqsavewhich disables local interrupts. Callingkfree_skbwith hard interrupts disabled can lead to kernel warnings and potential deadlocks. Should this usedev_kfree_skb_anyinstead?Quoted 3 lines
drivers/net/can/loongson_canfd.c new file +3
-0+ ndev->stats.tx_dropped++; +return NETDEV_TX_OK ; + }[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1105,37 +1105,25 @@ static void loongson_canfd_read_rx_frame(struct loongson_canfd_priv *priv, struct canfd_frame *cf,[ ... ]
Quoted 16 lines
drivers/net/can/loongson_canfd.c line 996
Quoted 10 lines
drivers/net/can/loongson_canfd.c new file +10
-0+/* Data Length Code */ + len =FIELD_GET (REG_FRAME_META1_DLC , meta1); +if (len >8 ) { +if (meta1 ®_FRAME_META1_FDF ) + len = wc <<2 ; +else + len =8 ; + } + + cf->len = len;[Severity: Medium]
Does this properly support theCAN_CTRLMODE_CC_LEN8_DLCfeature? For Classic CAN frames with a DLC greater than 8, the length is appropriately capped to 8, but the original DLC is not saved tocf->len8_dlc. This might break userspace protocols that rely on the original DLC data.
Yes, I should use the relevant standard APIs, as follows:
/* 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);Quoted 14 lines
drivers/net/can/loongson_canfd.c line 1002
Quoted 7 lines
drivers/net/can/loongson_canfd.c new file +6
-0+if (unlikely (len > wc *LOONGSON_CANFD_DW_BYTE )) + len = wc *LOONGSON_CANFD_DW_BYTE ; + +/* Data */ +for (i =0 ; i < len; i +=LOONGSON_CANFD_DW_BYTE ) +regmap_read (priv->regmap,LOONGSON_CANFD_RX_DATA , (u32 *)(cf->data + i));[Severity: High]
Can a malformed hardware word count cause a heap buffer overflow? The len variable is checked against the hardware-provided word count, but not againstCANFD_MAX_DLEN. If the hardware yields an invalid read word count, the length could exceed the 64-byte payload limit, causing out of bounds memory writes to the skb data.
I think using the method described above to obtain cf->len should resolve this issue.
Quoted 19 lines
[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1164,28 +1164,22 @@ static int loongson_canfd_rx(struct net_device *ndev)[ ... ]
drivers/net/can/loongson_canfd.c line 1041
Quoted 7 lines
drivers/net/can/loongson_canfd.c new file +7
-0+/* Flexible Data-rate Format */ +if (meta1 ®_FRAME_META1_FDF ) + skb =alloc_canfd_skb (ndev, &cf); +else + skb =alloc_can_skb (ndev, (struct can_frame **)&cf); +if (unlikely (!skb)) +return 0 ;[Severity: High]
If the skb allocation fails, does returning 0 early permanently stall the NAPI loop? Since the rest of the data payload is not read from the hardware FIFO, the RX stream might desynchronize. Also, returning 0 here causes the caller to exit its loop without completing NAPI, potentially leaving it permanently in a scheduled state.
Quoted 11 lines
drivers/net/can/loongson_canfd.c line 1046
Quoted 5 lines
drivers/net/can/loongson_canfd.c new file +5
-0+ +loongson_canfd_read_rx_frame (priv, cf, meta0, meta1); + +if (cf->can_id !=CAN_RTR_FLAG ) + stats->rx_bytes += cf->len;[Severity: Medium]
Is this RTR check accurate? Thecan_idfield contains both the CAN ID and the flags, so a direct inequality comparison toCAN_RTR_FLAGmight evaluate true for almost all RTR frames, erroneously inflating the RX byte statistics for frames with no actual payload.
Emm, it should be:
if (!(cf->can_id & CAN_RTR_FLAG))
Quoted 30 lines
[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1203,32 +1203,26 @@ static enum can_state loongson_canfd_read_fault_state(struct net_device *ndev)[ ... ]
drivers/net/can/loongson_canfd.c line 1069
Quoted 3 lines
drivers/net/can/loongson_canfd.c new file +3
-0+switch (FIELD_GET (REG_FSTAT_MASK , fstat)) { +case REG_FSTAT_ERA : +u32 ewl, erl, rec_tec, max_tec;[Severity: Low]
Will this compile on strict C11 compilers? The C11 standard does not allow variable declarations immediately following a switch case label.[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1278,82 +1278,28 @@ static void loongson_canfd_err_interrupt(struct net_device *ndev, u32 isr)[ ... ]
drivers/net/can/loongson_canfd.c line 1172
Quoted 8 lines
drivers/net/can/loongson_canfd.c new file +8
-0+if ((isr ®_INT_STAT_FCSI ) || (isr ®_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 permanently disable other interrupts? If arbitration lost or bus error flags are pending, this assignment destroys them. Later in the function, isr is shifted to unmask interrupts, meaning the erased flags will never be unmasked again.
Quoted 13 lines
drivers/net/can/loongson_canfd.c line 1179
Quoted 7 lines
drivers/net/can/loongson_canfd.c new file +7
-0+ 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 this prematurely restart the hardware upon a bus-off event? Instantly writing the error reset command clears the hardware error state and rejoins the bus immediately, ignoring the restart delay contract enforced by the corecan_bus_offtimer.
The meaning of REG_CMD_ERCRST is as follows:
Used to clear the error count while waiting for the controller to rejoin the bus after entering BUS_OFF.
A 1 can be written to this register at any time before or after entering BUS_OFF; it takes effect once and then becomes invalid.
This command is used only to reset the count and does not affect the reboot.
Quoted 12 lines
[ ... ]
drivers/net/can/loongson_canfd.c line 1212
Quoted 4 lines
drivers/net/can/loongson_canfd.c new file +4
-0+case CAN_STATE_ERROR_ACTIVE : + cf->can_id |=CAN_ERR_CNT ; + cf->data[1 ] =CAN_ERR_CRTL_ACTIVE ; + cf->data[6 ] = bec.txerr;[Severity: High]
Could this lead to a null pointer dereference? Thealloc_can_err_skbcall earlier in the function can fail and leave cf uninitialized. While the other switch cases check if skb is valid before modifying cf data, this case does not, which would crash if memory pressure prevents allocation.
Add skb check, as follows:
if (skb) {
cf->can_id |= CAN_ERR_CNT;
cf->data[1] = CAN_ERR_CRTL_ACTIVE;
cf->data[6] = bec.txerr;
cf->data[7] = bec.rxerr;
}Quoted 73 lines
[ ... ]
drivers/net/can/loongson_canfd.c line 1238
Quoted 6 lines
drivers/net/can/loongson_canfd.c new file +6
-0+/* Check for Bus Error interrupt */ +if (isr ®_INT_STAT_BEI ) { + isr =REG_INT_STAT_BEI ; +netdev_err (ndev,"Bus Error interrupt\n" ); + priv->can.can_stats.bus_error++; + stats->rx_errors++;[Severity: Medium]
Is it safe to incrementstats->rx_errorswithout locking here? This counter is modified from hardirq context here and concurrently from softirq context in the NAPI poll routine, which could cause lost increments due to data races on 32-bit systems.[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1391,37 +1391,25 @@ static int loongson_canfd_rx_napi(struct napi_struct *napi, int quota)[ ... ]
drivers/net/can/loongson_canfd.c line 1278
Quoted 10 lines
drivers/net/can/loongson_canfd.c new file +10
-0+/* Check for RX FIFO Overflow */ +regmap_read (priv->regmap,LOONGSON_CANFD_STAT , &sts); +if (sts ®_STAT_DOR ) { +struct net_device_stats *stats = &ndev->stats; +struct can_frame *cf; +struct sk_buff *skb; + +netdev_info (ndev,"Loongson canfd RX overflow\n" ); + stats->rx_over_errors++; + stats->rx_errors++;[Severity: Medium]
As noted above,stats->rx_errorsis updated concurrently in both softirq and hardirq contexts without locking.[ ... ]
drivers/net/can/loongson_canfd.c line 1292
Quoted 2 lines
drivers/net/can/loongson_canfd.c new file +2
-0+if (rxbuf_is_empty && ret !=0 ) { +if (napi_complete_done (napi, work_done)) {[Severity: High]
Should this check ifwork_doneis less than quota? If exactly quota frames are processed and the hardware buffer becomes empty, this logic callsnapi_complete_doneand then returns quota to the networking core. Returning the full quota instructs the core to reschedule the instance, but the state has already been manipulated, which can corrupt the NAPI poll list.[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1425,33 +1425,18 @@ static void loongson_canfd_tx_interrupt(struct net_device *ndev)[ ... ]
drivers/net/can/loongson_canfd.c line 1320
Quoted 6 lines
drivers/net/can/loongson_canfd.c new file +6
-0+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 ;[Severity: High]
Could this result in an inescapable infinite loop? When the driver writes a clear command to the posted MMIO register at the end of the loop, it immediately reads the status register back in the while condition. Because the hardware takes cycles to clear the bit, the old status is read, causing the driver to repeatedly process the same buffer and spin in the ISR.drivers/net/can/loongson_canfd.c line 1322
Quoted 2 lines
drivers/net/can/loongson_canfd.c new file +2
-0+case TX_BS_FAIL : +u32 cnt;[Severity: Low]
Similar to the fault state read function, this violates C11 syntax by declaring a variable immediately following a switch case label.[ ... ]
Quoted 4 lines
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1584,20 +1584,14 @@ static void loongson_canfd_chip_stop(struct net_device *ndev) {struct loongson_canfd_priv *priv =netdev_priv (ndev);
Quoted 10 lines
drivers/net/can/loongson_canfd.c static void loongson_canfd_chip_stop(struct net_device *ndev) · line 1590
Quoted 4 lines
drivers/net/can/loongson_canfd.c new file +0
-0/* Disable interrupts and disable CAN */ regmap_update_bits (priv->regmap,LOONGSON_CANFD_INT_ENA ,REG_INT_ENA_CLR ,REG_INT_ENA_CLR );regmap_update_bits (priv->regmap,LOONGSON_CANFD_INT_MASK ,REG_INT_MASK_SET ,REG_INT_MASK_SET );[Severity: High]
Are theseregmap_update_bitscalls safe for W1S/W1C command registers? This function reads the current 1s from the lower half ofINT_ENAand writes them back along with the clear mask in the upper half, which commands the hardware to simultaneously set and clear the exact same interrupts.
Use regmap_write() directly, as follows:
static void loongson_canfd_disable_interrupts(struct loongson_canfd_priv *priv)
{
regmap_write(priv->regmap, LOONGSON_CANFD_INT_ENA,
FIELD_MAX(REG_INT_ENA_CLR) << 16);
regmap_write(priv->regmap, LOONGSON_CANFD_INT_MASK, FIELD_MAX(REG_INT_MASK_SET));
}
Quoted 4 lines
[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1604,45 +1604,22 @@ static int loongson_canfd_open(struct net_device *ndev)[ ... ]
Quoted 12 lines
drivers/net/can/loongson_canfd.c line 1496
Quoted 6 lines
drivers/net/can/loongson_canfd.c new file +6
-0+err_chip_start: +free_irq (ndev->irq, ndev); +err_irq: +close_candev (ndev); +return ret; +}[Severity: High]
Are we missing anapi_disablecall on these error paths? If the driver fails to open, the candev is closed but NAPI is left enabled. A subsequent attempt to bring the interface up will callnapi_enableon an already enabled instance, triggering aBUG_ONin the core.
Update as follows:
exit_free_irq:
free_irq(ndev->irq, ndev);
exit_napi_disable:
napi_disable(&priv->napi);
close_candev(ndev);
return ret;Quoted 4 lines
[ ... ]
Quoted 1 line
drivers/net/can/loongson_canfd.c new file +0
-0@@ -1690,46 +1690,24 @@ static int loongson_canfd_probe(struct platform_device *pdev)[ ... ]
Quoted 14 lines
drivers/net/can/loongson_canfd.c line 1611
Quoted 7 lines
drivers/net/can/loongson_canfd.c new file +7
-0+ regmap =devm_regmap_init_mmio (dev, base, &loongson_cangfd_regmap); +if (IS_ERR (regmap)) +return PTR_ERR (regmap); + +device_property_read_u32 (dev,"clock-frequency" , &clk_rate); + + irq =platform_get_irq (pdev,0 );[Severity: High]
Should the return value ofdevice_property_read_u32be checked? If the property is missing,clk_rateretains uninitialized stack garbage which is then assigned topriv->can.clock.freq, leading to unpredictable hardware timing.
Update as follows:
ret = device_property_read_u32(dev, "clock-frequency", &clk_rate);
if (ret)
return dev_err_probe(dev, ret, "Missing clock-frequency
property\n");
Signature · 2 lines
Thanks. Binbin
PATCH v3 2/2 can: loongson_canfd: Add RXDMA support
In reply to Binbin ZhouAdd optional DMA support for RX path using the Loongson APB CMC DMA engine. When a DMA channel is successfully requested, the driver:
- Uses DMA cyclic transfers to write incoming CAN frames directly to a coherent DMA buffer
- Replaces RXBNEI (RX buffer not empty interrupt) with DMADI (DMA done interrupt)
- Dynamically switches between DMA and PIO modes based on channel availability
This significantly reduces CPU intervention under high RX load, especially beneficial for CAN FD at higher data rates.
--- drivers/net/can/Kconfig | 2 +- drivers/net/can/loongson_canfd.c | 202 ++++++++++++++++++++++++++++--- 2 files changed, 185 insertions(+), 19 deletions(-)
drivers/net/can/Kconfig +1 -1
@@ -191,7 +191,7 @@ config CAN_KVASER_PCIEFD config CAN_LOONGSON_CANFD tristate "Loongson CAN-FD controller" - depends on HAS_IOMEM + depends on HAS_IOMEM && (LOONGSON2_APB_CMC_DMA || COMPILE_TEST) select REGMAP_MMIO help This is a canfd driver switch for the Loongson platform,
drivers/net/can/loongson_canfd.c +184 -18
327 lines not on this page · raw message
Signature · 1 line
2.52.0
[email protected]
AI reviewRe: [PATCH v3 2/2] can: loongson_canfd: Add RXDMA support
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [High] NAPI polling loop reads incomplete CAN frames ahead of the DMA engine, corrupting the ring buffer read pointer.
- [High] DMA initialization failure breaks PIO fallback (hanging RX) and causes double-free on driver removal.
Pre-existing issues:
- [High] Missing bounds check on CAN frame data length allows a heap buffer overflow.
- [High] SKB allocation failure causes permanent NAPI deadlock and RX stream desynchronization. --
commit 60c5f9dce521c51839cd00b85f3676b903c1d307
Author: Binbin Zhou <[email protected]>
can: loongson_canfd: Add RXDMA support
Add optional DMA support for RX path using the Loongson APB CMC DMA engine. When a DMA channel is successfully requested, the driver:
- Uses DMA cyclic transfers to write incoming CAN frames directly to a coherent DMA buffer
Quoted 4 lines
drivers/net/can/loongson_canfd.c +0
-0
[ ... ]
Quoted 9 lines
drivers/net/can/loongson_canfd.c +9
-0+static void loongson_canfd_rxdma_remove (struct loongson_canfd_priv *priv,struct device *dev) +{ +if (!priv->rx_ch) +return ; + +dmaengine_terminate_sync (priv->rx_ch); +loongson_canfd_rxdma_free (priv, dev); +dma_release_channel (priv->rx_ch); +}
[ ... ]
drivers/net/can/loongson_canfd.c static void loongson_canfd_set_txbuf_cmd(struct net_device *ndev, · line 590
Quoted 11 lines
drivers/net/can/loongson_canfd.c +11
-0+static bool loongson_canfd_get_rxbuf_empty_in_dma (struct loongson_canfd_priv *priv) +{ +struct dma_tx_state state; +enum dma_status status; + + status =dmaengine_tx_status (priv->rx_ch, priv->rx_ch->cookie, &state); +if (status !=DMA_IN_PROGRESS ) +return true; + +return priv->last_res == (state.residue /DMA_SLAVE_BUSWIDTH_4_BYTES ); +}
[Severity: High]
Could this implementation cause the CPU to read incomplete frames ahead of the DMA engine?
The cyclic DMA engine writes to memory word-by-word. As soon as the DMA transfers a single word (like meta0), the residue changes and this function returns false.
Because the NAPI loop executing on the CPU is faster than the DMA engine, it appears loongson_canfd_rx() will unconditionally read the entire frame (including meta1 and data words) before the DMA engine has actually transferred them.
Does this structurally guarantee stream desynchronization and data corruption under load?
[ ... ]
drivers/net/can/loongson_canfd.c static void loongson_canfd_read_rx_frame(struct loongson_canfd_priv *priv, struc · line 1152
Quoted 13 lines
drivers/net/can/loongson_canfd.c +2
-2@@ -999,10 +1143,10 @@ static void loongson_canfd_read_rx_frame(struct loongson_canfd_priv *priv, struc/* Data */ for (i =0 ; i < len; i +=LOONGSON_CANFD_DW_BYTE ) -regmap_read (priv->regmap,LOONGSON_CANFD_RX_DATA , (u32 *)(cf->data + i)); + *(u32 *)(cf->data + i) = priv->get_rx_data (priv);while (unlikely (i < wc *LOONGSON_CANFD_DW_BYTE )) { -regmap_read (priv->regmap,LOONGSON_CANFD_RX_DATA , &data); + priv->get_rx_data (priv); i +=LOONGSON_CANFD_DW_BYTE ; } }
[Severity: High]
This is a pre-existing issue, but is it possible for len to exceed the size of cf->data and cause a heap buffer overflow?
The cf->data buffer is a fixed 64-byte array. len is calculated based on the RWCNT field from meta1, but there appears to be no bounds check verifying that len <= CANFD_MAX_DLEN.
If the hardware provides a malformed RWCNT, or if the CPU reads garbage data as meta1 (due to the DMA race mentioned above), won't this write past the end of the allocated SKB and corrupt the skb_shared_info and adjacent heap memory?
[ ... ]
drivers/net/can/loongson_canfd.c static int loongson_canfd_rx_napi(struct napi_struct *napi, int quota) · line 1437
Quoted 3 lines
drivers/net/can/loongson_canfd.c +0
-0@@ -1291,12 +1436,11 @@ static int loongson_canfd_rx_napi(struct napi_struct *napi, int quota)if (rxbuf_is_empty && ret !=0 ) {if (napi_complete_done (napi, work_done)) {
[Severity: High]
This isn't a bug introduced by this patch, but what happens if SKB allocation fails inside loongson_canfd_rx()?
Looking at loongson_canfd_rx(), if alloc_can_skb() or alloc_canfd_skb() fails due to memory exhaustion, it returns 0 immediately without consuming the rest of the frame's words:
if (unlikely(!skb)) return 0;
This seems to cause two separate issues:
1. It desynchronizes the data stream, as subsequent polls will misinterpret remaining payload words as the headers of the next frame.
2. It breaks the NAPI loop because ret == 0 causes this completion condition (rxbuf_is_empty && ret != 0) to evaluate to false.
By skipping napi_complete_done() and returning a value less than the quota, the NAPI state bit NAPIF_STATE_SCHED remains permanently set but removed from the poll list. Doesn't this permanently deadlock the receiver until a module reload?
[ ... ]
Quoted 13 lines
drivers/net/can/loongson_canfd.c +9
-0@@ -1646,6 +1791,24 @@ static int loongson_canfd_probe(struct platform_device *pdev) ndev->ethtool_ops = &loongson_canfd_ethtool_ops;SET_NETDEV_DEV (ndev, dev); + priv->get_rx_data = loongson_canfd_get_rxdata_in_poll; + priv->get_rxbuf_empty = loongson_canfd_get_rxbuf_empty_in_poll; + + priv->rx_ch =dma_request_chan (dev,"rx" ); +if (PTR_ERR (priv->rx_ch) == -EPROBE_DEFER ) +return -EPROBE_DEFER ; + +if (IS_ERR (priv->rx_ch)) { +dev_warn (dev,"Fall back in poll mode for any non-deferral error.\n" );
drivers/net/can/loongson_canfd.c static int loongson_canfd_probe(struct platform_device *pdev) · line 1812
Quoted 10 lines
drivers/net/can/loongson_canfd.c +9
-0+ priv->rx_ch =NULL ; + } + + ret =loongson_canfd_rxdma_init (ndev); +if (!ret) { + priv->get_rx_data = loongson_canfd_get_rxdata_in_dma; + priv->get_rxbuf_empty = loongson_canfd_get_rxbuf_empty_in_dma; + priv->last_res =LOONGSON_CANFD_RXDMA_NUM ; + }netif_napi_add (ndev, &priv->napi, loongson_canfd_rx_napi);
[Severity: High]
What happens if loongson_canfd_rxdma_init() fails?
If loongson_canfd_rxdma_init() returns an error (e.g., dma_alloc_coherent fails), it frees the buffer but does not clear priv->rx_ch or release the channel. The probe function ignores the error and proceeds with the intent to fall back to PIO.
However, since priv->rx_ch remains non-NULL, loongson_canfd_chip_start() will incorrectly configure the controller to use the DMADI interrupt instead of the RBNEI PIO interrupt, permanently breaking reception.
Additionally, upon module removal, loongson_canfd_rxdma_remove() checks priv->rx_ch and calls loongson_canfd_rxdma_free() again. Since the buffer was already freed during the failed init, won't this trigger a double-free?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2