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.
