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] [day] [month] [year] [list]

Message-Id: <DCP8XXYDW0EI.2QLN0NYPOWXVJ@baylibre.com>
Date: Wed, 10 Sep 2025 18:04:38 +0200
From: "Markus Schneider-Pargmann" <msp@...libre.com>
To: "Marc Kleine-Budde" <mkl@...gutronix.de>
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 Wed Sep 10, 2025 at 5:06 PM CEST, Marc Kleine-Budde wrote:
> 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.

The interrupts are acked just before calling m_can_coalescing_update().
I think ideally the unwanted interrupts should be disabled quick,
especially if there are SPI transfers for handling packets which take
some time.

Another point is to refresh the timer consistently so it doesn't trigger
because we are late refreshing it. The runtime of the interrupt handler
depends on the amount of work in the fifo.

Best
Markus

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