[PATCH] ARM: mmp: replace NO_IRQ

[PATCH] ARM: mmp: replace NO_IRQ

[Arnd Bergmann]

The mmp platform has its own definitions with the old NO_IRQ meaning,
but compares against the global NO_IRQ macro that we should have
removed long ago.

The specific usage in arch/arm/mach-mmp/devices.c is awkward, but
fixing it properly would require a larger scale rewrite of the entire
file, or even using devicetree for all machines. As I'm not able to
do that any time soon, let's make the current behavior more explit
instead and avoid the literal use of NO_IRQ.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm/mach-mmp/devices.c | 2 +-
 arch/arm/mach-mmp/irqs.h    | 8 +++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-mmp/devices.c b/arch/arm/mach-mmp/devices.c
index 671c7a09ab3d..d0cd9687d6b2 100644
--- a/arch/arm/mach-mmp/devices.c
+++ b/arch/arm/mach-mmp/devices.c
@@ -39,7 +39,7 @@ int __init pxa_register_device(struct pxa_device_desc *desc,
 		nres++;
 	}
 
-	if (desc->irq != NO_IRQ) {
+	if (desc->irq != IRQ_MMP_NONE) {
 		res[nres].start	= desc->irq;
 		res[nres].end	= desc->irq;
 		res[nres].flags	= IORESOURCE_IRQ;
diff --git a/arch/arm/mach-mmp/irqs.h b/arch/arm/mach-mmp/irqs.h
index fb492a50a817..4f9f27ae4dbc 100644
--- a/arch/arm/mach-mmp/irqs.h
+++ b/arch/arm/mach-mmp/irqs.h
@@ -1,10 +1,12 @@
 #ifndef __ASM_MACH_IRQS_H
 #define __ASM_MACH_IRQS_H
 
+#define IRQ_MMP_NONE			(-1)
+
 /*
  * Interrupt numbers for PXA168
  */
-#define IRQ_PXA168_NONE			(-1)
+#define IRQ_PXA168_NONE			IRQ_MMP_NONE
 #define IRQ_PXA168_SSP4			0
 #define IRQ_PXA168_SSP3			1
 #define IRQ_PXA168_SSP2			2
@@ -54,7 +56,7 @@
 /*
  * Interrupt numbers for PXA910
  */
-#define IRQ_PXA910_NONE			(-1)
+#define IRQ_PXA910_NONE			IRQ_MMP_NONE
 #define IRQ_PXA910_AIRQ			0
 #define IRQ_PXA910_SSP3			1
 #define IRQ_PXA910_SSP2			2
@@ -116,7 +118,7 @@
 /*
  * Interrupt numbers for MMP2
  */
-#define IRQ_MMP2_NONE			(-1)
+#define IRQ_MMP2_NONE			IRQ_MMP_NONE
 #define IRQ_MMP2_SSP1			0
 #define IRQ_MMP2_SSP2			1
 #define IRQ_MMP2_SSPA1			2
-- 
2.9.0

Re: [PATCH] ARM: mmp: replace NO_IRQ

[Russell King - ARM Linux]

On Tue, Sep 06, 2016 at 04:07:56PM +0200, Arnd Bergmann wrote:
> The mmp platform has its own definitions with the old NO_IRQ meaning,
> but compares against the global NO_IRQ macro that we should have
> removed long ago.
> 
> The specific usage in arch/arm/mach-mmp/devices.c is awkward, but
> fixing it properly would require a larger scale rewrite of the entire
> file, or even using devicetree for all machines. As I'm not able to
> do that any time soon, let's make the current behavior more explit
> instead and avoid the literal use of NO_IRQ.

So this probably continues to be a problem, but we hide it from the
NO_IRQ brigade.  I think it would be better to leave it as-is until
it can be fixed up correctly, rather than trying to hide it.

-- 
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.

Re: [PATCH] ARM: mmp: replace NO_IRQ

[Arnd Bergmann]

On Tuesday, September 6, 2016 3:24:42 PM CEST Russell King - ARM Linux wrote:
> On Tue, Sep 06, 2016 at 04:07:56PM +0200, Arnd Bergmann wrote:
> > The mmp platform has its own definitions with the old NO_IRQ meaning,
> > but compares against the global NO_IRQ macro that we should have
> > removed long ago.
> > 
> > The specific usage in arch/arm/mach-mmp/devices.c is awkward, but
> > fixing it properly would require a larger scale rewrite of the entire
> > file, or even using devicetree for all machines. As I'm not able to
> > do that any time soon, let's make the current behavior more explit
> > instead and avoid the literal use of NO_IRQ.
> 
> So this probably continues to be a problem, but we hide it from the
> NO_IRQ brigade.  I think it would be better to leave it as-is until
> it can be fixed up correctly, rather than trying to hide it.

Linus explicitly asked for NO_IRQ to be finally killed off last week[1],
and I've prepared all patches for that now, this is the last one
aside from removing the macro.

The patches I sent last week for this have already been accepted into
linux-next, these are the remaining ones:

001d34e3d6e1 ARM: remove NO_IRQ definition
106bee9d4f69 [SUBMITTED 20160906] ARM: mmp: replace NO_IRQ
96d744cef97a [SUBMITTED 20160906] ARM: orion5x: remove extraneous NO_IRQ
0dadcd937ef8 [SUBMITTED 20160906] ARM: orion5x: avoid NO_IRQ in orion_ge00_switch_init
fb0558152b70 [SUBMITTED 20160906] ARM: mvebu/orion: remove NO_IRQ check from device init
309ac9ebb2eb [SUBMITTED 20160906] ARM: mv78xx0: simplify ethernet device creation
c50d19cfb4c8 [SUBMITTED 20160906] pcmcia: soc-common: remove incorrect NO_IRQ use
c9f05678577e [SUBMITTED 20160906] mfd: ucb1x00: remove NO_IRQ check
393ce6162224 [SUBMITTED 20160906] ARM: common/locomo: remove NO_IRQ check
1c78db6236a8 [SUBMITTED 20160906] ARM: common/sa1111: remove NO_IRQ check
ed8f68ba549b [SUBMITTED 20160906] ptp: ixp46x: remove NO_IRQ handling
ed4cdcd25a34 [SUBMITTED 20160906] mmc: davinci: remove incorrect NO_IRQ use
95ddf97fc805 [SUBMITTED 20160903] crypto: mv_cesa: remove NO_IRQ reference

I'm experimenting with cleaning up the file some more, but it's unclear
if doing it another way is an actual improvement, or if a larger change
is worth the risk for regressions, given how little interest there is
in this platform in general.

	Arnd

[1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2016-September/003803.html

Re: [PATCH] ARM: mmp: replace NO_IRQ

[Russell King - ARM Linux]

On Tue, Sep 06, 2016 at 09:28:17PM +0200, Arnd Bergmann wrote:
> On Tuesday, September 6, 2016 3:24:42 PM CEST Russell King - ARM Linux wrote:
> > On Tue, Sep 06, 2016 at 04:07:56PM +0200, Arnd Bergmann wrote:
> > > The mmp platform has its own definitions with the old NO_IRQ meaning,
> > > but compares against the global NO_IRQ macro that we should have
> > > removed long ago.
> > > 
> > > The specific usage in arch/arm/mach-mmp/devices.c is awkward, but
> > > fixing it properly would require a larger scale rewrite of the entire
> > > file, or even using devicetree for all machines. As I'm not able to
> > > do that any time soon, let's make the current behavior more explit
> > > instead and avoid the literal use of NO_IRQ.
> > 
> > So this probably continues to be a problem, but we hide it from the
> > NO_IRQ brigade.  I think it would be better to leave it as-is until
> > it can be fixed up correctly, rather than trying to hide it.
> 
> Linus explicitly asked for NO_IRQ to be finally killed off last week[1],
> and I've prepared all patches for that now, this is the last one
> aside from removing the macro.

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.

> I'm experimenting with cleaning up the file some more, but it's unclear
> if doing it another way is an actual improvement, or if a larger change
> is worth the risk for regressions, given how little interest there is
> in this platform in general.

If there's little interest in it, and little stomach to fix it, maybe
the better thing is to remove it if no one has an interest in it?

> [1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2016-September/003803.html

What Linus has failed to understand is that the reason why we've kept
NO_IRQ as -1 is that changing NO_IRQ to 0 results in regressions - I've
been through as much as the code that I'm personally happy to convert
each time this has come up, and what remains has been the stuff that
I've not been happy to touch through fear of breaking it.

So, changing NO_IRQ to 0 results in regressions.  Trying to fix the
sites probably results in regressions too (I've already seen one
example with your UCB1x00 patch of such breakage caused by mindless
"conversion".)

I guess you're just happier to cause regressions to keep Linus happy
than I am.

-- 
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.

Re: [PATCH] ARM: mmp: replace NO_IRQ

[Linus Torvalds]

On Tue, Sep 6, 2016 at 12:44 PM, Russell King - ARM Linux
<linux@armlinux.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?

The main reason I'd like to _really_ make NO_IRQ go away is that it
seems that some people copy it from existing drivers, or just think
it's the RightThing(tm) to do because it looks so plausible.

So in that sense I wouldn't actually mind "merely hiding it".

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.

So hiding things inside a particular driver (or a particular
subsystem) may be hacky from the standpoint of that particular driver
or subsystem, but from a "big issue" standpoint I don't mind at all.

           Linus

Re: [PATCH] ARM: mmp: replace NO_IRQ

[Russell King - ARM Linux]

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@armlinux.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.

Re: [PATCH] ARM: mmp: replace NO_IRQ

[Arnd Bergmann]

On Tuesday, September 6, 2016 10:22:06 PM CEST Russell King - ARM Linux wrote:
> 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.

>From looking at how this driver has been handled elsewhere, the plan
seems to be to do it one architecture at a time, and almost
all have killed off NO_IRQ at this point, making it impossible
to use IRQ 0 on an NCR5380 derivative.

The driver has had this snippet since 2014 with commit 22f5f10d2dad
("ncr5380: Fix SCSI_IRQ_NONE bugs"):

#ifndef NO_IRQ
#define NO_IRQ          0
#endif

and this is used on almost all architectures now, including the
two other ones that have architecture specific drivers (powerpc
and m68k).

The architectures that still provide NO_IRQ are:

c6x, openrisc: these have no ISA, PCI or any platform
	specific	NCR5380 variant, so they are irrelevant here.
powerpc: defines NO_IRQ as 0, and is in the process of removing
	that.
sparc, parisc, mn10300: these have PCI slots but no ISA slots, and
	could have DMX3191D in theory, but none of the other front-ends.
	The dmx3191d driver doesn't use interrupts, so that's fine too.
arm: I've submitted patches for all other uses of the NO_IRQ macro
	that are possible on ARM, so this driver remains.

If we can show that no ARM machine uses NCR5380 with a valid IRQ 0,
thenthe definition can be removed from arch/arm/include/asm/irq.h
as soon as my last patch is merged.

We can also trivially remove the defintitions of NO_IRQ from c6x,
mn10300, openrisc, parisc and sparc, as nothing relies on them,
and Michael Ellerman has a series for all the powerpc drivers.
	
FWIW, there are a couple of other drivers that use the same
#ifdef trick as NCR5380:

drivers/ata/sata_dwc_460ex.c:#ifndef NO_IRQ
drivers/ata/sata_dwc_460ex.c:#define NO_IRQ		0
drivers/ata/sata_dwc_460ex.c-#endif
-> wrong, must use hardcoded '0'

drivers/input/touchscreen/ucb1400_ts.c:#ifndef NO_IRQ
drivers/input/touchscreen/ucb1400_ts.c:#define NO_IRQ	0
drivers/input/touchscreen/ucb1400_ts.c-#endif
-> wrong, must use hardcoded '0'

drivers/mmc/host/of_mmc_spi.c:#ifndef NO_IRQ
drivers/mmc/host/of_mmc_spi.c:#define NO_IRQ 0
drivers/mmc/host/of_mmc_spi.c-#endif
-> unused since the only user was found broken and fixed

drivers/pcmcia/pd6729.c:#ifndef NO_IRQ
drivers/pcmcia/pd6729.c:#define NO_IRQ	((unsigned int)(0))
drivers/pcmcia/pd6729.c-#endif
-> wrong, must use hardcoded '0'

drivers/rtc/rtc-m48t59.c:#ifndef NO_IRQ
drivers/rtc/rtc-m48t59.c:#define NO_IRQ	(-1)
drivers/rtc/rtc-m48t59.c-#endif
-> correct, but the only platform using it doesn't provide
   an interrupt

I guess I'll send patches for these five drivers too.

	Arnd

Re: [PATCH] ARM: mmp: replace NO_IRQ

[Arnd Bergmann]

On Tuesday, September 6, 2016 8:44:43 PM CEST Russell King - ARM Linux wrote:
> On Tue, Sep 06, 2016 at 09:28:17PM +0200, Arnd Bergmann wrote:
> > On Tuesday, September 6, 2016 3:24:42 PM CEST Russell King - ARM Linux wrote:
> > > On Tue, Sep 06, 2016 at 04:07:56PM +0200, Arnd Bergmann wrote:
> > I'm experimenting with cleaning up the file some more, but it's unclear
> > if doing it another way is an actual improvement, or if a larger change
> > is worth the risk for regressions, given how little interest there is
> > in this platform in general.
> 
> If there's little interest in it, and little stomach to fix it, maybe
> the better thing is to remove it if no one has an interest in it?
> 
> > [1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2016-September/003803.html
> 
> What Linus has failed to understand is that the reason why we've kept
> NO_IRQ as -1 is that changing NO_IRQ to 0 results in regressions - I've
> been through as much as the code that I'm personally happy to convert
> each time this has come up, and what remains has been the stuff that
> I've not been happy to touch through fear of breaking it.

Out of the 20 patches os so that I did for the complete removal on ARM,
a clear majority was fixing code that is already broken (usually in
error handling code paths that are never exercised in practice though).

> So, changing NO_IRQ to 0 results in regressions.  Trying to fix the
> sites probably results in regressions too (I've already seen one
> example with your UCB1x00 patch of such breakage caused by mindless
> "conversion".)

The patch was correct, the only problem that you pointed out already
was that it needs to be applied on top of your patch. I didn't
check when the file was touched the last time but only looked at
the current state in linux-next that happened to be from your
patch last week.

	Arnd

Re: [PATCH] ARM: mmp: replace NO_IRQ

[Russell King - ARM Linux]

On Tue, Sep 06, 2016 at 10:19:18PM +0200, Arnd Bergmann wrote:
> The patch was correct, the only problem that you pointed out already
> was that it needs to be applied on top of your patch. I didn't
> check when the file was touched the last time but only looked at
> the current state in linux-next that happened to be from your
> patch last week.

The patch is not correct on its own - and, as far as I'm aware, my
patch to ucb1x00-core.c should not be in linux-next - it is part of
a series which is still - partly - a work-in-progress, and the patch
has dependencies on other changes in the series which are not obvious.

I see now that Lee has picked it up - that's just great, because it
does have a dependency on gpio changes.  That's _why_ it's part of
the large series.  Had Lee even bothered to read the fscking covering
message to the sub-series that he was covered on, he'd have realised
that he shouldn't have taken it yet.

The series was posted to get review comments and testing, not for
people to start cherry-picking patches from it as they damn-well
wish.

-- 
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.