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

[Finn Thain]

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.