linux-kernel - Re: [PATCH] ARM: mmp: replace NO

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]

Date:   Tue, 6 Sep 2016 22:22:06 +0100
From:   Russell King - ARM Linux <linux@...linux.org.uk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Arnd Bergmann <arnd@...db.de>, Eric Miao <eric.y.miao@...il.com>,
        Haojian Zhuang <haojian.zhuang@...il.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ARM: mmp: replace NO_IRQ

On Tue, Sep 06, 2016 at 01:03:42PM -0700, Linus Torvalds wrote:
> On Tue, Sep 6, 2016 at 12:44 PM, Russell King - ARM Linux
> <linux@...linux.org.uk> wrote:
> >
> > My point still stands though.  Merely hiding it doesn't make the problem
> > go away - it's just the same problem but now it won't be as visible, and
> > as such it'll probably never get resolved.
> 
> How much of a legacy thing is this?

I thought we'd got it down to just being a legacy issue, and I thought
there was concensus on "no new NO_IRQ users" as well.  What we probably
failed to do was to get a rule into checkpatch to pick up on new users
of the macro.

> It may not *fix* that particular driver or subsystem, but if it's
> sufficiently well hidden or specialized, at least it won't cause the
> pattern to be copied in the future, I'd hope.

The thing that I'm really worried about is this is going to cause
regressions, especially when the "lets get rid of NO_IRQ" involves
converting from "irq == NO_IRQ" to "!irq" - which is logically a
completely different test - especially if there's cases where irq
can end up being "NO_IRQ" and the definition of NO_IRQ is -1.

I think I said last time that such conversions should be done in a
mindful way of that - converting these places to be <= 0 so we catch
_both_ the !irq and irq == NO_IRQ cases until we've killed NO_IRQ off
and are using irq == 0 meaning "there is no IRQ" universally.

The reason for this is that there's some _really_ awkward cases.
For example:

drivers/scsi/arm/oak.c: host->irq = NO_IRQ;
drivers/scsi/NCR5380.c: tmp[0] = IDENTIFY(((instance->irq == NO_IRQ) ? 0 : 1), cmd->device->lun);

oak uses NCR5380.  NCR5380 is shared across multiple architectures
which have a random selection of NO_IRQ defined as 0 or -1.  To
convert this without regression takes a multi-step process:

1. Verify all architectures using NCR5380 never pass 0 as a valid IRQ
   (they shouldn't today - I think this is true for ARM in this
   instance.)
2. change NCR5380 to recognise both -1 and 0 as being invalid IRQs
   (with a <= 0 test) and kill NO_IRQ in the NCR5380 code.
3. Kill off the uses of NO_IRQ being passed into the NCR5380 code,
   passing 0 instead.
4. Optionally (and preferably) change the test to be !instance->irq.

If we just do the "eliminate NO_IRQ by changing it to constant 0" in
either NCR5380 or oak.c on its own, we're going to end up with a
regression - at least until the other catches up.

This is my concern - some of the interactions are not simple, and it's
certainly not a case that merely replacing each NO_IRQ with 0 or -1
resolves the problem - and it's clear that doing such a thing will
cause regressions.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.