Hello!
On 2/14/22 11:54 AM, Geert Uytterhoeven 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.
Thanx! :-)
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?
Yes.
[...]
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?
Yes.
--- 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.
Hm, you're right... should be <= 0 there, it seems.
As this adds missing error handling, this is to be fixed by a separate patch later?
Propagating errors from the probe() method is a matter of separate patch, yes.
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".
Yes, probably... TY!
Gr{oetje,eeting}s,
Geert
MBR, Sergey