Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
- Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional
- From: Sergey Shtylyov <s.shtylyov@xxxxxx>
- Date: Sat, 15 Jan 2022 23:22:16 +0300
- Cc: Andrew Lunn <andrew@xxxxxxx>, Ulf Hansson <ulf.hansson@xxxxxxxxxx>, Vignesh Raghavendra <vigneshr@xxxxxx>, KVM list <kvm@xxxxxxxxxxxxxxx>, "Rafael J. Wysocki" <rafael@xxxxxxxxxx>, <linux-iio@xxxxxxxxxxxxxxx>, "Linus Walleij" <linus.walleij@xxxxxxxxxx>, Amit Kucheria <amitk@xxxxxxxxxx>, "ALSA Development Mailing List" <alsa-devel@xxxxxxxxxxxxxxxx>, Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>, Jaroslav Kysela <perex@xxxxxxxx>, Guenter Roeck <groeck@xxxxxxxxxxxx>, Thierry Reding <thierry.reding@xxxxxxxxx>, MTD Maling List <linux-mtd@xxxxxxxxxxxxxxxxxxx>, Linux I2C <linux-i2c@xxxxxxxxxxxxxxx>, Miquel Raynal <miquel.raynal@xxxxxxxxxxx>, <linux-phy@xxxxxxxxxxxxxxxxxxx>, Jiri Slaby <jirislaby@xxxxxxxxxx>, <openipmi-developer@xxxxxxxxxxxxxxxxxxxxx>, "David S. Miller" <davem@xxxxxxxxxxxxx>, Khuong Dinh <khuong@xxxxxxxxxxxxxxxxxxxxxx>, Florian Fainelli <f.fainelli@xxxxxxxxx>, Matthias Schiffer <matthias.schiffer@xxxxxxxxxxxxxxx>, Joakim Zhang <qiangqing.zhang@xxxxxxx>, Kamal Dasu <kdasu.kdev@xxxxxxxxx>, Lee Jones <lee.jones@xxxxxxxxxx>, "Bartosz Golaszewski" <brgl@xxxxxxxx>, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>, Kishon Vijay Abraham I <kishon@xxxxxx>, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>, "open list:SERIAL DRIVERS" <linux-serial@xxxxxxxxxxxxxxx>, Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>, bcm-kernel-feedback-list <bcm-kernel-feedback-list@xxxxxxxxxxxx>, Zhang Rui <rui.zhang@xxxxxxxxx>, Linux PWM List <linux-pwm@xxxxxxxxxxxxxxx>, Robert Richter <rric@xxxxxxxxxx>, Saravanan Sekar <sravanhome@xxxxxxxxx>, Corey Minyard <minyard@xxxxxxx>, Linux PM list <linux-pm@xxxxxxxxxxxxxxx>, Liam Girdwood <lgirdwood@xxxxxxxxx>, Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>, John Garry <john.garry@xxxxxxxxxx>, Peter Korsgaard <peter@xxxxxxxxxxxxx>, "William Breathitt Gray" <vilhelm.gray@xxxxxxxxx>, Mark Gross <markgross@xxxxxxxxxx>, Hans de Goede <hdegoede@xxxxxxxxxx>, Alex Williamson <alex.williamson@xxxxxxxxxx>, Mark Brown <broonie@xxxxxxxxxx>, "Borislav Petkov" <bp@xxxxxxxxx>, Sebastian Reichel <sre@xxxxxxxxxx>, Jakub Kicinski <kuba@xxxxxxxxxx>, Matthias Brugger <matthias.bgg@xxxxxxxxx>, Takashi Iwai <tiwai@xxxxxxxx>, <platform-driver-x86@xxxxxxxxxxxxxxx>, Benson Leung <bleung@xxxxxxxxxxxx>, Linux ARM <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>, <linux-edac@xxxxxxxxxxxxxxx>, Tony Luck <tony.luck@xxxxxxxxx>, Mun Yew Tham <mun.yew.tham@xxxxxxxxx>, Eric Auger <eric.auger@xxxxxxxxxx>, "Greg Kroah-Hartman" <gregkh@xxxxxxxxxxxxxxxxxxx>, "open list:GPIO SUBSYSTEM" <linux-gpio@xxxxxxxxxxxxxxx>, Cornelia Huck <cohuck@xxxxxxxxxx>, "Linux MMC List" <linux-mmc@xxxxxxxxxxxxxxx>, Linux Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>, linux-spi <linux-spi@xxxxxxxxxxxxxxx>, Linux-Renesas <linux-renesas-soc@xxxxxxxxxxxxxxx>, Vinod Koul <vkoul@xxxxxxxxxx>, James Morse <james.morse@xxxxxxx>, Zha Qipeng <qipeng.zha@xxxxxxxxx>, Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx>, Richard Weinberger <richard@xxxxxx>, Niklas Söderlund <niklas.soderlund@xxxxxxxxxxxx>, <linux-mediatek@xxxxxxxxxxxxxxxxxxx>, "Brian Norris" <computersforpeace@xxxxxxxxx>, <netdev@xxxxxxxxxxxxxxx>
- In-reply-to: <20220114202226.ugzklxv4wzr6egwj@pengutronix.de>
- Organization: Open Mobile Platform
- References: <20220110201014.mtajyrfcfznfhyqm@pengutronix.de> <YdyilpjC6rtz6toJ@lunn.ch> <CAMuHMdWK3RKVXRzMASN4HaYfLckdS7rBvSopafq+iPADtGEUzA@mail.gmail.com> <20220112085009.dbasceh3obfok5dc@pengutronix.de> <CAMuHMdWsMGPiQaPS0-PJ_+Mc5VQ37YdLfbHr_aS40kB+SfW-aw@mail.gmail.com> <20220112213121.5ruae5mxwj6t3qiy@pengutronix.de> <Yd9L9SZ+g13iyKab@sirena.org.uk> <29f0c65d-77f2-e5b2-f6cc-422add8a707d@omp.ru> <20220114092557.jrkfx7ihg26ekzci@pengutronix.de> <61b80939-357d-14f5-df99-b8d102a4e1a1@omp.ru> <20220114202226.ugzklxv4wzr6egwj@pengutronix.de>
- User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1
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
[Index of Archives]
[Linux Kernel]
[Linux ARM (vger)]
[Linux ARM MSM]
[Linux Omap]
[Linux Arm]
[Linux Tegra]
[Fedora ARM]
[Linux for Samsung SOC]
[eCos]
[Linux Fastboot]
[Gcc Help]
[Git]
[DCCP]
[IETF Announce]
[Security]
[Linux MIPS]
[Yosemite Campsites]
|