Hello!
On 1/19/22 1:26 AM, Uwe Kleine-König wrote:
[...]
> However for an interupt this cannot work. You will always have to check > if the irq is actually there or not because if it's not you cannot just > ignore that. So there is no benefit of an optional irq. > > Leaving error message reporting aside, the introduction of > platform_get_irq_optional() allows to change > > irq = platform_get_irq(...); > if (irq < 0 && irq != -ENXIO) { > return irq; > } else if (irq >= 0) {
Rather (irq > 0) actually, IRQ0 is considered invalid (but still returned).
This is a topic I don't feel strong for, so I'm sloppy here. If changing this is all that is needed to convince you of my point ...
Note that we should absolutely (and first of all) stop returning 0 from platform_get_irq() on a "real" IRQ0. Handling that "still good" zero absolutely doesn't scale e.g. for the subsystems (like libata) which take 0 as an indication that the polling mode should be used... We can't afford to be sloppy here. ;-)
Then maybe do that really first?
I'm doing it first already:
https://lore.kernel.org/all/5e001ec1-d3f1-bcb8-7f30-a6301fd9930c@omp.ru/
This series is atop of the above patch...
Ah, I missed that (probably because I didn't get the cover letter).
I didn't recheck, but is this what the driver changes in your patch is about?
Partly, yes. We can afford to play with the meaning of 0 after the above patch.
But the changes that are in patch 1 are all needed?
Yes, they follow from the platform_get_irq_optional() changing the sense of its result...
[...]
For my part I'd say this doesn't justify the change, but at least I could better life with the reasoning. If you start at:
irq = platform_get_irq_optional(...) if (irq < 0 && irq != -ENXIO) return irq else if (irq > 0) setup_irq(irq); else setup_polling()
I'd change that to
irq = platform_get_irq_optional(...) if (irq > 0) /* or >= 0 ? */
Not >= 0, no...
setup_irq(irq)else if (irq == -ENXIO) setup_polling() else return irq
This still has to mention -ENXIO, but this is ok and checking for 0 just hardcodes a different return value.
I think comparing with 0 is simpler (and shorter) than with -ENXIO, if you consider the RISC CPUs, like e.g. MIPS...
Hmm, I don't know MIPS good enough to judge. So I created a small C
MIPS has read-only register 0 (containing 0 :-)) which should simplify things. But I'd have to check the actual object code... yeah, MIPS has a branching instruction that compares 2 registers and branches on the result'; with -ENXIO you'd have to load an immediate value into a register first...
file:
$ cat test.c #include <errno.h>
int platform_get_irq_optional(void); void a(void);
int func_0() { int irq = platform_get_irq_optional();
if (irq == 0) a();}
int func_enxio() { int irq = platform_get_irq_optional();
if (irq == -ENXIO) a();}
With some cross compilers as provided by Debian doing
$CC -c -O3 test.c
Mhm, do we really use -O3 to build the kernel?
nm --size-sort test.o
I get:
compiler | size of func_0 | size of func_enxio ================================+==================|==================== aarch64-linux-gnu-gcc | 0000000000000024 | 0000000000000028 arm-linux-gnueabi-gcc | 00000018 | 00000018 arm-linux-gnueabihf-gcc | 00000010 | 00000012
Hm, 2 bytes only -- perhaps Thumb mode?
i686-linux-gnu-gcc | 0000002a | 0000002a
Expected.
mips64el-linux-gnuabi64-gcc | 0000000000000054 | 000000000000005c
That's even more than expected -- 64-bit mode used?
powerpc-linux-gnu-gcc | 00000058 | 00000058
Well, they say
s390x-linux-gnu-gcc | 000000000000002e | 0000000000000030 x86_64-linux-gnu-gcc | 0000000000000022 | 0000000000000022
Again, as expected...
So you save some bytes indeed.
I see you have a lot of spare time (unlike me!). :-)
Anyhow, I think if you still want to change platform_get_irq_optional you should add a few patches converting some drivers which demonstrates the improvement for the callers.
Mhm, I did include all the drivers where the IRQ checks have to be modified, not sure what else you want me to touch...
I somehow expected that the changes that are now necessary (or possible) to callers makes them prettier somehow. Looking at your patch again:
I think they do...
- drivers/counter/interrupt-cnt.c This one is strange in my eyes because it tests the return value of gpiod_get_optional against NULL :-(
Mhm, how is this connected with my patch? :-/
- drivers/edac/xgene_edac.c This one just wants a silent irq lookup and then throws away the error code returned by platform_get_irq_optional() to return -EINVAL. Not so nice, is it?
I have dropped this file from my (to be posted yet) v2... sorry that it took so long...
- drivers/gpio/gpio-altera.c This one just wants a silent irq lookup. And maybe it should only goto skip_irq if the irq was not found, but on an other error code abort the probe?!
That's debatable... but anyway it's a matter of a separate (follow up) patch...
- drivers/gpio/gpio-mvebu.c Similar to gpio-altera.c: Wants a silent irq and improved error handling.
Same as above...
- drivers/i2c/busses/i2c-brcmstb.c A bit ugly that we now have dev->irq == 0 if the irq isn't available, but if requesting the irq failed irq = -1 is used?
This doesn't matter much really but can change to 0, if you want... :-)
- drivers/mmc/host/sh_mmcif.c Broken error handling. This one wants to abort on irq[1] < 0 (with your changed semantic).
Again, matter of a separate patch (I don't have the guily hardware anymore but I guess Geert could help with that).
I stopped here.
Note that most of your complaints are about the existing driver logic -- which my patch just couldn't deal with... I currently don't have the bandwidth for addressing all your complaints; some (more obvious) I'm goiing to address as the time permits, the draft patches have been done already...
It seems quite common that drivers assume a value < 0 returned by platform_get_irq means not-found
Of course, before this patch -ENXIO meant IRQ-not-found, many drivers don't bother to filter it out separately (for simplicity?).
and don't care for -EPROBE_DEFER (what else can happen?).
Hm, I haven't really seen a lot the probe dererral mishandling in the code touched by at least my patch #1...
Changing a relevant function in that mess seems unfortunate here :-\
You seem to have some spare time and I'm getting distracted contrariwise... want to help? :-)
Best regards Uwe
MBR, Sergey