lkml.org 
[lkml]   [2020]   [Dec]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] arch: fix 'unexpected IRQ trap at vector' warnings
From
Date
On 09.12.20 00:01, Thomas Gleixner wrote:

> There are a few situations why it is invoked or not:
>
> 1) The original x86 usage is not longer using it because it complains
> rightfully about a vector being raised which has no interrupt
> descriptor associated to it. So the original reason for naming it
> vector is gone long ago. It emits:
>
> pr_emerg_ratelimited("%s: %d.%u No irq handler for vector\n",
> __func__, smp_processor_id(), vector);
>
> Directly from the x86 C entry point without ever invoking that
> function. Pretty popular error message due to some AMD BIOS
> wreckage. :)

Of course, the term "vector" should be replaced by something like
"irqnr" or "virq", but I didn't have name changes within scope - just
wanted to fix the printing of that number, as i've stupled over it while
working on something different and wondered why the number differed from
what I had expected, until I seen that it prints hex instead of decimal.

But if you prefer a more complete cleanup, I'll be happy to do it.

> 3) It's invoked from __handle_domain_irq() when the 'hwirq' which is
> handed in by the caller does not resolve to a mapped Linux
> interrupt which is pretty much the same as the x86 situation above
> in #1, but it prints useless data.
>
> It prints 'irq' which is invalid but it does not print the really
> interesting 'hwirq' which was handed in by the caller and did
> not resolve.

I wouldn't say the irq-nr isn't interesting. In my particular case it
was quite what I've been looking for. But you're right, hwirq should
also be printed.

> In this case the Linux irq number is uninteresting as it is known
> to be invalid and simply is not mapped and therefore does not
> exist.

In my case it came in from generic_handle_irq(), and in this case this
irq number (IMHO) has been valid, but nobody handled it, so it went to
ack_bad_irq.

Of course, if this function is meant as a fallback to ack some not
otherwise handled IRQ on the hw, the linux irq number indeed isn't quite
helpful (unless we expect that code to do a lookup to the hw irq).

... rethinking this further ... shouldn't we also pass in even more data
(eg. irq_desc, irqchip, ...), so this function can check which hw to
actually talk to ?

> 4) It's invoked from the dummy irq chip which is installed for a
> couple of truly virtual interrupts where the invocation of
> dummy_irq_chip::irq_ack() is indicating wreckage.
>
> In that case the Linux irq number is the thing which is printed.
>
> So no. It's not just inconsistent it's in some places outright
> wrong. What we really want is:
>
> ack_bad_irq(int hwirq, int virq)

is 'int' correct here ?

BTW: I also wonder why the virq is unsigned int, while hwirq (eg. in
struct irq_data) is unsigned long. shouldn't the virtual number space
be at least as big (or even bigger) than the hw one ?

{
> if (hwirq >= 0)
> print_useful_info(hwirq);
> if (virq > 0)
> print_useful_info(virq);
> arch_try_to_ack(hwirq, virq);
> }
>
> for this to make sense. Just fixing the existing printk() to be less
> wrong is not really an improvement.

Okay, makes sense.

OTOH: since both callers (dummychip.c, handle.c) already dump out before
ack_bad_irq(), do we need to print out anything at all ?

I've also seen that many archs increase a counter (some use long, others
atomic_t) - should we also consolidate this in an arch-independent way
in handle.c (or does kstat_incr_irqs_this_cpu already do this) ?

--mtx

--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

\
 
 \ /
  Last update: 2020-12-15 21:35    [W:0.087 / U:0.528 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site