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

> -----Original Message-----
> From: Hamby, Jake (US)
> Sent: Friday, September 6, 2024 4:20 PM
> To: linux-can@vger.kernel.org; linux-kernel@vger.kernel.org
> 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@Teledyne.com>

On 06.09.2024 23:19:51, Hamby, Jake (US) wrote:
> 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.

Thank you very much for your contribution! I believe you have solved the
problem I was investigating last Friday. I still need to test it with
your solution though.

This patch does not contain your Signed-off-by [1]. Add it to your next
patches; "git commit -s" will add it automatically. For this patch, you
can reply and include it in your email!

[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

> 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.

My apologies for the duplicate post. I had to change Outlook from Windows to UTF-8 encoding and then I wasn't patient enough to wait to see if that fixed my failure to post.

Best Regards,
Jake Hamby