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

[Marc Zyngier]

On 2022-01-12 17:53, 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)?

Fixing SH would be a good thing.

Thanks,

        M.
--
Jazz is not dead. It just smells funny...