net: can: mpfs_can: add Polarfire SoC MSS CAN
Subsystems (2)
PATCH 1/1 net: can: mpfs_can: add Polarfire SoC MSS CAN
From: Naga Sureshkumar Relli <[email protected]>
The Microchip Polarfire SoC has two instances of CAN devices inside the MSS block. Add driver to support this MSS CAN.
---
Hey Marc,
I've finally got around to sending this driver.
Unfortunately the author left microchip (which is part of why sending it took so long). I've done some mechanical cleanup, but left it mostly functionally unchanged mainly because I lack a setup to test it thoroughly were I to do so.
I'm hoping that what's here is by and large okay on that front, and maybe that sasiko will point some stuff out if there's stuff I missed. I did remove/modify some code, so hopefully I caught all the obviously problematic stuff.
The IP that we are using seems to lack an internal reset, so the reset for the whole peripheral is used instead, and I wonder if it is overeager? Doing ip link set xxx up causes a double reset, for example.
There's no binding of course, because that's been upstream for quite a while!
--- MAINTAINERS | 1 + drivers/net/can/Kconfig | 9 + drivers/net/can/Makefile | 1 + drivers/net/can/mpfs_can.c | 882 +++++++++++++++++++++++++++++++++++++ 4 files changed, 893 insertions(+) create mode 100644 drivers/net/can/mpfs_can.c
MAINTAINERS +1 -0
@@ -23258,6 +23258,7 @@ F: drivers/firmware/microchip/mpfs-auto-update.c F: drivers/gpio/gpio-mpfs.c F: drivers/i2c/busses/i2c-microchip-corei2c.c F: drivers/mailbox/mailbox-mpfs.c +F: drivers/net/can/mpfs_can.c F: drivers/pci/controller/plda/pcie-microchip-host.c F: drivers/pinctrl/microchip/pinctrl-mpfs-iomux0.c F: drivers/pinctrl/microchip/pinctrl-mpfs-mssio.c
drivers/net/can/Kconfig +9 -0
@@ -189,6 +189,15 @@ config CAN_KVASER_PCIEFD Kvaser M.2 PCIe 4xCAN Kvaser PCIe 8xCAN +config CAN_POLARFIRE_SOC + tristate "Microchip Polarfire SoC CAN" + depends on ARCH_MICROCHIP || COMPILE_TEST + help + Driver for the CAN controllers on Microchip Polarfire SoC. + + This driver can be built as a module. If so, the module will be + called mpfs_can. + config CAN_SLCAN tristate "Serial / USB serial CAN Adaptors (slcan)" depends on TTY
drivers/net/can/Makefile +1 -0
@@ -30,6 +30,7 @@ obj-$(CONFIG_CAN_KVASER_PCIEFD) += kvaser_pciefd/ obj-$(CONFIG_CAN_MSCAN) += mscan/ obj-$(CONFIG_CAN_M_CAN) += m_can/ obj-$(CONFIG_CAN_PEAK_PCIEFD) += peak_canfd/ +obj-$(CONFIG_CAN_POLARFIRE_SOC) += mpfs_can.o obj-$(CONFIG_CAN_SJA1000) += sja1000/ obj-$(CONFIG_CAN_SUN4I) += sun4i_can.o obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o
drivers/net/can/mpfs_can.c new file +882 -0
882 lines not on this page · raw message
Signature · 1 line
2.53.0
[email protected]
AI reviewThank 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…
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 <[email protected]>
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.
Quoted 6 lines
drivers/net/can/mpfs_can.c new file +0
-0@@ -0,0 +1,882 @@
[ ... ]
drivers/net/can/mpfs_can.c line 182
Quoted 8 lines
drivers/net/can/mpfs_can.c new file +8
-0+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.
Quoted 9 lines
drivers/net/can/mpfs_can.c new file +9
-0+ .brp_max = 32767, + .brp_inc = 1, +}; + +static int mpfs_can_reset (struct net_device *ndev) +{ +struct mpfs_can_priv *priv =netdev_priv (ndev); +int ret; +
drivers/net/can/mpfs_can.c line 201
Quoted 10 lines
drivers/net/can/mpfs_can.c new file +10
-0+/* 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.
Quoted 3 lines
drivers/net/can/mpfs_can.c new file +3
-0+ +return 0 ; +}
[ ... ]
Quoted 9 lines
drivers/net/can/mpfs_can.c new file +9
-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()?
drivers/net/can/mpfs_can.c line 251
Quoted 10 lines
drivers/net/can/mpfs_can.c new file +10
-0+ 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.
Quoted 3 lines
drivers/net/can/mpfs_can.c new file +3
-0+ +/* Enable interrupts */ +writel (MPFS_CAN_IER_MASK , priv->base +MPFS_CAN_IER_OFFSET );
[ ... ]
Quoted 13 lines
drivers/net/can/mpfs_can.c new file +13
-0+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);
drivers/net/can/mpfs_can.c line 402
Quoted 10 lines
drivers/net/can/mpfs_can.c new file +10
-0+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.
Quoted 10 lines
drivers/net/can/mpfs_can.c new file +10
-0+ +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;
drivers/net/can/mpfs_can.c line 422
Quoted 10 lines
drivers/net/can/mpfs_can.c new file +10
-0+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.
Quoted 5 lines
drivers/net/can/mpfs_can.c new file +5
-0+ + id =readl (priv->base +MPFS_CAN_RX_MSG_ID_OFFSET (buf_off)); + ctrl =readl (priv->base +MPFS_CAN_RX_MSG_CTR_OFFSET (buf_off)); + +if (ctrl &MPFS_CAN_RXMSG_CTRL_CMD_IDE_MASK ) {
drivers/net/can/mpfs_can.c line 437
Quoted 10 lines
drivers/net/can/mpfs_can.c new file +10
-0+ 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.
Quoted 3 lines
drivers/net/can/mpfs_can.c new file +3
-0+ + data[0 ] =readl (priv->base +MPFS_CAN_RX_DATAL_OFFSET (buf_off)); + data[1 ] =readl (priv->base +MPFS_CAN_RX_DATAH_OFFSET (buf_off));
[ ... ]
Quoted 6 lines
drivers/net/can/mpfs_can.c new file +6
-0+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;
drivers/net/can/mpfs_can.c line 496
Quoted 10 lines
drivers/net/can/mpfs_can.c new file +10
-0+ + 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.
Quoted 11 lines
drivers/net/can/mpfs_can.c new file +11
-0+ 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; + } + } +}
[ ... ]
Quoted 8 lines
drivers/net/can/mpfs_can.c new file +8
-0+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 );
[ ... ]
Quoted 7 lines
drivers/net/can/mpfs_can.c new file +7
-0+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 ; + } + }
drivers/net/can/mpfs_can.c line 616
Quoted 10 lines
drivers/net/can/mpfs_can.c new file +10
-0+ +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.
Quoted 14 lines
drivers/net/can/mpfs_can.c new file +14
-0+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; +
drivers/net/can/mpfs_can.c line 640
Quoted 10 lines
drivers/net/can/mpfs_can.c new file +10
-0+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?
Quoted 10 lines
drivers/net/can/mpfs_can.c new file +10
-0+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);
drivers/net/can/mpfs_can.c line 660
Quoted 10 lines
drivers/net/can/mpfs_can.c new file +10
-0+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.
Quoted 9 lines
drivers/net/can/mpfs_can.c new file +9
-0+ 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); +}
[ ... ]
drivers/net/can/mpfs_can.c line 719
Quoted 7 lines
drivers/net/can/mpfs_can.c new file +7
-0+static int mpfs_can_open (struct net_device *ndev) +{ +struct mpfs_can_priv *priv =netdev_priv (ndev); +int ret; + + ret =request_irq (ndev->irq, mpfs_can_interrupt, priv->irq_flags, + ndev->name, ndev);
[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.
Quoted 23 lines
drivers/net/can/mpfs_can.c new file +23
-0+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 ;
[ ... ]
Quoted 19 lines
drivers/net/can/mpfs_can.c new file +19
-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 ; +
drivers/net/can/mpfs_can.c line 813
Quoted 10 lines
drivers/net/can/mpfs_can.c new file +10
-0+ 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.
drivers/net/can/mpfs_can.c line 820
Quoted 7 lines
drivers/net/can/mpfs_can.c new file +7
-0+ 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.
drivers/net/can/mpfs_can.c line 828
Quoted 8 lines
drivers/net/can/mpfs_can.c new file +8
-0+ + 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?
Quoted 21 lines
drivers/net/can/mpfs_can.c new file +21
-0+ +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;
drivers/net/can/mpfs_can.c line 859
Quoted 10 lines
drivers/net/can/mpfs_can.c new file +10
-0+} + +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?
Quoted 1 line
+}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-lens-constrict-6dc7d287c647@spud?part=1
Quoted 12 lines · Conor Dooley
From: Naga Sureshkumar Relli <[email protected]>
The Microchip Polarfire SoC has two instances of CAN devices inside the MSS block. Add driver to support this MSS CAN.
Signed-off-by Naga Sureshkumar Relli <[email protected]>Signed-off-by Conor Dooley <[email protected]>---
Hey Marc,
I've finally got around to sending this driver.
Quoted 8 lines
Unfortunately the author left microchip (which is part of why sending it took so long). I've done some mechanical cleanup, but left it mostly functionally unchanged mainly because I lack a setup to test it thoroughly were I to do so.
I'm hoping that what's here is by and large okay on that front, and maybe that sasiko will point some stuff out if there's stuff I missed. I did remove/modify some code, so hopefully I caught all the obviously problematic stuff.
Apparently there's a lot that I did not catch, given there's a wad of complaints from sashiko. Certainly feel free to ignore this version until I resolve the dozen or so things that it has raised.
Thanks,
Conor.
Quoted 7 lines
The IP that we are using seems to lack an internal reset, so the reset for the whole peripheral is used instead, and I wonder if it is overeager? Doing ip link set xxx up causes a double reset, for example.
There's no binding of course, because that's been upstream for quite a while!