linux-kernel - RE: [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>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <DM8PR14MB522141E1B64C5AAEA8239D18EF9A2@DM8PR14MB5221.namprd14.prod.outlook.com>
Date: Tue, 10 Sep 2024 16:59:48 +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: RE: [PATCH] can: m_can: Enable NAPI before enabling interrupts

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

Signed-off-by: "Jake Hamby" <Jake.Hamby@...edyne.com>