All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Uwe Kleine-König" <Uwe.Kleine-Koenig@digi.com>
To: <linux-kernel@vger.kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@elte.hu>
Subject: [PATCH v2] handle failure of irqchip->set_type in setup_irq
Date: Fri, 4 Jul 2008 12:46:34 +0200	[thread overview]
Message-ID: <20080704104634.GA31634@digi.com> (raw)
In-Reply-To: <20080625131101.GA6205@digi.com>

set_type returns an int indicating success or failure, but up to now
setup_irq ignores that.

In my case this resulted in a machine hang:
gpio-keys requested IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, but
arm/ns9xxx can only trigger on one direction so set_type didn't touch
the configuration which happens do default to a level sensitiveness and
returned -EINVAL.  setup_irq ignored that and unmasked the irq.  This
resulted in an endless triggering of the gpio-key interrupt service
routine which effectively killed the machine.

With this patch applied setup_irq propagates the error to the caller.

Note that before in the case 

	chip && !chip->set_type && !chip->name

a NULL pointer was feed to printk.  This is fixed, too.

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
Hello,

Changes since initial post:

 - improve commit log (hopefully)
 - move code to a dedicated function to improve readability and code
   line length.
 - include the symbolic name of the failing callback in the error
   message.

Best regards
Uwe

 kernel/irq/manage.c |   72 ++++++++++++++++++++++++++++++++++----------------
 1 files changed, 49 insertions(+), 23 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 46d6611..178b966 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -270,6 +270,35 @@ void compat_irq_chip_set_default_handler(struct irq_desc *desc)
 		desc->handle_irq = NULL;
 }
 
+static int __irq_set_trigger(struct irq_chip *chip, unsigned int irq,
+		unsigned long flags)
+{
+	int ret;
+
+	if (!chip || !chip->set_type) {
+		/*
+		 * IRQF_TRIGGER_* but the PIC does not support multiple
+		 * flow-types?
+		 */
+		pr_warning("No set_type function for IRQ %d (%s)\n", irq,
+				chip ? (chip->name ? : "unknown") : "unknown");
+		return 0;
+	}
+
+	ret = chip->set_type(irq, flags & IRQF_TRIGGER_MASK);
+
+	if (ret) {
+		char buf[100];
+
+		snprintf(buf, sizeof(buf), KERN_ERR
+				"setting flow type for irq %u failed (%%s)\n",
+				irq);
+		print_fn_descriptor_symbol(buf, chip->set_type);
+	}
+
+	return ret;
+}
+
 /*
  * Internal function to register an irqaction - typically used to
  * allocate special interrupts that are part of the architecture.
@@ -281,6 +310,7 @@ int setup_irq(unsigned int irq, struct irqaction *new)
 	const char *old_name = NULL;
 	unsigned long flags;
 	int shared = 0;
+	int ret;
 
 	if (irq >= NR_IRQS)
 		return -EINVAL;
@@ -338,37 +368,26 @@ int setup_irq(unsigned int irq, struct irqaction *new)
 		shared = 1;
 	}
 
-	*p = new;
-
-	/* Exclude IRQ from balancing */
-	if (new->flags & IRQF_NOBALANCING)
-		desc->status |= IRQ_NO_BALANCING;
-
 	if (!shared) {
 		irq_chip_set_defaults(desc->chip);
 
-#if defined(CONFIG_IRQ_PER_CPU)
-		if (new->flags & IRQF_PERCPU)
-			desc->status |= IRQ_PER_CPU;
-#endif
-
 		/* Setup the type (level, edge polarity) if configured: */
 		if (new->flags & IRQF_TRIGGER_MASK) {
-			if (desc->chip && desc->chip->set_type)
-				desc->chip->set_type(irq,
-						new->flags & IRQF_TRIGGER_MASK);
-			else
-				/*
-				 * IRQF_TRIGGER_* but the PIC does not support
-				 * multiple flow-types?
-				 */
-				printk(KERN_WARNING "No IRQF_TRIGGER set_type "
-				       "function for IRQ %d (%s)\n", irq,
-				       desc->chip ? desc->chip->name :
-				       "unknown");
+			ret = __irq_set_trigger(desc->chip, irq, new->flags);
+
+			if (ret) {
+				spin_unlock_irqrestore(&desc->lock, flags);
+				return ret;
+			}
+
 		} else
 			compat_irq_chip_set_default_handler(desc);
 
+#if defined(CONFIG_IRQ_PER_CPU)
+		if (new->flags & IRQF_PERCPU)
+			desc->status |= IRQ_PER_CPU;
+#endif
+
 		desc->status &= ~(IRQ_AUTODETECT | IRQ_WAITING |
 				  IRQ_INPROGRESS | IRQ_SPURIOUS_DISABLED);
 
@@ -383,6 +402,13 @@ int setup_irq(unsigned int irq, struct irqaction *new)
 			/* Undo nested disables: */
 			desc->depth = 1;
 	}
+
+	*p = new;
+
+	/* Exclude IRQ from balancing */
+	if (new->flags & IRQF_NOBALANCING)
+		desc->status |= IRQ_NO_BALANCING;
+
 	/* Reset broken irq detection when installing new handler */
 	desc->irq_count = 0;
 	desc->irqs_unhandled = 0;
-- 
1.5.6


  parent reply	other threads:[~2008-07-04 10:46 UTC|newest]

Thread overview: 111+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-06-25 13:11 [PATCH] handle failure of irqchip->set_type in setup_irq Uwe Kleine-König
2008-07-02  9:17 ` Uwe Kleine-König
2008-07-02  9:49   ` Andrew Morton
2008-07-02 10:09     ` Uwe Kleine-König
2008-07-04 10:46 ` Uwe Kleine-König [this message]
2008-07-04 17:17   ` [PATCH v2] " Andrew Morton
2008-07-04 18:43     ` Uwe Kleine-König
2008-07-04 19:08       ` Andrew Morton
2008-07-09 13:13         ` Uwe Kleine-König
2008-07-09 21:52           ` Andrew Morton
2008-07-10  8:23             ` Uwe Kleine-König
2008-07-10  8:28               ` Andrew Morton
     [not found]   ` <20080704111540.ddffd241.akpm@linux-foundation.org>
     [not found]     ` <alpine.LFD.1.10.0807041147450.2815@woody.linux-foundation.org>
2008-07-04 20:02       ` the printk problem Linus Torvalds
2008-07-04 20:02         ` Linus Torvalds
2008-07-04 20:27         ` Andrew Morton
2008-07-04 20:27           ` Andrew Morton
2008-07-04 20:41           ` Linus Torvalds
2008-07-04 20:41             ` Linus Torvalds
2008-07-04 20:42           ` Matthew Wilcox
2008-07-04 20:42             ` Matthew Wilcox
2008-07-04 22:01             ` Andrew Morton
2008-07-04 22:01               ` Andrew Morton
2008-07-04 22:01               ` Andrew Morton
2008-07-05  2:03               ` Matthew Wilcox
2008-07-05  2:03                 ` Matthew Wilcox
2008-07-05  2:03                 ` Matthew Wilcox
2008-07-22 10:05                 ` [PATCH] Make u64 long long on all architectures (was: the printk problem) Andrew Morton
2008-07-22 10:05                   ` Andrew Morton
2008-07-22 10:05                   ` [PATCH] Make u64 long long on all architectures (was: the Andrew Morton
2008-07-22 10:36                   ` [PATCH] Make u64 long long on all architectures (was: the printk problem) Michael Ellerman
2008-07-22 10:36                     ` Michael Ellerman
2008-07-22 10:36                     ` [PATCH] Make u64 long long on all architectures (was: the Michael Ellerman
2008-07-22 10:53                     ` [PATCH] Make u64 long long on all architectures (was: the printk problem) Andrew Morton
2008-07-22 10:53                       ` Andrew Morton
2008-07-22 10:53                       ` [PATCH] Make u64 long long on all architectures (was: the Andrew Morton
2008-07-22 11:36                     ` [PATCH] Make u64 long long on all architectures (was: the printk problem) Benjamin Herrenschmidt
2008-07-22 11:36                       ` Benjamin Herrenschmidt
2008-07-22 11:36                       ` [PATCH] Make u64 long long on all architectures (was: the Benjamin Herrenschmidt
2008-07-22 11:35                   ` [PATCH] Make u64 long long on all architectures (was: the printk problem) Benjamin Herrenschmidt
2008-07-22 11:35                     ` Benjamin Herrenschmidt
2008-07-22 11:35                     ` [PATCH] Make u64 long long on all architectures (was: the Benjamin Herrenschmidt
2008-07-05 10:20               ` the printk problem Denys Vlasenko
2008-07-05 10:20                 ` Denys Vlasenko
2008-07-05 10:20                 ` Denys Vlasenko
2008-07-05 11:33               ` Jan Engelhardt
2008-07-05 11:33                 ` Jan Engelhardt
2008-07-05 11:33                 ` Jan Engelhardt
2008-07-04 22:58             ` Benjamin Herrenschmidt
2008-07-04 22:58               ` Benjamin Herrenschmidt
2008-07-04 20:36         ` Matthew Wilcox
2008-07-04 20:36           ` Matthew Wilcox
2008-07-08  1:44           ` Kyle McMartin
2008-07-08  1:44             ` Kyle McMartin
2008-07-04 23:00         ` Benjamin Herrenschmidt
2008-07-04 23:00           ` Benjamin Herrenschmidt
2008-07-04 23:25           ` Linus Torvalds
2008-07-04 23:25             ` Linus Torvalds
2008-07-05 22:32             ` Linus Torvalds
2008-07-05 22:32               ` Linus Torvalds
2008-07-05 22:57               ` Arjan van de Ven
2008-07-05 22:57                 ` Arjan van de Ven
2008-07-06  5:27               ` Ingo Molnar
2008-07-06  5:27                 ` Ingo Molnar
2008-07-06  5:37                 ` Linus Torvalds
2008-07-06  5:37                   ` Linus Torvalds
2008-07-06  5:37                   ` Linus Torvalds
2008-07-06  5:53                   ` Ingo Molnar
2008-07-06  5:53                     ` Ingo Molnar
2008-07-06  5:53                     ` Ingo Molnar
2008-07-06  6:13                     ` Ingo Molnar
2008-07-06  6:13                       ` Ingo Molnar
2008-07-06  6:13                       ` Ingo Molnar
2008-07-07  1:14             ` Benjamin Herrenschmidt
2008-07-07  1:14               ` Benjamin Herrenschmidt
2008-07-07  3:26               ` Stephen Rothwell
2008-07-07  3:28                 ` Michael Ellerman
2008-07-07  4:59                   ` Stephen Rothwell
2008-07-07  3:43                 ` Benjamin Herrenschmidt
2008-07-05 12:52         ` Vegard Nossum
2008-07-05 12:52           ` Vegard Nossum
2008-07-05 12:52           ` Vegard Nossum
2008-07-05 13:24           ` Jan Engelhardt
2008-07-05 13:24             ` Jan Engelhardt
2008-07-05 13:24             ` Jan Engelhardt
2008-07-05 13:50             ` Vegard Nossum
2008-07-05 13:50               ` Vegard Nossum
2008-07-05 13:50               ` Vegard Nossum
2008-07-05 14:07               ` Jan Engelhardt
2008-07-05 14:07                 ` Jan Engelhardt
2008-07-05 14:07                 ` Jan Engelhardt
2008-07-05 17:56           ` Linus Torvalds
2008-07-05 17:56             ` Linus Torvalds
2008-07-05 17:56             ` Linus Torvalds
2008-07-05 18:40             ` Jan Engelhardt
2008-07-05 18:40               ` Jan Engelhardt
2008-07-05 18:40               ` Jan Engelhardt
2008-07-05 18:44               ` Linus Torvalds
2008-07-05 18:44                 ` Linus Torvalds
2008-07-05 18:44                 ` Linus Torvalds
2008-07-05 18:41             ` Vegard Nossum
2008-07-05 18:41               ` Vegard Nossum
2008-07-05 18:41               ` Vegard Nossum
2008-07-05 18:52               ` Matthew Wilcox
2008-07-05 18:52                 ` Matthew Wilcox
2008-07-05 18:52                 ` Matthew Wilcox
2008-07-06  0:02                 ` Pekka Enberg
2008-07-06  0:02                   ` Pekka Enberg
2008-07-06  0:02                   ` Pekka Enberg
2008-07-06  5:17                   ` Randy Dunlap
2008-07-06  5:17                     ` Randy Dunlap
2008-07-06  5:17                     ` Randy Dunlap

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080704104634.GA31634@digi.com \
    --to=uwe.kleine-koenig@digi.com \
    --cc=akpm@linux-foundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.