Re: [PATCH] platform: finally disallow IRQ0 in platform_get_irq() and its ilk

[Greg Kroah-Hartman]

On Wed, Jan 12, 2022 at 08:53:51PM +0300, Sergey Shtylyov wrote:
> On 1/4/22 12:26 PM, Marc Zyngier wrote:
> 
> > [Adding Geert]
> > 
> > On Sat, 06 Nov 2021 20:26:47 +0000,
> > Sergey Shtylyov <s.shtylyov@xxxxxx> wrote:
> >>
> >> The commit a85a6c86c25b ("driver core: platform: Clarify that IRQ 0 is
> >> invalid") only calls WARN() when IRQ0 is about to be returned, however
> >> using IRQ0 is considered invalid (according to Linus) outside the arch/
> >> code where it's used by the i8253 drivers. Many driver subsystems treat
> >> 0 specially (e.g. as an indication of the polling mode by libata), so
> >> the users of platform_get_irq[_byname]() in them would have to filter
> >> out IRQ0 explicitly and this (quite obviously) doesn't scale...
> >> Let's finally get this straight and return -EINVAL instead of IRQ0!
> >>
> >> Fixes: a85a6c86c25b ("driver core: platform: Clarify that IRQ 0 is invalid")
> >> Signed-off-by: Sergey Shtylyov <s.shtylyov@xxxxxx>
> >>
> >> ---
> >> The patch is against the 'driver-core-linus' branch of Greg Kroah-Hartman's
> >> 'driver-core.git' repo.
> >>
> >>  drivers/base/platform.c |    6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> Index: driver-core/drivers/base/platform.c
> >> ===================================================================
> >> --- driver-core.orig/drivers/base/platform.c
> >> +++ driver-core/drivers/base/platform.c
> >> @@ -231,7 +231,8 @@ int platform_get_irq_optional(struct pla
> >>  out_not_found:
> >>  	ret = -ENXIO;
> >>  out:
> >> -	WARN(ret == 0, "0 is an invalid IRQ number\n");
> >> +	if (WARN(!ret, "0 is an invalid IRQ number\n"))
> >> +		return -EINVAL;
> >>  	return ret;
> >>  }
> >>  EXPORT_SYMBOL_GPL(platform_get_irq_optional);
> >> @@ -445,7 +446,8 @@ static int __platform_get_irq_byname(str
> >>  
> >>  	r = platform_get_resource_byname(dev, IORESOURCE_IRQ, name);
> >>  	if (r) {
> >> -		WARN(r->start == 0, "0 is an invalid IRQ number\n");
> >> +		if (WARN(!r->start, "0 is an invalid IRQ number\n"))
> >> +			return -EINVAL;
> >>  		return r->start;
> >>  	}
> > 
> > Geert recently mentioned that a few architectures (such as sh?) still
> > use IRQ0 as something valid in limited cases.
> > 
> > From my PoV, this patch is fine, but please be prepared to fix things
> > in a couple of years when someone decides to boot a recent kernel on
> > their pet dinosaur. With that in mind:
> > 
> > Acked-by: Marc Zyngier <maz@xxxxxxxxxx>
> 
>    Greg, so would that ACK be enough? Is there a chance this patch gets finally included
> into 5.17-rc1? Or should I look into fixing the recently found arch/sh/ issue 1st (as you
> can see, just WARN()'ing about IRQ0 wasn't enough to get this fixed)?

5.17-rc1 is way too late, sorry.  It would have had to be in my tree
early last week to make it there, my pull requests are already sent to
Linus.

Please fix this up properly, and resend the correct patch series, there
is no rush.

thanks,

greg k-h