Received: by 2002:a05:6a10:af89:0:0:0:0 with SMTP id iu9csp1177751pxb; Fri, 21 Jan 2022 11:32:51 -0800 (PST) X-Google-Smtp-Source: ABdhPJx0t9jJA0Svhp2HPTX5CUE2rLLI0nUiA1SDNQDZ6ZMmsUJb4+ZcooUj70slyQdSfS8Yvxv3 X-Received: by 2002:a17:902:6bc9:b0:149:b7bf:9b33 with SMTP id m9-20020a1709026bc900b00149b7bf9b33mr5420221plt.18.1642793570915; Fri, 21 Jan 2022 11:32:50 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1642793570; cv=none; d=google.com; s=arc-20160816; b=bPCtjVkbZ6n9RrNg2XmD5obtPxBMCadPxYjKlEYiZfYiBMhS7jojkF68PCxdIgS75m rL5hcG1rmcrQQwAKEsSqYMuUa34qerg3OfW2yEfjsm52bG8JAX6BYdCyPdwUULmjtyLO 9+RDBcM/JkmU/U4AXUXcOuuxLoBKD8tg6pbJkmJBH8nHBj+wMgjQegLXZQd0ZU/jZqNJ /I2XBRaulQCtiW+YZayqfTkchl/+mpB9qvzk4hdantMWoS48Il7rA8m/kLDq8j95go8a 5Tn/hEve1KlNP+DyGivgt9ksblO3jFdzeAosewDybWM7hJ2UEBN8Yl/TMjN3BqjmASpg YW2g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:content-language :in-reply-to:mime-version:user-agent:date:message-id:organization :references:cc:to:subject:from:dkim-filter; bh=UNUDFyVhArGgAp94vjqiKS3CPyvZ/gfaOxlwgxzstu4=; b=hjSEmYbCeQLU7mSswQzeJ2dhBCVB+qTku6mj3Sy1VH4szOapG7o2hW43w8QAqrl88Z IxptsUHbkN7FoLyq13jylrfXuAvMtsnORU8awBC3YoxsfChlvoKRVCsCoCBXrfNhMQLA ynS1qDz1sx+VZJsSpEFDyVW4YAuifKOB5K+ZLJZgNYYAT9LFZKEW4JtwngFhovmxvUh+ e+3KCN5J5sIyYd1N+zzDQ21OAqfj0WGi45baEZB0To7WkDR3a/sDwXgO8oXMWn7KOBKb swRSSTE3ocfzC/ypFvOkgIydKV+1650mRStldjcBirfb212qTL6Wn9IEB0wNNQuLIAHB QC7w== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id bc8si6509357plb.587.2022.01.21.11.32.37; Fri, 21 Jan 2022 11:32:50 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1355791AbiASPfF (ORCPT + 99 others); Wed, 19 Jan 2022 10:35:05 -0500 Received: from mxout03.lancloud.ru ([45.84.86.113]:36576 "EHLO mxout03.lancloud.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238476AbiASPfC (ORCPT ); Wed, 19 Jan 2022 10:35:02 -0500 Received: from LanCloud DKIM-Filter: OpenDKIM Filter v2.11.0 mxout03.lancloud.ru 761F020F6A92 Received: from LanCloud Received: from LanCloud Received: from LanCloud From: Sergey Shtylyov Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional To: =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= CC: Andrew Lunn , Ulf Hansson , Vignesh Raghavendra , KVM list , "Rafael J. Wysocki" , , "Linus Walleij" , Amit Kucheria , "ALSA Development Mailing List" , Andy Shevchenko , Jaroslav Kysela , Guenter Roeck , Thierry Reding , MTD Maling List , Linux I2C , Miquel Raynal , , Jiri Slaby , , "Khuong Dinh" , Florian Fainelli , Matthias Schiffer , Joakim Zhang , Kamal Dasu , Greg Kroah-Hartman , Lee Jones , "Bartosz Golaszewski" , Daniel Lezcano , Kishon Vijay Abraham I , Geert Uytterhoeven , "open list:SERIAL DRIVERS" , bcm-kernel-feedback-list , Zhang Rui , Linux PWM List , Robert Richter , Saravanan Sekar , Corey Minyard , Linux PM list , Linux Kernel Mailing List , Mauro Carvalho Chehab , John Garry , Peter Korsgaard , William Breathitt Gray , Mark Gross , "open list:GPIO SUBSYSTEM" , Alex Williamson , Mark Brown , Borislav Petkov , "Sebastian Reichel" , Eric Auger , Jakub Kicinski , Matthias Brugger , Takashi Iwai , , Benson Leung , Linux ARM , , Tony Luck , Mun Yew Tham , Hans de Goede , , Yoshihiro Shimoda , Cornelia Huck , "Linux MMC List" , Liam Girdwood , linux-spi , Linux-Renesas , Vinod Koul , "James Morse" , Zha Qipeng , "Pengutronix Kernel Team" , Richard Weinberger , =?UTF-8?Q?Niklas_S=c3=b6derlund?= , , Brian Norris , "David S. Miller" References: <20220112213121.5ruae5mxwj6t3qiy@pengutronix.de> <29f0c65d-77f2-e5b2-f6cc-422add8a707d@omp.ru> <20220114092557.jrkfx7ihg26ekzci@pengutronix.de> <61b80939-357d-14f5-df99-b8d102a4e1a1@omp.ru> <20220114202226.ugzklxv4wzr6egwj@pengutronix.de> <57af1851-9341-985e-7b28-d2ba86770ecb@omp.ru> <20220117084732.cdy2sash5hxp4lwo@pengutronix.de> <68d3bb7a-7572-7495-d295-e1d512ef509e@omp.ru> <20220118222606.3iwuzbenl7g6oeiq@pengutronix.de> Organization: Open Mobile Platform Message-ID: Date: Wed, 19 Jan 2022 18:34:53 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20220118222606.3iwuzbenl7g6oeiq@pengutronix.de> Content-Type: text/plain; charset="windows-1252" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [192.168.11.198] X-ClientProxiedBy: LFEXT02.lancloud.ru (fd00:f066::142) To LFEX1907.lancloud.ru (fd00:f066::207) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello! On 1/19/22 1:26 AM, Uwe Kleine-K?nig wrote: [...] >>>>>>> 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 ... >>>> >>>> Note that we should absolutely (and first of all) stop returning 0 from platform_get_irq() >>>> on a "real" IRQ0. Handling that "still good" zero absolutely doesn't scale e.g. for the subsystems >>>> (like libata) which take 0 as an indication that the polling mode should be used... We can't afford >>>> to be sloppy here. ;-) >>> >>> Then maybe do that really first? >> >> I'm doing it first already: >> >> https://lore.kernel.org/all/5e001ec1-d3f1-bcb8-7f30-a6301fd9930c@omp.ru/ >> >> This series is atop of the above patch... > > Ah, I missed that (probably because I didn't get the cover letter). > >>> I didn't recheck, but is this what the >>> driver changes in your patch is about? >> >> Partly, yes. We can afford to play with the meaning of 0 after the above patch. > > But the changes that are in patch 1 are all needed? Yes, they follow from the platform_get_irq_optional() changing the sense of its result... [...] >>> For my part I'd say this doesn't justify the change, but at least I >>> could better life with the reasoning. If you start at: >>> >>> irq = platform_get_irq_optional(...) >>> if (irq < 0 && irq != -ENXIO) >>> return irq >>> else if (irq > 0) >>> setup_irq(irq); >>> else >>> setup_polling() >>> >>> I'd change that to >>> >>> irq = platform_get_irq_optional(...) >>> if (irq > 0) /* or >= 0 ? */ >> >> Not >= 0, no... >> >>> setup_irq(irq) >>> else if (irq == -ENXIO) >>> setup_polling() >>> else >>> return irq >>> >>> This still has to mention -ENXIO, but this is ok and checking for 0 just >>> hardcodes a different return value. >> >> I think comparing with 0 is simpler (and shorter) than with -ENXIO, if you >> consider the RISC CPUs, like e.g. MIPS... > > Hmm, I don't know MIPS good enough to judge. So I created a small C MIPS has read-only register 0 (containing 0 :-)) which should simplify things. But I'd have to check the actual object code... yeah, MIPS has a branching instruction that compares 2 registers and branches on the result'; with -ENXIO you'd have to load an immediate value into a register first... > file: > > $ cat test.c > #include > > int platform_get_irq_optional(void); > void a(void); > > int func_0() > { > int irq = platform_get_irq_optional(); > > if (irq == 0) > a(); > } > > int func_enxio() > { > int irq = platform_get_irq_optional(); > > if (irq == -ENXIO) > a(); > } > > With some cross compilers as provided by Debian doing > > $CC -c -O3 test.c Mhm, do we really use -O3 to build the kernel? > nm --size-sort test.o > > I get: > > compiler | size of func_0 | size of func_enxio > ================================+==================|==================== > aarch64-linux-gnu-gcc | 0000000000000024 | 0000000000000028 > arm-linux-gnueabi-gcc | 00000018 | 00000018 > arm-linux-gnueabihf-gcc | 00000010 | 00000012 Hm, 2 bytes only -- perhaps Thumb mode? > i686-linux-gnu-gcc | 0000002a | 0000002a Expected. > mips64el-linux-gnuabi64-gcc | 0000000000000054 | 000000000000005c That's even more than expected -- 64-bit mode used? > powerpc-linux-gnu-gcc | 00000058 | 00000058 Well, they say > s390x-linux-gnu-gcc | 000000000000002e | 0000000000000030 > x86_64-linux-gnu-gcc | 0000000000000022 | 0000000000000022 Again, as expected... > So you save some bytes indeed. I see you have a lot of spare time (unlike me!). :-) >>> Anyhow, I think if you still want to change platform_get_irq_optional >>> you should add a few patches converting some drivers which demonstrates >>> the improvement for the callers. >> >> Mhm, I did include all the drivers where the IRQ checks have to be modified, >> not sure what else you want me to touch... > > I somehow expected that the changes that are now necessary (or possible) > to callers makes them prettier somehow. Looking at your patch again: I think they do... > > - drivers/counter/interrupt-cnt.c > This one is strange in my eyes because it tests the return value of > gpiod_get_optional against NULL :-( Mhm, how is this connected with my patch? :-/ > - drivers/edac/xgene_edac.c > This one just wants a silent irq lookup and then throws away the > error code returned by platform_get_irq_optional() to return -EINVAL. > Not so nice, is it? I have dropped this file from my (to be posted yet) v2... sorry that it took so long... > - drivers/gpio/gpio-altera.c > This one just wants a silent irq lookup. And maybe it should only > goto skip_irq if the irq was not found, but on an other error code > abort the probe?! That's debatable... but anyway it's a matter of a separate (follow up) patch... > > - drivers/gpio/gpio-mvebu.c > Similar to gpio-altera.c: Wants a silent irq and improved error > handling. Same as above... > - drivers/i2c/busses/i2c-brcmstb.c > A bit ugly that we now have dev->irq == 0 if the irq isn't available, > but if requesting the irq failed irq = -1 is used? This doesn't matter much really but can change to 0, if you want... :-) > > - drivers/mmc/host/sh_mmcif.c > Broken error handling. This one wants to abort on irq[1] < 0 (with > your changed semantic). Again, matter of a separate patch (I don't have the guily hardware anymore but I guess Geert could help with that). > I stopped here. Note that most of your complaints are about the existing driver logic -- which my patch just couldn't deal with... I currently don't have the bandwidth for addressing all your complaints; some (more obvious) I'm goiing to address as the time permits, the draft patches have been done already... > It seems quite common that drivers assume a value < 0 returned by > platform_get_irq means not-found Of course, before this patch -ENXIO meant IRQ-not-found, many drivers don't bother to filter it out separately (for simplicity?). > and don't care for -EPROBE_DEFER (what else can happen?). Hm, I haven't really seen a lot the probe dererral mishandling in the code touched by at least my patch #1... > Changing a relevant function in that mess seems unfortunate here :-\ You seem to have some spare time and I'm getting distracted contrariwise... want to help? :-) > Best regards > Uwe MBR, Sergey