linux-kernel - Re: [PATCH v2 2/7] can: m_can: only handle active 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250910-fair-fast-uakari-4f734e-mkl@pengutronix.de>
Date: Wed, 10 Sep 2025 16:28:54 +0200
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Markus Schneider-Pargmann <msp@...libre.com>
Cc: Chandrasekar Ramakrishnan <rcsekar@...sung.com>, 
	Vincent Mailhol <mailhol.vincent@...adoo.fr>, Patrik Flykt <patrik.flykt@...ux.intel.com>, 
	Dong Aisheng <b29396@...escale.com>, Varka Bhadram <varkabhadram@...il.com>, 
	Wu Bo <wubo.oduw@...il.com>, Philipp Zabel <p.zabel@...gutronix.de>, 
	linux-can@...r.kernel.org, linux-kernel@...r.kernel.org, kernel@...gutronix.de
Subject: Re: [PATCH v2 2/7] can: m_can: only handle active interrupts

On 10.09.2025 10:41:28, Markus Schneider-Pargmann wrote:
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index fe74dbd2c966..16b38e6c3985 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -1057,6 +1057,7 @@ static int m_can_poll(struct napi_struct *napi, int quota)
> >  	u32 irqstatus;
> >  
> >  	irqstatus = cdev->irqstatus | m_can_read(cdev, M_CAN_IR);
> > +	irqstatus &= cdev->active_interrupts;
> >  
> >  	work_done = m_can_rx_handler(dev, quota, irqstatus);
> >  
> > @@ -1243,6 +1244,8 @@ static int m_can_interrupt_handler(struct m_can_classdev *cdev)
> >  	}
> >  
> >  	m_can_coalescing_update(cdev, ir);
> > +
> > +	ir &= cdev->active_interrupts;
> 
> m_can_coalescing_update() can change active_interrupts, meaning the
> interrupt that caused the interrupt handler to run may be disabled in
> active_interrupts above and then masked in this added line. Would that
> still work or does it confuse the hardware?

I think m_can_coalescing_update() expects the RX/TX will be cleared. Are
the following comments OK...

| diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
| index 16b38e6c3985..8cb9cc1cddbf 100644
| --- a/drivers/net/can/m_can/m_can.c
| +++ b/drivers/net/can/m_can/m_can.c
| @@ -1188,28 +1188,39 @@ static int m_can_echo_tx_event(struct net_device *dev)
|  
|  static void m_can_coalescing_update(struct m_can_classdev *cdev, u32 ir)
|  {
|          u32 new_interrupts = cdev->active_interrupts;
|          bool enable_rx_timer = false;
|          bool enable_tx_timer = false;
|  
|          if (!cdev->net->irq)
|                  return;
|  
| +        /* If there is a packet in the FIFO then:
| +         * - start timer
| +         * - disable not empty IRQ
| +         * - handle FIFO
                ^^^^^^^^^^^

...especially this one?

| +         */
|          if (cdev->rx_coalesce_usecs_irq > 0 && (ir & (IR_RF0N | IR_RF0W))) {
|                  enable_rx_timer = true;
|                  new_interrupts &= ~IR_RF0N;
|          }
|          if (cdev->tx_coalesce_usecs_irq > 0 && (ir & (IR_TEFN | IR_TEFW))) {
|                  enable_tx_timer = true;
|                  new_interrupts &= ~IR_TEFN;
|          }
| +
| +        /* If:
| +         * - timer is not going to be start
| +         * - and timer is not active
| +         * -> then enable FIFO empty IRQ
| +         */
|          if (!enable_rx_timer && !hrtimer_active(&cdev->hrtimer))
|                  new_interrupts |= IR_RF0N;
|          if (!enable_tx_timer && !hrtimer_active(&cdev->hrtimer))
|                  new_interrupts |= IR_TEFN;
|  
|          m_can_interrupt_enable(cdev, new_interrupts);
|          if (enable_rx_timer | enable_tx_timer)
|                  hrtimer_start(&cdev->hrtimer, cdev->irq_timer_wait,
|                                HRTIMER_MODE_REL);
|  }

Currently the m_can_coalescing_update() is called at the beginning of
the IRQ handler. Does it make sense to move it to the end and pass the
unmasked M_CAN_IR?

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   |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)