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: Hans de Goede <hdegoede@xxxxxxxxxx>
- Date: Wed, 12 Jan 2022 16:13:54 +0100
- Cc: Andrew Lunn <andrew@xxxxxxx>, Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>, Ulf Hansson <ulf.hansson@xxxxxxxxxx>, Vignesh Raghavendra <vigneshr@xxxxxx>, KVM list <kvm@xxxxxxxxxxxxxxx>, linux-iio@xxxxxxxxxxxxxxx, Linus Walleij <linus.walleij@xxxxxxxxxx>, Amit Kucheria <amitk@xxxxxxxxxx>, ALSA Development Mailing List <alsa-devel@xxxxxxxxxxxxxxxx>, Liam Girdwood <lgirdwood@xxxxxxxxx>, 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>, "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>, Tony Luck <tony.luck@xxxxxxxxx>, Kishon Vijay Abraham I <kishon@xxxxxx>, bcm-kernel-feedback-list <bcm-kernel-feedback-list@xxxxxxxxxxxx>, "open list:SERIAL DRIVERS" <linux-serial@xxxxxxxxxxxxxxx>, Jakub Kicinski <kuba@xxxxxxxxxx>, Zhang Rui <rui.zhang@xxxxxxxxx>, Matthias Brugger <matthias.bgg@xxxxxxxxx>, Platform Driver <platform-driver-x86@xxxxxxxxxxxxxxx>, Linux PWM List <linux-pwm@xxxxxxxxxxxxxxx>, Robert Richter <rric@xxxxxxxxxx>, Saravanan Sekar <sravanhome@xxxxxxxxx>, Corey Minyard <minyard@xxxxxxx>, Linux PM list <linux-pm@xxxxxxxxxxxxxxx>, Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>, John Garry <john.garry@xxxxxxxxxx>, Peter Korsgaard <peter@xxxxxxxxxxxxx>, William Breathitt Gray <vilhelm.gray@xxxxxxxxx>, Mark Gross <markgross@xxxxxxxxxx>, "open list:GPIO SUBSYSTEM" <linux-gpio@xxxxxxxxxxxxxxx>, Alex Williamson <alex.williamson@xxxxxxxxxx>, Mark Brown <broonie@xxxxxxxxxx>, Borislav Petkov <bp@xxxxxxxxx>, Eric Auger <eric.auger@xxxxxxxxxx>, Takashi Iwai <tiwai@xxxxxxxx>, Jaroslav Kysela <perex@xxxxxxxx>, openipmi-developer@xxxxxxxxxxxxxxxxxxxxx, Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>, Benson Leung <bleung@xxxxxxxxxxxx>, Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx>, Linux ARM <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>, "open list:EDAC-CORE" <linux-edac@xxxxxxxxxxxxxxx>, Richard Weinberger <richard@xxxxxx>, Mun Yew Tham <mun.yew.tham@xxxxxxxxx>, Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>, Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>, 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>, Sebastian Reichel <sre@xxxxxxxxxx>, Niklas Söderlund <niklas.soderlund@xxxxxxxxxxxx>, "moderated list:ARM/Mediatek SoC..." <linux-mediatek@xxxxxxxxxxxxxxxxxxx>, Brian Norris <computersforpeace@xxxxxxxxx>, netdev <netdev@xxxxxxxxxxxxxxx>
- In-reply-to: <78a17bae-435b-e35e-b2dc-1166777725a0@omp.ru>
- References: <20220110195449.12448-1-s.shtylyov@omp.ru> <20220110195449.12448-2-s.shtylyov@omp.ru> <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> <Yd7Z3Qwevb/lEwQZ@lunn.ch> <CAMuHMdV2cGvqMppwt9xhpze=pcnHfTozDZMjwT1DkivLD+_nbQ@mail.gmail.com> <CAJZ5v0iyAHtDe1kFObQorXOX0Xraxac0j29Dh+8sq7zxzbsmcQ@mail.gmail.com> <78a17bae-435b-e35e-b2dc-1166777725a0@omp.ru>
- User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.3.0
Hi,
On 1/12/22 16:05, Sergey Shtylyov wrote:
> On 1/12/22 5:41 PM, Rafael J. Wysocki wrote:
>
> [...]
>>>>> 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.
>>>
>>>> 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.
>>>
>>>> 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.
>>>
>>>> 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.
>>>
>>>> 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
>>>>> 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().
Regards,
Hans
[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]
|