lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 14 Jan 2022 22:14:10 +0300 From: Sergey Shtylyov <s.shtylyov@....ru> To: Uwe Kleine-König <u.kleine-koenig@...gutronix.de> CC: Mark Brown <broonie@...nel.org>, Andrew Lunn <andrew@...n.ch>, Ulf Hansson <ulf.hansson@...aro.org>, Vignesh Raghavendra <vigneshr@...com>, KVM list <kvm@...r.kernel.org>, "Rafael J. Wysocki" <rafael@...nel.org>, <linux-iio@...r.kernel.org>, Linus Walleij <linus.walleij@...aro.org>, "Amit Kucheria" <amitk@...nel.org>, ALSA Development Mailing List <alsa-devel@...a-project.org>, Jaroslav Kysela <perex@...ex.cz>, "Guenter Roeck" <groeck@...omium.org>, Thierry Reding <thierry.reding@...il.com>, "MTD Maling List" <linux-mtd@...ts.infradead.org>, Linux I2C <linux-i2c@...r.kernel.org>, Miquel Raynal <miquel.raynal@...tlin.com>, <linux-phy@...ts.infradead.org>, Jiri Slaby <jirislaby@...nel.org>, "Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>, Khuong Dinh <khuong@...amperecomputing.com>, Florian Fainelli <f.fainelli@...il.com>, Matthias Schiffer <matthias.schiffer@...tq-group.com>, Kamal Dasu <kdasu.kdev@...il.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Lee Jones" <lee.jones@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>, "Daniel Lezcano" <daniel.lezcano@...aro.org>, Kishon Vijay Abraham I <kishon@...com>, Geert Uytterhoeven <geert@...ux-m68k.org>, "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>, bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>, Zhang Rui <rui.zhang@...el.com>, <platform-driver-x86@...r.kernel.org>, Linux PWM List <linux-pwm@...r.kernel.org>, Robert Richter <rric@...nel.org>, "Saravanan Sekar" <sravanhome@...il.com>, Corey Minyard <minyard@....org>, Linux PM list <linux-pm@...r.kernel.org>, Liam Girdwood <lgirdwood@...il.com>, "Mauro Carvalho Chehab" <mchehab@...nel.org>, John Garry <john.garry@...wei.com>, Takashi Iwai <tiwai@...e.com>, Peter Korsgaard <peter@...sgaard.com>, "William Breathitt Gray" <vilhelm.gray@...il.com>, Mark Gross <markgross@...nel.org>, Hans de Goede <hdegoede@...hat.com>, Alex Williamson <alex.williamson@...hat.com>, Borislav Petkov <bp@...en8.de>, Eric Auger <eric.auger@...hat.com>, Jakub Kicinski <kuba@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>, <openipmi-developer@...ts.sourceforge.net>, "Andy Shevchenko" <andriy.shevchenko@...ux.intel.com>, Benson Leung <bleung@...omium.org>, Pengutronix Kernel Team <kernel@...gutronix.de>, "Linux ARM" <linux-arm-kernel@...ts.infradead.org>, <linux-edac@...r.kernel.org>, Tony Luck <tony.luck@...el.com>, Richard Weinberger <richard@....at>, "Mun Yew Tham" <mun.yew.tham@...el.com>, "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>, <netdev@...r.kernel.org>, Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>, Cornelia Huck <cohuck@...hat.com>, "Linux MMC List" <linux-mmc@...r.kernel.org>, Joakim Zhang <qiangqing.zhang@....com>, linux-spi <linux-spi@...r.kernel.org>, Linux-Renesas <linux-renesas-soc@...r.kernel.org>, Vinod Koul <vkoul@...nel.org>, "James Morse" <james.morse@....com>, Zha Qipeng <qipeng.zha@...el.com>, "Sebastian Reichel" <sre@...nel.org>, Niklas Söderlund <niklas.soderlund@...natech.se>, <linux-mediatek@...ts.infradead.org>, "Brian Norris" <computersforpeace@...il.com>, "David S. Miller" <davem@...emloft.net> Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional 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... > 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? > 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). > ... 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? 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 ;-)). > 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? > Now I'm unwilling to continue the discussion unless there pops up a > suggestion that results in a considerable part (say > 10%) of the > drivers using platform_get_irq_optional not having to check if the > return value corresponds to "not found". Note that many actual drivers don't follow the pattern prescribed in the comment to platform_get_irq_optional() and their check for an optional IRQ look like irq < 0 (and, after my patches, irq <= 0). Maybe we shouldn't even bother returning the error codes and just return 0 for all kinds of misfortunes instead? :-) > Best regards > Uwe MBR, Sergey
Powered by blists - more mailing lists