Date:   Mon, 14 Feb 2022 10:01:14 +0100
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
Cc:     Sergey Shtylyov <s.shtylyov@....ru>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        linux-kernel@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Vignesh Raghavendra <vigneshr@...com>,
        Jiri Slaby <jirislaby@...nel.org>, linux-iio@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>,
        Amit Kucheria <amitk@...nel.org>, alsa-devel@...a-project.org,
        Liam Girdwood <lgirdwood@...il.com>,
        linux-phy@...ts.infradead.org,
        Thierry Reding <thierry.reding@...il.com>,
        linux-mtd@...ts.infradead.org, linux-i2c@...r.kernel.org,
        linux-gpio@...r.kernel.org,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Guenter Roeck <groeck@...omium.org>, linux-spi@...r.kernel.org,
        Lee Jones <lee.jones@...aro.org>,
        openipmi-developer@...ts.sourceforge.net,
        Peter Korsgaard <peter@...sgaard.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Matthias Schiffer <matthias.schiffer@...tq-group.com>,
        kvm@...r.kernel.org, Kamal Dasu <kdasu.kdev@...il.com>,
        Richard Weinberger <richard@....at>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Kishon Vijay Abraham I <kishon@...com>,
        bcm-kernel-feedback-list@...adcom.com,
        linux-serial@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
        Zhang Rui <rui.zhang@...el.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        platform-driver-x86@...r.kernel.org, linux-pwm@...r.kernel.org,
        Zha Qipeng <qipeng.zha@...el.com>,
        Corey Minyard <minyard@....org>, linux-pm@...r.kernel.org,
        John Garry <john.garry@...wei.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>,
        linux-mediatek@...ts.infradead.org,
        Matthias Brugger <matthias.bgg@...il.com>,
        Takashi Iwai <tiwai@...e.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Benson Leung <bleung@...omium.org>,
        linux-arm-kernel@...ts.infradead.org,
        Mun Yew Tham <mun.yew.tham@...el.com>,
        Eric Auger <eric.auger@...hat.com>, netdev@...r.kernel.org,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        Cornelia Huck <cohuck@...hat.com>, linux-mmc@...r.kernel.org,
        Joakim Zhang <qiangqing.zhang@....com>,
        Oleksij Rempel <linux@...pel-privat.de>,
        linux-renesas-soc@...r.kernel.org, Vinod Koul <vkoul@...nel.org>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Niklas Söderlund <niklas.soderlund@...natech.se>,
        Brian Norris <computersforpeace@...il.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional

Hi Uwe,

On Mon, Feb 14, 2022 at 8:29 AM Uwe Kleine-König
<u.kleine-koenig@...gutronix.de> wrote:
> On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> > This patch is based on the former Andy Shevchenko's patch:
> >
> > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
> >
> > Currently platform_get_irq_optional() returns an error code even if IRQ
> > resource simply has not been found.  It prevents the callers from being
> > error code agnostic in their error handling:
> >
> >       ret = platform_get_irq_optional(...);
> >       if (ret < 0 && ret != -ENXIO)
> >               return ret; // respect deferred probe
> >       if (ret > 0)
> >               ...we get an IRQ...
> >
> > All other *_optional() APIs seem to return 0 or NULL in case an optional
> > resource is not available.  Let's follow this good example, so that the
> > callers would look like:
> >
> >       ret = platform_get_irq_optional(...);
> >       if (ret < 0)
> >               return ret;
> >       if (ret > 0)
> >               ...we get an IRQ...
> >
> > Reported-by: Matthias Schiffer <matthias.schiffer@...tq-group.com>
> > Signed-off-by: Sergey Shtylyov <s.shtylyov@....ru>
>
> While this patch is better than v1, I still don't like it for the
> reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
> consider the real advantage for the other _get_optional() functions.)

IMHO the real advantage is the simplified error handling, which is the
area where most of the current bugs are. So I applaud the core change.

Also IMHO, the dummy value handling is a red herring.  Contrary to
optional clocks and resets, a missing optional interrupt does not
always mean there is nothing to do: in case of polling, something
else must definitely be done.  So even if request_irq() would accept
a dummy interrupt zero and just do nothing, it would give the false
impression that that is all there is to do, while an actual check
for zero with polling code handling may still need to be present,
thus leading to more not less bugs.

> Apart from that, I think the subject is badly chosen. With "Make
> somefunc() optional" I would expect that you introduce a Kconfig symbol
> that results in the function not being available when disabled.

Agreed.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Powered by blists - more mailing lists