On Tue, Jan 18, 2022 at 11:21:45PM +0300, Sergey Shtylyov wrote:

Hello!

On 1/17/22 11:47 AM, Uwe Kleine-König wrote:

[...]

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

To me it sounds much more logical for the driver to check if an
optional irq is non-zero (available) or zero (not available), than to
sprinkle around checks for -ENXIO. In addition, you have to remember
that this one returns -ENXIO, while other APIs use -ENOENT or -ENOSYS
(or some other error code) to indicate absence. I thought not having
to care about the actual error code was the main reason behind the
introduction of the *_optional() APIs.

quoted

No, the main benefit of gpiod_get_optional() (and clk_get_optional()) is
that you can handle an absent GPIO (or clk) as if it were available.

   Hm, I've just looked at these and must note that they match 1:1 with
platform_get_irq_optional(). Unfortunately, we can't however behave the
same way in request_irq() -- because it has to support IRQ0 for the sake
of i8253 drivers in arch/...

Let me reformulate your statement to the IMHO equivalent:

	If you set aside the differences between
	platform_get_irq_optional() and gpiod_get_optional(),

   Sorry, I should make it clear this is actually the diff between a would-be
platform_get_irq_optional() after my patch, not the current code...

The similarity is that with your patch both gpiod_get_optional() and
platform_get_irq_optional() return NULL and 0 on not-found. The relevant
difference however is that for a gpiod NULL is a dummy value, while for
irqs it's not. So the similarity is only syntactically, but not
semantically.

   I have noting to say here, rather than optional IRQ could well have a different
meaning than for clk/gpio/etc.

[...]

quoted

quoted

quoted

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/ (local)

   This series is atop of the above patch...

Ah, I missed that (probably because I didn't get the cover letter).

quoted

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?
 

quoted

After some more thoughts I wonder if your focus isn't to align
platform_get_irq_optional to (clk|gpiod|regulator)_get_optional, but to
simplify return code checking. Because with your change we have:

 - < 0 -> error
 - == 0 -> no irq
 - > 0 -> irq

   Mainly, yes. That's why the code examples were given in the description.

quoted

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

quoted

		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
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
	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
i686-linux-gnu-gcc		|         0000002a |         0000002a
mips64el-linux-gnuabi64-gcc	| 0000000000000054 | 000000000000005c
powerpc-linux-gnu-gcc		|         00000058 |         00000058
s390x-linux-gnu-gcc		| 000000000000002e | 0000000000000030
x86_64-linux-gnu-gcc		| 0000000000000022 | 0000000000000022

So you save some bytes indeed.

quoted

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:

 - drivers/counter/interrupt-cnt.c
   This one is strange in my eyes because it tests the return value of
   gpiod_get_optional against NULL :-(

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

 - 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?!

 - drivers/gpio/gpio-mvebu.c
   Similar to gpio-altera.c: Wants a silent irq and improved error
   handling.

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

 - drivers/mmc/host/sh_mmcif.c
   Broken error handling. This one wants to abort on irq[1] < 0 (with
   your changed semantic).

I stopped here.

It seems quite common that drivers assume a value < 0 returned by
platform_get_irq means not-found and don't care for -EPROBE_DEFER (what
else can happen?) Changing a relevant function in that mess seems
unfortunate here :-\

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |