All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	Aman Sharma <amanharitsh123@gmail.com>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	Bjorn Helgaas <bhelgaas@google.com>
Subject: Re: [PATCH v2 1/2] driver core: platform: Clarify that IRQ 0 is invalid
Date: Mon, 4 May 2020 21:07:21 +0200	[thread overview]
Message-ID: <20200504190721.GA2810934@kroah.com> (raw)
In-Reply-To: <20200504180822.GA282766@bjorn-Precision-5520>

On Mon, May 04, 2020 at 01:08:22PM -0500, Bjorn Helgaas wrote:
> 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@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;
>  	}
>  

I like that, but you said this is something that the platform people
should only see when bringing up a new system, so maybe the WARN() is
fine.  It's not user-triggerable, so your original is ok.

sorry for the noise,

greg k-h

  reply	other threads:[~2020-05-04 19:07 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-01 22:40 [PATCH v2 0/2] PCI: Check for platform_get_irq() failure consistently Bjorn Helgaas
2020-05-01 22:40 ` [PATCH v2 1/2] driver core: platform: Clarify that IRQ 0 is invalid Bjorn Helgaas
2020-05-02  6:15   ` Greg Kroah-Hartman
2020-05-04 18:08     ` Bjorn Helgaas
2020-05-04 19:07       ` Greg Kroah-Hartman [this message]
2020-05-04 22:26         ` Bjorn Helgaas
2020-05-05 13:35           ` Greg Kroah-Hartman
2020-05-01 22:40 ` [PATCH v2 2/2] PCI: Check for platform_get_irq() failure consistently Bjorn Helgaas
2020-05-12 12:35 ` [PATCH v2 0/2] " Linus Walleij
2020-05-13 17:44 ` Bjorn Helgaas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200504190721.GA2810934@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=amanharitsh123@gmail.com \
    --cc=bhelgaas@google.com \
    --cc=helgaas@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=rafael@kernel.org \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.