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

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

[Sergey Shtylyov]

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@omp.ru>

---
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;
 	}
 

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

[Greg Kroah-Hartman]

On Sat, Nov 06, 2021 at 11:26:47PM +0300, Sergey Shtylyov 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@omp.ru>
> 
> ---
> 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;

You need to get approval from the interrrupt developers for this type of
change, as it is a change and might break existing platforms, right?

thanks,

greg k-h

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

[Sergey Shtylyov]

Hello!

On 11/26/21 7:20 PM, Greg Kroah-Hartman 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@omp.ru>
>>
>> ---
>> 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;
> 
> You need to get approval from the interrrupt developers for this type of
> change, as it is a change and might break existing platforms, right?

   Well, that's always possible... :-)

   I do remember we had issues with the serial driver on the Alchemy MIPS SoCs
back in the 2.6.1x time frame: UART0 used IRQ0 there and the serial driver treated
0 as an indication of the polling mode, IIRC... Now Alchemies have the SoC IRQ0
mapped to the Linux IRQ8, so this is not a problem anymore (the 1st 8 IRQs are
mapped to the MIPS CPU IRQs and IRQ0 isn't even used)...

> thanks,
> 
> greg k-h

MBR, Sergey

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

[Andy Shevchenko]

On Sat, Nov 06, 2021 at 11:26:47PM +0300, Sergey Shtylyov 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!

You are changing the return value of platform_get_irq_optional().
The problem here is the proposed change doesn't bring any value in such
case. platform_get_irq_optional() should be able (at the end of the day)
to return 3 types of values (as other APIs do):
	 > 0: success
	== 0: IRQ not found
	 < 0: an error that must be consumed by the caller

0 is unexpected result for non-optional APIs and there you may try to play
tricks (like replacing it by error code).

There was a discussion around the topic:
https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/T/#u

Wanna help?

> Fixes: a85a6c86c25b ("driver core: platform: Clarify that IRQ 0 is invalid")

Not sure.

-- 
With Best Regards,
Andy Shevchenko

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

[Sergey Shtylyov]

On 12/9/21 11:06 PM, Andy Shevchenko 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!
> 
> You are changing the return value of platform_get_irq_optional().
> The problem here is the proposed change doesn't bring any value in such
> case. platform_get_irq_optional() should be able (at the end of the day)
> to return 3 types of values (as other APIs do):
> 	 > 0: success
> 	== 0: IRQ not found
> 	 < 0: an error that must be consumed by the caller

   I remember that was in your patch that got reverted right after being merged. ;-)
IMHO returning both error code and 0 on failure is a sign of a misdesigned API, it
makes the failure check unnecessarily complex and error prone.

> 0 is unexpected result for non-optional APIs and there you may try to play
> tricks (like replacing it by error code).
> 
> There was a discussion around the topic:
> https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/T/#u

   I don't see much of the discussion there...

> Wanna help?

   No, I'm afraid you're on your own here... 

>> Fixes: a85a6c86c25b ("driver core: platform: Clarify that IRQ 0 is invalid")
> 
> Not sure.

   Why? It fixes gthe IRQ0 problem, so that you don't have to check for IRQ0 in many callers
(for the subsytems that treat 0 as s/th special, like polling mode)... If you have something
to improve, you can do that atop of this patch...

MBR, Sergey

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

[Andy Shevchenko]

On Thu, Dec 09, 2021 at 11:21:41PM +0300, Sergey Shtylyov wrote:
> On 12/9/21 11:06 PM, Andy Shevchenko 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!
> > 
> > You are changing the return value of platform_get_irq_optional().
> > The problem here is the proposed change doesn't bring any value in such
> > case. platform_get_irq_optional() should be able (at the end of the day)
> > to return 3 types of values (as other APIs do):
> > 	 > 0: success
> > 	== 0: IRQ not found
> > 	 < 0: an error that must be consumed by the caller
> 
>    I remember that was in your patch that got reverted right after being merged. ;-)
> IMHO returning both error code and 0 on failure is a sign of a misdesigned API, it
> makes the failure check unnecessarily complex and error prone.

I dunno what you are talking about when you mentioned "0 on failure" because 0
is not the failure, that's what I'm trying to tell.

> > 0 is unexpected result for non-optional APIs and there you may try to play
> > tricks (like replacing it by error code).
> > 
> > There was a discussion around the topic:
> > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/T/#u
> 
>    I don't see much of the discussion there...

Indeed, it was split between two threads. Another one is this:
https://lore.kernel.org/linux-serial/20210407101713.8694-1-andriy.shevchenko@linux.intel.com/T/#u

> > Wanna help?
> 
>    No, I'm afraid you're on your own here... 
> 
> >> Fixes: a85a6c86c25b ("driver core: platform: Clarify that IRQ 0 is invalid")
> > 
> > Not sure.
> 
>    Why? It fixes gthe IRQ0 problem, so that you don't have to check for IRQ0 in many callers
> (for the subsytems that treat 0 as s/th special, like polling mode)... If you have something
> to improve, you can do that atop of this patch...

Because first we need to fix all users of platform_get_irq_optional().

-- 
With Best Regards,
Andy Shevchenko

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

[Marc Zyngier]

[Adding Geert]

On Sat, 06 Nov 2021 20:26:47 +0000,
Sergey Shtylyov <s.shtylyov@omp.ru> 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@omp.ru>
> 
> ---
> 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@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.

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

[Geert Uytterhoeven]

On Tue, Jan 4, 2022 at 10:26 AM Marc Zyngier <maz@kernel.org> wrote:
> [Adding Geert]
>
> On Sat, 06 Nov 2021 20:26:47 +0000,
> Sergey Shtylyov <s.shtylyov@omp.ru> 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@omp.ru>

> > --- 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.

https://lore.kernel.org/all/CAMuHMdUg3=q7gyaVHP0XcYUOo3PQUUv8Hc8wp5faVQ+bTBpg4A@mail.gmail.com

TL;DR: Probably only smsc911x Ethernet on the AP-SH4A-3A and
AP-SH4AD-0A boards, which should trigger the warning since v5.8.

> 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@kernel.org>

TBH, I don't see much point in this patch, as the WARN() has been
there since a while, and the end goal is to return zero instead of
-ENXIO for no interrupt, right?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Marc Zyngier]

On Tue, 04 Jan 2022 09:47:21 +0000,
Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> On Tue, Jan 4, 2022 at 10:26 AM Marc Zyngier <maz@kernel.org> wrote:
> > [Adding Geert]
> >
> > On Sat, 06 Nov 2021 20:26:47 +0000,
> > Sergey Shtylyov <s.shtylyov@omp.ru> 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@omp.ru>
> 
> > > --- 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.
> 
> https://lore.kernel.org/all/CAMuHMdUg3=q7gyaVHP0XcYUOo3PQUUv8Hc8wp5faVQ+bTBpg4A@mail.gmail.com
> 
> TL;DR: Probably only smsc911x Ethernet on the AP-SH4A-3A and
> AP-SH4AD-0A boards, which should trigger the warning since v5.8.
> 
> > 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@kernel.org>
> 
> TBH, I don't see much point in this patch, as the WARN() has been
> there since a while, and the end goal is to return zero instead of
> -ENXIO for no interrupt, right?

I think the end-goal is to never return 0. Either we return a valid
interrupt number, or we return an error. It should be the
responsibility of the caller to decide what they want to do in the
error case.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

[Greg Kroah-Hartman]

On Tue, Jan 04, 2022 at 10:48:00AM +0000, Marc Zyngier wrote:
> On Tue, 04 Jan 2022 09:47:21 +0000,
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > 
> > On Tue, Jan 4, 2022 at 10:26 AM Marc Zyngier <maz@kernel.org> wrote:
> > > [Adding Geert]
> > >
> > > On Sat, 06 Nov 2021 20:26:47 +0000,
> > > Sergey Shtylyov <s.shtylyov@omp.ru> 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@omp.ru>
> > 
> > > > --- 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.
> > 
> > https://lore.kernel.org/all/CAMuHMdUg3=q7gyaVHP0XcYUOo3PQUUv8Hc8wp5faVQ+bTBpg4A@mail.gmail.com
> > 
> > TL;DR: Probably only smsc911x Ethernet on the AP-SH4A-3A and
> > AP-SH4AD-0A boards, which should trigger the warning since v5.8.
> > 
> > > 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@kernel.org>
> > 
> > TBH, I don't see much point in this patch, as the WARN() has been
> > there since a while, and the end goal is to return zero instead of
> > -ENXIO for no interrupt, right?
> 
> I think the end-goal is to never return 0. Either we return a valid
> interrupt number, or we return an error. It should be the
> responsibility of the caller to decide what they want to do in the
> error case.

As 0 still is a valid irq for some platforms (as mentioned above), then
how is this ever going to be possible?

thanks,

greg k-h

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

[Marc Zyngier]

On Tue, 04 Jan 2022 10:53:34 +0000,
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> On Tue, Jan 04, 2022 at 10:48:00AM +0000, Marc Zyngier wrote:
> > On Tue, 04 Jan 2022 09:47:21 +0000,
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > 
> > > On Tue, Jan 4, 2022 at 10:26 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > Geert recently mentioned that a few architectures (such as sh?) still
> > > > use IRQ0 as something valid in limited cases.
> > > 
> > > https://lore.kernel.org/all/CAMuHMdUg3=q7gyaVHP0XcYUOo3PQUUv8Hc8wp5faVQ+bTBpg4A@mail.gmail.com
> > > 
> > > TL;DR: Probably only smsc911x Ethernet on the AP-SH4A-3A and
> > > AP-SH4AD-0A boards, which should trigger the warning since v5.8.
> > > 
> > > > 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@kernel.org>
> > > 
> > > TBH, I don't see much point in this patch, as the WARN() has been
> > > there since a while, and the end goal is to return zero instead of
> > > -ENXIO for no interrupt, right?
> > 
> > I think the end-goal is to never return 0. Either we return a valid
> > interrupt number, or we return an error. It should be the
> > responsibility of the caller to decide what they want to do in the
> > error case.
> 
> As 0 still is a valid irq for some platforms (as mentioned above), then
> how is this ever going to be possible?

Fixing the offending platforms should be a pre-requisite. At least the
ones we know about.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

[Geert Uytterhoeven]

Hi Marc,

On Tue, Jan 4, 2022 at 11:48 AM Marc Zyngier <maz@kernel.org> wrote:
> On Tue, 04 Jan 2022 09:47:21 +0000,
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, Jan 4, 2022 at 10:26 AM Marc Zyngier <maz@kernel.org> wrote:
> > > [Adding Geert]
> > >
> > > On Sat, 06 Nov 2021 20:26:47 +0000,
> > > Sergey Shtylyov <s.shtylyov@omp.ru> 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@omp.ru>
> >
> > > > --- 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.
> >
> > https://lore.kernel.org/all/CAMuHMdUg3=q7gyaVHP0XcYUOo3PQUUv8Hc8wp5faVQ+bTBpg4A@mail.gmail.com
> >
> > TL;DR: Probably only smsc911x Ethernet on the AP-SH4A-3A and
> > AP-SH4AD-0A boards, which should trigger the warning since v5.8.
> >
> > > 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@kernel.org>
> >
> > TBH, I don't see much point in this patch, as the WARN() has been
> > there since a while, and the end goal is to return zero instead of
> > -ENXIO for no interrupt, right?
>
> I think the end-goal is to never return 0. Either we return a valid
> interrupt number, or we return an error. It should be the
> responsibility of the caller to decide what they want to do in the
> error case.

This is platform_get_irq_optional(). All other *_optional() APIs
return 0 (or NULL[1]) in case the optional resource is not available.

[1] Most (all?) return pointers, NULL, or a negative error code.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Sergey Shtylyov]

On 1/4/22 12:47 PM, Geert Uytterhoeven 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@omp.ru>
> 
>>> --- 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.
> 
> https://lore.kernel.org/all/CAMuHMdUg3=q7gyaVHP0XcYUOo3PQUUv8Hc8wp5faVQ+bTBpg4A@mail.gmail.com
> 
> TL;DR: Probably only smsc911x Ethernet on the AP-SH4A-3A and
> AP-SH4AD-0A boards, which should trigger the warning since v5.8.

   Gr... indeed these use IRQ0 and should cause WARN. Do you have any idea how to avoid this?

>> 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@kernel.org>
> 
> TBH, I don't see much point in this patch, as the WARN() has been
> there since a while,

   Yet there's WARN() -- which (at the end of day) should be avoided.

> and the end goal is to return zero instead of
> -ENXIO for no interrupt, right?

   I don't care that much about platform_get_irq_optional() (Andy does),
I do care about its caller, platform_get_irq(). The end goal here is to
avoid WARN() and avoid having to handle IRQ0 in this function's callers.

> Gr{oetje,eeting}s,
> 
>                         Geert
[...]

MBR, Sergey

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

[Sergey Shtylyov]

On 12/10/21 2:17 PM, Andy Shevchenko 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!
>>>
>>> You are changing the return value of platform_get_irq_optional().
>>> The problem here is the proposed change doesn't bring any value in such
>>> case. platform_get_irq_optional() should be able (at the end of the day)
>>> to return 3 types of values (as other APIs do):
>>> 	 > 0: success
>>> 	== 0: IRQ not found
>>> 	 < 0: an error that must be consumed by the caller
>>
>>    I remember that was in your patch that got reverted right after being merged. ;-)
>> IMHO returning both error code and 0 on failure is a sign of a misdesigned API, it
>> makes the failure check unnecessarily complex and error prone.
> 
> I dunno what you are talking about when you mentioned "0 on failure" because 0
> is not the failure, that's what I'm trying to tell.

   OK.
 
>>> 0 is unexpected result for non-optional APIs and there you may try to play
>>> tricks (like replacing it by error code).
>>>
>>> There was a discussion around the topic:
>>> https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/T/#u
>>
>>    I don't see much of the discussion there...
> 
> Indeed, it was split between two threads. Another one is this:
> https://lore.kernel.org/linux-serial/20210407101713.8694-1-andriy.shevchenko@linux.intel.com/T/#u

   OK.

>>> Wanna help?
>>
>>    No, I'm afraid you're on your own here... 

   Tell me please, how far you've got with this by now?
   (I've already started to add the fixups to your patch -- unfortunately, this change has to be
done atomically, not piecemeal.)

>>>> Fixes: a85a6c86c25b ("driver core: platform: Clarify that IRQ 0 is invalid")
>>>
>>> Not sure.
>>
>>    Why? It fixes gthe IRQ0 problem, so that you don't have to check for IRQ0 in many callers
>> (for the subsytems that treat 0 as s/th special, like polling mode)... If you have something
>> to improve, you can do that atop of this patch...
> 
> Because first we need to fix all users of platform_get_irq_optional().

   I still don't understand why your issue should be fixed 1st -- but I don't really care about
the order...

MBR, Sergey

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

[Sergey Shtylyov]

Hello!

On 1/4/22 12:26 PM, Marc Zyngier 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@omp.ru>
>>
>> ---
>> 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.

   Yeah, valid IRQ0 on some SH boards -- which I believe could soon be addressed.

> 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:

   Of course, I'm prepared.

> 
> Acked-by: Marc Zyngier <maz@kernel.org>

   Thank you!

MBR, Sergey

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

[Andy Shevchenko]

On Tue, Jan 4, 2022 at 3:14 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Marc,
>
> On Tue, Jan 4, 2022 at 11:48 AM Marc Zyngier <maz@kernel.org> wrote:
> > On Tue, 04 Jan 2022 09:47:21 +0000,
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Tue, Jan 4, 2022 at 10:26 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > [Adding Geert]
> > > >
> > > > On Sat, 06 Nov 2021 20:26:47 +0000,
> > > > Sergey Shtylyov <s.shtylyov@omp.ru> 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@omp.ru>
> > >
> > > > > --- 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.
> > >
> > > https://lore.kernel.org/all/CAMuHMdUg3=q7gyaVHP0XcYUOo3PQUUv8Hc8wp5faVQ+bTBpg4A@mail.gmail.com
> > >
> > > TL;DR: Probably only smsc911x Ethernet on the AP-SH4A-3A and
> > > AP-SH4AD-0A boards, which should trigger the warning since v5.8.
> > >
> > > > 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@kernel.org>
> > >
> > > TBH, I don't see much point in this patch, as the WARN() has been
> > > there since a while, and the end goal is to return zero instead of
> > > -ENXIO for no interrupt, right?
> >
> > I think the end-goal is to never return 0. Either we return a valid
> > interrupt number, or we return an error. It should be the
> > responsibility of the caller to decide what they want to do in the
> > error case.
>
> This is platform_get_irq_optional(). All other *_optional() APIs
> return 0 (or NULL[1]) in case the optional resource is not available.

+1 to Geert's p.o.v. here. The platform_get_irq() and (non-optional)
Co should never return 0, while _optional variants as Geert explained.

> [1] Most (all?) return pointers, NULL, or a negative error code.



-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Jan 4, 2022 at 2:08 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Jan 04, 2022 at 10:48:00AM +0000, Marc Zyngier wrote:
> > On Tue, 04 Jan 2022 09:47:21 +0000,
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:

...

> > I think the end-goal is to never return 0. Either we return a valid
> > interrupt number, or we return an error. It should be the
> > responsibility of the caller to decide what they want to do in the
> > error case.
>
> As 0 still is a valid irq for some platforms (as mentioned above), then
> how is this ever going to be possible?

To avoid that we should first convert that SH case to use IRQ domains
instead of putting some arbitrary (HW dependent) values to IRQ
resources. When that is done it won't use vIRQ0 anymore. So, I agree
with Marc on that, except _optinal variants, but it doesn't contradict
the basic idea.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Jan 5, 2022 at 9:40 AM Sergey Shtylyov <s.shtylyov@omp.ru> wrote:
> On 12/10/21 2:17 PM, Andy Shevchenko wrote:

> >>> Wanna help?
> >>
> >>    No, I'm afraid you're on your own here...
>
>    Tell me please, how far you've got with this by now?
>    (I've already started to add the fixups to your patch -- unfortunately, this change has to be
> done atomically, not piecemeal.)

I just returned from vacation and will have another one soon, I don't
think I will be doing much for the next couple of weeks.


> >>>> Fixes: a85a6c86c25b ("driver core: platform: Clarify that IRQ 0 is invalid")
> >>>
> >>> Not sure.
> >>
> >>    Why? It fixes gthe IRQ0 problem, so that you don't have to check for IRQ0 in many callers
> >> (for the subsytems that treat 0 as s/th special, like polling mode)... If you have something
> >> to improve, you can do that atop of this patch...
> >
> > Because first we need to fix all users of platform_get_irq_optional().
>
>    I still don't understand why your issue should be fixed 1st -- but I don't really care about
> the order...

See my other comments on the discussion.
The rough roadmap is:
1) check which drivers are still subjects of vIRQ0 which is retrieved
via IRQ resource
2) fix them accordingly (for example, by transforming to IRQ domains)
3) convert platform_get_irq() and Co (including optional variants) to
follow the pattern
 a) non-optional APIs never return 0
 b) optional APIs return negative error, or positive vIRQ or 0 when
IRQ not found

Alternatively you may put a big comment in the drivers first and use
platform_get_resource() for retrieving IRQ0 without WARN(). Then they
will be subject to fix later on.

-- 
With Best Regards,
Andy Shevchenko

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

[Sergey Shtylyov]

On 1/5/22 1:09 PM, Andy Shevchenko wrote:

>>>>> Wanna help?
>>>>
>>>>    No, I'm afraid you're on your own here...
>>
>>    Tell me please, how far you've got with this by now?
>>    (I've already started to add the fixups to your patch -- unfortunately, this change has to be
>> done atomically, not piecemeal.)

   Now I'm doing "the 2nd pass" over the platform_get_irq_optional() patch, and I think I'm done
with the 2nd, platform_get_irq_byname_optional() patch... Expecting to post those 2 early next week.

> I just returned from vacation and will have another one soon, I don't
> think I will be doing much for the next couple of weeks.

   You've settled in well -- having (several!) vacations while I've been doing your homework for you
during my holidays. B-)

>>>>>> Fixes: a85a6c86c25b ("driver core: platform: Clarify that IRQ 0 is invalid")
>>>>>
>>>>> Not sure.
>>>>
>>>>    Why? It fixes gthe IRQ0 problem, so that you don't have to check for IRQ0 in many callers
>>>> (for the subsytems that treat 0 as s/th special, like polling mode)... If you have something
>>>> to improve, you can do that atop of this patch...
>>>
>>> Because first we need to fix all users of platform_get_irq_optional().
>>
>>    I still don't understand why your issue should be fixed 1st -- but I don't really care about
>> the order...
> 
> See my other comments on the discussion.
> The rough roadmap is:
> 1) check which drivers are still subjects of vIRQ0 which is retrieved
> via IRQ resource

   We have WARN() added for that -- which isn't even limited to the static IRQ resources...

> 2) fix them accordingly (for example, by transforming to IRQ domains)

   I think we may choose to do a quick workaround, with the IRQ domain transformation somewhat
deferred...

> 3) convert platform_get_irq() and Co (including optional variants) to
> follow the pattern
>  a) non-optional APIs never return 0
>  b) optional APIs return negative error, or positive vIRQ or 0 when
> IRQ not found

   Yeah, and that means that we should 1st convert your platform_do_get_irq() (I'm renaming it)to not return 0 on IRQ0 -- in order to avoid the ambiguity with the "IRQ not found" outcome.

> Alternatively you may put a big comment in the drivers first and use
> platform_get_resource() for retrieving IRQ0 without WARN(). Then they
> will be subject to fix later on.

  No -- that would be a step backward, I think...

MBR, Sergey

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

[Sergey Shtylyov]

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@omp.ru> 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@omp.ru>
>>
>> ---
>> 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@kernel.org>

   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)?

> 	M.

MBR, Sergey

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@omp.ru> 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@omp.ru>
>>> 
>>> ---
>>> 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@kernel.org>
> 
>    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...

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@omp.ru> 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@omp.ru>
> >>
> >> ---
> >> 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@kernel.org>
> 
>    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

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

[Sergey Shtylyov]

On 1/12/22 9:08 PM, Marc Zyngier 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@omp.ru>
>>>>
>>>> ---
>>>> 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@kernel.org>
>>
>>    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.

   Who argues with that? :-)
   However, I don't think it should be a pre-requisite for this patch,
so that we have extra time until 5.17 final... actually, I had couple
quick workarounds in mind; the problem however is that we don't seem to
have  the targets for testing... :-(

> Thanks,
> 
>         M.

MBR, Sergey

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

[Sergey Shtylyov]

On 1/12/22 9:19 PM, Greg Kroah-Hartman 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@omp.ru>
>>>>
>>>> ---
>>>> 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@kernel.org>
>>
>>    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.

   That's too bad...

> It would have had to be in my tree
> early last week to make it there,

   Marc's ACK was posted on January 4th, early last week in my calendar! ;-)

> my pull requests are already sent to
> Linus.

   Ah! Too bad.

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

   What series do you mean here, 2-patch series based atop of this patch?

> thanks,
> 
> greg k-h

MBR, Sergey

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

[Sergey Shtylyov]

On 1/12/22 9:08 PM, Marc Zyngier 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@omp.ru>
>>>>
>>>> ---
>>>> 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@kernel.org>
>>
>>    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.

   Patch posted now, see:

https://lore.kernel.org/all/2f419ed2-66b8-4098-7cd3-0fe698d341c9@omp.ru/.

> Thanks,
> 
>         M.

MBR, Sergey