Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional

[Sergey Shtylyov <s.shtylyov@xxxxxx>]

On 1/14/22 11:22 PM, Uwe Kleine-König wrote:
> On Fri, Jan 14, 2022 at 10:14:10PM +0300, Sergey Shtylyov wrote:
>> On 1/14/22 12:25 PM, Uwe Kleine-König wrote:
>>
>>>>>>> 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.
>>>>>
>>>>>> 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.
> 
>>> 	platform_get_irq_optional() is like gpiod_get_optional().
>>>
>>> The introduction of gpiod_get_optional() made it possible to simplify
>>> the following code:
>>>
>>> 	reset_gpio = gpiod_get(...)
>>> 	if IS_ERR(reset_gpio):
>>> 		error = PTR_ERR(reset_gpio)
>>> 		if error != -ENDEV:
>>
>>    ENODEV?
> 
> Yes, typo.
> 
>>> 			return error
>>> 	else:
>>> 		gpiod_set_direction(reset_gpiod, INACTIVE)
>>>
>>> to
>>>
>>> 	reset_gpio = gpiod_get_optional(....)
>>> 	if IS_ERR(reset_gpio):
>>> 		return reset_gpio
>>> 	gpiod_set_direction(reset_gpiod, INACTIVE)
>>>
>>> and I never need to actually know if the reset_gpio actually exists.
>>> Either the line is put into its inactive state, or it doesn't exist and
>>> then gpiod_set_direction is a noop. For a regulator or a clk this works
>>> in a similar way.
>>>
>>> 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 ...

   See below. :-)

>>> 		... setup irq operation ...
>>> 	} else { /* irq == -ENXIO */
>>> 		... setup polling ...
>>> 	}
>>>
>>> to
>>> 	
>>> 	irq = platform_get_irq_optional(...);
>>> 	if (irq < 0 && irq != -ENXIO) {
>>> 		return irq;
>>> 	} else if (irq >= 0) {
>>> 		... setup irq operation ...
>>> 	} else { /* irq == -ENXIO */
>>> 		... setup polling ...
>>> 	}
>>>
>>> which isn't a win. When changing the return value as you suggest, it can
>>> be changed instead to:
>>>
>>> 	irq = platform_get_irq_optional(...);
>>> 	if (irq < 0) {
>>> 		return irq;
>>> 	} else if (irq > 0) {
>>> 		... setup irq operation ...
>>> 	} else { /* irq == 0 */
>>> 		... setup polling ...
>>> 	}
>>>
>>> which is a tad nicer. If that is your goal however I ask you to also
>>> change the semantic of platform_get_irq() to return 0 on "not found".
>>
>>     Well, I'm not totally opposed to that... but would there be a considerable win?

> Well, compared to your suggestion of making platform_get_irq_optional()
> return 0 on "not-found" the considerable win would be that
> platform_get_irq_optional() and platform_get_irq() are not different

   They would really be the same function if we do that. But...

> just because platform_get_irq() is to hard to change.

   It's not just that, of course. If you make platform_get_irq() return 0
ISO -ENXIO, you'd have to add the handling of that 0 to all the callers,
and that won't be as simple as:

	if (irq < 0)
		return irq;

since we can't just propagate 0 upstream, we'd have to return something like
-ENXIO (or whatever error we see fit). Does that really scale well?

>> Anyway, we should 1st stop returning 0 for "valid" IRQs -- this is done by my patch
>> the discussed patch series are atop of.
>>
>>> Note the win is considerably less compared to gpiod_get_optional vs
>>
>>    If there's any at all... We'd basically have to touch /all/ platform_get_irq()
>> calls (and get an even larger CC list ;-)).
> 
> You got me wrong here. I meant that even if you change both
> platform_get_irq() and platform_get_irq_optional() to return 0 on
> "not-found", the win is small compared to the benefit of having both

   There's no win at all, it seems.

> clk_get() and clk_get_optional().
> 
>>> gpiod_get however. And then it still lacks the semantic of a dummy irq
>>> which IMHO forfeits the right to call it ..._optional().
>>
>>    Not quite grasping it... Why e.g. clk_get() doesn't return 0 for a not found clock?
> 
> Because NULL is not an error value for clk and when calling clk_get()
> you want a failure when the clk you asked for isn't available.
> 
> Sure you could do the following in a case where you want to insist the
> clk to be actually available:
> 
> 	clk = clk_get_optional(...)
> 	if (IS_ERR_OR_NULL(clk)) {
> 		err = PTR_ERR(clk) || -ENODEV;
> 		return dev_err_probe(dev, err, ....);
> 	}
> 
> but this is more ugly than
> 
> 	clk = clk_get(...)
> 	if (IS_ERR(clk)) {
> 		err = PTR_ERR(clk);
> 		return dev_err_probe(dev, err, ....);
> 	}
> 
> Additionally the first usage would hard-code in the drivers that NULL is
> the dummy value which you might want to consider a layer violation.

   Unfortunately, we don't have a single layer in case of IRQs... There's
no platform_request_irq() (yet? :-)).

> You have to understand that for clk (and regulator and gpiod) NULL is a
> valid descriptor that can actually be used, it just has no effect. So
> this is a convenience value for the case "If the clk/regulator/gpiod in
> question isn't available, there is nothing to do". This is what makes
> clk_get_optional() and the others really useful and justifies their
> existence. This doesn't apply to platform_get_irq_optional().

   I do understand that. However, IRQs are a different beast with their
own justifications...

> So clk_get() is sane and sensible for cases where you need the clk to be
> there. It doesn't emit an error message, because the caller knows better
> if it's worth an error message and in some cases the caller can also
> emit a better error message than clk_get() itself.

   I haven't been thinking about the IRQ error messages at all (yet?)...
And when I start thinking about it, it doesn't seem that bad, perhaps
even saves a lot of the .rodata section... :-)

> clk_get_optional() is sane and sensible for cases where the clk might be
> absent and it helps you because you don't have to differentiate between
> "not found" and "there is an actual resource".
> 
> The reason for platform_get_irq_optional()'s existence is just that
> platform_get_irq() emits an error message which is wrong or suboptimal

   I think you are very wrong here. The real reason is to simplify the
callers.

> in some cases (and IMHO is platform_get_irq() root fault). It doesn't
> simplify handling the "not found" case.

   Oh, it does... you don't have to special-case 0 when handling its result.
In my book, it's a major simplification.

> So let's not pretend by the
> choice of function names that there is a similarity between clk_get() +
> clk_get_optional() and platform_get_irq() + platform_get_irq_optional().

   OK, no similarity. But that's well justified.

> And as you cannot change platform_get_irq_optional() to return a working
> dummy value, IMHO the only sane way out is renaming it.

   Your rename really focused on the wrong aspect of the function, I think...

> Best regards
> Uwe

MBR, Sergey