linux-kernel - Re: [PATCH 1/2] platform: make platform_get_irq

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 linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 18 Jan 2022 10:09:13 +0100
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
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, netdev <netdev@...r.kernel.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        openipmi-developer@...ts.sourceforge.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>,
        bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        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>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        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>,
        Eric Auger <eric.auger@...hat.com>,
        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>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        Cornelia Huck <cohuck@...hat.com>,
        Linux MMC List <linux-mmc@...r.kernel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        linux-spi <linux-spi@...r.kernel.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        Sergey Shtylyov <s.shtylyov@....ru>,
        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>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional

Hello Geert,

On Tue, Jan 18, 2022 at 09:25:01AM +0100, Geert Uytterhoeven wrote:
> On Mon, Jan 17, 2022 at 6:06 PM Uwe Kleine-König
> <u.kleine-koenig@...gutronix.de> wrote:
> > On Mon, Jan 17, 2022 at 02:08:19PM +0100, Geert Uytterhoeven wrote:
> > > On Mon, Jan 17, 2022 at 12:49 PM Uwe Kleine-König
> > > <u.kleine-koenig@...gutronix.de> wrote:
> > > > > So there are three reasons: because the absence of an optional IRQ
> > > > > is not an error, and thus that should not cause (a) an error code
> > > > > to be returned, and (b) an error message to be printed, and (c)
> > > > > because it can simplify the logic in device drivers.
> > > >
> > > > I don't agree to (a). If the value signaling not-found is -ENXIO or 0
> > > > (or -ENODEV) doesn't matter much. I wouldn't deviate from the return
> > > > code semantics of platform_get_irq() just for having to check against 0
> > > > instead of -ENXIO. Zero is then just another magic value.
> > >
> > > Zero is a natural magic value (also for pointers).
> > > Errors are always negative.
> > > Positive values are cookies (or pointers) associated with success.
> >
> > Yeah, the issue where we don't agree is if "not-found" is special enough
> > to deserve the natural magic value. For me -ENXIO is magic enough to
> > handle the absence of an irq line. I consider it even the better magic
> > value.
> 
> It differs from other subsystems (clk, gpio, reset), which do return
> zero on not found.

IMHO it doesn't matter at all that the return value is zero, relevant is
the semantic of the returned value. For clk, gpio, reset and regulator
NULL is a usable dummy, for irqs it's not. So what you do with the value
returned by platform_get_irq_whatever() is: you compare it with the
(magic?) not-found value, and if it matches, you enter a suitable
if-block.

For the (clk|gpiod|regulator)_get_optional() you don't have to check
against the magic not-found value (so no implementation detail magic
leaks into the caller code) and just pass it to the next API function.
(And my expectation would be that if you chose to represent not-found by
(void *)66 instead of NULL, you won't have to adapt any user, just the
framework internal checks. This is a good thing!)

> What's the point in having *_optional() APIs if they just return the
> same values as the non-optional ones?

The upside is that functions with a similar name have similar semantics.
But I agree, having platform_get_irq_optional() with the same return
value for not-found is bad. Changing the return semantic is only one way
to cope with that, renaming such the actual semantic difference is
obvious from the function name is another (IMHO better one). 

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)