Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <c46ddb954cfe45d9849c911271d7ec23@hisilicon.com>
 <CAK8P3a2adJsz5hRT_eMzSoHnUBC+aK9HZ18=oAYCZ-gisEkd1w@mail.gmail.com>
 <24e0652b3afa48cdbf7c83287e43c087@hisilicon.com> <CAK8P3a0fwMe9LaXMfKjH46yvt6o-euZJZ4HXtVRPhYbKvAUPKg@mail.gmail.com>
 <0b766dba0b004ced94131e158cd8e67d@hisilicon.com> <CAK8P3a2ZnKeeZ-zEWO+vHogs0DdLuDrZet61cSmJe_UMYhtaWQ@mail.gmail.com>
 <5148eb2aaceb42d78087bc6d8ce15183@hisilicon.com> <5fcea94e-6fc9-c340-d7d2-4ae8b69890b8@telegraphics.com.au>
 <0c0ea8eca77c45ea89f2d4432580211c@hisilicon.com> <28d4b91d-1774-a8a-df97-7ac9b365c2@telegraphics.com.au>
In-Reply-To: <28d4b91d-1774-a8a-df97-7ac9b365c2@telegraphics.com.au>
From:   Arnd Bergmann <arnd@kernel.org>
Date:   Thu, 18 Feb 2021 12:19:43 +0100
Message-ID: <CAK8P3a0VquJPxvS8B=2kLQ5y=h5BftJDR7WJYmj3hgQ8yQY5=Q@mail.gmail.com>
Subject: Re: [RFC] IRQ handlers run with some high-priority interrupts(not
 NMI) enabled on some platform
To:     Finn Thain <fthain@telegraphics.com.au>
Cc:     "Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "arnd@arndb.de" <arnd@arndb.de>,
        "geert@linux-m68k.org" <geert@linux-m68k.org>,
        "funaho@jurai.org" <funaho@jurai.org>,
        "philb@gnu.org" <philb@gnu.org>, "corbet@lwn.net" <corbet@lwn.net>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "linux-m68k@lists.linux-m68k.org" <linux-m68k@lists.linux-m68k.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Feb 18, 2021 at 6:30 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> On Wed, 17 Feb 2021, Song Bao Hua (Barry Song) wrote:
> > > On Sat, 13 Feb 2021, Song Bao Hua (Barry Song) wrote:
> > >
> > > 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.
> >
> > Basically, we wrote drivers with the assumption that this driver will be
> > cross-platform. (Of course there are some drivers which can only work on
> > one platform, for example, if the IP of the device is only used in one
> > platform as an internal component of a specific SoC.)
> >
> > So once a device has two or more interrupts, we need to consider one
> > interrupt might preempt another one on m68k on the same cpu if we also
> > want to support this driver on m68k. this usually doesn't matter on
> > other platforms.
>
> When users show up who desire to run your drivers on their platform, you
> can expect them to bring patches and a MAINTAINERS file entry. AFAIK,
> Linux development has always worked that way.

This is only part of the picture though. We also also constantly trying to
generalize the internal interfaces, to make sure that platforms work the same
way and that it's possible to write drivers in a portable way without having
to rely on platform maintainers to point out the differences.

I think it would make a lot of sense to remove the architecture differences
here by making m68k work the same way as the others and documenting
that as the expected behavior.

You are probably right that there are no specific bugs on m68k machines
that rely on the nested hardirqs today, but I think they only get away with it
because

a) there is no SMP support on m68k, so it likely doesn't run into the more
    subtle cases with lock ordering that you could get when you have
    hardirq handlers on multiple CPUs in parallel

b) there is a very limited number of device drivers that are actually used
    on m68k, in particular only M54xx has PCI support, but that in turn has
    a different interrupt scheme.

Changing the behavior on m68k clearly has its own regression risk, but
it could be done as a configuration option that defaults to the traditional
behavior on machines that have not been verified to be well-behaved
without nested hardirqs, and hidden on machines that do not need it
(any more).

As far as I can tell, the only reason you would actually need nested
hardirqs is when a low-priority interrupt has to perform expensive
I/O processing, but we've had countless other methods to do the
same over the years (at least bottom half, softirq, taskqueue, tasklet,
keventd, workqueue, kthread, threaded interrupt handlers and
probably others).

It would probably not be too hard to go through all drivers that are normally
enabled and see if their interrupt handler should be changed to use
deferred processing (threaded irq handler or workqueue) instead of
doing expensive I/O in hardirq context and relying on priorities.

FWIW, these are the drivers in m68k multi_defconfig that register an
interrupt:

$ find build/tmp/{drivers,sound} -name \*.o | xargs grep request_.*irq
| cut -f 3- -d/ | cut -f 1 -d\  | sed -e "s:o$:c:g"
drivers/block/amiflop.c
drivers/ide/ide-probe.c
drivers/input/joystick/amijoy.c
drivers/input/keyboard/amikbd.c
drivers/input/keyboard/hilkbd.c
drivers/input/mouse/amimouse.c
drivers/input/serio/hp_sdc.c
drivers/input/serio/q40kbd.c
drivers/macintosh/via-cuda.c
drivers/macintosh/via-macii.c
drivers/macintosh/via-pmu.c
drivers/misc/dummy-irq.c
drivers/net/ethernet/8390/apne.c
drivers/net/ethernet/8390/ax88796.c
drivers/net/ethernet/8390/hydra.c
drivers/net/ethernet/8390/mac8390.c
drivers/net/ethernet/8390/ne.c
drivers/net/ethernet/8390/zorro8390.c
drivers/net/ethernet/amd/7990.c
drivers/net/ethernet/amd/a2065.c
drivers/net/ethernet/amd/ariadne.c
drivers/net/ethernet/amd/atarilance.c
drivers/net/ethernet/amd/sun3lance.c
drivers/net/ethernet/apple/macmace.c
drivers/net/ethernet/cirrus/mac89x0.c
drivers/net/ethernet/i825xx/82596.c
drivers/net/ethernet/natsemi/macsonic.c
drivers/net/ethernet/smsc/smc91x.c
drivers/net/phy/phy.c
drivers/parport/parport_amiga.c
drivers/parport/parport_atari.c
drivers/parport/parport_mfc3.c
drivers/parport/parport_pc.c
drivers/scsi/a2091.c
drivers/scsi/a3000.c
drivers/scsi/a4000t.c
drivers/scsi/atari_scsi.c
drivers/scsi/bvme6000_scsi.c
drivers/scsi/gvp11.c
drivers/scsi/mac_esp.c
drivers/scsi/mac_scsi.c
drivers/scsi/mvme147.c
drivers/scsi/mvme16x_scsi.c
drivers/scsi/sun3x_esp.c
drivers/scsi/zorro7xx.c
drivers/scsi/zorro_esp.c
drivers/tty/amiserial.c
drivers/tty/serial/pmac_zilog.c
drivers/video/fbdev/amifb.c
drivers/video/fbdev/atafb.c
sound/oss/dmasound/dmasound_atari.c
sound/oss/dmasound/dmasound_paula.c
sound/oss/dmasound/dmasound_q40.c

Most of these are normal short-lived interrupts that only transfer
a few bytes or schedule deferred processing of some sort.
Most of the scsi and network drivers process the data in
a softirq, so those are generally fine here, but I do see that 8390
(ne2000) ethernet and the drivers/ide drivers do transfer their
data in hardirq context.

        Arnd