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

On 4/19/23 02:27, 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.

Last I heard Rich and his family were on sabbatical in Indonesia. He pops up
from time to time but hasn't answered the last couple pokes I made at him.

He handed this patch off to Jeff Dionne who handed it off to me. Here is the
patch publicly posted on Rich's website two years ago:

  https://git.musl-libc.org/cgit/linux-sh/commit/?h=v5.16%2bj2&id=beb1f3ae8ad6

Does Linux require copyright assignments now? I thought even the FSF had finally
dropped that requirement...

Rob