Re: [PATCH] driver core: platform: Rename platform_get_irq_optional() to platform_get_irq_silent()

[Uwe Kleine-König]

On Fri, Jan 14, 2022 at 08:55:07PM +0300, Sergey Shtylyov wrote:
> On 1/14/22 12:42 AM, Florian Fainelli wrote:
> 
> >> The subsystems regulator, clk and gpio have the concept of a dummy
> >> resource. For regulator, clk and gpio there is a semantic difference
> >> between the regular _get() function and the _get_optional() variant.
> >> (One might return the dummy resource, the other won't. Unfortunately
> >> which one implements which isn't the same for these three.) The
> >> difference between platform_get_irq() and platform_get_irq_optional() is
> >> only that the former might emit an error message and the later won't.
> >>
> >> To prevent people's expectations that there is a semantic difference
> >> between these too, rename platform_get_irq_optional() to
> >> platform_get_irq_silent() to make the actual difference more obvious.
> >>
> >> The #define for the old name can and should be removed once all patches
> >> currently in flux still relying on platform_get_irq_optional() are
> >> fixed.
> >>
> >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> [...]
> >>>> I think at least c) is easy to resolve because
> >>>> platform_get_irq_optional() isn't that old yet and mechanically
> >>>> replacing it by platform_get_irq_silent() should be easy and safe.
> >>>> And this is orthogonal to the discussion if -ENOXIO is a sensible return
> >>>> value and if it's as easy as it could be to work with errors on irq
> >>>> lookups.
> >>>
> >>> It'd certainly be good to name anything that doesn't correspond to one
> >>> of the existing semantics for the API (!) something different rather
> >>> than adding yet another potentially overloaded meaning.
> >>
> >> It seems we're (at least) three who agree about this. Here is a patch
> >> fixing the name.
> > 
> > From an API naming perspective this does not make much sense anymore with the name chosen,
> > it is understood that whent he function is called platform_get_irq_optional(), optional applies
> > to the IRQ. An optional IRQ is something people can reason about because it makes sense.
> 
>    Right! :-)
> 
> > What is a a "silent" IRQ however? It does not apply to the object it is trying to fetch to
> > anymore, but to the message that may not be printed in case the resource failed to be obtained,
> > because said resource is optional. Woah, that's quite a stretch.
> 
>    Right again! :-)

So you oppose to the name chosen, but not the renaming as such. I
already asked Florian if he has a better name, do you have a
constructive suggestion? What about platform_silently_get_irq? Or
platform_get_irq_silently?

Do you agree that it's unfortunate that platform_get_irq_optional() has a
different usage convention than clk_get_optional() and
gpiod_get_optional()?

Do you agree that the difference between platform_get_irq_optional() and
platform_get_irq() is only that one of them issues an error message and
the other doesn't?

Currently the return values of platform_get_irq_optional() and
platform_get_irq() are identical. Do you agree that any change to clean
up the return value convention of platform_get_irq_optional() also
would be sensible for platform_get_irq()?

Do you agree that changing the way how return values of
platform_get_irq_optional() have to be evaluated without adapting
platform_get_irq() in the same way, artifially breaks the releation
between these two functions?

> > Following the discussion and original 2 patches set from Sergey, it is not entirely clear to me
> > anymore what is it that we are trying to fix.
> 
>    Andy and me tried to fix the platform_get_irq[_byname]_optional() value, corresponding to
> a missing (optional) IRQ resource from -ENXIO to 0, in order to keep the callers error code
> agnostic. This change completely aligns e.g. platform_get_irq_optional() with clk_get_optional()
> and gpiod_get_optional()...

Did you read and understand my objection? Yes, in the not found case the
return value is a 32-bit or 64-bit long zero value (0 or NULL) for these
functions. But for irqs you cannot treat that as an irq number. For clks
this works, and this is the fact that justifies the "optional" in the
name and that simplifies further handling without having to check if the
value returned by clk_get_optional corresponds to the dummy clk or a
real one. Just returning 0 for not-found doesn't justify "optional" in
the name. Or do you think kmalloc should better be called
kmalloc_optional because it returns NULL if there is no more memory
available?

The only thing you accomplish with returning 0 instead of -ENXIO is that
the check for this value in the callers has to be adapted. But as you
cannot handle 0 and a "normal" irq in the same way (i.e. pass it to
request_irq) 

In my eyes error code agnostic isn't a sensible goal. If you ask for a
resource and it's not there and your driver can cope and this cannot be
done by just treating the returned value as a valid resource, making it
explicit that the error code for "not found" is handled is a good thing.
In my opinion it's not a good enough reason to change a function's
return conventions just that I can handle one of the various results
using

	if (ret == 0)

instead of

	if (ret == -ENXIO)

Also there is no justification to change the value for "not found" to
zero. The next developer might be annoyed by having to check for
-EPROBE_DEFER and wants to introduce a special function that behaves
like platform_get_irq, just returns 0 when platform_get_irq returns
-ENOENT.

>    Unforunately, we can't "fix" request_irq() and company to treat 0 as missing IRQ -- they have
> to keep the ability to get called from the arch/ code (that doesn't use platform_get_irq(), etc.

Note that even if request_irq would be a noop for 0, the biggest part of
the drivers wouldn't be done with request_irq(0, ...) because in the
absense of an irq something has to be done about it.

Oh my, I failed to not continue this discussion really badly. But I
really cannot stand people arguing for wrong things and ignoring my
reasoning completely. In case you feel the same way: I agree that -ENXIO
is the wrong value for not-found. But to change this wrong behaviour to
another wrong behaviour isn't an improvement.

Best regards
Uwe

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

Attachment: signature.asc
Description: PGP signature