> -----Original Message-----
> From: Hamby, Jake (US)
> Sent: Friday, September 6, 2024 4:20 PM
> To: linux-can@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: [PATCH] can: m_can: Enable NAPI before enabling interrupts
>
> If any error flags are set when bringing up the CAN device, e.g. due to
> CAN bus traffic before initializing the device, when m_can_start is
> called and interrupts are enabled, m_can_isr is called immediately,
> which disables all CAN interrupts and calls napi_schedule.
>
> Because napi_enable isn't called until later in m_can_open, the call to
> napi_schedule never schedules the m_can_poll callback and the device is
> left with interrupts disabled and can't receive any CAN packets until
> rebooted. This can be verified by running "cansend" from another device
> before setting the bitrate and calling "ip link set up can0" on the test
> device. Adding debug lines to m_can_isr shows it's called with flags
> (IR_EP | IR_EW | IR_CRCE), which calls m_can_disable_all_interrupts and
> napi_schedule, and then m_can_poll is never called.
>
> Move the call to napi_enable above the call to m_can_start to enable any
> initial interrupt flags to be handled by m_can_poll so that interrupts
> are reenabled. Add a call to napi_disable in the error handling section
> of m_can_open, to handle the case where later functions return errors.
>
> Also, in m_can_close, move the call to napi_disable below the call to
> m_can_stop to ensure all interrupts are handled when bringing down the
> device. This race condition is much less likely to occur.
>
> While testing, I noticed that IR_TSW (timestamp wraparound) fires at
> about 1 Hz, but the driver doesn't care about it. Add it to the list of
> interrupts to disable in m_can_chip_config to reduce unneeded wakeups.
>
> Tested on a Microchip SAMA7G54 MPU. The fix should be applicable to any
> SoC with a Bosch M_CAN controller.
> ---
> drivers/net/can/m_can/m_can.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c
> b/drivers/net/can/m_can/m_can.c
> index 012c3d22b01d..4ced830f5ece 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1434,7 +1434,8 @@ static int m_can_chip_config(struct net_device
> *dev)
>
> /* Disable unused interrupts */
> interrupts &= ~(IR_ARA | IR_ELO | IR_DRX | IR_TEFF | IR_TFE | IR_TCF
> |
> - IR_HPM | IR_RF1F | IR_RF1W | IR_RF1N | IR_RF0F);
> + IR_HPM | IR_RF1F | IR_RF1W | IR_RF1N | IR_RF0F |
> + IR_TSW);
>
> err = m_can_config_enable(cdev);
> if (err)
> @@ -1763,13 +1764,14 @@ static int m_can_close(struct net_device *dev)
>
> netif_stop_queue(dev);
>
> - if (!cdev->is_peripheral)
> - napi_disable(&cdev->napi);
> -
> m_can_stop(dev);
> m_can_clk_stop(cdev);
> free_irq(dev->irq, dev);
>
> + /* disable NAPI after disabling interrupts */
> + if (!cdev->is_peripheral)
> + napi_disable(&cdev->napi);
> +
> m_can_clean(dev);
>
> if (cdev->is_peripheral) {
> @@ -2031,6 +2033,10 @@ static int m_can_open(struct net_device *dev)
> if (cdev->is_peripheral)
> can_rx_offload_enable(&cdev->offload);
>
> + /* enable NAPI before enabling interrupts */
> + if (!cdev->is_peripheral)
> + napi_enable(&cdev->napi);
> +
> /* register interrupt handler */
> if (cdev->is_peripheral) {
> cdev->tx_wq = alloc_ordered_workqueue("mcan_wq",
> @@ -2063,9 +2069,6 @@ static int m_can_open(struct net_device *dev)
> if (err)
> goto exit_start_fail;
>
> - if (!cdev->is_peripheral)
> - napi_enable(&cdev->napi);
> -
> netif_start_queue(dev);
>
> return 0;
> @@ -2077,6 +2080,8 @@ static int m_can_open(struct net_device *dev)
> if (cdev->is_peripheral)
> destroy_workqueue(cdev->tx_wq);
> out_wq_fail:
> + if (!cdev->is_peripheral)
> + napi_disable(&cdev->napi);
> if (cdev->is_peripheral)
> can_rx_offload_disable(&cdev->offload);
> close_candev(dev);
> --
> 2.34.1
>
> Best Regards,
> Jake Hamby
Signed-off-by: "Jake Hamby" <Jake.Hamby@xxxxxxxxxxxx>
