[patch 0/5] genirq: Distangle irq_request/release_resources() from irq_desc->lock

[patch 0/5] genirq: Distangle irq_request/release_resources() from irq_desc->lock

[Thomas Gleixner]

The irq_request/release_resources() callbacks are a problem on RT as they
are called under irq_desc->lock with interrupts disabled.

Aside of that calling them under irq_desc->lock is conceptually wrong and
has a (hard to trigger and probably theoretical) issue in free_irq(). See
patch 4/5 for a detailed explanation.

The series contains also a fix for the exynos gpio driver, which fiddles
with the irq masking in the resource callbacks, which is bogus and
potentially harmful.

Finally it moves the irq timings deallocation out of the spin locked region.

Thanks,

	tglx
---
 drivers/pinctrl/samsung/pinctrl-exynos.c |    4 --
 include/linux/irqdesc.h                  |    3 +
 kernel/irq/irqdesc.c                     |    1 
 kernel/irq/manage.c                      |   47 +++++++++++++++++++------------
 4 files changed, 34 insertions(+), 21 deletions(-)

[patch 1/5] pinctrl: samsung: Remove bogus irq_[un]mask from resource management

[Thomas Gleixner]

[-- Attachment #1: pinctrl--samsung--Remove-bogus-irqmask-unmask.patch --]
[-- Type: text/plain, Size: 1454 bytes --]

The irq chip callbacks irq_request/release_resources() have absolutely no
business with masking and unmasking the irq.

The core code unmasks the interrupt after complete setup and masks it
before invoking irq_release_resources().

The unmask is actually harmful as it happens before the interrupt is
completely initialized in __setup_irq().

Remove it.

Fixes: f6a8249f9e55 ("pinctrl: exynos: Lock GPIOs as interrupts when used as EINTs")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Tomasz Figa <tomasz.figa@gmail.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Kukjin Kim <kgene@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-samsung-soc@vger.kernel.org
Cc: linux-gpio@vger.kernel.org
---
 drivers/pinctrl/samsung/pinctrl-exynos.c |    4 ----
 1 file changed, 4 deletions(-)

--- a/drivers/pinctrl/samsung/pinctrl-exynos.c
+++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
@@ -205,8 +205,6 @@ static int exynos_irq_request_resources(
 
 	spin_unlock_irqrestore(&bank->slock, flags);
 
-	exynos_irq_unmask(irqd);
-
 	return 0;
 }
 
@@ -226,8 +224,6 @@ static void exynos_irq_release_resources
 	shift = irqd->hwirq * bank_type->fld_width[PINCFG_TYPE_FUNC];
 	mask = (1 << bank_type->fld_width[PINCFG_TYPE_FUNC]) - 1;
 
-	exynos_irq_mask(irqd);
-
 	spin_lock_irqsave(&bank->slock, flags);
 
 	con = readl(bank->eint_base + reg_con);

Re: [patch 1/5] pinctrl: samsung: Remove bogus irq_[un]mask from resource management

[Tomasz Figa]

Hi Thomas,

2017-06-30 6:33 GMT+09:00 Thomas Gleixner <tglx@linutronix.de>:
> The irq chip callbacks irq_request/release_resources() have absolutely no
> business with masking and unmasking the irq.
>
> The core code unmasks the interrupt after complete setup and masks it
> before invoking irq_release_resources().
>
> The unmask is actually harmful as it happens before the interrupt is
> completely initialized in __setup_irq().
>
> Remove it.

Good catch, thanks! (Note that the original patch of mine [1] did that
in .irq_startup()/.irq_shutdown(), which was for some reason changed
later, but I don't remember the exact story.)

[1] https://patchwork.kernel.org/patch/4466431/

Acked-by: Tomasz Figa <tomasz.figa@gmail.com>

Sylwester, Krzysztof, would you be able to do some basic test?

Best regards,
Tomasz

>
> Fixes: f6a8249f9e55 ("pinctrl: exynos: Lock GPIOs as interrupts when used as EINTs")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Tomasz Figa <tomasz.figa@gmail.com>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Kukjin Kim <kgene@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-samsung-soc@vger.kernel.org
> Cc: linux-gpio@vger.kernel.org
> ---
>  drivers/pinctrl/samsung/pinctrl-exynos.c |    4 ----
>  1 file changed, 4 deletions(-)
>
> --- a/drivers/pinctrl/samsung/pinctrl-exynos.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
> @@ -205,8 +205,6 @@ static int exynos_irq_request_resources(
>
>         spin_unlock_irqrestore(&bank->slock, flags);
>
> -       exynos_irq_unmask(irqd);
> -
>         return 0;
>  }
>
> @@ -226,8 +224,6 @@ static void exynos_irq_release_resources
>         shift = irqd->hwirq * bank_type->fld_width[PINCFG_TYPE_FUNC];
>         mask = (1 << bank_type->fld_width[PINCFG_TYPE_FUNC]) - 1;
>
> -       exynos_irq_mask(irqd);
> -
>         spin_lock_irqsave(&bank->slock, flags);
>
>         con = readl(bank->eint_base + reg_con);
>
>

Re: [patch 1/5] pinctrl: samsung: Remove bogus irq_[un]mask from resource management

[Krzysztof Kozlowski]

On Fri, Jun 30, 2017 at 4:47 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Thomas,
>
> 2017-06-30 6:33 GMT+09:00 Thomas Gleixner <tglx@linutronix.de>:
>> The irq chip callbacks irq_request/release_resources() have absolutely no
>> business with masking and unmasking the irq.
>>
>> The core code unmasks the interrupt after complete setup and masks it
>> before invoking irq_release_resources().
>>
>> The unmask is actually harmful as it happens before the interrupt is
>> completely initialized in __setup_irq().
>>
>> Remove it.
>
> Good catch, thanks! (Note that the original patch of mine [1] did that
> in .irq_startup()/.irq_shutdown(), which was for some reason changed
> later, but I don't remember the exact story.)
>
> [1] https://patchwork.kernel.org/patch/4466431/
>
> Acked-by: Tomasz Figa <tomasz.figa@gmail.com>
>
> Sylwester, Krzysztof, would you be able to do some basic test?

I suppose this was not tested so yes - I have platforms do this. I
understand that checking any non-shared GPIO interrupt should be
sufficient to test, right?

Best regards,
Krzysztof

Re: [patch 1/5] pinctrl: samsung: Remove bogus irq_[un]mask from resource management

[Tomasz Figa]

2017-06-30 15:02 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>:
> On Fri, Jun 30, 2017 at 4:47 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> Hi Thomas,
>>
>> 2017-06-30 6:33 GMT+09:00 Thomas Gleixner <tglx@linutronix.de>:
>>> The irq chip callbacks irq_request/release_resources() have absolutely no
>>> business with masking and unmasking the irq.
>>>
>>> The core code unmasks the interrupt after complete setup and masks it
>>> before invoking irq_release_resources().
>>>
>>> The unmask is actually harmful as it happens before the interrupt is
>>> completely initialized in __setup_irq().
>>>
>>> Remove it.
>>
>> Good catch, thanks! (Note that the original patch of mine [1] did that
>> in .irq_startup()/.irq_shutdown(), which was for some reason changed
>> later, but I don't remember the exact story.)
>>
>> [1] https://patchwork.kernel.org/patch/4466431/
>>
>> Acked-by: Tomasz Figa <tomasz.figa@gmail.com>
>>
>> Sylwester, Krzysztof, would you be able to do some basic test?
>
> I suppose this was not tested so yes - I have platforms do this. I
> understand that checking any non-shared GPIO interrupt should be
> sufficient to test, right?

I think any interrupt from the Exynos pin controller should work, even
shared one. I'd expect irq_request_resources() to be invoked for
shared interrupts as well, otherwise we have a problem... (I quickly
looked through kernel/irq/manage.c and it seems to be invoked for the
first __setup_irq() call even for shared interrupts.)

Thanks.

Best regards,
Tomasz

Re: [patch 1/5] pinctrl: samsung: Remove bogus irq_[un]mask from resource management

[Linus Walleij]

On Thu, Jun 29, 2017 at 11:33 PM, Thomas Gleixner <tglx@linutronix.de> wrote:

> The irq chip callbacks irq_request/release_resources() have absolutely no
> business with masking and unmasking the irq.
>
> The core code unmasks the interrupt after complete setup and masks it
> before invoking irq_release_resources().
>
> The unmask is actually harmful as it happens before the interrupt is
> completely initialized in __setup_irq().
>
> Remove it.
>
> Fixes: f6a8249f9e55 ("pinctrl: exynos: Lock GPIOs as interrupts when used as EINTs")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Tomasz Figa <tomasz.figa@gmail.com>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Kukjin Kim <kgene@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-samsung-soc@vger.kernel.org
> Cc: linux-gpio@vger.kernel.org

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Does this patch have a dependency on the rest of the series or should
I just apply it as-is?

Yours,
Linus Walleij

Re: [patch 1/5] pinctrl: samsung: Remove bogus irq_[un]mask from resource management

[Thomas Gleixner]

On Fri, 30 Jun 2017, Linus Walleij wrote:
> On Thu, Jun 29, 2017 at 11:33 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > The irq chip callbacks irq_request/release_resources() have absolutely no
> > business with masking and unmasking the irq.
> >
> > The core code unmasks the interrupt after complete setup and masks it
> > before invoking irq_release_resources().
> >
> > The unmask is actually harmful as it happens before the interrupt is
> > completely initialized in __setup_irq().
> >
> > Remove it.
> >
> > Fixes: f6a8249f9e55 ("pinctrl: exynos: Lock GPIOs as interrupts when used as EINTs")
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Tomasz Figa <tomasz.figa@gmail.com>
> > Cc: Krzysztof Kozlowski <krzk@kernel.org>
> > Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Kukjin Kim <kgene@kernel.org>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-samsung-soc@vger.kernel.org
> > Cc: linux-gpio@vger.kernel.org
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Does this patch have a dependency on the rest of the series or should
> I just apply it as-is?

Has no dependecies at all.

Re: [patch 1/5] pinctrl: samsung: Remove bogus irq_[un]mask from resource management

[Linus Walleij]

On Thu, Jun 29, 2017 at 11:33 PM, Thomas Gleixner <tglx@linutronix.de> wrote:

> The irq chip callbacks irq_request/release_resources() have absolutely no
> business with masking and unmasking the irq.
>
> The core code unmasks the interrupt after complete setup and masks it
> before invoking irq_release_resources().
>
> The unmask is actually harmful as it happens before the interrupt is
> completely initialized in __setup_irq().
>
> Remove it.
>
> Fixes: f6a8249f9e55 ("pinctrl: exynos: Lock GPIOs as interrupts when used as EINTs")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Tomasz Figa <tomasz.figa@gmail.com>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Kukjin Kim <kgene@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-samsung-soc@vger.kernel.org
> Cc: linux-gpio@vger.kernel.org

Patch applied directly since it has no deps and fixes queued stuff for v4.13.
I guess Krysztof will make sure it doesn't break anything.

Yours,
Linus Walleij

Re: [patch 1/5] pinctrl: samsung: Remove bogus irq_[un]mask from resource management

[Krzysztof Kozlowski]

On Fri, Jun 30, 2017 at 03:53:18PM +0200, Linus Walleij wrote:
> On Thu, Jun 29, 2017 at 11:33 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > The irq chip callbacks irq_request/release_resources() have absolutely no
> > business with masking and unmasking the irq.
> >
> > The core code unmasks the interrupt after complete setup and masks it
> > before invoking irq_release_resources().
> >
> > The unmask is actually harmful as it happens before the interrupt is
> > completely initialized in __setup_irq().
> >
> > Remove it.
> >
> > Fixes: f6a8249f9e55 ("pinctrl: exynos: Lock GPIOs as interrupts when used as EINTs")
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Tomasz Figa <tomasz.figa@gmail.com>
> > Cc: Krzysztof Kozlowski <krzk@kernel.org>
> > Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Kukjin Kim <kgene@kernel.org>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-samsung-soc@vger.kernel.org
> > Cc: linux-gpio@vger.kernel.org
> 
> Patch applied directly since it has no deps and fixes queued stuff for v4.13.
> I guess Krysztof will make sure it doesn't break anything.

Everything seems to work fine with the patchset.

Best regards,
Krzysztof

[patch 2/5] genirq: Move bus locking into __setup_irq()

[Thomas Gleixner]

[-- Attachment #1: genirq--Move-locking-into-_-setup_irq--.patch --]
[-- Type: text/plain, Size: 1969 bytes --]

There is no point in having the irq_bus_lock() protection around all
callers to __setup_irq().

Move it into __setup_irq(). This is also a preparatory patch for addressing
the issues with the irq resource callbacks.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/irq/manage.c |   13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1167,6 +1167,8 @@ static int
 	if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)
 		new->flags &= ~IRQF_ONESHOT;
 
+	chip_bus_lock(desc);
+
 	/*
 	 * The following block of code has to be executed atomically
 	 */
@@ -1347,6 +1349,7 @@ static int
 	}
 
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
+	chip_bus_sync_unlock(desc);
 
 	irq_setup_timings(desc, new);
 
@@ -1378,6 +1381,8 @@ static int
 out_unlock:
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 
+	chip_bus_sync_unlock(desc);
+
 out_thread:
 	if (new->thread) {
 		struct task_struct *t = new->thread;
@@ -1417,9 +1422,7 @@ int setup_irq(unsigned int irq, struct i
 	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);
@@ -1674,9 +1677,7 @@ int request_threaded_irq(unsigned int ir
 		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 +1925,7 @@ int setup_percpu_irq(unsigned int irq, s
 	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);
@@ -1980,9 +1979,7 @@ int request_percpu_irq(unsigned int irq,
 		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);

[tip:irq/urgent] genirq: Move bus locking into __setup_irq()

[tip-bot for Thomas Gleixner]

Commit-ID:  3a90795e1e885167209056a1a90be965add30e25
Gitweb:     http://git.kernel.org/tip/3a90795e1e885167209056a1a90be965add30e25
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 29 Jun 2017 23:33:36 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 4 Jul 2017 12:46:15 +0200

genirq: Move bus locking into __setup_irq()

There is no point in having the irq_bus_lock() protection around all
callers to __setup_irq().

Move it into __setup_irq(). This is also a preparatory patch for addressing
the issues with the irq resource callbacks.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Julia Cartwright <julia@ni.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Doug Anderson <dianders@chromium.org>
Cc: linux-rockchip@lists.infradead.org
Cc: John Keeping <john@metanate.com>
Cc: linux-gpio@vger.kernel.org
Link: http://lkml.kernel.org/r/20170629214343.960949031@linutronix.de

---
 kernel/irq/manage.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 5c11c17..0934e02 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1167,6 +1167,8 @@ __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;
 
+	chip_bus_lock(desc);
+
 	/*
 	 * The following block of code has to be executed atomically
 	 */
@@ -1347,6 +1349,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	}
 
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
+	chip_bus_sync_unlock(desc);
 
 	irq_setup_timings(desc, new);
 
@@ -1378,6 +1381,8 @@ mismatch:
 out_unlock:
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 
+	chip_bus_sync_unlock(desc);
+
 out_thread:
 	if (new->thread) {
 		struct task_struct *t = new->thread;
@@ -1417,9 +1422,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);
@@ -1674,9 +1677,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 +1925,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);
@@ -1980,9 +1979,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);

[patch 3/5] genirq: Add mutex to irq desc to serialize request/free_irq()

[Thomas Gleixner]

[-- Attachment #1: genirq--Add-mutex-to-irq-desc-to-serialize-request-free.patch --]
[-- Type: text/plain, Size: 2687 bytes --]

The irq_request/release_resources() callbacks ar currently invoked under
desc->lock with interrupts disabled. This is a source of problems on RT and
conceptually not required.

Add a seperate mutex to struct irq_desc which allows to serialize
request/free_irq(), which can be used to move the resource functions out of
the desc->lock held region.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/irqdesc.h |    3 +++
 kernel/irq/irqdesc.c    |    1 +
 kernel/irq/manage.c     |    8 ++++++++
 3 files changed, 12 insertions(+)

--- 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;
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -373,6 +373,7 @@ static struct irq_desc *alloc_desc(int i
 
 	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);
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1167,6 +1167,8 @@ static int
 	if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)
 		new->flags &= ~IRQF_ONESHOT;
 
+	mutex_lock(&desc->request_mutex);
+
 	chip_bus_lock(desc);
 
 	/*
@@ -1350,6 +1352,7 @@ static int
 
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 	chip_bus_sync_unlock(desc);
+	mutex_unlock(&desc->request_mutex);
 
 	irq_setup_timings(desc, new);
 
@@ -1383,6 +1386,8 @@ static int
 
 	chip_bus_sync_unlock(desc);
 
+	mutex_unlock(&desc->request_mutex);
+
 out_thread:
 	if (new->thread) {
 		struct task_struct *t = new->thread;
@@ -1446,6 +1451,7 @@ static struct irqaction *__free_irq(unsi
 	if (!desc)
 		return NULL;
 
+	mutex_lock(&desc->request_mutex);
 	chip_bus_lock(desc);
 	raw_spin_lock_irqsave(&desc->lock, flags);
 
@@ -1521,6 +1527,8 @@ static struct irqaction *__free_irq(unsi
 		}
 	}
 
+	mutex_unlock(&desc->request_mutex);
+
 	irq_chip_pm_put(&desc->irq_data);
 	module_put(desc->owner);
 	kfree(action->secondary);

[tip:irq/urgent] genirq: Add mutex to irq desc to serialize request/free_irq()

[tip-bot for Thomas Gleixner]

Commit-ID:  9114014cf4e6df0b22d764380ae1fc54f1a7a8b2
Gitweb:     http://git.kernel.org/tip/9114014cf4e6df0b22d764380ae1fc54f1a7a8b2
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 29 Jun 2017 23:33:37 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 4 Jul 2017 12:46:16 +0200

genirq: Add mutex to irq desc to serialize request/free_irq()

The irq_request/release_resources() callbacks ar currently invoked under
desc->lock with interrupts disabled. This is a source of problems on RT and
conceptually not required.

Add a seperate mutex to struct irq_desc which allows to serialize
request/free_irq(), which can be used to move the resource functions out of
the desc->lock held region.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Julia Cartwright <julia@ni.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Doug Anderson <dianders@chromium.org>
Cc: linux-rockchip@lists.infradead.org
Cc: John Keeping <john@metanate.com>
Cc: linux-gpio@vger.kernel.org
Link: http://lkml.kernel.org/r/20170629214344.039220922@linutronix.de

---
 include/linux/irqdesc.h | 3 +++
 kernel/irq/irqdesc.c    | 1 +
 kernel/irq/manage.c     | 8 ++++++++
 3 files changed, 12 insertions(+)

diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index d425a3a..3e90a09 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/irqdesc.c b/kernel/irq/irqdesc.c
index 948b50e..906a67e 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/manage.c b/kernel/irq/manage.c
index 0934e02..0139908 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1167,6 +1167,8 @@ __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);
+
 	chip_bus_lock(desc);
 
 	/*
@@ -1350,6 +1352,7 @@ __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);
 
@@ -1383,6 +1386,8 @@ out_unlock:
 
 	chip_bus_sync_unlock(desc);
 
+	mutex_unlock(&desc->request_mutex);
+
 out_thread:
 	if (new->thread) {
 		struct task_struct *t = new->thread;
@@ -1446,6 +1451,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);
 
@@ -1521,6 +1527,8 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 		}
 	}
 
+	mutex_unlock(&desc->request_mutex);
+
 	irq_chip_pm_put(&desc->irq_data);
 	module_put(desc->owner);
 	kfree(action->secondary);

[patch 4/5] genirq: Move irq resource handling out of spinlocked region

[Thomas Gleixner]

[-- Attachment #1: genirq--Move-irq-resource-handling-out-of-spinlocked-region.patch --]
[-- Type: text/plain, Size: 2286 bytes --]

Aside of being conceptually wrong, there is also an actual (hard to
trigger and mostly theoretical) problem.

CPU0				CPU1
free_irq(X)			interrupt X
				spin_lock(desc->lock)
				wake irq thread()
				spin_unlock(desc->lock)
spin_lock(desc->lock)
remove action()
shutdown_irq()			
release_resources()		thread_handler()
spin_unlock(desc->lock)		  access released resources.

synchronize_irq()

Move the release resources invocation after synchronize_irq() so it's
guaranteed that the threaded handler has finished.

Move the resource request call out of the desc->lock held region as well,
so the invocation context is the same for both request and release.

This solves the problems with those functions on RT as well.
 
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/irq/manage.c |   23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1168,6 +1168,14 @@ static int
 		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);
 
@@ -1271,13 +1279,6 @@ static int
 	}
 
 	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: */
@@ -1386,6 +1387,10 @@ static int
 
 	chip_bus_sync_unlock(desc);
 
+	if (!desc->action)
+		irq_release_resources(desc);
+
+out_mutex:
 	mutex_unlock(&desc->request_mutex);
 
 out_thread:
@@ -1484,7 +1489,6 @@ static struct irqaction *__free_irq(unsi
 	if (!desc->action) {
 		irq_settings_clr_disable_unlazy(desc);
 		irq_shutdown(desc);
-		irq_release_resources(desc);
 		irq_remove_timings(desc);
 	}
 
@@ -1527,6 +1531,9 @@ static struct irqaction *__free_irq(unsi
 		}
 	}
 
+	if (!desc->action)
+		irq_release_resources(desc);
+
 	mutex_unlock(&desc->request_mutex);
 
 	irq_chip_pm_put(&desc->irq_data);

[tip:irq/urgent] genirq: Move irq resource handling out of spinlocked region

[tip-bot for Thomas Gleixner]

Commit-ID:  46e48e257360f0845fe17089713cbad4db611e70
Gitweb:     http://git.kernel.org/tip/46e48e257360f0845fe17089713cbad4db611e70
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 29 Jun 2017 23:33:38 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 4 Jul 2017 12:46:16 +0200

genirq: Move irq resource handling out of spinlocked region

Aside of being conceptually wrong, there is also an actual (hard to
trigger and mostly theoretical) problem.

CPU0				CPU1
free_irq(X)			interrupt X
				spin_lock(desc->lock)
				wake irq thread()
				spin_unlock(desc->lock)
spin_lock(desc->lock)
remove action()
shutdown_irq()			
release_resources()		thread_handler()
spin_unlock(desc->lock)		  access released resources.

synchronize_irq()

Move the release resources invocation after synchronize_irq() so it's
guaranteed that the threaded handler has finished.

Move the resource request call out of the desc->lock held region as well,
so the invocation context is the same for both request and release.

This solves the problems with those functions on RT as well.
 
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Julia Cartwright <julia@ni.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Doug Anderson <dianders@chromium.org>
Cc: linux-rockchip@lists.infradead.org
Cc: John Keeping <john@metanate.com>
Cc: linux-gpio@vger.kernel.org
Link: http://lkml.kernel.org/r/20170629214344.117028181@linutronix.de

---
 kernel/irq/manage.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 0139908..3e69343 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1168,6 +1168,14 @@ __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);
 
@@ -1271,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: */
@@ -1386,6 +1387,10 @@ out_unlock:
 
 	chip_bus_sync_unlock(desc);
 
+	if (!desc->action)
+		irq_release_resources(desc);
+
+out_mutex:
 	mutex_unlock(&desc->request_mutex);
 
 out_thread:
@@ -1484,7 +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);
 	}
 
@@ -1527,6 +1531,9 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 		}
 	}
 
+	if (!desc->action)
+		irq_release_resources(desc);
+
 	mutex_unlock(&desc->request_mutex);
 
 	irq_chip_pm_put(&desc->irq_data);

[patch 5/5] genirq/timings: Move free timings out of spinlocked region

[Thomas Gleixner]

[-- Attachment #1: genirq-timings--Move-free-timings-out-of-spinlocked-region.patch --]
[-- Type: text/plain, Size: 750 bytes --]

No point to do memory management from a interrupt disabled spin locked
region.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 kernel/irq/manage.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1489,7 +1489,6 @@ static struct irqaction *__free_irq(unsi
 	if (!desc->action) {
 		irq_settings_clr_disable_unlazy(desc);
 		irq_shutdown(desc);
-		irq_remove_timings(desc);
 	}
 
 #ifdef CONFIG_SMP
@@ -1531,8 +1530,10 @@ static struct irqaction *__free_irq(unsi
 		}
 	}
 
-	if (!desc->action)
+	if (!desc->action) {
 		irq_release_resources(desc);
+		irq_remove_timings(desc);
+	}
 
 	mutex_unlock(&desc->request_mutex);
 

[tip:irq/urgent] genirq/timings: Move free timings out of spinlocked region

[tip-bot for Thomas Gleixner]

Commit-ID:  2343877fbda701599653e63f8dcc318aa1bf15ee
Gitweb:     http://git.kernel.org/tip/2343877fbda701599653e63f8dcc318aa1bf15ee
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 29 Jun 2017 23:33:39 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 4 Jul 2017 12:46:16 +0200

genirq/timings: Move free timings out of spinlocked region

No point to do memory management from a interrupt disabled spin locked
region.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Julia Cartwright <julia@ni.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Doug Anderson <dianders@chromium.org>
Cc: linux-rockchip@lists.infradead.org
Cc: John Keeping <john@metanate.com>
Cc: linux-gpio@vger.kernel.org
Link: http://lkml.kernel.org/r/20170629214344.196130646@linutronix.de

---
 kernel/irq/manage.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 3e69343..91e1f23 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1489,7 +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_remove_timings(desc);
 	}
 
 #ifdef CONFIG_SMP
@@ -1531,8 +1530,10 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 		}
 	}
 
-	if (!desc->action)
+	if (!desc->action) {
 		irq_release_resources(desc);
+		irq_remove_timings(desc);
+	}
 
 	mutex_unlock(&desc->request_mutex);
 

Re: [patch 0/5] genirq: Distangle irq_request/release_resources() from irq_desc->lock

[Marc Zyngier]

On 29/06/17 22:33, Thomas Gleixner wrote:
> The irq_request/release_resources() callbacks are a problem on RT as they
> are called under irq_desc->lock with interrupts disabled.
> 
> Aside of that calling them under irq_desc->lock is conceptually wrong and
> has a (hard to trigger and probably theoretical) issue in free_irq(). See
> patch 4/5 for a detailed explanation.
> 
> The series contains also a fix for the exynos gpio driver, which fiddles
> with the irq masking in the resource callbacks, which is bogus and
> potentially harmful.
> 
> Finally it moves the irq timings deallocation out of the spin locked region.
> 
> Thanks,
> 
> 	tglx
> ---
>  drivers/pinctrl/samsung/pinctrl-exynos.c |    4 --
>  include/linux/irqdesc.h                  |    3 +
>  kernel/irq/irqdesc.c                     |    1 
>  kernel/irq/manage.c                      |   47 +++++++++++++++++++------------
>  4 files changed, 34 insertions(+), 21 deletions(-)

For the series:

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...