[PATCH] irqchip/jcore-aic: Fix missing allocation of IRQ descriptors

[PATCH] irqchip/jcore-aic: Fix missing allocation of IRQ descriptors

[John Paul Adrian Glaubitz]

The initialization function for the J-Core AIC aic_irq_of_init() is
currently missing the call to irq_alloc_descs() which allocates and
initializes all the IRQ descriptors. Add missing function call and
return the error code from irq_alloc_descs() in case the allocation
fails.

Fixes: 981b58f66cfc ("irqchip/jcore-aic: Add J-Core AIC driver")
Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
---
 drivers/irqchip/irq-jcore-aic.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/irqchip/irq-jcore-aic.c b/drivers/irqchip/irq-jcore-aic.c
index 5f47d8ee4ae3..b9dcc8e78c75 100644
--- a/drivers/irqchip/irq-jcore-aic.c
+++ b/drivers/irqchip/irq-jcore-aic.c
@@ -68,6 +68,7 @@ static int __init aic_irq_of_init(struct device_node *node,
 	unsigned min_irq = JCORE_AIC2_MIN_HWIRQ;
 	unsigned dom_sz = JCORE_AIC_MAX_HWIRQ+1;
 	struct irq_domain *domain;
+	int ret;
 
 	pr_info("Initializing J-Core AIC\n");
 
@@ -100,6 +101,12 @@ static int __init aic_irq_of_init(struct device_node *node,
 	jcore_aic.irq_unmask = noop;
 	jcore_aic.name = "AIC";
 
+	ret = irq_alloc_descs(-1, min_irq, dom_sz - min_irq,
+			      of_node_to_nid(node));
+
+	if (ret < 0)
+		return ret;
+
 	domain = irq_domain_add_legacy(node, dom_sz - min_irq, min_irq, min_irq,
 				       &jcore_aic_irqdomain_ops,
 				       &jcore_aic);
-- 
2.39.2

Re: [PATCH] irqchip/jcore-aic: Fix missing allocation of IRQ descriptors

[Marc Zyngier]

On 2023-05-10 17:33, John Paul Adrian Glaubitz wrote:
> The initialization function for the J-Core AIC aic_irq_of_init() is
> currently missing the call to irq_alloc_descs() which allocates and
> initializes all the IRQ descriptors. Add missing function call and
> return the error code from irq_alloc_descs() in case the allocation
> fails.
> 
> Fixes: 981b58f66cfc ("irqchip/jcore-aic: Add J-Core AIC driver")
> Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> ---
>  drivers/irqchip/irq-jcore-aic.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-jcore-aic.c 
> b/drivers/irqchip/irq-jcore-aic.c
> index 5f47d8ee4ae3..b9dcc8e78c75 100644
> --- a/drivers/irqchip/irq-jcore-aic.c
> +++ b/drivers/irqchip/irq-jcore-aic.c
> @@ -68,6 +68,7 @@ static int __init aic_irq_of_init(struct device_node 
> *node,
>  	unsigned min_irq = JCORE_AIC2_MIN_HWIRQ;
>  	unsigned dom_sz = JCORE_AIC_MAX_HWIRQ+1;
>  	struct irq_domain *domain;
> +	int ret;
> 
>  	pr_info("Initializing J-Core AIC\n");
> 
> @@ -100,6 +101,12 @@ static int __init aic_irq_of_init(struct 
> device_node *node,
>  	jcore_aic.irq_unmask = noop;
>  	jcore_aic.name = "AIC";
> 
> +	ret = irq_alloc_descs(-1, min_irq, dom_sz - min_irq,
> +			      of_node_to_nid(node));
> +
> +	if (ret < 0)
> +		return ret;
> +
>  	domain = irq_domain_add_legacy(node, dom_sz - min_irq, min_irq, 
> min_irq,
>  				       &jcore_aic_irqdomain_ops,
>  				       &jcore_aic);

[- Jason]

It really begs the question: how has it ever been working before?

Is there any plan to modernise the port and get it to allocate
irq_descs on demand, as we do on most architectures?

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

Re: [PATCH] irqchip/jcore-aic: Fix missing allocation of IRQ descriptors

[John Paul Adrian Glaubitz]

Hi Jason!

On Thu, 2023-05-11 at 07:56 +0100, Marc Zyngier wrote:
> On 2023-05-10 17:33, John Paul Adrian Glaubitz wrote:
> > The initialization function for the J-Core AIC aic_irq_of_init() is
> > currently missing the call to irq_alloc_descs() which allocates and
> > initializes all the IRQ descriptors. Add missing function call and
> > return the error code from irq_alloc_descs() in case the allocation
> > fails.
> > 
> > Fixes: 981b58f66cfc ("irqchip/jcore-aic: Add J-Core AIC driver")
> > Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> > ---
> >  drivers/irqchip/irq-jcore-aic.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/irqchip/irq-jcore-aic.c 
> > b/drivers/irqchip/irq-jcore-aic.c
> > index 5f47d8ee4ae3..b9dcc8e78c75 100644
> > --- a/drivers/irqchip/irq-jcore-aic.c
> > +++ b/drivers/irqchip/irq-jcore-aic.c
> > @@ -68,6 +68,7 @@ static int __init aic_irq_of_init(struct device_node 
> > *node,
> >  	unsigned min_irq = JCORE_AIC2_MIN_HWIRQ;
> >  	unsigned dom_sz = JCORE_AIC_MAX_HWIRQ+1;
> >  	struct irq_domain *domain;
> > +	int ret;
> > 
> >  	pr_info("Initializing J-Core AIC\n");
> > 
> > @@ -100,6 +101,12 @@ static int __init aic_irq_of_init(struct 
> > device_node *node,
> >  	jcore_aic.irq_unmask = noop;
> >  	jcore_aic.name = "AIC";
> > 
> > +	ret = irq_alloc_descs(-1, min_irq, dom_sz - min_irq,
> > +			      of_node_to_nid(node));
> > +
> > +	if (ret < 0)
> > +		return ret;
> > +
> >  	domain = irq_domain_add_legacy(node, dom_sz - min_irq, min_irq, 
> > min_irq,
> >  				       &jcore_aic_irqdomain_ops,
> >  				       &jcore_aic);
> 
> [- Jason]
> 
> It really begs the question: how has it ever been working before?

Users already used a locally patched kernel to work around this problem.

> Is there any plan to modernise the port and get it to allocate
> irq_descs on demand, as we do on most architectures?

Yes, there are plans to modernize the port. We're first working on upstreaming
all kinds of patches that have been queuing up over the time.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] irqchip/jcore-aic: Fix missing allocation of IRQ descriptors

[Marc Zyngier]

On Thu, 11 May 2023 08:22:20 +0100,
John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> wrote:
> 
> Hi Jason!

???

> 
> On Thu, 2023-05-11 at 07:56 +0100, Marc Zyngier wrote:
> > On 2023-05-10 17:33, John Paul Adrian Glaubitz wrote:
> > > The initialization function for the J-Core AIC aic_irq_of_init() is
> > > currently missing the call to irq_alloc_descs() which allocates and
> > > initializes all the IRQ descriptors. Add missing function call and
> > > return the error code from irq_alloc_descs() in case the allocation
> > > fails.
> > > 
> > > Fixes: 981b58f66cfc ("irqchip/jcore-aic: Add J-Core AIC driver")
> > > Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> > > ---
> > >  drivers/irqchip/irq-jcore-aic.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/irqchip/irq-jcore-aic.c 
> > > b/drivers/irqchip/irq-jcore-aic.c
> > > index 5f47d8ee4ae3..b9dcc8e78c75 100644
> > > --- a/drivers/irqchip/irq-jcore-aic.c
> > > +++ b/drivers/irqchip/irq-jcore-aic.c
> > > @@ -68,6 +68,7 @@ static int __init aic_irq_of_init(struct device_node 
> > > *node,
> > >  	unsigned min_irq = JCORE_AIC2_MIN_HWIRQ;
> > >  	unsigned dom_sz = JCORE_AIC_MAX_HWIRQ+1;
> > >  	struct irq_domain *domain;
> > > +	int ret;
> > > 
> > >  	pr_info("Initializing J-Core AIC\n");
> > > 
> > > @@ -100,6 +101,12 @@ static int __init aic_irq_of_init(struct 
> > > device_node *node,
> > >  	jcore_aic.irq_unmask = noop;
> > >  	jcore_aic.name = "AIC";
> > > 
> > > +	ret = irq_alloc_descs(-1, min_irq, dom_sz - min_irq,
> > > +			      of_node_to_nid(node));
> > > +
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > >  	domain = irq_domain_add_legacy(node, dom_sz - min_irq, min_irq, 
> > > min_irq,
> > >  				       &jcore_aic_irqdomain_ops,
> > >  				       &jcore_aic);
> > 
> > [- Jason]
> > 
> > It really begs the question: how has it ever been working before?
> 
> Users already used a locally patched kernel to work around this problem.

You're not answering my question. Does it mean JCore never worked
upstream?

> > Is there any plan to modernise the port and get it to allocate
> > irq_descs on demand, as we do on most architectures?
> 
> Yes, there are plans to modernize the port. We're first working on
> upstreaming all kinds of patches that have been queuing up over the
> time.

I'd rather you skip that step and focus on making it work as a modern
architecture. This really looks like ARM circa 2007... :-/

	M.

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

Re: [PATCH] irqchip/jcore-aic: Fix missing allocation of IRQ descriptors

[John Paul Adrian Glaubitz]

On Thu, 2023-05-11 at 09:47 +0100, Marc Zyngier wrote:
> On Thu, 11 May 2023 08:22:20 +0100,
> John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> wrote:
> > 
> > Hi Jason!
> 
> ???

Sorry, I was confused by this:

> > > [- Jason]
> > > 
> > > It really begs the question: how has it ever been working before?
> > 
> > Users already used a locally patched kernel to work around this problem.
> 
> You're not answering my question. Does it mean JCore never worked
> upstream?

It did still work which is why the previously suggested change was to make a
failing call to irq_alloc_descs() non-fatal. The boards still booted up.

> > > Is there any plan to modernise the port and get it to allocate
> > > irq_descs on demand, as we do on most architectures?
> > 
> > Yes, there are plans to modernize the port. We're first working on
> > upstreaming all kinds of patches that have been queuing up over the
> > time.
> 
> I'd rather you skip that step and focus on making it work as a modern
> architecture. This really looks like ARM circa 2007... :-/

We have a patch-set for switching it to device tree in the pipeline.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] irqchip/jcore-aic: Fix missing allocation of IRQ descriptors

[Geert Uytterhoeven]

Hi Adrian,

On Wed, May 10, 2023 at 6:36 PM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> The initialization function for the J-Core AIC aic_irq_of_init() is
> currently missing the call to irq_alloc_descs() which allocates and
> initializes all the IRQ descriptors. Add missing function call and
> return the error code from irq_alloc_descs() in case the allocation
> fails.
>
> Fixes: 981b58f66cfc ("irqchip/jcore-aic: Add J-Core AIC driver")
> Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

Thanks for your patch!

I am not an IRQ expert, so I'd like to leave the technical parts for
e.g. Marc.  But I think you should add to the description that this
is based on a patch by Rich.

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] irqchip/jcore-aic: Fix missing allocation of IRQ descriptors

[John Paul Adrian Glaubitz]

Hi Geert!

On Thu, 2023-05-11 at 16:35 +0200, Geert Uytterhoeven wrote:
> Hi Adrian,
> 
> On Wed, May 10, 2023 at 6:36 PM John Paul Adrian Glaubitz
> <glaubitz@physik.fu-berlin.de> wrote:
> > The initialization function for the J-Core AIC aic_irq_of_init() is
> > currently missing the call to irq_alloc_descs() which allocates and
> > initializes all the IRQ descriptors. Add missing function call and
> > return the error code from irq_alloc_descs() in case the allocation
> > fails.
> > 
> > Fixes: 981b58f66cfc ("irqchip/jcore-aic: Add J-Core AIC driver")
> > Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> 
> Thanks for your patch!
> 
> I am not an IRQ expert, so I'd like to leave the technical parts for
> e.g. Marc.  But I think you should add to the description that this
> is based on a patch by Rich.

Will do! Thanks for the suggestions! I read through the IRQ source code
and checked what other architectures do and I think the first parameter
should be "-1" not 0.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] irqchip/jcore-aic: Fix missing allocation of IRQ descriptors

[Rob Landley]

On 5/10/23 11:33, John Paul Adrian Glaubitz wrote:
> The initialization function for the J-Core AIC aic_irq_of_init() is
> currently missing the call to irq_alloc_descs() which allocates and
> initializes all the IRQ descriptors. Add missing function call and
> return the error code from irq_alloc_descs() in case the allocation
> fails.
> 
> Fixes: 981b58f66cfc ("irqchip/jcore-aic: Add J-Core AIC driver")
> Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

Tested-by: Rob Landley <rob@landley.net>

Rob

Re: [PATCH] irqchip/jcore-aic: Fix missing allocation of IRQ descriptors

[Marc Zyngier]

On Thu, 11 May 2023 10:03:01 +0100,
John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> wrote:
> 
> On Thu, 2023-05-11 at 09:47 +0100, Marc Zyngier wrote:
> > On Thu, 11 May 2023 08:22:20 +0100,
> > John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> wrote:
> > > 
> > > Hi Jason!
> > 
> > ???
> 
> Sorry, I was confused by this:
> 
> > > > [- Jason]
> > > > 
> > > > It really begs the question: how has it ever been working before?
> > > 
> > > Users already used a locally patched kernel to work around this problem.
> > 
> > You're not answering my question. Does it mean JCore never worked
> > upstream?
> 
> It did still work which is why the previously suggested change was to make a
> failing call to irq_alloc_descs() non-fatal. The boards still booted
> up.

I don't get it. Either the descriptors are already allocated, and you
don't need this call, or they were never allocated and this never
worked. Which one is it?

> 
> > > > Is there any plan to modernise the port and get it to allocate
> > > > irq_descs on demand, as we do on most architectures?
> > > 
> > > Yes, there are plans to modernize the port. We're first working on
> > > upstreaming all kinds of patches that have been queuing up over the
> > > time.
> > 
> > I'd rather you skip that step and focus on making it work as a modern
> > architecture. This really looks like ARM circa 2007... :-/
> 
> We have a patch-set for switching it to device tree in the pipeline.

Again: why aren't we reviewing that instead of beating a long dead
horse?

	M.

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

Re: [PATCH] irqchip/jcore-aic: Fix missing allocation of IRQ descriptors

[John Paul Adrian Glaubitz]

On Tue, 2023-05-16 at 11:06 +0100, Marc Zyngier wrote:
> > It did still work which is why the previously suggested change was to make a
> > failing call to irq_alloc_descs() non-fatal. The boards still booted
> > up.
> 
> I don't get it. Either the descriptors are already allocated, and you
> don't need this call, or they were never allocated and this never
> worked. Which one is it?

I haven't tried it myself yet. The original report is that the kernel prints
a lot of backtraces for each IRQ descriptor not allocated but still works:

> SH generic board support: scanning for interrupt controllers
> Initializing J-Core AIC
> ------------[ cut here ]------------
> error: virq16 is not allocated
> WARNING: CPU: 0 PID: 0 at kernel/irq/irqdomain.c:571
> irq_domain_associate+0x120/0x178
> 
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.19.0-rc2 #1
> PC is at irq_domain_associate+0x120/0x178
> PR is at irq_domain_associate+0x120/0x178
> PC  : 10049b90 SP  : 103bdec0 SR  : 400001f1
> R0  : 0000001e R1  : 1042d024 R2  : 1042d024 R3  : 00000028
> R4  : 00000001 R5  : 0006f1ff R6  : 00000008 R7  : 103bde04
> R8  : 1200c000 R9  : 00000010 R10 : 00000000 R11 : 00000010
> R12 : 10049a70 R13 : 103bfcac R14 : 1030a398
> MACH: 00000000 MACL: 00057fa8 GBR : 00000000 PR  : 10049b90
> 
> Call trace:
>  [<100496f0>] __irq_domain_add+0x80/0x1dc
>  [<10049cd2>] irq_domain_create_legacy+0x46/0x68
>  [<10049a70>] irq_domain_associate+0x0/0x178
>  [<104517da>] aic_irq_of_init+0x82/0xd8
>  [<1020ab90>] of_iomap+0x0/0x30
>  [<1031df1c>] _printk+0x0/0x24
>  [<1045630c>] of_irq_init+0xe4/0x228
>  [<100a5a10>] kfree+0x0/0x250
>  [<10042376>] vprintk_emit+0xde/0x1fc
>  [<1004239c>] vprintk_emit+0x104/0x1fc
>  [<10309940>] strlen+0x0/0x60
>  [<100424a6>] vprintk_default+0x12/0x20
>  [<10309940>] strlen+0x0/0x60
>  [<10002a2c>] arch_local_save_flags+0x0/0x8
>  [<1031df1c>] _printk+0x0/0x24
>  [<104456f8>] init_IRQ+0x14/0x28
>  [<10309940>] strlen+0x0/0x60
>  [<10002a2c>] arch_local_save_flags+0x0/0x8
>  [<1031df1c>] _printk+0x0/0x24
>  [<1044394c>] start_kernel+0x3b8/0x73c
>  [<1044320c>] unknown_bootoption+0x0/0x170
>  [<1000202a>] _stext+0x2a/0x34
> 
> Code:
>   10049b8a:  mov.l     10049bd8 <irq_domain_associate+0x168/0x178>, r4  !
> 10393da0 <0x10393da0>
>   10049b8c:  jsr       @r1
>   10049b8e:  mov       r11, r5
> ->10049b90:  trapa     #62
>   10049b92:  bra       10049b0e
>   10049b94:  mov       #-22, r12
>   10049b96:  mov.l     10049bd0 <irq_domain_associate+0x160/0x178>, r1  !
> 1031da2c <__warn_printk+0x0/0x38>
>   10049b98:  mov.l     10049bdc <irq_domain_associate+0x16c/0x178>, r4  !
> 10393dc0 <0x10393dc0>
>   10049b9a:  jsr       @r1
> 
> ---[ end trace 0000000000000000 ]---

Adding the call to irq_alloc_descs() addresses the problem.

> > > > > Is there any plan to modernise the port and get it to allocate
> > > > > irq_descs on demand, as we do on most architectures?
> > > > 
> > > > Yes, there are plans to modernize the port. We're first working on
> > > > upstreaming all kinds of patches that have been queuing up over the
> > > > time.
> > > 
> > > I'd rather you skip that step and focus on making it work as a modern
> > > architecture. This really looks like ARM circa 2007... :-/
> > 
> > We have a patch-set for switching it to device tree in the pipeline.
> 
> Again: why aren't we reviewing that instead of beating a long dead
> horse?

Because we're not there yet. I just picked up maintenance of the SuperH port and we
want to fix various bugs first before tackling the big changes to the architecture.

Thanks,
Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

[irqchip: irq/irqchip-next] irqchip/jcore-aic: Fix missing allocation of IRQ descriptors

[irqchip-bot for John Paul Adrian Glaubitz]

The following commit has been merged into the irq/irqchip-next branch of irqchip:

Commit-ID:     4848229494a323eeaab62eee5574ef9f7de80374
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/4848229494a323eeaab62eee5574ef9f7de80374
Author:        John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
AuthorDate:    Wed, 10 May 2023 18:33:42 +02:00
Committer:     Marc Zyngier <maz@kernel.org>
CommitterDate: Sat, 17 Jun 2023 07:54:48 +01:00

irqchip/jcore-aic: Fix missing allocation of IRQ descriptors

The initialization function for the J-Core AIC aic_irq_of_init() is
currently missing the call to irq_alloc_descs() which allocates and
initializes all the IRQ descriptors. Add missing function call and
return the error code from irq_alloc_descs() in case the allocation
fails.

Fixes: 981b58f66cfc ("irqchip/jcore-aic: Add J-Core AIC driver")
Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
Tested-by: Rob Landley <rob@landley.net>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20230510163343.43090-1-glaubitz@physik.fu-berlin.de
---
 drivers/irqchip/irq-jcore-aic.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/irqchip/irq-jcore-aic.c b/drivers/irqchip/irq-jcore-aic.c
index 5f47d8e..b9dcc8e 100644
--- a/drivers/irqchip/irq-jcore-aic.c
+++ b/drivers/irqchip/irq-jcore-aic.c
@@ -68,6 +68,7 @@ static int __init aic_irq_of_init(struct device_node *node,
 	unsigned min_irq = JCORE_AIC2_MIN_HWIRQ;
 	unsigned dom_sz = JCORE_AIC_MAX_HWIRQ+1;
 	struct irq_domain *domain;
+	int ret;
 
 	pr_info("Initializing J-Core AIC\n");
 
@@ -100,6 +101,12 @@ static int __init aic_irq_of_init(struct device_node *node,
 	jcore_aic.irq_unmask = noop;
 	jcore_aic.name = "AIC";
 
+	ret = irq_alloc_descs(-1, min_irq, dom_sz - min_irq,
+			      of_node_to_nid(node));
+
+	if (ret < 0)
+		return ret;
+
 	domain = irq_domain_add_legacy(node, dom_sz - min_irq, min_irq, min_irq,
 				       &jcore_aic_irqdomain_ops,
 				       &jcore_aic);