* Re: [PATCH] platform: finally disallow IRQ0 in platform_get_irq() and its ilk
2022-01-04 9:26 ` Marc Zyngier
@ 2022-01-04 9:47 ` Geert Uytterhoeven
2022-01-04 10:48 ` Marc Zyngier
2022-01-04 12:23 ` Sergey Shtylyov
2022-01-04 20:30 ` Sergey Shtylyov
2022-01-12 17:53 ` Sergey Shtylyov
2 siblings, 2 replies; 25+ messages in thread
From: Geert Uytterhoeven @ 2022-01-04 9:47 UTC (permalink / raw)
To: Marc Zyngier
Cc: Sergey Shtylyov, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel, Thomas Gleixner
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] platform: finally disallow IRQ0 in platform_get_irq() and its ilk
2022-01-04 9:47 ` Geert Uytterhoeven
@ 2022-01-04 10:48 ` Marc Zyngier
2022-01-04 10:53 ` Greg Kroah-Hartman
2022-01-04 11:24 ` Geert Uytterhoeven
2022-01-04 12:23 ` Sergey Shtylyov
1 sibling, 2 replies; 25+ messages in thread
From: Marc Zyngier @ 2022-01-04 10:48 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Sergey Shtylyov, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel, Thomas Gleixner
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] platform: finally disallow IRQ0 in platform_get_irq() and its ilk
2022-01-04 10:48 ` Marc Zyngier
@ 2022-01-04 10:53 ` Greg Kroah-Hartman
2022-01-04 11:13 ` Marc Zyngier
2022-01-05 10:02 ` Andy Shevchenko
2022-01-04 11:24 ` Geert Uytterhoeven
1 sibling, 2 replies; 25+ messages in thread
From: Greg Kroah-Hartman @ 2022-01-04 10:53 UTC (permalink / raw)
To: Marc Zyngier
Cc: Geert Uytterhoeven, Sergey Shtylyov, Rafael J. Wysocki,
linux-kernel, Thomas Gleixner
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] platform: finally disallow IRQ0 in platform_get_irq() and its ilk
2022-01-04 10:53 ` Greg Kroah-Hartman
@ 2022-01-04 11:13 ` Marc Zyngier
2022-01-05 10:02 ` Andy Shevchenko
1 sibling, 0 replies; 25+ messages in thread
From: Marc Zyngier @ 2022-01-04 11:13 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Geert Uytterhoeven, Sergey Shtylyov, Rafael J. Wysocki,
linux-kernel, Thomas Gleixner
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] platform: finally disallow IRQ0 in platform_get_irq() and its ilk
2022-01-04 10:53 ` Greg Kroah-Hartman
2022-01-04 11:13 ` Marc Zyngier
@ 2022-01-05 10:02 ` Andy Shevchenko
1 sibling, 0 replies; 25+ messages in thread
From: Andy Shevchenko @ 2022-01-05 10:02 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Marc Zyngier, Geert Uytterhoeven, Sergey Shtylyov,
Rafael J. Wysocki, Linux Kernel Mailing List, Thomas Gleixner
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] platform: finally disallow IRQ0 in platform_get_irq() and its ilk
2022-01-04 10:48 ` Marc Zyngier
2022-01-04 10:53 ` Greg Kroah-Hartman
@ 2022-01-04 11:24 ` Geert Uytterhoeven
2022-01-05 9:59 ` Andy Shevchenko
1 sibling, 1 reply; 25+ messages in thread
From: Geert Uytterhoeven @ 2022-01-04 11:24 UTC (permalink / raw)
To: Marc Zyngier
Cc: Sergey Shtylyov, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel, Thomas Gleixner
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] platform: finally disallow IRQ0 in platform_get_irq() and its ilk
2022-01-04 11:24 ` Geert Uytterhoeven
@ 2022-01-05 9:59 ` Andy Shevchenko
0 siblings, 0 replies; 25+ messages in thread
From: Andy Shevchenko @ 2022-01-05 9:59 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Marc Zyngier, Sergey Shtylyov, Greg Kroah-Hartman,
Rafael J. Wysocki, Linux Kernel Mailing List, Thomas Gleixner
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] platform: finally disallow IRQ0 in platform_get_irq() and its ilk
2022-01-04 9:47 ` Geert Uytterhoeven
2022-01-04 10:48 ` Marc Zyngier
@ 2022-01-04 12:23 ` Sergey Shtylyov
1 sibling, 0 replies; 25+ messages in thread
From: Sergey Shtylyov @ 2022-01-04 12:23 UTC (permalink / raw)
To: Geert Uytterhoeven, Marc Zyngier
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel, Thomas Gleixner
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] platform: finally disallow IRQ0 in platform_get_irq() and its ilk
2022-01-04 9:26 ` Marc Zyngier
2022-01-04 9:47 ` Geert Uytterhoeven
@ 2022-01-04 20:30 ` Sergey Shtylyov
2022-01-12 17:53 ` Sergey Shtylyov
2 siblings, 0 replies; 25+ messages in thread
From: Sergey Shtylyov @ 2022-01-04 20:30 UTC (permalink / raw)
To: Marc Zyngier
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel,
Thomas Gleixner, Geert Uytterhoeven
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] platform: finally disallow IRQ0 in platform_get_irq() and its ilk
2022-01-04 9:26 ` Marc Zyngier
2022-01-04 9:47 ` Geert Uytterhoeven
2022-01-04 20:30 ` Sergey Shtylyov
@ 2022-01-12 17:53 ` Sergey Shtylyov
2022-01-12 18:08 ` Marc Zyngier
2022-01-12 18:19 ` Greg Kroah-Hartman
2 siblings, 2 replies; 25+ messages in thread
From: Sergey Shtylyov @ 2022-01-12 17:53 UTC (permalink / raw)
To: Marc Zyngier
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel,
Thomas Gleixner, Geert Uytterhoeven
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] platform: finally disallow IRQ0 in platform_get_irq() and its ilk
2022-01-12 17:53 ` Sergey Shtylyov
@ 2022-01-12 18:08 ` Marc Zyngier
2022-01-12 20:08 ` Sergey Shtylyov
2022-02-11 20:20 ` Sergey Shtylyov
2022-01-12 18:19 ` Greg Kroah-Hartman
1 sibling, 2 replies; 25+ messages in thread
From: Marc Zyngier @ 2022-01-12 18:08 UTC (permalink / raw)
To: Sergey Shtylyov
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel,
Thomas Gleixner, Geert Uytterhoeven
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...
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] platform: finally disallow IRQ0 in platform_get_irq() and its ilk
2022-01-12 18:08 ` Marc Zyngier
@ 2022-01-12 20:08 ` Sergey Shtylyov
2022-02-11 20:20 ` Sergey Shtylyov
1 sibling, 0 replies; 25+ messages in thread
From: Sergey Shtylyov @ 2022-01-12 20:08 UTC (permalink / raw)
To: Marc Zyngier
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel,
Thomas Gleixner, Geert Uytterhoeven
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] platform: finally disallow IRQ0 in platform_get_irq() and its ilk
2022-01-12 18:08 ` Marc Zyngier
2022-01-12 20:08 ` Sergey Shtylyov
@ 2022-02-11 20:20 ` Sergey Shtylyov
1 sibling, 0 replies; 25+ messages in thread
From: Sergey Shtylyov @ 2022-02-11 20:20 UTC (permalink / raw)
To: Marc Zyngier
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel,
Thomas Gleixner, Geert Uytterhoeven
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] platform: finally disallow IRQ0 in platform_get_irq() and its ilk
2022-01-12 17:53 ` Sergey Shtylyov
2022-01-12 18:08 ` Marc Zyngier
@ 2022-01-12 18:19 ` Greg Kroah-Hartman
2022-01-12 20:20 ` Sergey Shtylyov
1 sibling, 1 reply; 25+ messages in thread
From: Greg Kroah-Hartman @ 2022-01-12 18:19 UTC (permalink / raw)
To: Sergey Shtylyov
Cc: Marc Zyngier, Rafael J. Wysocki, linux-kernel, Thomas Gleixner,
Geert Uytterhoeven
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] platform: finally disallow IRQ0 in platform_get_irq() and its ilk
2022-01-12 18:19 ` Greg Kroah-Hartman
@ 2022-01-12 20:20 ` Sergey Shtylyov
0 siblings, 0 replies; 25+ messages in thread
From: Sergey Shtylyov @ 2022-01-12 20:20 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Marc Zyngier, Rafael J. Wysocki, linux-kernel, Thomas Gleixner,
Geert Uytterhoeven
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
^ permalink raw reply [flat|nested] 25+ messages in thread