linux-kernel - [PATCH] can: m_can: Enable NAPI before enabling interrupts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <CO6PR14MB5217EC2D2C791F527CEB40C3EF9E2@CO6PR14MB5217.namprd14.prod.outlook.com>
Date: Fri, 6 Sep 2024 21:02:25 +0000
From: "Hamby, Jake (US)" <Jake.Hamby@...edyne.com>
To: "linux-can@...r.kernel.org" <linux-can@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.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

Teledyne Confidential; Commercially Sensitive Business Data