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

On Wed, 2023-04-19 at 09:27 +0200, Geert Uytterhoeven wrote:
> 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.

So, my original assessment that the patch title is misleading was correct then?

If Rich is not going to send a v2 of the patch anytime soon, I can do that myself.

Adrian

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