netdev - Re: [PATCH v2 2/2] can: m_can: fix missed interrupts with m_can

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: <20240923-hot-overjoyed-stork-12e138-mkl@pengutronix.de>
Date: Mon, 23 Sep 2024 12:53:50 +0200
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Markus Schneider-Pargmann <msp@...libre.com>
Cc: Matthias Schiffer <matthias.schiffer@...tq-group.com>, 
	Chandrasekar Ramakrishnan <rcsekar@...sung.com>, Vincent Mailhol <mailhol.vincent@...adoo.fr>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Martin Hundebøll <martin@...nix.com>, "Felipe Balbi (Intel)" <balbi@...nel.org>, 
	Raymond Tan <raymond.tan@...el.com>, Jarkko Nikula <jarkko.nikula@...ux.intel.com>, 
	linux-can@...r.kernel.org, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux@...tq-group.com
Subject: Re: [PATCH v2 2/2] can: m_can: fix missed interrupts with m_can_pci

On 23.09.2024 12:07:33, Markus Schneider-Pargmann wrote:
> > > > diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> > > > index 92b2bd8628e6b..8c17eb94d2f98 100644
> > > > --- a/drivers/net/can/m_can/m_can.h
> > > > +++ b/drivers/net/can/m_can/m_can.h
> > > > @@ -99,6 +99,7 @@ struct m_can_classdev {
> > > >  	int pm_clock_support;
> > > >  	int pm_wake_source;
> > > >  	int is_peripheral;
> > > > +	bool is_edge_triggered;
> > > 
> > > To avoid confusion could you rename it to irq_edge_triggered or
> > > something similar, to make clear that it is not about the chip but the
> > > way the interrupt line is connected?
> > 
> > Will do.
> > 
> > > 
> > > Also I am not sure it is possible, but could you use
> > > irq_get_trigger_type() to see if it is a level or edge based interrupt?
> > > Then we wouldn't need this additional parameter at all and could just
> > > detect it in m_can.c.
> > 
> > Unfortunately that doesn't seem to work. irq_get_trigger_type() only returns a meaningful value
> > after the IRQ has been requested. I thought about requesting the IRQ with IRQF_NO_AUTOEN and then
> > filling in the irq_edge_triggered field before enabling the IRQ, but IRQF_NO_AUTOEN is incompatible
> > with IRQF_SHARED.
> 
> The mentioned function works for me on ARM and DT even before
> irq_request_threaded_irq() was called.
> 
> Also I am probably missing something here. Afer requesting the irq, the
> interrupts are not enabled yet right? So can't you just request it and
> check the triggertype immediately afterwards?

You have to distinguish between the IRQ controller and the IRQ source in
your device. You must be able to handle IRQs right after request_irq().
If you IP core is in reset or has all IRQs masked, this will probably
not happen, but with shared IRQ one of the other IRQ sources on the IRQ
can trigger and your IRQ handler will be called. See
CONFIG_DEBUG_SHIRQ_FIXME and CONFIG_DEBUG_SHIRQ_FIXME.

> > Of course there are ways around this - checking irq_get_trigger_type() from the ISR itself, or
> > adding more locking, but neither seems quite worthwhile to me. Would you agree with this?
> > 
> > Maybe there is some other way to find out the trigger type that would be set when the IRQ is
> > requested? I don't know what that would be however - so I'd just keep setting the flag statically
> > for m_can_pci and leave a dynamic solution for future improvement.
> 
> No if it doesn't work easily the parameter is probably the best option.

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