Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional

Hi Sergey,

On Sat, Feb 12, 2022 at 9:17 PM Sergey Shtylyov s.shtylyov@omp.ru wrote:

This patch is based on the former Andy Shevchenko's patch:

https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux....

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@ew.tq-group.com Signed-off-by: Sergey Shtylyov s.shtylyov@omp.ru

---

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...

--- 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);

--- 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?

--- 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?

            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

--- 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".

@@ -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