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: Sat, 15 Jan 2022 23:22:16 +0300 From: Sergey Shtylyov <s.shtylyov@....ru> To: Uwe Kleine-König <u.kleine-koenig@...gutronix.de> CC: 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>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 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>, <openipmi-developer@...ts.sourceforge.net>, "David S. Miller" <davem@...emloft.net>, Khuong Dinh <khuong@...amperecomputing.com>, Florian Fainelli <f.fainelli@...il.com>, Matthias Schiffer <matthias.schiffer@...tq-group.com>, Joakim Zhang <qiangqing.zhang@....com>, Kamal Dasu <kdasu.kdev@...il.com>, 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>, Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>, bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>, Zhang Rui <rui.zhang@...el.com>, 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>, 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>, Mark Brown <broonie@...nel.org>, "Borislav Petkov" <bp@...en8.de>, Sebastian Reichel <sre@...nel.org>, Jakub Kicinski <kuba@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>, Takashi Iwai <tiwai@...e.com>, <platform-driver-x86@...r.kernel.org>, Benson Leung <bleung@...omium.org>, Linux ARM <linux-arm-kernel@...ts.infradead.org>, <linux-edac@...r.kernel.org>, Tony Luck <tony.luck@...el.com>, Mun Yew Tham <mun.yew.tham@...el.com>, Eric Auger <eric.auger@...hat.com>, "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>, "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>, Cornelia Huck <cohuck@...hat.com>, "Linux MMC List" <linux-mmc@...r.kernel.org>, Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, 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>, Pengutronix Kernel Team <kernel@...gutronix.de>, Richard Weinberger <richard@....at>, Niklas Söderlund <niklas.soderlund@...natech.se>, <linux-mediatek@...ts.infradead.org>, "Brian Norris" <computersforpeace@...il.com>, <netdev@...r.kernel.org> Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional 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
Powered by blists - more mailing lists