On Wed, Jan 12, 2022 at 4:14 PM Hans de Goede [off-list ref] wrote:

Hi,

On 1/12/22 16:05, Sergey Shtylyov wrote:

quoted

On 1/12/22 5:41 PM, Rafael J. Wysocki wrote:

[...]

quoted

quoted

quoted

quoted

If an optional IRQ is not present, drivers either just ignore it (e.g.
for devices that can have multiple interrupts or a single muxed IRQ),
or they have to resort to polling. For the latter, fall-back handling
is needed elsewhere in the driver.
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.

Hi,
The *_optional() functions return an error code if there has been a
real error which should be reported up the call stack. This excludes
whatever error code indicates the requested resource does not exist,
which can be -ENODEV etc. If the device does not exist, a magic cookie
is returned which appears to be a valid resources but in fact is
not. So the users of these functions just need to check for an error
code, and fail the probe if present.

Agreed.

Note that in most (all?) other cases, the return type is a pointer
(e.g. to struct clk), and NULL is the magic cookie.

quoted

You seems to be suggesting in binary return value: non-zero
(available) or zero (not available)

Only in case of success. In case of a real failure, an error code
must be returned.

quoted

This discards the error code when something goes wrong. That is useful
information to have, so we should not be discarding it.

No, the error code must be retained in case of failure.

quoted

IRQ don't currently have a magic cookie value. One option would be to
add such a magic cookie to the subsystem. Otherwise, since 0 is
invalid, return 0 to indicate the IRQ does not exist.

Exactly. And using 0 means the similar code can be used as for other
subsystems, where NULL would be returned.

The only remaining difference is the "dummy cookie can be passed
to other functions" behavior.  Which is IMHO a valid difference,
as unlike with e.g. clk_prepare_enable(), you do pass extra data to
request_irq(), and sometimes you do need to handle the absence of
the interrupt using e.g. polling.

quoted

The request for a script checking this then makes sense. However, i
don't know how well coccinelle/sparse can track values across function
calls. They probably can check for:

   ret = irq_get_optional()
   if (ret < 0)
      return ret;

A missing if < 0 statement somewhere later is very likely to be an
error. A comparison of <= 0 is also likely to be an error. A check for

quoted

0 before calling any other IRQ functions would be good. I'm

surprised such a check does not already existing in the IRQ API, but
there are probably historical reasons for that.

There are still a few platforms where IRQ 0 does exist.

Not just a few even.  This happens on a reasonably recent x86 PC:

rafael@gratch:~/work/linux-pm> head -2 /proc/interrupts
           CPU0       CPU1       CPU2       CPU3       CPU4       CPU5
  0:         10          0          0          0          0          0
 IR-IO-APIC    2-edge
timer

   IIRC Linus has proclaimed that IRQ0 was valid for the i8253 driver (living in
arch/x86/); IRQ0 only was frowned upon when returned by platform_get_irq() and its
ilk.

MBR, Sergey

Right, platform_get_irq() has this:

        WARN(ret == 0, "0 is an invalid IRQ number\n");

So given that platform_get_irq() returning 0 is not expected, it seems
reasonable for platform_get_irq_optional() to use 0 as a special
"no irq available" return value, matching the NULL returned by
gpiod_get_optional().

Sounds reasonable to me.