Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1683709 > unrolled thread

[GIT pull] irq updates for 4.13

Back to article view | Back to linux.kernel

Contents

  [GIT pull] irq updates for 4.13 Thomas Gleixner <tglx@linutronix.de> - 2017-07-09 11:00 +0200
    Re: [GIT pull] irq updates for 4.13 Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-10 15:40 +0200
      Re: [GIT pull] irq updates for 4.13 Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-10 19:10 +0200
        Re: [GIT pull] irq updates for 4.13 Pavel Machek <pavel@ucw.cz> - 2017-07-10 21:40 +0200
        Re: [GIT pull] irq updates for 4.13 Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-10 22:20 +0200
          Re: [GIT pull] irq updates for 4.13 Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-10 23:30 +0200
        Re: [GIT pull] irq updates for 4.13 Thomas Gleixner <tglx@linutronix.de> - 2017-07-11 09:00 +0200
          Re: [GIT pull] irq updates for 4.13 Thomas Gleixner <tglx@linutronix.de> - 2017-07-11 11:50 +0200
            Re: [GIT pull] irq updates for 4.13 Tony Lindgren <tony@atomide.com> - 2017-07-11 16:00 +0200
              Re: [GIT pull] irq updates for 4.13 Thomas Gleixner <tglx@linutronix.de> - 2017-07-11 16:50 +0200
                Re: [GIT pull] irq updates for 4.13 Thomas Gleixner <tglx@linutronix.de> - 2017-07-11 17:10 +0200
                  Re: [GIT pull] irq updates for 4.13 Tony Lindgren <tony@atomide.com> - 2017-07-11 17:50 +0200
                Re: [GIT pull] irq updates for 4.13 Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-11 17:50 +0200
                  Re: [GIT pull] irq updates for 4.13 Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-11 18:20 +0200
                  Re: [GIT pull] irq updates for 4.13 Tony Lindgren <tony@atomide.com> - 2017-07-11 18:20 +0200
                    Re: [GIT pull] irq updates for 4.13 Thomas Gleixner <tglx@linutronix.de> - 2017-07-11 19:20 +0200
                      Re: [GIT pull] irq updates for 4.13 Tony Lindgren <tony@atomide.com> - 2017-07-11 19:40 +0200
                  Re: [GIT pull] irq updates for 4.13 Thomas Gleixner <tglx@linutronix.de> - 2017-07-11 18:30 +0200
                    Re: [GIT pull] irq updates for 4.13 Tony Lindgren <tony@atomide.com> - 2017-07-11 18:40 +0200
                    Re: [GIT pull] irq updates for 4.13 Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-11 18:40 +0200
                      Re: [GIT pull] irq updates for 4.13 Thomas Gleixner <tglx@linutronix.de> - 2017-07-11 20:00 +0200
                        Re: [GIT pull] irq updates for 4.13 Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-11 20:20 +0200
                          Re: [GIT pull] irq updates for 4.13 Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-11 23:40 +0200
                          Re: [GIT pull] irq updates for 4.13 Thomas Gleixner <tglx@tglx.de> - 2017-07-12 00:10 +0200
                            Re: [GIT pull] irq updates for 4.13 Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-12 00:10 +0200
                            Re: [GIT pull] irq updates for 4.13 Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-12 01:00 +0200
                              Re: [GIT pull] irq updates for 4.13 Tony Lindgren <tony@atomide.com> - 2017-07-12 07:30 +0200
                                Re: [GIT pull] irq updates for 4.13 Pavel Machek <pavel@ucw.cz> - 2017-07-15 22:30 +0200
                                  Re: [GIT pull] irq updates for 4.13 Tony Lindgren <tony@atomide.com> - 2017-07-17 08:30 +0200
                                  Re: [GIT pull] irq updates for 4.13 Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-17 22:10 +0200
                                    Re: [GIT pull] irq updates for 4.13 Pavel Machek <pavel@ucw.cz> - 2017-07-17 23:40 +0200
                Re: [GIT pull] irq updates for 4.13 Grygorii Strashko <grygorii.strashko@ti.com> - 2017-07-11 17:50 +0200
                  Re: [GIT pull] irq updates for 4.13 Tony Lindgren <tony@atomide.com> - 2017-07-11 18:20 +0200
                  Re: [GIT pull] irq updates for 4.13 Geert Uytterhoeven <geert@linux-m68k.org> - 2017-07-12 10:10 +0200
              Re: [GIT pull] irq updates for 4.13 Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-11 16:50 +0200
                Re: [GIT pull] irq updates for 4.13 Tony Lindgren <tony@atomide.com> - 2017-07-11 18:30 +0200
                  Re: [GIT pull] irq updates for 4.13 Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-11 18:40 +0200
          Re: [GIT pull] irq updates for 4.13 Thomas Gleixner <tglx@linutronix.de> - 2017-07-11 12:00 +0200
            Re: [GIT pull] irq updates for 4.13 Thomas Gleixner <tglx@linutronix.de> - 2017-07-11 13:00 +0200
              Re: [GIT pull] irq updates for 4.13 Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-11 13:30 +0200
                Re: [GIT pull] irq updates for 4.13 Thomas Gleixner <tglx@linutronix.de> - 2017-07-11 15:30 +0200
                Re: [GIT pull] irq updates for 4.13 Marc Zyngier <marc.zyngier@arm.com> - 2017-07-11 16:00 +0200
                  Re: [GIT pull] irq updates for 4.13 Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-11 16:50 +0200

Page 1 of 3  [1] 2 3  Next page →

#1683709 — [GIT pull] irq updates for 4.13

Linus,

please pull the latest irq-urgent-for-linus git tree from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq-urgent-for-linus

This update contains:

  - A few fixes mopping up the fallout of the big irq overhaul

  - Move the interrupt resource management logic out of the spin locked,
    irq disabled region to avoid unnecessary restrictions of the resource
    callbacks

  - Preparation for reworking the per cpu irq request function.

Thanks,

	tglx

------------------>
Daniel Lezcano (1):
      genirq: Allow to pass the IRQF_TIMER flag with percpu irq request

Geert Uytterhoeven (1):
      genirq: Force inlining of __irq_startup_managed to prevent build failure

Marc Zyngier (1):
      irqdomain: Allow ACPI device nodes to be used as irqdomain identifiers

Sebastian Ott (1):
      genirq/debugfs: Fix build for !CONFIG_IRQ_DOMAIN

Thomas Gleixner (5):
      genirq: Move bus locking into __setup_irq()
      genirq: Add mutex to irq desc to serialize request/free_irq()
      genirq: Move irq resource handling out of spinlocked region
      genirq/timings: Move free timings out of spinlocked region
      genirq/debugfs: Remove redundant NULL pointer check


 include/linux/interrupt.h | 11 ++++++++-
 include/linux/irqdesc.h   |  3 +++
 kernel/irq/chip.c         |  2 +-
 kernel/irq/internals.h    |  4 ++-
 kernel/irq/irqdesc.c      |  1 +
 kernel/irq/irqdomain.c    | 19 +++++++++++++--
 kernel/irq/manage.c       | 62 ++++++++++++++++++++++++++++++-----------------
 7 files changed, 75 insertions(+), 27 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 37f8e354f564..5ac6e238555e 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -152,8 +152,17 @@ request_any_context_irq(unsigned int irq, irq_handler_t handler,
 			unsigned long flags, const char *name, void *dev_id);
 
 extern int __must_check
+__request_percpu_irq(unsigned int irq, irq_handler_t handler,
+		     unsigned long flags, const char *devname,
+		     void __percpu *percpu_dev_id);
+
+static inline int __must_check
 request_percpu_irq(unsigned int irq, irq_handler_t handler,
-		   const char *devname, void __percpu *percpu_dev_id);
+		   const char *devname, void __percpu *percpu_dev_id)
+{
+	return __request_percpu_irq(irq, handler, 0,
+				    devname, percpu_dev_id);
+}
 
 extern const void *free_irq(unsigned int, void *);
 extern void free_percpu_irq(unsigned int, void __percpu *);
diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index d425a3a09722..3e90a094798d 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -3,6 +3,7 @@
 
 #include <linux/rcupdate.h>
 #include <linux/kobject.h>
+#include <linux/mutex.h>
 
 /*
  * Core internal functions to deal with irq descriptors
@@ -45,6 +46,7 @@ struct pt_regs;
  *			IRQF_FORCE_RESUME set
  * @rcu:		rcu head for delayed free
  * @kobj:		kobject used to represent this struct in sysfs
+ * @request_mutex:	mutex to protect request/free before locking desc->lock
  * @dir:		/proc/irq/ procfs entry
  * @debugfs_file:	dentry for the debugfs file
  * @name:		flow handler name for /proc/interrupts output
@@ -96,6 +98,7 @@ struct irq_desc {
 	struct rcu_head		rcu;
 	struct kobject		kobj;
 #endif
+	struct mutex		request_mutex;
 	int			parent_irq;
 	struct module		*owner;
 	const char		*name;
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 2e30d925a40d..aa5497dfb29e 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -234,7 +234,7 @@ __irq_startup_managed(struct irq_desc *desc, struct cpumask *aff, bool force)
 	return IRQ_STARTUP_MANAGED;
 }
 #else
-static int
+static __always_inline int
 __irq_startup_managed(struct irq_desc *desc, struct cpumask *aff, bool force)
 {
 	return IRQ_STARTUP_NORMAL;
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 9da14d125df4..dbfba9933ed2 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -437,7 +437,9 @@ static inline void irq_remove_debugfs_entry(struct irq_desc *desc)
 # ifdef CONFIG_IRQ_DOMAIN
 void irq_domain_debugfs_init(struct dentry *root);
 # else
-static inline void irq_domain_debugfs_init(struct dentry *root);
+static inline void irq_domain_debugfs_init(struct dentry *root)
+{
+}
 # endif
 #else /* CONFIG_GENERIC_IRQ_DEBUGFS */
 static inline void irq_add_debugfs_entry(unsigned int irq, struct irq_desc *d)
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 948b50e78549..906a67e58391 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -373,6 +373,7 @@ static struct irq_desc *alloc_desc(int irq, int node, unsigned int flags,
 
 	raw_spin_lock_init(&desc->lock);
 	lockdep_set_class(&desc->lock, &irq_desc_lock_class);
+	mutex_init(&desc->request_mutex);
 	init_rcu_head(&desc->rcu);
 
 	desc_set_defaults(irq, desc, node, affinity, owner);
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 14fe862aa2e3..f1f251479aa6 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1,5 +1,6 @@
 #define pr_fmt(fmt)  "irq: " fmt
 
+#include <linux/acpi.h>
 #include <linux/debugfs.h>
 #include <linux/hardirq.h>
 #include <linux/interrupt.h>
@@ -155,6 +156,21 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, int size,
 			domain->name = fwid->name;
 			break;
 		}
+#ifdef CONFIG_ACPI
+	} else if (is_acpi_device_node(fwnode)) {
+		struct acpi_buffer buf = {
+			.length = ACPI_ALLOCATE_BUFFER,
+		};
+		acpi_handle handle;
+
+		handle = acpi_device_handle(to_acpi_device_node(fwnode));
+		if (acpi_get_name(handle, ACPI_FULL_PATHNAME, &buf) == AE_OK) {
+			domain->name = buf.pointer;
+			domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
+		}
+
+		domain->fwnode = fwnode;
+#endif
 	} else if (of_node) {
 		char *name;
 
@@ -1667,8 +1683,7 @@ static void debugfs_add_domain_dir(struct irq_domain *d)
 
 static void debugfs_remove_domain_dir(struct irq_domain *d)
 {
-	if (d->debugfs_file)
-		debugfs_remove(d->debugfs_file);
+	debugfs_remove(d->debugfs_file);
 }
 
 void __init irq_domain_debugfs_init(struct dentry *root)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 5c11c1730ba5..5624b2dd6b58 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1167,6 +1167,18 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)
 		new->flags &= ~IRQF_ONESHOT;
 
+	mutex_lock(&desc->request_mutex);
+	if (!desc->action) {
+		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_mutex;
+		}
+	}
+
+	chip_bus_lock(desc);
+
 	/*
 	 * The following block of code has to be executed atomically
 	 */
@@ -1267,13 +1279,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	}
 
 	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_unlock;
-		}
-
 		init_waitqueue_head(&desc->wait_for_threads);
 
 		/* Setup the type (level, edge polarity) if configured: */
@@ -1347,6 +1352,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	}
 
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
+	chip_bus_sync_unlock(desc);
+	mutex_unlock(&desc->request_mutex);
 
 	irq_setup_timings(desc, new);
 
@@ -1378,6 +1385,14 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 out_unlock:
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 
+	chip_bus_sync_unlock(desc);
+
+	if (!desc->action)
+		irq_release_resources(desc);
+
+out_mutex:
+	mutex_unlock(&desc->request_mutex);
+
 out_thread:
 	if (new->thread) {
 		struct task_struct *t = new->thread;
@@ -1417,9 +1432,7 @@ int setup_irq(unsigned int irq, struct irqaction *act)
 	if (retval < 0)
 		return retval;
 
-	chip_bus_lock(desc);
 	retval = __setup_irq(irq, desc, act);
-	chip_bus_sync_unlock(desc);
 
 	if (retval)
 		irq_chip_pm_put(&desc->irq_data);
@@ -1443,6 +1456,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 	if (!desc)
 		return NULL;
 
+	mutex_lock(&desc->request_mutex);
 	chip_bus_lock(desc);
 	raw_spin_lock_irqsave(&desc->lock, flags);
 
@@ -1475,8 +1489,6 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 	if (!desc->action) {
 		irq_settings_clr_disable_unlazy(desc);
 		irq_shutdown(desc);
-		irq_release_resources(desc);
-		irq_remove_timings(desc);
 	}
 
 #ifdef CONFIG_SMP
@@ -1518,6 +1530,13 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 		}
 	}
 
+	if (!desc->action) {
+		irq_release_resources(desc);
+		irq_remove_timings(desc);
+	}
+
+	mutex_unlock(&desc->request_mutex);
+
 	irq_chip_pm_put(&desc->irq_data);
 	module_put(desc->owner);
 	kfree(action->secondary);
@@ -1674,9 +1693,7 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 		return retval;
 	}
 
-	chip_bus_lock(desc);
 	retval = __setup_irq(irq, desc, action);
-	chip_bus_sync_unlock(desc);
 
 	if (retval) {
 		irq_chip_pm_put(&desc->irq_data);
@@ -1924,9 +1941,7 @@ int setup_percpu_irq(unsigned int irq, struct irqaction *act)
 	if (retval < 0)
 		return retval;
 
-	chip_bus_lock(desc);
 	retval = __setup_irq(irq, desc, act);
-	chip_bus_sync_unlock(desc);
 
 	if (retval)
 		irq_chip_pm_put(&desc->irq_data);
@@ -1935,9 +1950,10 @@ int setup_percpu_irq(unsigned int irq, struct irqaction *act)
 }
 
 /**
- *	request_percpu_irq - allocate a percpu interrupt line
+ *	__request_percpu_irq - allocate a percpu interrupt line
  *	@irq: Interrupt line to allocate
  *	@handler: Function to be called when the IRQ occurs.
+ *	@flags: Interrupt type flags (IRQF_TIMER only)
  *	@devname: An ascii name for the claiming device
  *	@dev_id: A percpu cookie passed back to the handler function
  *
@@ -1950,8 +1966,9 @@ int setup_percpu_irq(unsigned int irq, struct irqaction *act)
  *	the handler gets called with the interrupted CPU's instance of
  *	that variable.
  */
-int request_percpu_irq(unsigned int irq, irq_handler_t handler,
-		       const char *devname, void __percpu *dev_id)
+int __request_percpu_irq(unsigned int irq, irq_handler_t handler,
+			 unsigned long flags, const char *devname,
+			 void __percpu *dev_id)
 {
 	struct irqaction *action;
 	struct irq_desc *desc;
@@ -1965,12 +1982,15 @@ int request_percpu_irq(unsigned int irq, irq_handler_t handler,
 	    !irq_settings_is_per_cpu_devid(desc))
 		return -EINVAL;
 
+	if (flags && flags != IRQF_TIMER)
+		return -EINVAL;
+
 	action = kzalloc(sizeof(struct irqaction), GFP_KERNEL);
 	if (!action)
 		return -ENOMEM;
 
 	action->handler = handler;
-	action->flags = IRQF_PERCPU | IRQF_NO_SUSPEND;
+	action->flags = flags | IRQF_PERCPU | IRQF_NO_SUSPEND;
 	action->name = devname;
 	action->percpu_dev_id = dev_id;
 
@@ -1980,9 +2000,7 @@ int request_percpu_irq(unsigned int irq, irq_handler_t handler,
 		return retval;
 	}
 
-	chip_bus_lock(desc);
 	retval = __setup_irq(irq, desc, action);
-	chip_bus_sync_unlock(desc);
 
 	if (retval) {
 		irq_chip_pm_put(&desc->irq_data);
@@ -1991,7 +2009,7 @@ int request_percpu_irq(unsigned int irq, irq_handler_t handler,
 
 	return retval;
 }
-EXPORT_SYMBOL_GPL(request_percpu_irq);
+EXPORT_SYMBOL_GPL(__request_percpu_irq);
 
 /**
  *	irq_get_irqchip_state - returns the irqchip state of a interrupt.

[toc] | [next] | [standalone]

#1684244

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Sun, Jul 09, 2017 at 10:49:57AM +0200, Thomas Gleixner wrote:
>   - Move the interrupt resource management logic out of the spin locked,
>     irq disabled region to avoid unnecessary restrictions of the resource
>     callbacks

This patch apparently breaks OMAP platform:

46e48e257360f0845fe17089713cbad4db611e70 is the first bad commit
commit 46e48e257360f0845fe17089713cbad4db611e70
Author: Thomas Gleixner <tglx@linutronix.de>
Date:   Thu Jun 29 23:33:38 2017 +0200

    genirq: Move irq resource handling out of spinlocked region

Boot failure log from Droid 4:

[    1.346984] cpcap-core spi1.0: CPCAP vendor: ST rev: 2.10 (1a)
[    1.354766] Unhandled fault: imprecise external abort (0x1406) at 0xfeffffff
[    1.354797] ------------[ cut here ]------------
[    1.354827] WARNING: CPU: 0 PID: 0 at drivers/bus/omap_l3_noc.c:147 l3_interrupt_handler+0x21c/0x348
[    1.354827] 44000000.ocp:L3 Custom Error: MASTER MPU TARGET L4CFG (Read): Data Access in User mode during Functional access
[    1.354827] Modules linked in:
[    1.354858] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.12.0-10335-gb8c0c26d6667 #516
[    1.354858] Hardware name: Generic OMAP4 (Flattened Device Tree)
[    1.354888] [<c0110460>] (unwind_backtrace) from [<c010c4ac>] (show_stack+0x10/0x14)
[    1.354888] [<c010c4ac>] (show_stack) from [<c0adca28>] (dump_stack+0xac/0xe0)
[    1.354919] [<c0adca28>] (dump_stack) from [<c013a940>] (__warn+0xd8/0x104)
[    1.354919] [<c013a940>] (__warn) from [<c013a9a0>] (warn_slowpath_fmt+0x34/0x44)
[    1.354919] [<c013a9a0>] (warn_slowpath_fmt) from [<c0508324>] (l3_interrupt_handler+0x21c/0x348)
[    1.354949] [<c0508324>] (l3_interrupt_handler) from [<c01aa1b4>] (__handle_irq_event_percpu+0x48/0x3b4)
[    1.354949] [<c01aa1b4>] (__handle_irq_event_percpu) from [<c01aa53c>] (handle_irq_event_percpu+0x1c/0x58)
[    1.354949] [<c01aa53c>] (handle_irq_event_percpu) from [<c01aa5b0>] (handle_irq_event+0x38/0x5c)
[    1.354980] [<c01aa5b0>] (handle_irq_event) from [<c01adb30>] (handle_fasteoi_irq+0xb4/0x178)
[    1.354980] [<c01adb30>] (handle_fasteoi_irq) from [<c01a94a8>] (generic_handle_irq+0x20/0x34)
[    1.354980] [<c01a94a8>] (generic_handle_irq) from [<c01a9a18>] (__handle_domain_irq+0x64/0xe0)
[    1.354980] [<c01a9a18>] (__handle_domain_irq) from [<c010155c>] (gic_handle_irq+0x54/0xb8)
[    1.355010] [<c010155c>] (gic_handle_irq) from [<c0af7e30>] (__irq_svc+0x70/0x98)
[    1.355010] Exception stack(0xc1001f38 to 0xc1001f80)
[    1.355010] 1f20:                                                       00000001 00000001
[    1.355041] 1f40: 00000000 c100abc0 c1000000 c1007bcc c1007b68 c0f8a938 c1007f68 c1046133
[    1.355041] 1f60: 00000000 00000000 00000000 c1001f88 c019a828 c010822c 20000013 ffffffff
[    1.355041] [<c0af7e30>] (__irq_svc) from [<c010822c>] (arch_cpu_idle+0x20/0x3c)
[    1.355072] [<c010822c>] (arch_cpu_idle) from [<c018b6b4>] (do_idle+0x168/0x21c)
[    1.355072] [<c018b6b4>] (do_idle) from [<c018bad4>] (cpu_startup_entry+0x18/0x1c)
[    1.355072] [<c018bad4>] (cpu_startup_entry) from [<c0f00c2c>] (start_kernel+0x348/0x3c0)
[    1.355102] [<c0f00c2c>] (start_kernel) from [<8000807c>] (0x8000807c)
[    1.355133] ---[ end trace 03269d8f047e066b ]---
[    1.584167] pgd = c0004000
[    1.586914] [feffffff] *pgd=00000000
[    1.590515] Internal error: : 1406 [#1] SMP ARM
[    1.595062] Modules linked in:
[    1.598144] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G        W       4.12.0-10335-gb8c0c26d6667 #516
[    1.607238] Hardware name: Generic OMAP4 (Flattened Device Tree)
[    1.613281] task: ee8aae00 task.stack: ee8ac000
[    1.617858] PC is at debug_lockdep_rcu_enabled+0x38/0x48
[    1.623199] LR is at lock_release+0x25c/0x360
[    1.627593] pc : [<c01b3788>]    lr : [<c019cc58>]    psr: 20000093
[    1.633880] sp : ee8adb80  ip : c10fe40c  fp : eea0fc10
[    1.639160] r10: 00000001  r9 : c10f4230  r8 : 60000093
[    1.644409] r7 : c1007b68  r6 : c051b3f8  r5 : eea0ce74  r4 : a0000013
[    1.650970] r3 : ee8aae00  r2 : 00000001  r1 : 00000003  r0 : 0000001f
[    1.657531] Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
[    1.664825] Control: 10c5387d  Table: 8000404a  DAC: 00000051
[    1.670593] Process swapper/0 (pid: 1, stack limit = 0xee8ac218)
[    1.676635] Stack: (0xee8adb80 to 0xee8ae000)
[    1.681030] db80: a0000013 c051b3e8 00000007 a0000013 eea0ce64 eea0ce64 00000007 eea0fd04
[    1.689270] dba0: c06208d8 eea0fc00 eea0fc10 c0af76ec 00000000 fc310134 eea0ce64 c051b3f8
[    1.697479] dbc0: 00000020 eea0cea4 eea0cc70 00000000 eea0fd04 c0514430 eea0cea4 eea0fc10
[    1.705718] dbe0: eef44cc0 c0514960 eea0fc00 00000021 eef44cc0 c01ac370 ee8000c0 60000013
[    1.713958] dc00: eed2d57c eef44cc0 00000000 c01aa628 eef42000 00000021 c06208d8 eea0fc00
[    1.722198] dc20: eea0fc10 c01ac73c 00002084 eef42000 00000204 c10a88cc 00000001 eee8a200
[    1.730407] dc40: 00000000 00000000 00000010 c0621460 c0da9f6c eef42000 00000000 00000111
[    1.738647] dc60: 00000021 00000084 00000084 eee8a200 eef22610 00000021 00000084 eef23800
[    1.746887] dc80: 00000010 eef2513c c0b69968 c0621618 c10a88cc ee8adc9c 00000004 eef23800
[    1.755126] dca0: eef23800 eee8a200 00000021 eef22710 eef25010 c062d618 ffffffff c10a88cc
[    1.763366] dcc0: eef22718 ee8aae00 00000001 00000000 c10a88cc 00000010 00000000 00000000
[    1.771575] dce0: eef23800 0000001a eef22710 00000000 00000010 00000013 00000000 00000000
[    1.779815] dd00: c0dba8a8 c062d7a0 0000000a 0000001a 00000000 00000013 eef23800 c10a887c
[    1.788055] dd20: 00000000 c10a888c 00000000 c06a2784 eef23800 c18c195c 00000000 c05fdd00
[    1.796295] dd40: 00000000 ee8add78 c05fde4c 00000001 00000000 c18c1918 00000000 c05fc374
[    1.804534] dd60: ee9f2ad4 eee7e154 eef23800 eef23834 c10b0dbc c05fd9bc eef23800 00000001
[    1.812744] dd80: c18c1918 eef23808 eef23800 c10b0dbc 00000000 c05fd044 eef23808 eef25800
[    1.820983] dda0: eef23800 c05fb4d0 00000000 eef23800 eef23a64 00000000 eef23800 eef25800
[    1.829223] ddc0: 00000000 eea6cc10 00000000 c0dba89c 00000000 c06a3958 eef25800 ef6e7b80
[    1.837463] dde0: eef23800 ef6e7bd0 00000000 c06a41a4 00000000 00000000 c06a3dc4 002dc6c0
[    1.845672] de00: eea6cc10 eef22990 eef25800 eef25800 eea6cc10 eea6cc10 c0dbaf80 c0dbaf78
[    1.853912] de20: 000001f0 c06a4670 00000000 eef25ce8 eef25800 eef25800 eea6cc10 c06a8090
[    1.862152] de40: 00000000 60000013 c1899108 00000004 21547a13 eea6cc10 ffffffed c10b1acc
[    1.870391] de60: fffffdfb 00000000 00000000 c0f67858 c0f005a8 c05ffb34 eea6cc10 c18c195c
[    1.878631] de80: 00000000 c10b1acc 00000000 c05fdd00 eea6cc10 c10b1acc eea6cc44 00000000
[    1.886840] dea0: c10fa000 00000007 c0f67858 c05fde48 00000000 c10b1acc c05fdd88 c05fc2c8
[    1.895080] dec0: ee8a46a4 eea67dd0 c10b1acc eee8ea00 c10a6210 c05fd248 c0dbaf88 c0f40af8
[    1.903320] dee0: 00000000 c10b1acc c0f40af8 00000000 c0e68a14 c05fec8c ffffe000 c0f40af8
[    1.911560] df00: 00000000 c0101874 00000138 00000000 efffec00 efffecdd c0e6a274 00000138
[    1.919769] df20: 00000138 c015f0b8 c0e68a14 00000000 00000006 00000006 efffecdd 00000000
[    1.928009] df40: c0f806b0 00000006 c10fa000 c0f6784c c0f80d58 c10fa000 c0f67850 c10fa000
[    1.936248] df60: 00000007 c0f00ea0 00000006 00000006 00000000 c0f005a8 c0af025c 00000138
[    1.944488] df80: 00000000 00000000 c0af025c 00000000 00000000 00000000 00000000 00000000
[    1.952728] dfa0: 00000000 c0af0264 00000000 c01077b0 00000000 00000000 00000000 00000000
[    1.960937] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    1.969177] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000 c0c0c0c0 c0c0c0c0
[    1.977416] [<c01b3788>] (debug_lockdep_rcu_enabled) from [<c019cc58>] (lock_release+0x25c/0x360)
[    1.986358] [<c019cc58>] (lock_release) from [<c0af76ec>] (_raw_spin_unlock_irqrestore+0x1c/0x44)
[    1.995300] [<c0af76ec>] (_raw_spin_unlock_irqrestore) from [<c051b3f8>] (omap_gpio_get_direction+0x38/0x44)
[    2.005187] [<c051b3f8>] (omap_gpio_get_direction) from [<c0514430>] (gpiochip_lock_as_irq+0x98/0xe4)
[    2.014495] [<c0514430>] (gpiochip_lock_as_irq) from [<c0514960>] (gpiochip_irq_reqres+0x2c/0x6c)
[    2.023406] [<c0514960>] (gpiochip_irq_reqres) from [<c01ac370>] (__setup_irq+0x478/0x6ec)
[    2.031738] [<c01ac370>] (__setup_irq) from [<c01ac73c>] (request_threaded_irq+0xcc/0x14c)
[    2.040069] [<c01ac73c>] (request_threaded_irq) from [<c0621460>] (regmap_add_irq_chip+0x740/0x8a0)
[    2.049194] [<c0621460>] (regmap_add_irq_chip) from [<c0621618>] (devm_regmap_add_irq_chip+0x58/0xb4)
[    2.058471] [<c0621618>] (devm_regmap_add_irq_chip) from [<c062d618>] (cpcap_init_irq_chip+0x138/0x16c)
[    2.067932] [<c062d618>] (cpcap_init_irq_chip) from [<c062d7a0>] (cpcap_probe+0x154/0x258)
[    2.076263] [<c062d7a0>] (cpcap_probe) from [<c06a2784>] (spi_drv_probe+0x7c/0xac)
[    2.083892] [<c06a2784>] (spi_drv_probe) from [<c05fdd00>] (driver_probe_device+0x260/0x2e8)
[    2.092407] [<c05fdd00>] (driver_probe_device) from [<c05fc374>] (bus_for_each_drv+0x64/0x98)
[    2.100982] [<c05fc374>] (bus_for_each_drv) from [<c05fd9bc>] (__device_attach+0xb0/0x118)
[    2.109313] [<c05fd9bc>] (__device_attach) from [<c05fd044>] (bus_probe_device+0x88/0x90)
[    2.117523] [<c05fd044>] (bus_probe_device) from [<c05fb4d0>] (device_add+0x3c8/0x57c)
[    2.125518] [<c05fb4d0>] (device_add) from [<c06a3958>] (spi_add_device+0x90/0x134)
[    2.133209] [<c06a3958>] (spi_add_device) from [<c06a41a4>] (spi_register_controller+0x350/0x7ec)
[    2.142150] [<c06a41a4>] (spi_register_controller) from [<c06a4670>] (devm_spi_register_controller+0x30/0x70)
[    2.152130] [<c06a4670>] (devm_spi_register_controller) from [<c06a8090>] (omap2_mcspi_probe+0x27c/0x358)
[    2.161773] [<c06a8090>] (omap2_mcspi_probe) from [<c05ffb34>] (platform_drv_probe+0x50/0xb0)
[    2.170349] [<c05ffb34>] (platform_drv_probe) from [<c05fdd00>] (driver_probe_device+0x260/0x2e8)
[    2.179290] [<c05fdd00>] (driver_probe_device) from [<c05fde48>] (__driver_attach+0xc0/0xc4)
[    2.187805] [<c05fde48>] (__driver_attach) from [<c05fc2c8>] (bus_for_each_dev+0x6c/0xa0)
[    2.196014] [<c05fc2c8>] (bus_for_each_dev) from [<c05fd248>] (bus_add_driver+0x100/0x210)
[    2.204345] [<c05fd248>] (bus_add_driver) from [<c05fec8c>] (driver_register+0x78/0xf4)
[    2.212402] [<c05fec8c>] (driver_register) from [<c0101874>] (do_one_initcall+0x3c/0x16c)
[    2.220642] [<c0101874>] (do_one_initcall) from [<c0f00ea0>] (kernel_init_freeable+0x1fc/0x2c4)
[    2.229400] [<c0f00ea0>] (kernel_init_freeable) from [<c0af0264>] (kernel_init+0x8/0x114)
[    2.237640] [<c0af0264>] (kernel_init) from [<c01077b0>] (ret_from_fork+0x14/0x24)
[    2.245269] Code: e3c3303f e593300c e59305ec e16f0f10 (e1a002a0)
[    2.251403] ---[ end trace 03269d8f047e066c ]---
[    2.256134] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
[    2.256134]
[    2.265350] CPU0: stopping
[    2.268096] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G      D W       4.12.0-10335-gb8c0c26d6667 #516
[    2.277221] Hardware name: Generic OMAP4 (Flattened Device Tree)
[    2.283294] [<c0110460>] (unwind_backtrace) from [<c010c4ac>] (show_stack+0x10/0x14)
[    2.291107] [<c010c4ac>] (show_stack) from [<c0adca28>] (dump_stack+0xac/0xe0)
[    2.298400] [<c0adca28>] (dump_stack) from [<c010e920>] (handle_IPI+0x300/0x408)
[    2.305847] [<c010e920>] (handle_IPI) from [<c01015a4>] (gic_handle_irq+0x9c/0xb8)
[    2.313476] [<c01015a4>] (gic_handle_irq) from [<c0af7e30>] (__irq_svc+0x70/0x98)
[    2.321044] Exception stack(0xc1001f38 to 0xc1001f80)
[    2.326141] 1f20:                                                       c0108228 00000000
[    2.334381] 1f40: 00000000 00000000 c1000000 c1007bcc c1007b68 c0f8a938 c1007f68 c1046133
[    2.342620] 1f60: 00000000 00000000 000006c8 c1001f88 c0108228 c010822c 60000013 ffffffff
[    2.350860] [<c0af7e30>] (__irq_svc) from [<c010822c>] (arch_cpu_idle+0x20/0x3c)
[    2.358337] [<c010822c>] (arch_cpu_idle) from [<c018b6b4>] (do_idle+0x168/0x21c)
[    2.365814] [<c018b6b4>] (do_idle) from [<c018bad4>] (cpu_startup_entry+0x18/0x1c)
[    2.373443] [<c018bad4>] (cpu_startup_entry) from [<c0f00c2c>] (start_kernel+0x348/0x3c0)
[    2.381683] [<c0f00c2c>] (start_kernel) from [<8000807c>] (0x8000807c)
[    2.388275] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b

Droid 4 boots current master again after applying the patch below
(which is git revet of above patch, but I provide the patch, since
it did not revet cleanly).

-- Sebastian

From 0f1adf86f7e526e655f39964ca987fc42911bd96 Mon Sep 17 00:00:00 2001
From: Sebastian Reichel <sre@kernel.org>
Date: Mon, 10 Jul 2017 14:52:50 +0200
Subject: [PATCH] Revert "genirq: Move irq resource handling out of spinlocked
 region"

This reverts commit 46e48e257360f0845fe17089713cbad4db611e70.
---
 kernel/irq/manage.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 5624b2dd6b58..528bfc39042b 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1168,14 +1168,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		new->flags &= ~IRQF_ONESHOT;
 
 	mutex_lock(&desc->request_mutex);
-	if (!desc->action) {
-		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_mutex;
-		}
-	}
 
 	chip_bus_lock(desc);
 
@@ -1279,6 +1271,13 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	}
 
 	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_unlock;
+		}
+
 		init_waitqueue_head(&desc->wait_for_threads);
 
 		/* Setup the type (level, edge polarity) if configured: */
@@ -1387,10 +1386,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 
 	chip_bus_sync_unlock(desc);
 
-	if (!desc->action)
-		irq_release_resources(desc);
-
-out_mutex:
 	mutex_unlock(&desc->request_mutex);
 
 out_thread:
-- 
2.13.2

[toc] | [prev] | [next] | [standalone]

#1684427

On Mon, Jul 10, 2017 at 6:35 AM, Sebastian Reichel
<sebastian.reichel@collabora.co.uk> wrote:
>
> This patch apparently breaks OMAP platform:
>
> 46e48e257360f0845fe17089713cbad4db611e70 is the first bad commit
> commit 46e48e257360f0845fe17089713cbad4db611e70
> Author: Thomas Gleixner <tglx@linutronix.de>
> Date:   Thu Jun 29 23:33:38 2017 +0200
>
>     genirq: Move irq resource handling out of spinlocked region
>
> Boot failure log from Droid 4:
> [ ... snip snip ..]
>
> Droid 4 boots current master again after applying the patch below
> (which is git revet of above patch, but I provide the patch, since
> it did not revet cleanly).

Hmm. Do you actually need the full revert?

I think it's only the __setup_irq() part that looks like it may be garbage.

For example, I think it releases the resources twice if the
__irq_set_trigger() call fails.

But it looks questionably in other ways too - notably, the change to
make the request call be in the same context as the freeing is done is
apparently done entirely for symmetry reasons, not for any actual
*reason* reasons.

So I suspect just the __setup_irq() parts should be reverted, because
they look both buggy and pointless. But the actual *real* part of the
patch was the two-liner __free_irq() part, and that looks sane to me.

So Sebastian, can you test if it's ok to revert just the __setup_irq()
part, but leave the smaller part in __free_irq() that just moves the
irq_release_resources() around at freeing time?

Thomas? Comments?

                   Linus

[toc] | [prev] | [next] | [standalone]

#1684603

[Multipart message — attachments visible in raw view] — view raw

Hi!

> > This patch apparently breaks OMAP platform:
> >
> > 46e48e257360f0845fe17089713cbad4db611e70 is the first bad commit
> > commit 46e48e257360f0845fe17089713cbad4db611e70
> > Author: Thomas Gleixner <tglx@linutronix.de>
> > Date:   Thu Jun 29 23:33:38 2017 +0200
> >
> >     genirq: Move irq resource handling out of spinlocked region
> >
> > Boot failure log from Droid 4:
> > [ ... snip snip ..]
> >
> > Droid 4 boots current master again after applying the patch below
> > (which is git revet of above patch, but I provide the patch, since
> > it did not revet cleanly).
> 
> Hmm. Do you actually need the full revert?
> 
> I think it's only the __setup_irq() part that looks like it may be garbage.
> 
> For example, I think it releases the resources twice if the
> __irq_set_trigger() call fails.
> 
> But it looks questionably in other ways too - notably, the change to
> make the request call be in the same context as the freeing is done is
> apparently done entirely for symmetry reasons, not for any actual
> *reason* reasons.
> 
> So I suspect just the __setup_irq() parts should be reverted, because
> they look both buggy and pointless. But the actual *real* part of the
> patch was the two-liner __free_irq() part, and that looks sane to me.
> 
> So Sebastian, can you test if it's ok to revert just the __setup_irq()
> part, but leave the smaller part in __free_irq() that just moves the
> irq_release_resources() around at freeing time?

If I understood it correctly, you wanted to test this:

And yes, this is enough to fix boot on N900 for me.

Thanks,
								Pavel

commit 285358d48dec82f13fa76724bff434897883d188
Author: Pavel <pavel@ucw.cz>
Date:   Mon Jul 10 21:35:25 2017 +0200

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 5624b2d..528bfc3 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1168,14 +1168,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		new->flags &= ~IRQF_ONESHOT;
 
 	mutex_lock(&desc->request_mutex);
-	if (!desc->action) {
-		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_mutex;
-		}
-	}
 
 	chip_bus_lock(desc);
 
@@ -1279,6 +1271,13 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	}
 
 	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_unlock;
+		}
+
 		init_waitqueue_head(&desc->wait_for_threads);
 
 		/* Setup the type (level, edge polarity) if configured: */
@@ -1387,10 +1386,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 
 	chip_bus_sync_unlock(desc);
 
-	if (!desc->action)
-		irq_release_resources(desc);
-
-out_mutex:
 	mutex_unlock(&desc->request_mutex);
 
 out_thread:


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1684617

[Multipart message — attachments visible in raw view] — view raw

Hi Linus,

On Mon, Jul 10, 2017 at 10:01:22AM -0700, Linus Torvalds wrote:
> On Mon, Jul 10, 2017 at 6:35 AM, Sebastian Reichel
> <sebastian.reichel@collabora.co.uk> wrote:
> >
> > This patch apparently breaks OMAP platform:
> >
> > 46e48e257360f0845fe17089713cbad4db611e70 is the first bad commit
> > commit 46e48e257360f0845fe17089713cbad4db611e70
> > Author: Thomas Gleixner <tglx@linutronix.de>
> > Date:   Thu Jun 29 23:33:38 2017 +0200
> >
> >     genirq: Move irq resource handling out of spinlocked region
> >
> > Boot failure log from Droid 4:
> > [ ... snip snip ..]
> >
> > Droid 4 boots current master again after applying the patch below
> > (which is git revet of above patch, but I provide the patch, since
> > it did not revet cleanly).
> 
> Hmm. Do you actually need the full revert?

It's technically not a full revert - I actually did not revert the
__free_irq changes.

> I think it's only the __setup_irq() part that looks like it may be garbage.
> 
> For example, I think it releases the resources twice if the
> __irq_set_trigger() call fails.
> 
> But it looks questionably in other ways too - notably, the change to
> make the request call be in the same context as the freeing is done is
> apparently done entirely for symmetry reasons, not for any actual
> *reason* reasons.
> 
> So I suspect just the __setup_irq() parts should be reverted, because
> they look both buggy and pointless. But the actual *real* part of the
> patch was the two-liner __free_irq() part, and that looks sane to me.
> 
> So Sebastian, can you test if it's ok to revert just the __setup_irq()
> part, but leave the smaller part in __free_irq() that just moves the
> irq_release_resources() around at freeing time?

Looking at my patch it implements what you describe (by coincidence,
since git revert could not do a clean revert) as far as I can see.
It seems Pavel also understood it this way, since his patch is identical 
to the one I provided.

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1684648

On Mon, Jul 10, 2017 at 1:15 PM, Sebastian Reichel
<sebastian.reichel@collabora.co.uk> wrote:
>>
>> So Sebastian, can you test if it's ok to revert just the __setup_irq()
>> part, but leave the smaller part in __free_irq() that just moves the
>> irq_release_resources() around at freeing time?
>
> Looking at my patch it implements what you describe (by coincidence,
> since git revert could not do a clean revert) as far as I can see.

Heh, yes, I didn't look at your patch as much as I looked at the
revert description.

And yes, going back to look at your patch it looks like it only
reverts the __setup_irq() parts.

Waiting for Thomas to comment on this whole thing..

                Linus

[toc] | [prev] | [next] | [standalone]

#1684813

On Mon, 10 Jul 2017, Linus Torvalds wrote:
> On Mon, Jul 10, 2017 at 6:35 AM, Sebastian Reichel
> <sebastian.reichel@collabora.co.uk> wrote:
> >
> > This patch apparently breaks OMAP platform:
> >
> > 46e48e257360f0845fe17089713cbad4db611e70 is the first bad commit
> > commit 46e48e257360f0845fe17089713cbad4db611e70
> > Author: Thomas Gleixner <tglx@linutronix.de>
> > Date:   Thu Jun 29 23:33:38 2017 +0200
> >
> >     genirq: Move irq resource handling out of spinlocked region
> >
> > Boot failure log from Droid 4:
> > [ ... snip snip ..]
> >
> > Droid 4 boots current master again after applying the patch below
> > (which is git revet of above patch, but I provide the patch, since
> > it did not revet cleanly).
> 
> Hmm. Do you actually need the full revert?
> 
> I think it's only the __setup_irq() part that looks like it may be garbage.
> 
> For example, I think it releases the resources twice if the
> __irq_set_trigger() call fails.

Yes, I missed that. Sorry.

> But it looks questionably in other ways too - notably, the change to
> make the request call be in the same context as the freeing is done is
> apparently done entirely for symmetry reasons, not for any actual
> *reason* reasons.

There is a reasons reason. The whole purpose was to move out the
request/free resources call from the spinlocked and irq disabled reason.
I noticed the free ordering issue, when I was working on that.

The fact that the patch breaks the OMAP boot, points to something else.

The only user of the irq_request_resources() callback at the moment is the
GPIO subsystem and some pinctrl drivers, which are not involved in the OMAP
case. In case of OMAP it uses the gpiolib generic implementation which
does:

      try_module_get(chip->gpiodev->owner);
      gpiochip_lock_as_irq(chip, d->hwirq);

I have no idea at the moment why this would break anything. The double
release in the __irq_set_trigger() error path is the only issue I can find
there.

Sebastian, can you please provide a .config and a full boot log, preferably
with initcall_debug on the kernel command line?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1684927

On Tue, 11 Jul 2017, Thomas Gleixner wrote:
> On Mon, 10 Jul 2017, Linus Torvalds wrote:
> > On Mon, Jul 10, 2017 at 6:35 AM, Sebastian Reichel
> > <sebastian.reichel@collabora.co.uk> wrote:
> > >
> > > This patch apparently breaks OMAP platform:
> > >
> > > 46e48e257360f0845fe17089713cbad4db611e70 is the first bad commit
> > > commit 46e48e257360f0845fe17089713cbad4db611e70
> > > Author: Thomas Gleixner <tglx@linutronix.de>
> > > Date:   Thu Jun 29 23:33:38 2017 +0200
> > >
> > >     genirq: Move irq resource handling out of spinlocked region
> > >
> > > Boot failure log from Droid 4:
> > > [ ... snip snip ..]
> > >
> > > Droid 4 boots current master again after applying the patch below
> > > (which is git revet of above patch, but I provide the patch, since
> > > it did not revet cleanly).
> > 
> > Hmm. Do you actually need the full revert?
> > 
> > I think it's only the __setup_irq() part that looks like it may be garbage.
> > 
> > For example, I think it releases the resources twice if the
> > __irq_set_trigger() call fails.
> 
> Yes, I missed that. Sorry.
> 
> > But it looks questionably in other ways too - notably, the change to
> > make the request call be in the same context as the freeing is done is
> > apparently done entirely for symmetry reasons, not for any actual
> > *reason* reasons.
> 
> There is a reasons reason. The whole purpose was to move out the
> request/free resources call from the spinlocked and irq disabled reason.
> I noticed the free ordering issue, when I was working on that.
> 
> The fact that the patch breaks the OMAP boot, points to something else.
> 
> The only user of the irq_request_resources() callback at the moment is the
> GPIO subsystem and some pinctrl drivers, which are not involved in the OMAP
> case. In case of OMAP it uses the gpiolib generic implementation which
> does:
> 
>       try_module_get(chip->gpiodev->owner);
>       gpiochip_lock_as_irq(chip, d->hwirq);

So Tony actually provided the part of dmesg which shows the initial
failure, which subsequently leads to the splat Sebastian reported.

Unhandled fault: external abort on non-linefetch (0x1028) at 0xfb050034
pgd = c0004000 [fb050034] *pgd=49011452(bad)
Internal error: : 1028 [#1] SMP ARM
Workqueue: events deferred_probe_work_func
task: ce1d41c0 task.stack: ce1fc000
PC is at omap_gpio_get_direction+0x2c/0x44
LR is at _raw_spin_lock_irqsave+0x40/0x4c
pc : [<c0509258>]    lr : [<c08263c4>]    psr: 60000093
sp : ce1fdb78  ip : c0dce42c  fp : ce22d810
r10: ce22d800  r9 : 00000000  r8 : ce22d900
r7 : 00000016  r6 : ce223864  r5 : fb050034  r4 : 00000020
r3 : ce1d41c0  r2 : 00000000  r1 : a0000013  r0 : a0000013
Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 80004019  DAC: 00000051
Process kworker/0:1 (pid: 14, stack limit = 0xce1fc218)

The callstack is:

omap_gpio_get_direction
gpiochip_lock_as_irq
gpiochip_irq_reqres
__setup_irq
request_threaded_irq
smc_probe
smc_drv_probe
platform_drv_probe
....

So the SMC91X network driver request an IRQ, which ends up calling into the
GPIO interrupt setup and that fails. I have no idea why that would not fail
with the patch reverted. Dusting off a Beaglebone board....

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1685044

* Thomas Gleixner <tglx@linutronix.de> [170711 02:48]:
> On Tue, 11 Jul 2017, Thomas Gleixner wrote:
> 
> So Tony actually provided the part of dmesg which shows the initial
> failure, which subsequently leads to the splat Sebastian reported.
> 
> Unhandled fault: external abort on non-linefetch (0x1028) at 0xfb050034
> pgd = c0004000 [fb050034] *pgd=49011452(bad)
> Internal error: : 1028 [#1] SMP ARM
> Workqueue: events deferred_probe_work_func
> task: ce1d41c0 task.stack: ce1fc000
> PC is at omap_gpio_get_direction+0x2c/0x44
> LR is at _raw_spin_lock_irqsave+0x40/0x4c
> pc : [<c0509258>]    lr : [<c08263c4>]    psr: 60000093
> sp : ce1fdb78  ip : c0dce42c  fp : ce22d810
> r10: ce22d800  r9 : 00000000  r8 : ce22d900
> r7 : 00000016  r6 : ce223864  r5 : fb050034  r4 : 00000020
> r3 : ce1d41c0  r2 : 00000000  r1 : a0000013  r0 : a0000013
> Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
> Control: 10c5387d  Table: 80004019  DAC: 00000051
> Process kworker/0:1 (pid: 14, stack limit = 0xce1fc218)
> 
> The callstack is:
> 
> omap_gpio_get_direction
> gpiochip_lock_as_irq
> gpiochip_irq_reqres
> __setup_irq
> request_threaded_irq
> smc_probe
> smc_drv_probe
> platform_drv_probe
> ....
> 
> So the SMC91X network driver request an IRQ, which ends up calling into the
> GPIO interrupt setup and that fails. I have no idea why that would not fail
> with the patch reverted. Dusting off a Beaglebone board....

And "external abort on non-linefetch" means something is not clocked
in this case. The following alone makes things boot for me again, but I don't
quite follow what has now changed with the ordering.. Thomas, any ideas?

Anyways, adding Linus W and Grygorii to Cc since things now point to
gpio-omap.

Regards,

Tony

8< ---------------------
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -919,13 +919,24 @@ static int omap_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
 	struct gpio_bank *bank;
 	unsigned long flags;
 	void __iomem *reg;
-	int dir;
+	int error, dir;
 
 	bank = gpiochip_get_data(chip);
 	reg = bank->base + bank->regs->direction;
+	error = pm_runtime_get_sync(bank->chip.parent);
+	if (error < 0) {
+		dev_err(bank->chip.parent,
+			"Could not enable gpio bank %p: %d\n",
+			bank, error);
+		pm_runtime_put_noidle(bank->chip.parent);
+
+		return error;
+	}
 	raw_spin_lock_irqsave(&bank->lock, flags);
 	dir = !!(readl_relaxed(reg) & BIT(offset));
 	raw_spin_unlock_irqrestore(&bank->lock, flags);
+	pm_runtime_put_sync(bank->chip.parent);
+
 	return dir;
 }
 
-- 
2.13.2

[toc] | [prev] | [next] | [standalone]

#1685091

On Tue, 11 Jul 2017, Tony Lindgren wrote:
> * Thomas Gleixner <tglx@linutronix.de> [170711 02:48]:
> And "external abort on non-linefetch" means something is not clocked
> in this case. The following alone makes things boot for me again, but I don't
> quite follow what has now changed with the ordering.. Thomas, any ideas?

Ah. Now that makes sense.

Unpatched the ordering is:

	  chip_bus_lock(desc);
	  irq_request_resources(desc);

Now the offending change reordered the calls. OMAP gpio has:

    omap_gpio_irq_bus_lock()
       pm_runtime_get_sync(bank->chip.parent);

So that at least explains the error. So that omap gpio irq chip (ab)uses
the bus_lock() callback to do runtime power management. Sigh, I did not
expect that. Let me have a deeper look if that's OMAP only or whether this
happens in other places as well.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1685120

On Tue, 11 Jul 2017, Thomas Gleixner wrote:
> On Tue, 11 Jul 2017, Tony Lindgren wrote:
> > * Thomas Gleixner <tglx@linutronix.de> [170711 02:48]:
> > And "external abort on non-linefetch" means something is not clocked
> > in this case. The following alone makes things boot for me again, but I don't
> > quite follow what has now changed with the ordering.. Thomas, any ideas?
> 
> Ah. Now that makes sense.
> 
> Unpatched the ordering is:
> 
> 	  chip_bus_lock(desc);
> 	  irq_request_resources(desc);
> 
> Now the offending change reordered the calls. OMAP gpio has:
> 
>     omap_gpio_irq_bus_lock()
>        pm_runtime_get_sync(bank->chip.parent);
> 
> So that at least explains the error. So that omap gpio irq chip (ab)uses
> the bus_lock() callback to do runtime power management. Sigh, I did not
> expect that. Let me have a deeper look if that's OMAP only or whether this
> happens in other places as well.

So OMAP-GPIO is the only driver which abuses bus_lock/unlock() in that way
and gets surprised.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1685164

* Thomas Gleixner <tglx@linutronix.de> [170711 08:07]:
> On Tue, 11 Jul 2017, Thomas Gleixner wrote:
> > On Tue, 11 Jul 2017, Tony Lindgren wrote:
> > > * Thomas Gleixner <tglx@linutronix.de> [170711 02:48]:
> > > And "external abort on non-linefetch" means something is not clocked
> > > in this case. The following alone makes things boot for me again, but I don't
> > > quite follow what has now changed with the ordering.. Thomas, any ideas?
> > 
> > Ah. Now that makes sense.
> > 
> > Unpatched the ordering is:
> > 
> > 	  chip_bus_lock(desc);
> > 	  irq_request_resources(desc);
> > 
> > Now the offending change reordered the calls. OMAP gpio has:
> > 
> >     omap_gpio_irq_bus_lock()
> >        pm_runtime_get_sync(bank->chip.parent);
> > 
> > So that at least explains the error. So that omap gpio irq chip (ab)uses
> > the bus_lock() callback to do runtime power management. Sigh, I did not
> > expect that. Let me have a deeper look if that's OMAP only or whether this
> > happens in other places as well.

OK that explains.

> So OMAP-GPIO is the only driver which abuses bus_lock/unlock() in that way
> and gets surprised.

OK. Grygorii, care to take a look if there's a better way to deal with
runtime PM for gpio-omap?

Meanwhile, below is my fix again with a proper description so we can
have working -rc1.

Regards,

Tony

8< ------
From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Tue, 11 Jul 2017 06:40:50 -0700
Subject: [PATCH] gpio: omap: Fix external abort on non-linefetch

Commit 46e48e257360 ("genirq: Move irq resource handling out of spinlocked
region") caused external abort on non-linefetch for n900 and droid 4. Turns
out this was caused by runtime PM use in gpio-omap in chip_bus_lock:

* Thomas Gleixner <tglx@linutronix.de> [170711 08:07]:
> > Unpatched the ordering is:
> >
> >       chip_bus_lock(desc);
> >       irq_request_resources(desc);
> >
> > Now the offending change reordered the calls. OMAP gpio has:
> >
> >     omap_gpio_irq_bus_lock()
> >        pm_runtime_get_sync(bank->chip.parent);
> >
> > So that at least explains the error. So that omap gpio irq chip (ab)uses
> > the bus_lock() callback to do runtime power management. Sigh, I did not
> > expect that.

Let's fix it with pm_runtime_get_sync() for now, then further improvments
can be done when we have a better way to deal with it.

Reported-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/gpio/gpio-omap.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -917,13 +917,24 @@ static int omap_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
 	struct gpio_bank *bank;
 	unsigned long flags;
 	void __iomem *reg;
-	int dir;
+	int error, dir;
 
 	bank = gpiochip_get_data(chip);
 	reg = bank->base + bank->regs->direction;
+	error = pm_runtime_get_sync(bank->chip.parent);
+	if (error < 0) {
+		dev_err(bank->chip.parent,
+			"Could not enable gpio bank %p: %d\n",
+			bank, error);
+		pm_runtime_put_noidle(bank->chip.parent);
+
+		return error;
+	}
 	raw_spin_lock_irqsave(&bank->lock, flags);
 	dir = !!(readl_relaxed(reg) & BIT(offset));
 	raw_spin_unlock_irqrestore(&bank->lock, flags);
+	pm_runtime_put_sync(bank->chip.parent);
+
 	return dir;
 }
 
-- 
2.13.2

[toc] | [prev] | [next] | [standalone]

#1685159

[Multipart message — attachments visible in raw view] — view raw

On Tue, Jul 11, 2017 at 7:41 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Ah. Now that makes sense.
>
> Unpatched the ordering is:
>
>           chip_bus_lock(desc);
>           irq_request_resources(desc);

I *looked* at that ordering and then went "Naah, that makes no sense".

But if that's the only issue, how about we just re-order those things
- we still don't need to move the irq_request_resources() into the
spinlock, we just move it to below the chip_bus_lock().

IOW, something like the (COMPLETELY UNTEESTED!) attached patch.

This assumes that the chip_bus_lock() thing is still ok for the RT
case, but it looks like it might be: the only other one I looked at
(apart from the gpio-omap one) used a mutex.

             Linus

[toc] | [prev] | [next] | [standalone]

#1685180

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Tue, Jul 11, 2017 at 08:40:10AM -0700, Linus Torvalds wrote:
> On Tue, Jul 11, 2017 at 7:41 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > Ah. Now that makes sense.
> >
> > Unpatched the ordering is:
> >
> >           chip_bus_lock(desc);
> >           irq_request_resources(desc);
> 
> I *looked* at that ordering and then went "Naah, that makes no sense".
> 
> But if that's the only issue, how about we just re-order those things
> - we still don't need to move the irq_request_resources() into the
> spinlock, we just move it to below the chip_bus_lock().
> 
> IOW, something like the (COMPLETELY UNTEESTED!) attached patch.

That patch on top of 9967468c0a10 fixes boot on Droid 4.

> This assumes that the chip_bus_lock() thing is still ok for the RT
> case, but it looks like it might be: the only other one I looked at
> (apart from the gpio-omap one) used a mutex.

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1685184

* Linus Torvalds <torvalds@linux-foundation.org> [170711 08:40]:
> On Tue, Jul 11, 2017 at 7:41 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > Ah. Now that makes sense.
> >
> > Unpatched the ordering is:
> >
> >           chip_bus_lock(desc);
> >           irq_request_resources(desc);
> 
> I *looked* at that ordering and then went "Naah, that makes no sense".
> 
> But if that's the only issue, how about we just re-order those things
> - we still don't need to move the irq_request_resources() into the
> spinlock, we just move it to below the chip_bus_lock().
> 
> IOW, something like the (COMPLETELY UNTEESTED!) attached patch.

Yeah that fixes the issue:

Tested-by: Tony Lindgren <tony@atomide.com>

> This assumes that the chip_bus_lock() thing is still ok for the RT
> case, but it looks like it might be: the only other one I looked at
> (apart from the gpio-omap one) used a mutex.

Yeah and the ordering below makes more sense to me at least. That is
assuming we want to call chip_bus_lock() before we start calling the
chip functions :)

Regards,

Tony


>  kernel/irq/manage.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 5624b2dd6b58..ea1b9404c041 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1168,17 +1168,17 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  		new->flags &= ~IRQF_ONESHOT;
>  
>  	mutex_lock(&desc->request_mutex);
> +	chip_bus_lock(desc);
> +
>  	if (!desc->action) {
>  		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_mutex;
> +			goto out_unlock_chip_bus;
>  		}
>  	}
>  
> -	chip_bus_lock(desc);
> -
>  	/*
>  	 * The following block of code has to be executed atomically
>  	 */
> @@ -1385,12 +1385,11 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  out_unlock:
>  	raw_spin_unlock_irqrestore(&desc->lock, flags);
>  
> -	chip_bus_sync_unlock(desc);
> -
>  	if (!desc->action)
>  		irq_release_resources(desc);
>  
> -out_mutex:
> +out_unlock_chip_bus:
> +	chip_bus_sync_unlock(desc);
>  	mutex_unlock(&desc->request_mutex);
>  
>  out_thread:

[toc] | [prev] | [next] | [standalone]

#1685228

On Tue, 11 Jul 2017, Tony Lindgren wrote:
> * Linus Torvalds <torvalds@linux-foundation.org> [170711 08:40]:
> > On Tue, Jul 11, 2017 at 7:41 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > >
> > > Ah. Now that makes sense.
> > >
> > > Unpatched the ordering is:
> > >
> > >           chip_bus_lock(desc);
> > >           irq_request_resources(desc);
> > 
> > I *looked* at that ordering and then went "Naah, that makes no sense".
> > 
> > But if that's the only issue, how about we just re-order those things
> > - we still don't need to move the irq_request_resources() into the
> > spinlock, we just move it to below the chip_bus_lock().
> > 
> > IOW, something like the (COMPLETELY UNTEESTED!) attached patch.
> 
> Yeah that fixes the issue:
> 
> Tested-by: Tony Lindgren <tony@atomide.com>
> 
> > This assumes that the chip_bus_lock() thing is still ok for the RT
> > case, but it looks like it might be: the only other one I looked at
> > (apart from the gpio-omap one) used a mutex.
> 
> Yeah and the ordering below makes more sense to me at least. That is
> assuming we want to call chip_bus_lock() before we start calling the
> chip functions :)

We can do that, just the free path is ugly and does not really work that
way.

__free_irq()
	....
	chip_bus_sync_unlock(desc);
	...
	synchronize_irq(irq);
	...
	if (!desc->action) {
		irq_release_resources();
		irq_remove_timings();
	}
	mutex_unlock(&desc->request_mutex);

We can't release request_mutex early otherwise we run into the issue of a
concurrent request_irq() trying to reuse stuff which we just release, but
we can't reacquire bus_lock under request_mutex either when we change the
lock ordering to bus_lock -> desc->request_mutex -> desc->lock.

We really want to have both the release_resources() and the
remove_timings() calls outside of the spinlocked region. That's not only a
RT issue, there have been requests for making the resource call 'sleepable'
for mainline as well.

Below is a slightly different fix, which keeps the lock order

      desc->request_mutex -> bus_lock -> desc->lock

intact and conditionally reacquired the bus lock for the release call.

Thanks,

	tglx

8<------------------------
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1036,13 +1036,20 @@ static int irq_request_resources(struct
 	return c->irq_request_resources ? c->irq_request_resources(d) : 0;
 }
 
-static void irq_release_resources(struct irq_desc *desc)
+static void irq_release_resources(struct irq_desc *desc, bool buslock)
 {
 	struct irq_data *d = &desc->irq_data;
 	struct irq_chip *c = d->chip;
 
-	if (c->irq_release_resources)
-		c->irq_release_resources(d);
+	if (!c->irq_release_resources)
+		return;
+	if (buslock)
+		chip_bus_lock(desc);
+
+	c->irq_release_resources(d);
+
+	if (buslock)
+		chip_bus_sync_unlock(desc);
 }
 
 static int
@@ -1168,17 +1175,16 @@ static int
 		new->flags &= ~IRQF_ONESHOT;
 
 	mutex_lock(&desc->request_mutex);
+	chip_bus_lock(desc);
 	if (!desc->action) {
 		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_mutex;
+			goto out_bus;
 		}
 	}
 
-	chip_bus_lock(desc);
-
 	/*
 	 * The following block of code has to be executed atomically
 	 */
@@ -1286,10 +1292,8 @@ static int
 			ret = __irq_set_trigger(desc,
 						new->flags & IRQF_TRIGGER_MASK);
 
-			if (ret) {
-				irq_release_resources(desc);
+			if (ret)
 				goto out_unlock;
-			}
 		}
 
 		desc->istate &= ~(IRQS_AUTODETECT | IRQS_SPURIOUS_DISABLED | \
@@ -1385,12 +1389,10 @@ static int
 out_unlock:
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 
-	chip_bus_sync_unlock(desc);
-
 	if (!desc->action)
-		irq_release_resources(desc);
-
-out_mutex:
+		irq_release_resources(desc, false);
+out_bus:
+	chip_bus_sync_unlock(desc);
 	mutex_unlock(&desc->request_mutex);
 
 out_thread:
@@ -1472,6 +1474,7 @@ static struct irqaction *__free_irq(unsi
 			WARN(1, "Trying to free already-free IRQ %d\n", irq);
 			raw_spin_unlock_irqrestore(&desc->lock, flags);
 			chip_bus_sync_unlock(desc);
+			mutex_unlock(&desc->request_mutex);
 			return NULL;
 		}
 
@@ -1531,7 +1534,7 @@ static struct irqaction *__free_irq(unsi
 	}
 
 	if (!desc->action) {
-		irq_release_resources(desc);
+		irq_release_resources(desc, true);
 		irq_remove_timings(desc);
 	}
 

[toc] | [prev] | [next] | [standalone]

#1685242

* Thomas Gleixner <tglx@linutronix.de> [170711 10:17]:
> On Tue, 11 Jul 2017, Tony Lindgren wrote:
> > * Linus Torvalds <torvalds@linux-foundation.org> [170711 08:40]:
> > > On Tue, Jul 11, 2017 at 7:41 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > > >
> > > > Ah. Now that makes sense.
> > > >
> > > > Unpatched the ordering is:
> > > >
> > > >           chip_bus_lock(desc);
> > > >           irq_request_resources(desc);
> > > 
> > > I *looked* at that ordering and then went "Naah, that makes no sense".
> > > 
> > > But if that's the only issue, how about we just re-order those things
> > > - we still don't need to move the irq_request_resources() into the
> > > spinlock, we just move it to below the chip_bus_lock().
> > > 
> > > IOW, something like the (COMPLETELY UNTEESTED!) attached patch.
> > 
> > Yeah that fixes the issue:
> > 
> > Tested-by: Tony Lindgren <tony@atomide.com>
> > 
> > > This assumes that the chip_bus_lock() thing is still ok for the RT
> > > case, but it looks like it might be: the only other one I looked at
> > > (apart from the gpio-omap one) used a mutex.
> > 
> > Yeah and the ordering below makes more sense to me at least. That is
> > assuming we want to call chip_bus_lock() before we start calling the
> > chip functions :)
> 
> We can do that, just the free path is ugly and does not really work that
> way.

OK

> __free_irq()
> 	....
> 	chip_bus_sync_unlock(desc);
> 	...
> 	synchronize_irq(irq);
> 	...
> 	if (!desc->action) {
> 		irq_release_resources();
> 		irq_remove_timings();
> 	}
> 	mutex_unlock(&desc->request_mutex);
> 
> We can't release request_mutex early otherwise we run into the issue of a
> concurrent request_irq() trying to reuse stuff which we just release, but
> we can't reacquire bus_lock under request_mutex either when we change the
> lock ordering to bus_lock -> desc->request_mutex -> desc->lock.
> 
> We really want to have both the release_resources() and the
> remove_timings() calls outside of the spinlocked region. That's not only a
> RT issue, there have been requests for making the resource call 'sleepable'
> for mainline as well.
> 
> Below is a slightly different fix, which keeps the lock order
> 
>       desc->request_mutex -> bus_lock -> desc->lock
> 
> intact and conditionally reacquired the bus lock for the release call.

Yeah that fixes the issue too:

Tested-by: Tony Lindgren <tony@atomide.com>

Regards,

Tony


> 8<------------------------
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1036,13 +1036,20 @@ static int irq_request_resources(struct
>  	return c->irq_request_resources ? c->irq_request_resources(d) : 0;
>  }
>  
> -static void irq_release_resources(struct irq_desc *desc)
> +static void irq_release_resources(struct irq_desc *desc, bool buslock)
>  {
>  	struct irq_data *d = &desc->irq_data;
>  	struct irq_chip *c = d->chip;
>  
> -	if (c->irq_release_resources)
> -		c->irq_release_resources(d);
> +	if (!c->irq_release_resources)
> +		return;
> +	if (buslock)
> +		chip_bus_lock(desc);
> +
> +	c->irq_release_resources(d);
> +
> +	if (buslock)
> +		chip_bus_sync_unlock(desc);
>  }
>  
>  static int
> @@ -1168,17 +1175,16 @@ static int
>  		new->flags &= ~IRQF_ONESHOT;
>  
>  	mutex_lock(&desc->request_mutex);
> +	chip_bus_lock(desc);
>  	if (!desc->action) {
>  		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_mutex;
> +			goto out_bus;
>  		}
>  	}
>  
> -	chip_bus_lock(desc);
> -
>  	/*
>  	 * The following block of code has to be executed atomically
>  	 */
> @@ -1286,10 +1292,8 @@ static int
>  			ret = __irq_set_trigger(desc,
>  						new->flags & IRQF_TRIGGER_MASK);
>  
> -			if (ret) {
> -				irq_release_resources(desc);
> +			if (ret)
>  				goto out_unlock;
> -			}
>  		}
>  
>  		desc->istate &= ~(IRQS_AUTODETECT | IRQS_SPURIOUS_DISABLED | \
> @@ -1385,12 +1389,10 @@ static int
>  out_unlock:
>  	raw_spin_unlock_irqrestore(&desc->lock, flags);
>  
> -	chip_bus_sync_unlock(desc);
> -
>  	if (!desc->action)
> -		irq_release_resources(desc);
> -
> -out_mutex:
> +		irq_release_resources(desc, false);
> +out_bus:
> +	chip_bus_sync_unlock(desc);
>  	mutex_unlock(&desc->request_mutex);
>  
>  out_thread:
> @@ -1472,6 +1474,7 @@ static struct irqaction *__free_irq(unsi
>  			WARN(1, "Trying to free already-free IRQ %d\n", irq);
>  			raw_spin_unlock_irqrestore(&desc->lock, flags);
>  			chip_bus_sync_unlock(desc);
> +			mutex_unlock(&desc->request_mutex);
>  			return NULL;
>  		}
>  
> @@ -1531,7 +1534,7 @@ static struct irqaction *__free_irq(unsi
>  	}
>  
>  	if (!desc->action) {
> -		irq_release_resources(desc);
> +		irq_release_resources(desc, true);
>  		irq_remove_timings(desc);
>  	}
>  
> 
> 
> 
> 
> 
> 

[toc] | [prev] | [next] | [standalone]

#1685196

On Tue, 11 Jul 2017, Linus Torvalds wrote:

> On Tue, Jul 11, 2017 at 7:41 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > Ah. Now that makes sense.
> >
> > Unpatched the ordering is:
> >
> >           chip_bus_lock(desc);
> >           irq_request_resources(desc);
> 
> I *looked* at that ordering and then went "Naah, that makes no sense".
> 
> But if that's the only issue, how about we just re-order those things
> - we still don't need to move the irq_request_resources() into the
> spinlock, we just move it to below the chip_bus_lock().
> 
> IOW, something like the (COMPLETELY UNTEESTED!) attached patch.
> 
> This assumes that the chip_bus_lock() thing is still ok for the RT
> case, but it looks like it might be: the only other one I looked at
> (apart from the gpio-omap one) used a mutex.

I looked through all of them and the only special case is gpio-omap.

What I do not understand here is that we have already power management
around all of that.

       irq_chip_pm_get(&desc->irq_data);
       ...
       chip_bus_lock(desc);
       ...
       chip_bus_unlock_sync(desc);
       ...
       irq_chip_pm_put(&desc->irq_data);

So why is that not sufficient and needs extra magic in that GPIO driver?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1685197

* Thomas Gleixner <tglx@linutronix.de> [170711 09:20]:
> On Tue, 11 Jul 2017, Linus Torvalds wrote:
> 
> > On Tue, Jul 11, 2017 at 7:41 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > >
> > > Ah. Now that makes sense.
> > >
> > > Unpatched the ordering is:
> > >
> > >           chip_bus_lock(desc);
> > >           irq_request_resources(desc);
> > 
> > I *looked* at that ordering and then went "Naah, that makes no sense".
> > 
> > But if that's the only issue, how about we just re-order those things
> > - we still don't need to move the irq_request_resources() into the
> > spinlock, we just move it to below the chip_bus_lock().
> > 
> > IOW, something like the (COMPLETELY UNTEESTED!) attached patch.
> > 
> > This assumes that the chip_bus_lock() thing is still ok for the RT
> > case, but it looks like it might be: the only other one I looked at
> > (apart from the gpio-omap one) used a mutex.
> 
> I looked through all of them and the only special case is gpio-omap.
> 
> What I do not understand here is that we have already power management
> around all of that.
> 
>        irq_chip_pm_get(&desc->irq_data);
>        ...
>        chip_bus_lock(desc);
>        ...
>        chip_bus_unlock_sync(desc);
>        ...
>        irq_chip_pm_put(&desc->irq_data);
> 
> So why is that not sufficient and needs extra magic in that GPIO driver?

Yeah it seems we should eventually be able to use irq_chip_pm_get()
like Grygorii just explained.

But aren't we currently calling chip functions with irq_request_resources()
outside the chip_bus_lock() too in addition to the gpio-omap runtime PM
issue? It seems that the patch from Linus fixes that, no?

Regards,

Tony

[toc] | [prev] | [next] | [standalone]

#1685204

On Tue, Jul 11, 2017 at 9:19 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> What I do not understand here is that we have already power management
> around all of that.
>
>        irq_chip_pm_get(&desc->irq_data);
>        ...
>        chip_bus_lock(desc);
>        ...
>        chip_bus_unlock_sync(desc);
>        ...
>        irq_chip_pm_put(&desc->irq_data);
>
> So why is that not sufficient and needs extra magic in that GPIO driver?

Well, irq_chip_pm_get/put() isn't called just over the operation, it's
called over the *whole* sequence of the irq being enabled at all.

So the different (right now) is that

 - chip_bus_lock/unlock_sync() is purely done around the actual
operations to set up and tear down the irq data.

   So this just covers the very short setup/teardown.

 - irq_chip_pm_get/put() is called around the *whole* "irqs can be active" block

   This covers the whole lifetime of the irq, from setup to free.

Very different.

I'd really prefer my simple patch for now, leaving everything working
the way it used to work. I *think* it's ok for RT too. Yes?

.. and then maybe longer term people can look more at this and clean
up the oddities.

               Linus

[toc] | [prev] | [next] | [standalone]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web