Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: 2022-02-14 08:54:31
Also in:
alsa-devel, kvm, linux-gpio, linux-i2c, linux-iio, linux-mediatek, linux-mmc, linux-phy, linux-pm, linux-pwm, linux-renesas-soc, linux-serial, linux-spi, lkml, platform-driver-x86
Hi Sergey, On Sat, Feb 12, 2022 at 9:17 PM Sergey Shtylyov [off-list ref] 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/ (local) 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 <redacted> Signed-off-by: Sergey Shtylyov <redacted> --- Changes in version 2:
Thanks for the update!
drivers/base/platform.c | 60 +++++++++++++++---------
The core change LGTM. I'm only looking at Renesas drivers below...
quoted hunk ↗ jump to hunk
--- a/drivers/mmc/host/sh_mmcif.c +++ b/drivers/mmc/host/sh_mmcif.c@@ -1465,14 +1465,14 @@ static int sh_mmcif_probe(struct platform_device *pdev) sh_mmcif_sync_reset(host); sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL); - name = irq[1] < 0 ? dev_name(dev) : "sh_mmc:error"; + name = irq[1] <= 0 ? dev_name(dev) : "sh_mmc:error";
"== 0" should be sufficient here, if the code above would bail out on errors returned by platform_get_irq_optional(), which it currently doesn't do. As this adds missing error handling, this is to be fixed by a separate patch later?
ret = devm_request_threaded_irq(dev, irq[0], sh_mmcif_intr,
sh_mmcif_irqt, 0, name, host);
if (ret) {
dev_err(dev, "request_irq error (%s)\n", name);
goto err_clk;
}
- if (irq[1] >= 0) {
+ if (irq[1] > 0) {OK.
ret = devm_request_threaded_irq(dev, irq[1],
sh_mmcif_intr, sh_mmcif_irqt,
0, "sh_mmc:int", host);quoted hunk ↗ jump to hunk
--- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c@@ -439,7 +439,7 @@ static int rcar_gen3_phy_usb2_init(struct phy *p) u32 val; int ret; - if (!rcar_gen3_is_any_rphy_initialized(channel) && channel->irq >= 0) { + if (!rcar_gen3_is_any_rphy_initialized(channel) && channel->irq > 0) { INIT_WORK(&channel->work, rcar_gen3_phy_usb2_work); ret = request_irq(channel->irq, rcar_gen3_phy_usb2_irq, IRQF_SHARED, dev_name(channel->dev), channel);@@ -486,7 +486,7 @@ static int rcar_gen3_phy_usb2_exit(struct phy *p) val &= ~USB2_INT_ENABLE_UCOM_INTEN; writel(val, usb2_base + USB2_INT_ENABLE); - if (channel->irq >= 0 && !rcar_gen3_is_any_rphy_initialized(channel)) + if (channel->irq > 0 && !rcar_gen3_is_any_rphy_initialized(channel)) free_irq(channel->irq, channel); return 0;
LGTM, but note that all errors returned by platform_get_irq_optional() are currently ignored, even real errors, which should be propagated up. As this adds missing error handling, this is to be fixed by a separate patch later?
quoted hunk ↗ jump to hunk
--- a/drivers/thermal/rcar_gen3_thermal.c +++ b/drivers/thermal/rcar_gen3_thermal.c@@ -432,6 +432,8 @@ static int rcar_gen3_thermal_request_irqs(struct rcar_gen3_thermal_priv *priv, irq = platform_get_irq_optional(pdev, i); if (irq < 0) return irq; + if (!irq) + return -ENXIO;
While correct, and preserving existing behavior, this looks strange to me. Probably this should return zero instead (i.e. the check above should be changed to "<= 0"), and the caller should start caring about and propagating up real errors. As this adds missing error handling, this is to be fixed by a separate patch later?
quoted hunk ↗ jump to hunk
irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d", dev_name(dev), i);diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c index fb65dc601b23..328ab074fd89 100644
quoted hunk ↗ jump to hunk
--- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c
I think you missed
#define SCIx_IRQ_IS_MUXED(port) \
((port)->irqs[SCIx_ERI_IRQ] == \
(port)->irqs[SCIx_RXI_IRQ]) || \
((port)->irqs[SCIx_ERI_IRQ] && \
((port)->irqs[SCIx_RXI_IRQ] < 0))
above? The last condition should become "<= 0".
quoted hunk ↗ jump to hunk
@@ -1915,7 +1915,7 @@ static int sci_request_irq(struct sci_port *port) * Certain port types won't support all of the * available interrupt sources. */ - if (unlikely(irq < 0)) + if (unlikely(irq <= 0)) continue; }@@ -1963,7 +1963,7 @@ static void sci_free_irq(struct sci_port *port) * Certain port types won't support all of the available * interrupt sources. */ - if (unlikely(irq < 0)) + if (unlikely(irq <= 0)) continue; /* Check if already freed (irq was muxed) */@@ -2875,7 +2875,7 @@ static int sci_init_single(struct platform_device *dev, if (sci_port->irqs[0] < 0) return -ENXIO; - if (sci_port->irqs[1] < 0) + if (sci_port->irqs[1] <= 0) for (i = 1; i < ARRAY_SIZE(sci_port->irqs); i++) sci_port->irqs[i] = sci_port->irqs[0];
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-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