On Sat, May 02, 2020 at 08:15:37AM +0200, Greg Kroah-Hartman wrote:
> On Fri, May 01, 2020 at 05:40:41PM -0500, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> >
> > These interfaces return a negative error number or an IRQ:
> >
> > platform_get_irq()
> > platform_get_irq_optional()
> > platform_get_irq_byname()
> > platform_get_irq_byname_optional()
> >
> > The function comments suggest checking for error like this:
> >
> > irq = platform_get_irq(...);
> > if (irq < 0)
> > return irq;
> >
> > which is what most callers (~900 of 1400) do, so it's implicit that IRQ 0
> > is invalid. But some callers check for "irq <= 0", and it's not obvious
> > from the source that we never return an IRQ 0.
> >
> > Make this more explicit by updating the comments to say that an IRQ number
> > is always non-zero and adding a WARN() if we ever do return zero. If we do
> > return IRQ 0, it likely indicates a bug in the arch-specific parts of
> > platform_get_irq().
>
> I worry about adding WARN() as there are systems that do panic_on_warn()
> and syzbot trips over this as well. I don't think that for this issue
> it would be a problem, but what really is this warning about that
> someone could do anything with?
>
> Other than that minor thing, this looks good to me, thanks for finally
> clearing this up.
What I'm concerned about is an arch that returns 0. Most drivers
don't check for 0 so they'll just try to use it, and things will fail
in some obscure way. My assumption is that if there really is no IRQ,
we should return -ENOENT or similar instead of 0.
I could be convinced that it's not worth warning about at all, or we
could do something like the following:
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 084cf1d23d3f..4afa5875e14d 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -220,7 +220,11 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
ret = -ENXIO;
#endif
out:
- WARN(ret == 0, "0 is an invalid IRQ number\n");
+ /* Returning zero here is likely a bug in the arch IRQ code */
+ if (ret == 0) {
+ pr_warn("0 is an invalid IRQ number\n");
+ dump_stack();
+ }
return ret;
}
EXPORT_SYMBOL_GPL(platform_get_irq_optional);
@@ -312,7 +316,11 @@ static int __platform_get_irq_byname(struct platform_device *dev,
r = platform_get_resource_byname(dev, IORESOURCE_IRQ, name);
if (r) {
- WARN(r->start == 0, "0 is an invalid IRQ number\n");
+ /* Returning zero here is likely a bug in the arch IRQ code */
+ if (r->start == 0) {
+ pr_warn("0 is an invalid IRQ number\n");
+ dump_stack();
+ }
return r->start;
}
