Re: [PATCH v2 1/2] driver core: platform: Clarify that IRQ 0 is invalid

[Bjorn Helgaas]

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 <bhelg...@google.com>
> > 
> > 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;
        }