Re: [RFC] IRQ handlers run with some high-priority interrupts(not NMI) enabled on some platform

From: Finn Thain
Date: Mon Feb 15 2021 - 17:23:24 EST


On Mon, 15 Feb 2021, Andy Shevchenko wrote:

> On Sun, Feb 14, 2021 at 7:12 AM Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx> wrote:
> > On Sat, 13 Feb 2021, Song Bao Hua (Barry Song) wrote:
> > > So what is really confusing and a pain to me is that:
> > > For years people like me have been writing device drivers
> > > with the idea that irq handlers run with interrupts
> > > disabled after those commits in genirq. So I don't need
> > > to care about if some other IRQs on the same cpu will
> > > jump out to access the data the current IRQ handler
> > > is accessing.
> > >
> > > but it turns out the assumption is not true on some platform.
> > > So should I start to program devices driver with the new idea
> > > interrupts can actually come while irqhandler is running?
> > >
> > > That's the question which really bothers me.
> > >
> >
> > That scenario seems a little contrived to me (drivers for two or more
> > devices sharing state through their interrupt handlers). Is it real?
> > I suppose every platform has its quirks. The irq lock in sonic_interrupt()
> > is only there because of a platform quirk (the same device can trigger
> > either of two IRQs). Anyway, no-one expects all drivers to work on all
> > platforms; I don't know why it bothers you so much when platforms differ.
>
> Isn't it any IRQ chip driver which may get IRQ on one or more lines
> simultaneously?
> The question here is can the handler be re-entrant on the same CPU in
> that case?
>

An irq_chip handler used only for interrupts having the same priority
level cannot be re-entered, thanks to the priority mask.

Where the priority levels are different, as in auto_irq_chip in
arch/m68k/kernel/ints.c, handle_simple_irq() can be re-entered but not
with the same descriptor (i.e. no shared state).

Documentation/core-api/genericirq.rst says,

The locking of chip registers is up to the architecture that defines
the chip primitives. The per-irq structure is protected via desc->lock,
by the generic layer.

Since the synchronization of chip registers is left up to the
architecture, I think the related code should be portable.

Looking in kernel/irq/*.c, I see that desc->lock is sometimes acquired
with raw_spin_lock_irq(&desc->lock) and sometimes
raw_spin_lock(&desc->lock).

I don't know whether there are portability issues lurking here; this code
is subtle and largely unfamiliar to me.