linux-kernel - [PATCH] genirq: better error messages for disabled IRQs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20120203184353.GB18471@Brahman>
Date:	Fri, 3 Feb 2012 13:43:53 -0500
From:	Edward Donovan <edward.donovan@...ble.net>
To:	torvalds@...ux-foundation.org
Cc:	linux-kernel@...r.kernel.org
Subject: [PATCH] genirq: better error messages for disabled IRQs

When an unhandled IRQ is disabled, we tell the user, "try booting
with the irqpoll option."  Right now, we do this even when they
*have* tried it.  We can see perfectly well what boot args they
used, so let's make the log more helpful and accurate.  (This
would have helped me, and plenty of other users I've seen.)

The error message is already in an 'if' statement, and this patch
just adds a couple branches to it.  It checks whether 'irqfixup'
or 'irqpoll' were called, and has three versions of the error
message, accordingly.

The main thing is to acknowledge when these options have not
worked, so users are not left wondering if their boot parameters
are being read, or why the kernel is not listening to them.

The second thing is to suggest irqfixup, before irqpoll.  It
takes less CPU, and is meant as the first recourse.  The docs
recommend it first; now we do, too.

I have tried to make the wording clear, but two things are
debatable:

* When irqpoll has not been enough, a few different things could
be happening.  I left it at "most likely the firmware is too
broken".  We could say more or less, as desired.

More questionable:

* We're communicating to users who are often in over their heads
at this point.  So I think clarity is very important.  For
clarity's sake, I'd prefer to change "nobody cared" to "not
handled".  But I wouldn't want to cut the tie to archived
discussions.  So I left "nobody cared", in parentheses, in the
initial message.  Perhaps that continuity is the most important
thing, and it should still always say "nobody cared".  I don't
know.

Signed-off-by: Edward Donovan <edward.donovan@...ble.net>
---
 kernel/irq/spurious.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/kernel/irq/spurious.c b/kernel/irq/spurious.c
index 611cd60..e855222 100644
--- a/kernel/irq/spurious.c
+++ b/kernel/irq/spurious.c
@@ -194,9 +194,18 @@ __report_bad_irq(unsigned int irq, struct irq_desc *desc,
 	if (bad_action_ret(action_ret)) {
 		printk(KERN_ERR "irq event %d: bogus return value %x\n",
 				irq, action_ret);
+	} else if (irqfixup == 1) {
+		printk(KERN_ERR "irq %d not handled, even with \"irqfixup\": "
+				"try booting with the \"irqpoll\" option.\n",
+				irq);
+	} else if (irqfixup == 2) {
+		printk(KERN_ERR "irq %d not handled, even with \"irqpoll\": "
+				"most likely the firmware is too broken.\n",
+				irq);
 	} else {
-		printk(KERN_ERR "irq %d: nobody cared (try booting with "
-				"the \"irqpoll\" option)\n", irq);
+		printk(KERN_ERR "irq %d not handled (nobody cared): try "
+				"booting with the \"irqfixup\" option.\n",
+				irq);
 	}
 	dump_stack();
 	printk(KERN_ERR "handlers:\n");
@@ -325,7 +334,7 @@ void note_interrupt(unsigned int irq, struct irq_desc *desc,
 	desc->irqs_unhandled = 0;
 }

-bool noirqdebug __read_mostly;
+int noirqdebug __read_mostly;

 int noirqdebug_setup(char *str)
 {
-- 
1.7.8.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/