Re: [PATCH] irq: Consider a negative return value of irq_startup() as an error

[Thomas Gleixner]

On Fri, 7 Mar 2014, Linus Walleij wrote:
> I'll see what Jean-Jacques comes up with and take it from there unless
> he's interested in taking it all the way.

Thought more about it while trying to come up with a persuasive
argument for the other Linus to take the irq_startup change that late
in the cycle:

Using startup is the wrong point in __setup_irq() because we call
chip->irq_set_type() before we call chip->irq_startup(). And we want
this to be in that order to avoid spurious interrupts.

Proper solution below. That leaves the startup/unmask logic untouched
and provides separate callbacks for this kind of requirements. That
makes it also simpler to have common functions for all gpios as you
don't have to do the mask/unmask dance in startup/shutdown. So you can
simply create gpio_irq_request/release_resources() and let the drivers
add those to their callbacks.

If you agree, I put that into a separate branch based on an upstream
-rc so you can pull it into your gpiolib stuff and work from there.

Thanks,

	tglx

---------------------->
Subject: genirq: Provide irq_request/release_resources chip callbacks
From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Date: Sat, 08 Mar 2014 08:59:58 +0100

For certain irq types, e.g. gpios, it's necessary to request resources
before starting up the irq.

This might fail so we cannot use the irq_startup() callback because we
might call the irq_set_type() callback before that which does not make
sense when the resource is not available. Calling irq_startup() before
irq_set_type() can lead to spurious interrupts which is not desired
either.

Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
---
 include/linux/irq.h |    6 ++++++
 kernel/irq/manage.c |   28 +++++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 1 deletion(-)

Index: linux-2.6/include/linux/irq.h
===================================================================
--- linux-2.6.orig/include/linux/irq.h
+++ linux-2.6/include/linux/irq.h
@@ -303,6 +303,10 @@ static inline irq_hw_number_t irqd_to_hw
  * @irq_pm_shutdown:	function called from core code on shutdown once per chip
  * @irq_calc_mask:	Optional function to set irq_data.mask for special cases
  * @irq_print_chip:	optional to print special chip info in show_interrupts
+ * @irq_request_resources:	optional to request resources before calling
+ *				any other callback related to this irq
+ * @irq_release_resources:	optional to release resources acquired with
+ *				irq_request_resources
  * @flags:		chip specific flags
  */
 struct irq_chip {
@@ -336,6 +340,8 @@ struct irq_chip {
 	void		(*irq_calc_mask)(struct irq_data *data);
 
 	void		(*irq_print_chip)(struct irq_data *data, struct seq_file *p);
+	int		(*irq_request_resources)(struct irq_data *data);
+	void		(*irq_release_resources)(struct irq_data *data);
 
 	unsigned long	flags;
 };
Index: linux-2.6/kernel/irq/manage.c
===================================================================
--- linux-2.6.orig/kernel/irq/manage.c
+++ linux-2.6/kernel/irq/manage.c
@@ -897,6 +897,23 @@ static void irq_setup_forced_threading(s
 	}
 }
 
+static int irq_request_resources(struct irq_desc *desc)
+{
+	struct irq_data *d = &desc->irq_data;
+	struct irq_chip *c = d->chip;
+
+	return c->irq_request_resources ? c->irq_request_resources(d) : 0;
+}
+
+static void irq_release_resources(struct irq_desc *desc)
+{
+	struct irq_data *d = &desc->irq_data;
+	struct irq_chip *c = d->chip;
+
+	if (c->irq_release_resources)
+		c->irq_release_resources(d);
+}
+
 /*
  * Internal function to register an irqaction - typically used to
  * allocate special interrupts that are part of the architecture.
@@ -1092,6 +1109,13 @@ __setup_irq(unsigned int irq, struct irq
 	}
 
 	if (!shared) {
+		ret = irq_request_resources(desc);
+		if (ret) {
+			pr_err("Failed to request resources for %s (irq %d) on irqchip %s\n",
+			       new->name, irq, desc->irq_data.chip->name);
+			goto out_mask;
+		}
+
 		init_waitqueue_head(&desc->wait_for_threads);
 
 		/* Setup the type (level, edge polarity) if configured: */
@@ -1262,8 +1286,10 @@ static struct irqaction *__free_irq(unsi
 	*action_ptr = action->next;
 
 	/* If this was the last handler, shut down the IRQ line: */
-	if (!desc->action)
+	if (!desc->action) {
 		irq_shutdown(desc);
+		irq_release_resources(desc);
+	}
 
 #ifdef CONFIG_SMP
 	/* make sure affinity_hint is cleaned up */





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