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-strange-hopeful-chamois-6c4b6f-mkl@pengutronix.de>
Date: Wed, 10 Sep 2025 17:06:49 +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 16:28:54, Marc Kleine-Budde wrote:
> 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);
> |  }

I can't reproduce the problem I had before. I will drop this patch for
now.

In an upcoming series, however, I would still like to move
can_coalescing_update() to the end of the IRQ handler.

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