diff mbox series

Fix J-core aic warning spam

Message ID ec905cf9-09de-a5d1-b8ee-0d874db4c301@landley.net (mailing list archive)
State New, archived
Headers show
Series Fix J-core aic warning spam | expand

Commit Message

Rob Landley April 18, 2023, 4:23 a.m. UTC
From: Rich Felker <dalias@libc.org>
Signed-off-by: Rob Landley <rob@landley.net>

Silence noisy boot messages (warning and stack dump for each IRQ) when booting
on J2 SOC.

---
 drivers/irqchip/irq-jcore-aic.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

John Paul Adrian Glaubitz April 18, 2023, 6:13 a.m. UTC | #1
Hi Rob!

On Mon, 2023-04-17 at 23:23 -0500, Rob Landley wrote:
> From: Rich Felker <dalias@libc.org>
> Signed-off-by: Rob Landley <rob@landley.net>
> 
> Silence noisy boot messages (warning and stack dump for each IRQ) when booting
> on J2 SOC.
> 
> ---
>  drivers/irqchip/irq-jcore-aic.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-jcore-aic.c b/drivers/irqchip/irq-jcore-aic.c
> index 5f47d8ee4ae3..730252cb7b08 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 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);
>  	domain = irq_domain_add_legacy(node, dom_sz - min_irq, min_irq, min_irq,
>  				       &jcore_aic_irqdomain_ops,
>  				       &jcore_aic);

This more looks like it's adding a missing call to irc_alloc_descs() rather than
silencing kernel messages. The latter would be a brushing over of an error while
the former would fix the actual problem, wouldn't it?

So, I think the patch title might be misleading.

Adrian
Geert Uytterhoeven April 18, 2023, 7:18 a.m. UTC | #2
Hi Adrian, Rob,

On Tue, Apr 18, 2023 at 8:19 AM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> On Mon, 2023-04-17 at 23:23 -0500, Rob Landley wrote:
> > From: Rich Felker <dalias@libc.org>
> > Signed-off-by: Rob Landley <rob@landley.net>
> >
> > 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.

> >       domain = irq_domain_add_legacy(node, dom_sz - min_irq, min_irq, min_irq,
> >                                      &jcore_aic_irqdomain_ops,
> >                                      &jcore_aic);
>
> This more looks like it's adding a missing call to irc_alloc_descs() rather than
> silencing kernel messages. The latter would be a brushing over of an error while
> the former would fix the actual problem, wouldn't it?
>
> So, I think the patch title might be misleading.

Indeed.

Gr{oetje,eeting}s,

                        Geert
Rob Landley April 18, 2023, 7:43 a.m. UTC | #3
On 4/18/23 01:13, John Paul Adrian Glaubitz wrote:
> Hi Rob!
> 
> On Mon, 2023-04-17 at 23:23 -0500, Rob Landley wrote:
>> From: Rich Felker <dalias@libc.org>
>> Signed-off-by: Rob Landley <rob@landley.net>
>> 
>> Silence noisy boot messages (warning and stack dump for each IRQ) when booting
>> on J2 SOC.
>>
>> ---
>>  drivers/irqchip/irq-jcore-aic.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/drivers/irqchip/irq-jcore-aic.c b/drivers/irqchip/irq-jcore-aic.c
>> index 5f47d8ee4ae3..730252cb7b08 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 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);
>>  	domain = irq_domain_add_legacy(node, dom_sz - min_irq, min_irq, min_irq,
>>  				       &jcore_aic_irqdomain_ops,
>>  				       &jcore_aic);
> 
> This more looks like it's adding a missing call to irc_alloc_descs() rather than
> silencing kernel messages. The latter would be a brushing over of an error while
> the former would fix the actual problem, wouldn't it?
> 
> So, I think the patch title might be misleading.

Rich never bothered to explain what he was doing:

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

At a quick glance it looks like the right fix to me (thus not a hack).

The lack of explanation is more pronounced in some of his other patches:

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

What actual problem is that trying to address? No idea. I don't seem to have hit
it, whatever it is, so didn't try to submit that anywhere.

These three:

http://git.musl-libc.org/cgit/linux-sh/commit/?h=v5.16%2bj2&id=4c7333b0fb9e
http://git.musl-libc.org/cgit/linux-sh/commit/?h=v5.16%2bj2&id=53ac9fc75ae0
http://git.musl-libc.org/cgit/linux-sh/commit/?h=v5.16%2bj2&id=262e1e5884da

Can _probably_ go upstream as is, but after my last round of such I still
haven't replied to Andrew Morton's

  https://lkml.iu.edu/hypermail/linux/kernel/2302.2/08040.html

Because

  https://landley.net/notes.html#22-02-2023
  https://landley.net/notes.html#24-02-2023

I've got another one since [attached] but the last time I pushed a one line
"removal of leftover debris in a header" obvious cleanup patch was
https://lkml.iu.edu/hypermail/linux/kernel/1603.2/00054.html and multiplying the
amount of explanation I had to do _then_ with 7 more years of continuing kernel
community ossification ala
https://web.archive.org/web/20200629223507/https://www.zdnet.com/article/linus-torvalds-looks-at-the-future-of-linux-kernel-developers-and-development/#:~:text=ages
and I expect getting that in to require an hour-long video presentation with
diagrams, and I am SO tired.

> Adrian

Rob
Rob Landley April 18, 2023, 8:09 a.m. UTC | #4
On 4/18/23 02:18, Geert Uytterhoeven wrote:
> Hi Adrian, Rob,
> 
> On Tue, Apr 18, 2023 at 8:19 AM John Paul Adrian Glaubitz
> <glaubitz@physik.fu-berlin.de> wrote:
>> On Mon, 2023-04-17 at 23:23 -0500, Rob Landley wrote:
>> > From: Rich Felker <dalias@libc.org>
>> > Signed-off-by: Rob Landley <rob@landley.net>
>> >
>> > 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?

(Not calling it at all technically worked, it was just noisy...)

Rob
John Paul Adrian Glaubitz April 18, 2023, 8:10 a.m. UTC | #5
On Tue, 2023-04-18 at 03:09 -0500, Rob Landley wrote:
> 
> On 4/18/23 02:18, Geert Uytterhoeven wrote:
> > Hi Adrian, Rob,
> > 
> > On Tue, Apr 18, 2023 at 8:19 AM John Paul Adrian Glaubitz
> > <glaubitz@physik.fu-berlin.de> wrote:
> > > On Mon, 2023-04-17 at 23:23 -0500, Rob Landley wrote:
> > > > From: Rich Felker <dalias@libc.org>
> > > > Signed-off-by: Rob Landley <rob@landley.net>
> > > > 
> > > > 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.

Adrian
Sergey Shtylyov April 18, 2023, 8:56 a.m. UTC | #6
On 4/18/23 10:18 AM, Geert Uytterhoeven wrote:
[...]
>>> From: Rich Felker <dalias@libc.org>
>>> Signed-off-by: Rob Landley <rob@landley.net>
>>>
>>> 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
[...]
>>> @@ -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.

   Then shounld't we use pr_err() (or stronger), not pr_info()?

[...]

> Gr{oetje,eeting}s,
> 
>                         Geert

MBR, Sergey
Rob Landley April 18, 2023, 9:14 a.m. UTC | #7
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:
>> > Hi Adrian, Rob,
>> > 
>> > On Tue, Apr 18, 2023 at 8:19 AM John Paul Adrian Glaubitz
>> > <glaubitz@physik.fu-berlin.de> wrote:
>> > > On Mon, 2023-04-17 at 23:23 -0500, Rob Landley wrote:
>> > > > From: Rich Felker <dalias@libc.org>
>> > > > Signed-off-by: Rob Landley <rob@landley.net>
>> > > > 
>> > > > 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...)

*shrug* You're the arch maintainer, you're welcome to change it. You can define
it to be as brittle as possible if you like. Me, I read "worse is better" long
ago (https://dreamsongs.com/RiseOfWorseIsBetter.html) and The Unix Philosophy by
Mike Gancarz (when it breaks, you get to keep the pieces) and generally try to
leave it up to the user what to do about things. I've also used a LOT of
secondhand hardware over the years that got repurposed after it stopped being
useful for its original task. (Right to repair and all that.)

But it's not my call...

Rob
Geert Uytterhoeven April 18, 2023, 9:16 a.m. UTC | #8
Hi Rob,

On Tue, Apr 18, 2023 at 10:59 AM Rob Landley <rob@landley.net> 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
> >> > <glaubitz@physik.fu-berlin.de> wrote:
> >> > > On Mon, 2023-04-17 at 23:23 -0500, Rob Landley wrote:
> >> > > > From: Rich Felker <dalias@libc.org>
> >> > > > Signed-off-by: Rob Landley <rob@landley.net>
> >> > > >
> >> > > > 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.

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?

Thanks!

Gr{oetje,eeting}s,

                        Geert
Rob Landley April 18, 2023, 9:18 a.m. UTC | #9
On 4/18/23 03:56, Sergey Shtylyov wrote:
> On 4/18/23 10:18 AM, Geert Uytterhoeven wrote:
> [...]
>>>> From: Rich Felker <dalias@libc.org>
>>>> Signed-off-by: Rob Landley <rob@landley.net>
>>>>
>>>> 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
> [...]
>>>> @@ -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.
> 
>    Then shounld't we use pr_err() (or stronger), not pr_info()?

That I agree with. Happy to repost the patch with that? (Or, you could just sed
's/pr_info/pr_err/' file.patch at your end if that's easier...)

Rob
Geert Uytterhoeven April 19, 2023, 7:27 a.m. UTC | #10
On Tue, Apr 18, 2023 at 11:56 PM Rob Landley <rob@landley.net> wrote:
>
> On 4/18/23 04:16, Geert Uytterhoeven wrote:
> > Hi Rob,
> >
> > On Tue, Apr 18, 2023 at 10:59 AM Rob Landley <rob@landley.net> 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
> >> >> > <glaubitz@physik.fu-berlin.de> wrote:
> >> >> > > On Mon, 2023-04-17 at 23:23 -0500, Rob Landley wrote:
> >> >> > > > From: Rich Felker <dalias@libc.org>
> >> >> > > > Signed-off-by: Rob Landley <rob@landley.net>
> >> >> > > >
> >> >> > > > 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
John Paul Adrian Glaubitz April 19, 2023, 7:29 a.m. UTC | #11
Hi Rob!

On Tue, 2023-04-18 at 16:19 -0500, Rob Landley wrote:
> (...)
> 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 ]---
> (...)
> rcu: srcu_init: Setting srcu_struct sizes based on contention.
> Initializing J-Core PIT at (ptrval) IRQ 16
> clocksource: jcore_pit_cs: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns:
> 1911260446 ns
> sched_clock: 32 bits at 1000MHz, resolution 1ns, wraps every 2147483647ns
> Local J-Core PIT init on cpu 0
> SH generic board support: scanning for clk providers
> Calibrating delay loop... 30.31 BogoMIPS (lpj=151552)
> pid_max: default: 32768 minimum: 301
> Mount-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
> Mountpoint-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
> CPU: J2
> rcu: Hierarchical SRCU implementation.
> printk: bootconsole [uartlite_a0] printing thread started
> smp: Bringing up secondary CPUs ...
> J2 SMP: requested start of cpu 1
> Local J-Core PIT init on cpu 1
> smp: Brought up 1 node, 2 CPUs
> SMP: Total of 2 processors activated (61.03 BogoMIPS).
> devtmpfs: initialized
> clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns:
> 19112604462750000 ns
> futex hash table entries: 512 (order: 1, 8192 bytes, linear)
> NET: Registered PF_NETLINK/PF_ROUTE protocol family
> clocksource: Switched to clocksource jcore_pit_cs
> NET: Registered PF_INET protocol family
> IP idents hash table entries: 2048 (order: 2, 16384 bytes, linear)
> tcp_listen_portaddr_hash hash table entries: 512 (order: 0, 4096 bytes, linear)
> Table-perturb hash table entries: 65536 (order: 6, 262144 bytes, linear)
> TCP established hash table entries: 1024 (order: 0, 4096 bytes, linear)
> TCP bind hash table entries: 1024 (order: 1, 8192 bytes, linear)
> TCP: Hash tables configured (established 1024 bind 1024)
> UDP hash table entries: 256 (order: 1, 8192 bytes, linear)
> UDP-Lite hash table entries: 256 (order: 1, 8192 bytes, linear)
> NET: Registered PF_UNIX/PF_LOCAL protocol family
> workingset: timestamp_bits=30 max_order=15 bucket_order=0
> squashfs: version 4.0 (2009/01/31) Phillip Lougher
> printk: console [ttyUL0] enabled
> printk: bootconsole [uartlite_a0] disabled
> printk: console [ttyUL0] printing thread started
> printk: bootconsole [uartlite_a0] printing thread stopped
> loop: module loaded
> jcore_spi abcd0040.spi: Runtime PM disabled, clock forced on.
> mmc_spi spi0.0: SD/MMC host mmc0, no DMA, no WP, no poweroff, cd polling
> NET: Registered PF_INET6 protocol family
> Segment Routing with IPv6
> In-situ OAM (IOAM) with IPv6
> NET: Registered PF_PACKET protocol family
> printk: console [netcon0] enabled
> netconsole: network logging started
> printk: console [netcon0] printing thread started
> mmc0: host does not support reading read-only switch, assuming write-enable
> mmc0: new SDHC card on SPI
> mmcblk0: mmc0:0000 SK32G 29.7 GiB
>  mmcblk0: p1
> devtmpfs: mounted
> Freeing unused kernel image (initmem) memory: 728K
> This architecture does not have kernel memory protection.
> Run /init as init process
> ifconfig: ioctl 8916: No such device
> sntp: sendto: Network unreachable
> sntp: time.google.com:123: Try again
> [?7hType exit when done.
> #

So, this definitely shows that we're missing the call to irq_alloc_descs() which means that the
original patch does not just address noisy boot messages but actually fixes the missing allocation
of IRQ descriptors which is why you're seeing all these error messages.

Thus, I would suggest adjusting the patch title and description as well as making the allocation
failure a fatal error as Geert suggested.

Adrian
John Paul Adrian Glaubitz April 19, 2023, 7:30 a.m. UTC | #12
On Wed, 2023-04-19 at 09:27 +0200, Geert Uytterhoeven wrote:
> On Tue, Apr 18, 2023 at 11:56 PM Rob Landley <rob@landley.net> wrote:
> > 
> > On 4/18/23 04:16, Geert Uytterhoeven wrote:
> > > Hi Rob,
> > > 
> > > On Tue, Apr 18, 2023 at 10:59 AM Rob Landley <rob@landley.net> 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
> > > > > > > <glaubitz@physik.fu-berlin.de> wrote:
> > > > > > > > On Mon, 2023-04-17 at 23:23 -0500, Rob Landley wrote:
> > > > > > > > > From: Rich Felker <dalias@libc.org>
> > > > > > > > > Signed-off-by: Rob Landley <rob@landley.net>
> > > > > > > > > 
> > > > > > > > > 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 April 19, 2023, 10:25 a.m. UTC | #13
Hi!

On Wed, 2023-04-19 at 09:29 +0200, John Paul Adrian Glaubitz wrote:
> Hi Rob!
> 
> On Tue, 2023-04-18 at 16:19 -0500, Rob Landley wrote:
> > (...)
> > 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 ]---
> > (...)
> > rcu: srcu_init: Setting srcu_struct sizes based on contention.
> > Initializing J-Core PIT at (ptrval) IRQ 16
> > clocksource: jcore_pit_cs: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns:
> > 1911260446 ns
> > sched_clock: 32 bits at 1000MHz, resolution 1ns, wraps every 2147483647ns
> > Local J-Core PIT init on cpu 0
> > SH generic board support: scanning for clk providers
> > Calibrating delay loop... 30.31 BogoMIPS (lpj=151552)
> > pid_max: default: 32768 minimum: 301
> > Mount-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
> > Mountpoint-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
> > CPU: J2
> > rcu: Hierarchical SRCU implementation.
> > printk: bootconsole [uartlite_a0] printing thread started
> > smp: Bringing up secondary CPUs ...
> > J2 SMP: requested start of cpu 1
> > Local J-Core PIT init on cpu 1
> > smp: Brought up 1 node, 2 CPUs
> > SMP: Total of 2 processors activated (61.03 BogoMIPS).
> > devtmpfs: initialized
> > clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns:
> > 19112604462750000 ns
> > futex hash table entries: 512 (order: 1, 8192 bytes, linear)
> > NET: Registered PF_NETLINK/PF_ROUTE protocol family
> > clocksource: Switched to clocksource jcore_pit_cs
> > NET: Registered PF_INET protocol family
> > IP idents hash table entries: 2048 (order: 2, 16384 bytes, linear)
> > tcp_listen_portaddr_hash hash table entries: 512 (order: 0, 4096 bytes, linear)
> > Table-perturb hash table entries: 65536 (order: 6, 262144 bytes, linear)
> > TCP established hash table entries: 1024 (order: 0, 4096 bytes, linear)
> > TCP bind hash table entries: 1024 (order: 1, 8192 bytes, linear)
> > TCP: Hash tables configured (established 1024 bind 1024)
> > UDP hash table entries: 256 (order: 1, 8192 bytes, linear)
> > UDP-Lite hash table entries: 256 (order: 1, 8192 bytes, linear)
> > NET: Registered PF_UNIX/PF_LOCAL protocol family
> > workingset: timestamp_bits=30 max_order=15 bucket_order=0
> > squashfs: version 4.0 (2009/01/31) Phillip Lougher
> > printk: console [ttyUL0] enabled
> > printk: bootconsole [uartlite_a0] disabled
> > printk: console [ttyUL0] printing thread started
> > printk: bootconsole [uartlite_a0] printing thread stopped
> > loop: module loaded
> > jcore_spi abcd0040.spi: Runtime PM disabled, clock forced on.
> > mmc_spi spi0.0: SD/MMC host mmc0, no DMA, no WP, no poweroff, cd polling
> > NET: Registered PF_INET6 protocol family
> > Segment Routing with IPv6
> > In-situ OAM (IOAM) with IPv6
> > NET: Registered PF_PACKET protocol family
> > printk: console [netcon0] enabled
> > netconsole: network logging started
> > printk: console [netcon0] printing thread started
> > mmc0: host does not support reading read-only switch, assuming write-enable
> > mmc0: new SDHC card on SPI
> > mmcblk0: mmc0:0000 SK32G 29.7 GiB
> >  mmcblk0: p1
> > devtmpfs: mounted
> > Freeing unused kernel image (initmem) memory: 728K
> > This architecture does not have kernel memory protection.
> > Run /init as init process
> > ifconfig: ioctl 8916: No such device
> > sntp: sendto: Network unreachable
> > sntp: time.google.com:123: Try again
> > [?7hType exit when done.
> > #
> 
> So, this definitely shows that we're missing the call to irq_alloc_descs() which means that the
> original patch does not just address noisy boot messages but actually fixes the missing allocation
> of IRQ descriptors which is why you're seeing all these error messages.
> 
> Thus, I would suggest adjusting the patch title and description as well as making the allocation
> failure a fatal error as Geert suggested.

I just wanted to perform a test build of the J2 kernel, but I'm getting lots of error messages
from the assembler about unknown opcodes when trying to build j2_defconfig.

Any ideas?

Adrian
Geert Uytterhoeven April 19, 2023, 11:53 a.m. UTC | #14
On Wed, Apr 19, 2023 at 12:25 PM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> I just wanted to perform a test build of the J2 kernel, but I'm getting lots of error messages
> from the assembler about unknown opcodes when trying to build j2_defconfig.

Looks like upstream gcc does not support -mj2, and thus the build fails
with "unknown opcode" due to the use of "cas.l".
Disabling CONFIG_SMP in j2_defconfig fixes/works around that.

Just checked with Arnd on IRC, who says opcodes/sh-opc.h was last updated
in a meaningful way back in 2005...

Gr{oetje,eeting}s,

                        Geert
D. Jeff Dionne April 19, 2023, 12:02 p.m. UTC | #15
On Apr 19, 2023, at 20:53, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Wed, Apr 19, 2023 at 12:25 PM John Paul Adrian Glaubitz
> <glaubitz@physik.fu-berlin.de> wrote:
>> I just wanted to perform a test build of the J2 kernel, but I'm getting lots of error messages
>> from the assembler about unknown opcodes when trying to build j2_defconfig.
> 
> Looks like upstream gcc does not support -mj2, and thus the build fails
> with "unknown opcode" due to the use of "cas.l".
> Disabling CONFIG_SMP in j2_defconfig fixes/works around that.

Rich was mainlining the GCC patches out of tree.  Most of that can be found here:
https://github.com/richfelker/musl-cross-make/blob/master/patches/gcc-9.4.0/0007-j2.diff

We need to look a bit more at the code generator before we upstream the J-Core patches, since the CPU roadmap does extend SHcompact.

Cheers,
J.

> 
> Just checked with Arnd on IRC, who says opcodes/sh-opc.h was last updated
> in a meaningful way back in 2005...
> 
> 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
D. Jeff Dionne April 19, 2023, 12:04 p.m. UTC | #16
On Apr 19, 2023, at 21:02, D. Jeff Dionne <djeffdionne@gmail.com> wrote:
> On Apr 19, 2023, at 20:53, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Wed, Apr 19, 2023 at 12:25 PM John Paul Adrian Glaubitz
>> <glaubitz@physik.fu-berlin.de> wrote:
>>> I just wanted to perform a test build of the J2 kernel, but I'm getting lots of error messages
>>> from the assembler about unknown opcodes when trying to build j2_defconfig.
>> 
>> Looks like upstream gcc does not support -mj2, and thus the build fails
>> with "unknown opcode" due to the use of "cas.l".
>> Disabling CONFIG_SMP in j2_defconfig fixes/works around that.
> 
> Rich was mainlining the GCC patches out of tree.  Most of that can be found here:
> https://github.com/richfelker/musl-cross-make/blob/master/patches/gcc-9.4.0/0007-j2.diff

Binutils side https://github.com/richfelker/musl-cross-make/blob/master/patches/binutils-2.33.1/0001-j2.diff

Other versions are also in the same repo.

J.

> 
> We need to look a bit more at the code generator before we upstream the J-Core patches, since the CPU roadmap does extend SHcompact.
> 
> Cheers,
> J.
> 
>> 
>> Just checked with Arnd on IRC, who says opcodes/sh-opc.h was last updated
>> in a meaningful way back in 2005...
>> 
>> 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
>
Rob Landley April 20, 2023, 1:06 a.m. UTC | #17
On 4/19/23 02:27, Geert Uytterhoeven wrote:
> On Tue, Apr 18, 2023 at 11:56 PM Rob Landley <rob@landley.net> wrote:
>>
>> On 4/18/23 04:16, Geert Uytterhoeven wrote:
>> > Hi Rob,
>> >
>> > On Tue, Apr 18, 2023 at 10:59 AM Rob Landley <rob@landley.net> 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
>> >> >> > <glaubitz@physik.fu-berlin.de> wrote:
>> >> >> > > On Mon, 2023-04-17 at 23:23 -0500, Rob Landley wrote:
>> >> >> > > > From: Rich Felker <dalias@libc.org>
>> >> >> > > > Signed-off-by: Rob Landley <rob@landley.net>
>> >> >> > > >
>> >> >> > > > 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
Rob Landley April 20, 2023, 1:24 a.m. UTC | #18
On 4/19/23 05:25, John Paul Adrian Glaubitz wrote:
> Hi!
> 
> On Wed, 2023-04-19 at 09:29 +0200, John Paul Adrian Glaubitz wrote:
>> Hi Rob!
>> 
>> On Tue, 2023-04-18 at 16:19 -0500, Rob Landley wrote:
>> > (...)
>> > 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 ]---
>> > (...)
>> > rcu: srcu_init: Setting srcu_struct sizes based on contention.
>> > Initializing J-Core PIT at (ptrval) IRQ 16
>> > clocksource: jcore_pit_cs: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns:
>> > 1911260446 ns
>> > sched_clock: 32 bits at 1000MHz, resolution 1ns, wraps every 2147483647ns
>> > Local J-Core PIT init on cpu 0
>> > SH generic board support: scanning for clk providers
>> > Calibrating delay loop... 30.31 BogoMIPS (lpj=151552)
>> > pid_max: default: 32768 minimum: 301
>> > Mount-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
>> > Mountpoint-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
>> > CPU: J2
>> > rcu: Hierarchical SRCU implementation.
>> > printk: bootconsole [uartlite_a0] printing thread started
>> > smp: Bringing up secondary CPUs ...
>> > J2 SMP: requested start of cpu 1
>> > Local J-Core PIT init on cpu 1
>> > smp: Brought up 1 node, 2 CPUs
>> > SMP: Total of 2 processors activated (61.03 BogoMIPS).
>> > devtmpfs: initialized
>> > clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns:
>> > 19112604462750000 ns
>> > futex hash table entries: 512 (order: 1, 8192 bytes, linear)
>> > NET: Registered PF_NETLINK/PF_ROUTE protocol family
>> > clocksource: Switched to clocksource jcore_pit_cs
>> > NET: Registered PF_INET protocol family
>> > IP idents hash table entries: 2048 (order: 2, 16384 bytes, linear)
>> > tcp_listen_portaddr_hash hash table entries: 512 (order: 0, 4096 bytes, linear)
>> > Table-perturb hash table entries: 65536 (order: 6, 262144 bytes, linear)
>> > TCP established hash table entries: 1024 (order: 0, 4096 bytes, linear)
>> > TCP bind hash table entries: 1024 (order: 1, 8192 bytes, linear)
>> > TCP: Hash tables configured (established 1024 bind 1024)
>> > UDP hash table entries: 256 (order: 1, 8192 bytes, linear)
>> > UDP-Lite hash table entries: 256 (order: 1, 8192 bytes, linear)
>> > NET: Registered PF_UNIX/PF_LOCAL protocol family
>> > workingset: timestamp_bits=30 max_order=15 bucket_order=0
>> > squashfs: version 4.0 (2009/01/31) Phillip Lougher
>> > printk: console [ttyUL0] enabled
>> > printk: bootconsole [uartlite_a0] disabled
>> > printk: console [ttyUL0] printing thread started
>> > printk: bootconsole [uartlite_a0] printing thread stopped
>> > loop: module loaded
>> > jcore_spi abcd0040.spi: Runtime PM disabled, clock forced on.
>> > mmc_spi spi0.0: SD/MMC host mmc0, no DMA, no WP, no poweroff, cd polling
>> > NET: Registered PF_INET6 protocol family
>> > Segment Routing with IPv6
>> > In-situ OAM (IOAM) with IPv6
>> > NET: Registered PF_PACKET protocol family
>> > printk: console [netcon0] enabled
>> > netconsole: network logging started
>> > printk: console [netcon0] printing thread started
>> > mmc0: host does not support reading read-only switch, assuming write-enable
>> > mmc0: new SDHC card on SPI
>> > mmcblk0: mmc0:0000 SK32G 29.7 GiB
>> >  mmcblk0: p1
>> > devtmpfs: mounted
>> > Freeing unused kernel image (initmem) memory: 728K
>> > This architecture does not have kernel memory protection.
>> > Run /init as init process
>> > ifconfig: ioctl 8916: No such device
>> > sntp: sendto: Network unreachable
>> > sntp: time.google.com:123: Try again
>> > [?7hType exit when done.
>> > #
>> 
>> So, this definitely shows that we're missing the call to irq_alloc_descs() which means that the
>> original patch does not just address noisy boot messages but actually fixes the missing allocation
>> of IRQ descriptors which is why you're seeing all these error messages.
>> 
>> Thus, I would suggest adjusting the patch title and description as well as making the allocation
>> failure a fatal error as Geert suggested.
> 
> I just wanted to perform a test build of the J2 kernel, but I'm getting lots of error messages
> from the assembler about unknown opcodes when trying to build j2_defconfig.

J-core binary toolchain tarball:

https://landley.net/toybox/downloads/binaries/toolchains/latest/sh2eb-linux-muslfdpic-cross.tar.xz

Build script on top of Rich's musl-cross-make:

https://landley.net/toybox/faq.html#cross2

https://github.com/landley/toybox/blob/master/scripts/mcm-buildall.sh

Fishing around in the for loop at the end of the script, the mcm-buildall.sh
invocation for _just_ sh2eb would be:

  ~/path/to/mcm-buildall.sh i686:: sh2eb:fdpic:--with-cpu=mj2

Alas, this will build an i686 compiler (twice!) before building the sh2eb
compiler, because the script makes portable toolchain tarballs which are
statically linked on the host, and you can't do that with glibc because Ulrich
Dr. Pepper was insane and his successors have been spineless, so it builds an
i686 musl compiler first. (Could do x86_64, but didn't see a reason to make it
less portable and use more memory? I should bench if it's noticeably faster...)

The script builds an i686 host compiler internally (see
BOOTSTRAP=i686-linux-musl near-ish the top), but gcc has this horrific two stage
build thing it does where it builds and then rebuilds builds itself with itself,
and that second i686 compiler would be the "builds with itself" one. (That's why
it's at the start of the list in the for loop.) If you try to build anything
ELSE with that compiler, autoconf winds up going "compiler can't create
executables" because the stupid, it burns...

Feel free to peel out simpler compiler toolchain builds if you've got a
preference? The interesting bits for your purposes are probably:

https://github.com/richfelker/musl-cross-make/blob/master/patches/gcc-9.4.0/0007-j2.diff

https://github.com/richfelker/musl-cross-make/blob/master/patches/binutils-2.33.1/0001-j2.diff

Jeff also has an sh2-elf toolchain build somewhere. That's more for bare metal,
but I've confirmed it builds the kernel (as of 5.x somewhere anyway).

Rob
Geert Uytterhoeven April 20, 2023, 7:11 a.m. UTC | #19
Hi Rob,

On Thu, Apr 20, 2023 at 2:51 AM Rob Landley <rob@landley.net> wrote:
> On 4/19/23 02:27, Geert Uytterhoeven wrote:
> > 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...

No, it doesn't, Linux just uses the DCO:

https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst#L379

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven April 20, 2023, 7:16 a.m. UTC | #20
Hi Rob,

On Thu, Apr 20, 2023 at 3:09 AM Rob Landley <rob@landley.net> wrote:
> On 4/19/23 05:25, John Paul Adrian Glaubitz wrote:
> > I just wanted to perform a test build of the J2 kernel, but I'm getting lots of error messages
> > from the assembler about unknown opcodes when trying to build j2_defconfig.
>
> J-core binary toolchain tarball:
>
> https://landley.net/toybox/downloads/binaries/toolchains/latest/sh2eb-linux-muslfdpic-cross.tar.xz
>
> Build script on top of Rich's musl-cross-make:
>
> https://landley.net/toybox/faq.html#cross2
>
> https://github.com/landley/toybox/blob/master/scripts/mcm-buildall.sh
>
> Fishing around in the for loop at the end of the script, the mcm-buildall.sh
> invocation for _just_ sh2eb would be:
>
>   ~/path/to/mcm-buildall.sh i686:: sh2eb:fdpic:--with-cpu=mj2
>
> Alas, this will build an i686 compiler (twice!) before building the sh2eb
> compiler, because the script makes portable toolchain tarballs which are
> statically linked on the host, and you can't do that with glibc because Ulrich
> Dr. Pepper was insane and his successors have been spineless, so it builds an
> i686 musl compiler first. (Could do x86_64, but didn't see a reason to make it
> less portable and use more memory? I should bench if it's noticeably faster...)

Good luck building Firefox with a 32-bit compiler ;-)

> The script builds an i686 host compiler internally (see
> BOOTSTRAP=i686-linux-musl near-ish the top), but gcc has this horrific two stage
> build thing it does where it builds and then rebuilds builds itself with itself,
> and that second i686 compiler would be the "builds with itself" one. (That's why
> it's at the start of the list in the for loop.) If you try to build anything
> ELSE with that compiler, autoconf winds up going "compiler can't create
> executables" because the stupid, it burns...
>
> Feel free to peel out simpler compiler toolchain builds if you've got a
> preference? The interesting bits for your purposes are probably:
>
> https://github.com/richfelker/musl-cross-make/blob/master/patches/gcc-9.4.0/0007-j2.diff
>
> https://github.com/richfelker/musl-cross-make/blob/master/patches/binutils-2.33.1/0001-j2.diff
>
> Jeff also has an sh2-elf toolchain build somewhere. That's more for bare metal,
> but I've confirmed it builds the kernel (as of 5.x somewhere anyway).

There's also [1], which is why it's good for Arnd to have the J-core
toolchain patches.

[1] https://mirrors.edge.kernel.org/pub/tools/crosstool/

Gr{oetje,eeting}s,

                        Geert
Rob Landley April 28, 2023, 3:24 a.m. UTC | #21
On 4/19/23 06:53, Geert Uytterhoeven wrote:
> On Wed, Apr 19, 2023 at 12:25 PM John Paul Adrian Glaubitz
> <glaubitz@physik.fu-berlin.de> wrote:
>> I just wanted to perform a test build of the J2 kernel, but I'm getting lots of error messages
>> from the assembler about unknown opcodes when trying to build j2_defconfig.
> 
> Looks like upstream gcc does not support -mj2, and thus the build fails
> with "unknown opcode" due to the use of "cas.l".
> Disabling CONFIG_SMP in j2_defconfig fixes/works around that.

Sorry I fell behind, busy week, catching up...

> Just checked with Arnd on IRC, who says opcodes/sh-opc.h was last updated
> in a meaningful way back in 2005...

The gcc developers still wanted physical copyright assignment signatures on
paper when we were submitting the j2 stuff upstream, and we had trouble getting
those from multiple developers on 3 continents, so appeasing them went on the
todo heap.

The kernel should build without the -mj2? (Or at least used to? Commit
f208b87b48d5 was tested at the time?) The j2 instruction set is mostly sh2 with
a couple backported instructions from sh3 (more efficient bit shifts), and it'll
work fine with the old sh2 versions (just less efficiently).

The other new instruction is cmpxchg (ala cas.l), but I thought we'd hardwired
the literal hex bytes into the kernel build so you didn't need the toolchain to
know about the new instruction? (It was only used directly in two places.) It's
quite possible somebody went "ew, don't do that" and thus it was "improved" not
to work with generally available toolchains. (I've only tested with the j2
toolchain in forever...)

In theory a single processor j2 build won't use cmpxchg, and should run on the
hardware just fine...

Rob
John Paul Adrian Glaubitz April 29, 2023, 8:07 p.m. UTC | #22
Hi Rob!

On Wed, 2023-04-19 at 20:24 -0500, Rob Landley wrote:
> J-core binary toolchain tarball:
> 
> https://landley.net/toybox/downloads/binaries/toolchains/latest/sh2eb-linux-muslfdpic-cross.tar.xz

OK, I can actually build a J2 kernel with that. Are there instructions somewhere which explain
how to boot such a kernel on a Turtle board? Then I can give it a try on my personal one.

And do you have a root filesystem available for J2 that I can use?

> Feel free to peel out simpler compiler toolchain builds if you've got a
> preference? The interesting bits for your purposes are probably:
> 
> https://github.com/richfelker/musl-cross-make/blob/master/patches/gcc-9.4.0/0007-j2.diff
> 
> https://github.com/richfelker/musl-cross-make/blob/master/patches/binutils-2.33.1/0001-j2.diff

Do you know who authored these patches? We should get these merged upstream which should be
easier these days since the FSF has lowered the barriers regarding the copyright assignment.

Adrian
Rob Landley April 30, 2023, 3:45 a.m. UTC | #23
On 4/29/23 15:07, John Paul Adrian Glaubitz wrote:
> Hi Rob!
> 
> On Wed, 2023-04-19 at 20:24 -0500, Rob Landley wrote:
>> J-core binary toolchain tarball:
>> 
>> https://landley.net/toybox/downloads/binaries/toolchains/latest/sh2eb-linux-muslfdpic-cross.tar.xz
> 
> OK, I can actually build a J2 kernel with that. Are there instructions somewhere which explain
> how to boot such a kernel on a Turtle board? Then I can give it a try on my personal one.

Just copy the vmlinux file to "vmlinux" on the sd card. (It needs an initramfs,
or in theory a built-in kernel command line from kconfig doing the root= thing,
but either way you should get serial output on the usb with something like "sudo
busybox microcom -s 115200 /dev/ttyACM0".)

I built 6.3 with toybox's mkroot.sh and booted it on the board, although that
was with a 10 patch stack in my tree. Only four of which are j-core related: one
is the boot warning spam, of them adds the ugly ethernet driver, one makes uio
work on nommu, and one adds support for a variable clock base because we plugged
in some peripheral hardware that wasn't clocked in base 10 back in
https://landley.net/notes-2020.html#04-11-2020 and... here, lemme just fling it
all up github:

  https://github.com/landley/linux/commits/turtle-v6.3

That's my current stack forwarded ported on top of 6.3. You probably don't need
most of that, but I booted it on the board a few days ago. (Yes, the ethernet
driver is terrible.)

> And do you have a root filesystem available for J2 that I can use?

I mostly use a toybox mkroot build. The easy way to build it yourself is:

  git clone https://github.com/landley/toybox
  cd toybox
  mkroot/mkroot.sh CROSS_COMPILE=/path/to/sh2eb-linux-muslfdpic-
  ls -l root/sh2eb

That bash script is only ~350 lines and reasonably self contained, probably 15
minutes to read through if you're curious. (I keep meaning to put an explainer
video on youtube...)

I can send a cpio.gz if you like, or dig up a busybox one if you'd prefer that?

>> The interesting bits for your purposes are probably:
>> 
>> https://github.com/richfelker/musl-cross-make/blob/master/patches/gcc-9.4.0/0007-j2.diff
>> 
>> https://github.com/richfelker/musl-cross-make/blob/master/patches/binutils-2.33.1/0001-j2.diff
> 
> Do you know who authored these patches?

I think some came from Sato-san and the rest from Rich, but I'm not 100% sure?
Rich has been the one maintaining them, and Jeff before that.

https://github.com/richfelker/musl-cross-make/commits/master/patches/gcc-5.3.0/0004-j2.diff
implies that the patch came from Patrick Oppenlander, who sounds familiar?
https://github.com/pattop

> We should get these merged upstream which should be
> easier these days since the FSF has lowered the barriers regarding the copyright assignment.

I'm all for it. Jeff had some more pending todo fixes for the toolchain that he
might want to sweep up into such a push.

Me, I want to switch over to llvm but Jeff thinks its optimizer is still
terrible for embedded systems. But then I've heard it had some pretty bad
regressions 12.0 to, so...

I'm still using 9.4 because when I tried to build 11.2 back in February it went:

sh2eb-linux-muslfdpic/src_gcc/libstdc++-v3/../libgcc/unwind-pe.h:270:25: error:
'_Unwind_gnu_Find_got' was not declared in this scope
  270 |               result += _Unwind_gnu_Find_got ((_Unwind_Ptr) u);

And I haven't circled around to try to dig up why yet. (I want to bisect the gcc
repo to see what commit broke it, but that involves getting the horrific
autoconf dependency stack working which went REALLY WEIRD since the last time I
looked at it, possibly version skew with debian oldstable package versions. The
tarball versions ship cooked ./configure scripts that don't need the full
gnu/aaaaaaaaah environment install with automake and everything...)

Rob
John Paul Adrian Glaubitz May 3, 2023, 8:32 a.m. UTC | #24
Hi Rob!

On Sat, 2023-04-29 at 22:45 -0500, Rob Landley wrote:
> On 4/29/23 15:07, John Paul Adrian Glaubitz wrote:
> > Hi Rob!
> > 
> > On Wed, 2023-04-19 at 20:24 -0500, Rob Landley wrote:
> > > J-core binary toolchain tarball:
> > > 
> > > https://landley.net/toybox/downloads/binaries/toolchains/latest/sh2eb-linux-muslfdpic-cross.tar.xz
> > 
> > OK, I can actually build a J2 kernel with that. Are there instructions somewhere which explain
> > how to boot such a kernel on a Turtle board? Then I can give it a try on my personal one.
> 
> Just copy the vmlinux file to "vmlinux" on the sd card. (It needs an initramfs,
> or in theory a built-in kernel command line from kconfig doing the root= thing,
> but either way you should get serial output on the usb with something like "sudo
> busybox microcom -s 115200 /dev/ttyACM0".)

Thanks. I found my J2 board in the meantime.

> I built 6.3 with toybox's mkroot.sh and booted it on the board, although that
> was with a 10 patch stack in my tree. Only four of which are j-core related: one
> is the boot warning spam, of them adds the ugly ethernet driver, one makes uio
> work on nommu, and one adds support for a variable clock base because we plugged
> in some peripheral hardware that wasn't clocked in base 10 back in
> https://landley.net/notes-2020.html#04-11-2020 and... here, lemme just fling it
> all up github:
> 
>   https://github.com/landley/linux/commits/turtle-v6.3

We should get as many of these patches upstreamed as possible.

I will also continue reviewing old patches from the mailing list, I already
assembled a list of older patches many of which have not been applied.

If possible, the original author of the various J2 patches should post them
for review.

> That's my current stack forwarded ported on top of 6.3. You probably don't need
> most of that, but I booted it on the board a few days ago. (Yes, the ethernet
> driver is terrible.)
> 
> > And do you have a root filesystem available for J2 that I can use?
> 
> I mostly use a toybox mkroot build. The easy way to build it yourself is:
> 
>   git clone https://github.com/landley/toybox
>   cd toybox
>   mkroot/mkroot.sh CROSS_COMPILE=/path/to/sh2eb-linux-muslfdpic-
>   ls -l root/sh2eb

Thanks. That should be enough.

> > Do you know who authored these patches?
> 
> I think some came from Sato-san and the rest from Rich, but I'm not 100% sure?
> Rich has been the one maintaining them, and Jeff before that.
> 
> https://github.com/richfelker/musl-cross-make/commits/master/patches/gcc-5.3.0/0004-j2.diff
> implies that the patch came from Patrick Oppenlander, who sounds familiar?
> https://github.com/pattop

Doesn't ring a bell at the moment.

> > We should get these merged upstream which should be
> > easier these days since the FSF has lowered the barriers regarding the copyright assignment.
> 
> I'm all for it. Jeff had some more pending todo fixes for the toolchain that he
> might want to sweep up into such a push.

Jeff, your turn, please ;-).

> Me, I want to switch over to llvm but Jeff thinks its optimizer is still
> terrible for embedded systems. But then I've heard it had some pretty bad
> regressions 12.0 to, so...

While SuperH/J2 is not an official LLVM target, there is actually an LLVM toolchain
for J2, see:

> https://github.com/francisvm/j2-llvm

> I'm still using 9.4 because when I tried to build 11.2 back in February it went:
> 
> sh2eb-linux-muslfdpic/src_gcc/libstdc++-v3/../libgcc/unwind-pe.h:270:25: error:
> '_Unwind_gnu_Find_got' was not declared in this scope
>   270 |               result += _Unwind_gnu_Find_got ((_Unwind_Ptr) u);
> 
> And I haven't circled around to try to dig up why yet. (I want to bisect the gcc
> repo to see what commit broke it, but that involves getting the horrific
> autoconf dependency stack working which went REALLY WEIRD since the last time I
> looked at it, possibly version skew with debian oldstable package versions. The
> tarball versions ship cooked ./configure scripts that don't need the full
> gnu/aaaaaaaaah environment install with automake and everything...)

GCC for SH itself works fine for me, so I assume an issue with your build environment.

Adrian
Rob Landley May 3, 2023, 12:39 p.m. UTC | #25
On 5/3/23 03:32, John Paul Adrian Glaubitz wrote:
> Hi Rob!
> 
> On Sat, 2023-04-29 at 22:45 -0500, Rob Landley wrote:
>> On 4/29/23 15:07, John Paul Adrian Glaubitz wrote:
>> > Hi Rob!
>> > 
>> > On Wed, 2023-04-19 at 20:24 -0500, Rob Landley wrote:
>> > > J-core binary toolchain tarball:
>> > > 
>> > > https://landley.net/toybox/downloads/binaries/toolchains/latest/sh2eb-linux-muslfdpic-cross.tar.xz
>> > 
>> > OK, I can actually build a J2 kernel with that. Are there instructions somewhere which explain
>> > how to boot such a kernel on a Turtle board? Then I can give it a try on my personal one.
>> 
>> Just copy the vmlinux file to "vmlinux" on the sd card. (It needs an initramfs,
>> or in theory a built-in kernel command line from kconfig doing the root= thing,
>> but either way you should get serial output on the usb with something like "sudo
>> busybox microcom -s 115200 /dev/ttyACM0".)
> 
> Thanks. I found my J2 board in the meantime.

Woo!

>> I built 6.3 with toybox's mkroot.sh and booted it on the board, although that
>> was with a 10 patch stack in my tree. Only four of which are j-core related: one
>> is the boot warning spam, of them adds the ugly ethernet driver, one makes uio
>> work on nommu, and one adds support for a variable clock base because we plugged
>> in some peripheral hardware that wasn't clocked in base 10 back in
>> https://landley.net/notes-2020.html#04-11-2020 and... here, lemme just fling it
>> all up github:
>> 
>>   https://github.com/landley/linux/commits/turtle-v6.3
> 
> We should get as many of these patches upstreamed as possible.

I'm all for it, and am very happy to hand patches off to people who can deal
with them, but I'm not personally very good at navigating the process of what
linux-kernel has become:

  https://landley.net/notes-2023.html#22-02-2023

I still feel guilty about not having replied to Andrew yet, but I dunno what to
_say_:

  https://landley.net/notes-2023.html#24-02-2023

Oh, that ncp cleanup patch should also remove SMB_SUPER_MAGIC which went away in
commit 939cbe5af5fb and USBDEVICE_SUPER_MAGIC which went away in commit
fb28d58b72aa. Those symbols _also_ went away more than 10 years ago. Whole lot
of dusty corners in this codebase nobody's bothered to clean them out in forever
because touching linux-kernel actively unpleasant, so people who might otherwise
speak up about the references to 2.1 and 2.2 kernel versions being maybe a bit
stale in
https://github.com/torvalds/linux/blob/v6.3/Documentation/filesystems/proc.rst#:~:text=may%20change%20slightly
don't think engaging with linux-kernel is worth it. (That whole "with enough
eyeballs all bugs are shallow" thing assumes you have participation from people
outside the core group. Which stops working when your community actively drives
them away...)

*shrug* I can respin the patch if you think it's worth trying? Last time I did
something similar I had to argue with people whose entire argument was "change
bad, accumulting cruft endlessly has no downsides":

  https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1102436.html

Which is odd because as the linux-kernel community ages and shrinks it's been
throwing multiple entire drivers overboard:

  http://www.phoronix.com/news/Linux-Disabling-RNDIS-Drivers
  https://fosstodon.org/@kernellogger/109697090093536263

But "change from outsider, bad!" and "we've driven too many developers away to
maintain the existing code" only conflict if you think that a shrinking clique
circling the wagons is a problem I guess. (I admit to bias on this one...)

> I will also continue reviewing old patches from the mailing list, I already
> assembled a list of older patches many of which have not been applied.

Cool, thanks. Some of the branches in Rich's repository have more, and I can dig
through my linux junk drawer here if your backlog runs low. :)

> If possible, the original author of the various J2 patches should post them
> for review.

I believe all the j2-specific kernel patches that weren't authored by me, Rich,
or Jeff have already gone in. Kernel patches from other people in our stacks
were generally saved from this list or lkml in the first place.

In my 6.3 patch stack, 1-6 are from me, 7 was from rich, 8 was Jeff and Rich, 9
is disgusting and shouldn't go in yet (only about 1/3 of that ethernet driver is
relevant), and 10 is a one line yank of an unnecessary config constraint with
explanation (done by Rich I think, we use uio on a nommu board to talk to
bespoke USB 2.0 hardware, among other things; I dunno what the unsupported
mmap() he mentioned would try to do on nommu but assuming it doesn't panic the
kernel we're fine because only a subset of the possible mmap options are _ever_
available on nommu (you can MAP_SHARED but not MAP_PRIVATE on files, anonymous
could theoretically MA_PRIVATE with range registers but I don't think any linux
port supports them?), so mmap not working for this isn't unusual in that context.)

>> > Do you know who authored these patches?
>> 
>> I think some came from Sato-san and the rest from Rich, but I'm not 100% sure?
>> Rich has been the one maintaining them, and Jeff before that.
>> 
>> https://github.com/richfelker/musl-cross-make/commits/master/patches/gcc-5.3.0/0004-j2.diff
>> implies that the patch came from Patrick Oppenlander, who sounds familiar?
>> https://github.com/pattop
> 
> Doesn't ring a bell at the moment.

He's around. He's contributed to toybox and musl and gcc on and off for years:

http://lists.landley.net/pipermail/toybox-landley.net/2017-September/017217.html
https://git.musl-libc.org/cgit/musl/commit/?id=a0d64dccbc8d
https://gcc.gnu.org/legacy-ml/gcc-help/2011-04/msg00067.html

And here's him offering a kernel signed-off-by:

https://lore.kernel.org/all/20200717011234.295692-1-patrick.oppenlander@gmail.com/T/

>> > We should get these merged upstream which should be
>> > easier these days since the FSF has lowered the barriers regarding the copyright assignment.
>> 
>> I'm all for it. Jeff had some more pending todo fixes for the toolchain that he
>> might want to sweep up into such a push.
> 
> Jeff, your turn, please ;-).
> 
>> Me, I want to switch over to llvm but Jeff thinks its optimizer is still
>> terrible for embedded systems. But then I've heard it had some pretty bad
>> regressions 12.0 to, so...
> 
> While SuperH/J2 is not an official LLVM target, there is actually an LLVM toolchain
> for J2, see:
> 
>> https://github.com/francisvm/j2-llvm

Yeah, I know. Alas, work on it stopped when the developer was hired by Apple:

  https://www.linkedin.com/in/francisvm/

That's actually the _second_ superh LLVM port I'm aware of, but we couldn't
convince Renesas to release theirs and it was kinda stale by that point anyway...

There's been some version skew since, but it doesn't look _that_ hard. Just... a
can of worms we've all been to busy with the other plates we're spinning to open.

>> I'm still using 9.4 because when I tried to build 11.2 back in February it went:
>> 
>> sh2eb-linux-muslfdpic/src_gcc/libstdc++-v3/../libgcc/unwind-pe.h:270:25: error:
>> '_Unwind_gnu_Find_got' was not declared in this scope
>>   270 |               result += _Unwind_gnu_Find_got ((_Unwind_Ptr) u);
>> 
>> And I haven't circled around to try to dig up why yet. (I want to bisect the gcc
>> repo to see what commit broke it, but that involves getting the horrific
>> autoconf dependency stack working which went REALLY WEIRD since the last time I
>> looked at it, possibly version skew with debian oldstable package versions. The
>> tarball versions ship cooked ./configure scripts that don't need the full
>> gnu/aaaaaaaaah environment install with automake and everything...)
> 
> GCC for SH itself works fine for me, so I assume an issue with your build environment.

I'm guessing you didn't build libstdc++? Without which you can't build a native
compiler because gcc is now written in C++. (GCC for _sh4_ works fine, including
native compiler. The sh2eb musl config builds the first pass but not the second,
some configuration that's not lining up, probably to do with fdpic given where
it broke. I need to bisect it to find out what the change was...)

> Adrian

Thanks,

Rob
diff mbox series

Patch

diff --git a/drivers/irqchip/irq-jcore-aic.c b/drivers/irqchip/irq-jcore-aic.c
index 5f47d8ee4ae3..730252cb7b08 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 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);
 	domain = irq_domain_add_legacy(node, dom_sz - min_irq, min_irq, min_irq,
 				       &jcore_aic_irqdomain_ops,
 				       &jcore_aic);