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: Wed, 19 Jan 2022 18:50:12 +0300 From: Sergey Shtylyov <s.shtylyov@....ru> To: Uwe Kleine-König <u.kleine-koenig@...gutronix.de> CC: 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>, Liam Girdwood <lgirdwood@...il.com>, <linux-iio@...r.kernel.org>, Linus Walleij <linus.walleij@...aro.org>, Amit Kucheria <amitk@...nel.org>, <alsa-devel@...a-project.org>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Sebastian Reichel <sre@...nel.org>, <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>, Lee Jones <lee.jones@...aro.org>, <openipmi-developer@...ts.sourceforge.net>, Saravanan Sekar <sravanhome@...il.com>, Khuong Dinh <khuong@...amperecomputing.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>, John Garry <john.garry@...wei.com>, "Robert Richter" <rric@...nel.org>, Zha Qipeng <qipeng.zha@...el.com>, Corey Minyard <minyard@....org>, <linux-pm@...r.kernel.org>, 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>, Matthias Brugger <matthias.bgg@...il.com>, Takashi Iwai <tiwai@...e.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, Benson Leung <bleung@...omium.org>, <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>, <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>, <linux-spi@...r.kernel.org>, <linux-renesas-soc@...r.kernel.org>, Vinod Koul <vkoul@...nel.org>, James Morse <james.morse@....com>, "Pengutronix Kernel Team" <kernel@...gutronix.de>, 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/19/22 6:02 PM, Uwe Kleine-König 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> >> [...] >> >> Please don't merge this as yet, I'm going thru this patch once again >> and have already found some sloppy code. :-/ > > Who would you expect to merge this? I would have expected Greg, but he Me too, it's his area, the message was addressed to Greg KH... > seems to have given up this thread. You instill too much uncertainty in him. :-) >>> diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c >>> index 7450904e330a..fdc63bfa5be4 100644 >>> --- a/drivers/char/ipmi/bt-bmc.c >>> +++ b/drivers/char/ipmi/bt-bmc.c >>> @@ -382,12 +382,14 @@ static int bt_bmc_config_irq(struct bt_bmc *bt_bmc, >>> bt_bmc->irq = platform_get_irq_optional(pdev, 0); >>> if (bt_bmc->irq < 0) >>> return bt_bmc->irq; >>> + if (!bt_bmc->irq) >>> + return 0; >> >> Hm, this is sloppy. Will recast and rebase to the -next branch. > > I didn't think about what you mean with sloppy, but the code is > equivalent to > > if (bt_bmc->irq <= 0) > return bt_bmc->irq; Exactly. [...] >>> diff --git a/drivers/edac/xgene_edac.c b/drivers/edac/xgene_edac.c >>> index 2ccd1db5e98f..0d1bdd27cd78 100644 >>> --- a/drivers/edac/xgene_edac.c >>> +++ b/drivers/edac/xgene_edac.c >>> @@ -1917,7 +1917,7 @@ static int xgene_edac_probe(struct platform_device *pdev) >>> >>> for (i = 0; i < 3; i++) { >>> irq = platform_get_irq_optional(pdev, i); >> >> Is *_optinal() even correct here? > > _optinal isn't correct, _optional maybe is. :-) No. :-) > Anyhow, look at e26124cd5f7099949109608845bba9e9bf96599c, the driver was > fixed not to print two error messages and the wrong option was picked. I think this patch is wrong... >>> - if (irq < 0) { >>> + if (irq <= 0) { >>> dev_err(&pdev->dev, "No IRQ resource\n"); This is what needed to be thrown overboard... :-) >>> rc = -EINVAL; >>> goto out_err; > > What's wrong here is that the return code is hardcoded ... This is wrong as well -- kills the deferred probing. I have 2 separate patches for this driver now... just need some time to get 'em ready... [...] >>> index bdf924b73e47..51289700a7ac 100644 >>> --- a/drivers/power/supply/mp2629_charger.c >>> +++ b/drivers/power/supply/mp2629_charger.c >>> @@ -581,9 +581,9 @@ static int mp2629_charger_probe(struct platform_device *pdev) >>> platform_set_drvdata(pdev, charger); >>> >>> irq = platform_get_irq_optional(to_platform_device(dev->parent), 0); >> >> Again, is *_optional() even correct here? >> >>> - if (irq < 0) { >>> + if (irq <= 0) { >>> dev_err(dev, "get irq fail: %d\n", irq); >>> - return irq; >>> + return irq < 0 ? irq : -ENXIO; > > Ack, could be simplified by switching to platform_get_irq(). Have a draft patch... > Best regards > Uwe MBR, Sergey
Powered by blists - more mailing lists