Re: [PATCH] Fix J-core aic warning spam

On Tue, Apr 18, 2023 at 11:56 PM Rob Landley <[email protected]> wrote:
>
> On 4/18/23 04:16, Geert Uytterhoeven wrote:
> > Hi Rob,
> >
> > On Tue, Apr 18, 2023 at 10:59 AM Rob Landley <[email protected]> wrote:
> >> On 4/18/23 03:10, John Paul Adrian Glaubitz wrote:
> >> > On Tue, 2023-04-18 at 03:09 -0500, Rob Landley wrote:
> >> >> On 4/18/23 02:18, Geert Uytterhoeven wrote:
> >> >> > On Tue, Apr 18, 2023 at 8:19 AM John Paul Adrian Glaubitz
> >> >> > <[email protected]> wrote:
> >> >> > > On Mon, 2023-04-17 at 23:23 -0500, Rob Landley wrote:
> >> >> > > > From: Rich Felker <[email protected]>
> >> >> > > > Signed-off-by: Rob Landley <[email protected]>
> >> >> > > >
> >> >> > > > Silence noisy boot messages (warning and stack dump for each IRQ) when booting
> >> >> > > > on J2 SOC.
> >> >> >
> >> >> > > > --- 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 rc;
> >> >> > > >
> >> >> > > >       pr_info("Initializing J-Core AIC\n");
> >> >> > > >
> >> >> > > > @@ -100,6 +101,11 @@ static int __init aic_irq_of_init(struct device_node *node,
> >> >> > > >       jcore_aic.irq_unmask = noop;
> >> >> > > >       jcore_aic.name = "AIC";
> >> >> > > >
> >> >> > > > +     rc = irq_alloc_descs(min_irq, min_irq, dom_sz - min_irq,
> >> >> > > > +                          of_node_to_nid(node));
> >> >> > > > +     if (rc < 0)
> >> >> > > > +             pr_info("Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
> >> >> > > > +                     min_irq);
> >> >> >
> >> >> > This is a fatal error, so please bail out, instead of continuing.
> >> >>
> >> >> If it can continue, it's not a fatal error. (Some pieces of hardware might not
> >> >> come up, but the board might still be usable.) If it can't continue, how does
> >> >> the _type_ of failure matter?
> >> >
> >> > I would still consider it fatal if any of the integral board components failed to
> >> > initialize. I don't think we want users to boot up their system into such an undefined
> >> > state.
> >>
> >> So if the network card doesn't work, kernel panic? If it's fatal, why does the
> >> function return? It could have called panic() instead. How does panicing _help_?
> >> (If the driver loads and the hardware works, we're good. If it doesn't, it won't
> >> work and they'll notice...)
> >
> > I didn't suggest to call panic(), just return rc.
>
> Ah, I misunderstood.
>
> > Diving deeper, irq_alloc_descs() can fail only when passing bad or severely
> > out-of-range values, so that's very unlikely.
> >
> > BTW, what are the noisy boot messages? What's the call chain?
>
> I have a log lying around somewhere...

Thanks!

It was a bit too large for the list, so I only kept the first relevant
part below...

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

OK, so virq 16-127 are non-functional without this fix.

One other thing to consider when sending a v2: v1 lacks an SoB
from the original author.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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