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

Groups > linux.kernel > #1654084 > unrolled thread

[patch 0/2] genirq: Handle NOAUTOEN interrupts correctly

Back to article view | Back to linux.kernel

Contents

  [patch 0/2] genirq: Handle NOAUTOEN interrupts correctly Thomas Gleixner <tglx@linutronix.de> - 2017-05-31 12:10 +0200
    [patch 1/2] genirq: Handle NOAUTOEN interrupt setup proper Thomas Gleixner <tglx@linutronix.de> - 2017-05-31 12:10 +0200
      Re: [patch 1/2] genirq: Handle NOAUTOEN interrupt setup proper Marc Zyngier <marc.zyngier@arm.com> - 2017-05-31 16:00 +0200
        Re: [patch 1/2] genirq: Handle NOAUTOEN interrupt setup proper Thomas Gleixner <tglx@linutronix.de> - 2017-05-31 17:20 +0200
      [tip:irq/core] genirq: Handle NOAUTOEN interrupt setup proper tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-06-04 15:00 +0200
    [patch 2/2] genirq: Warn when IRQ_NOAUTOEN is used with shared  interrupts Thomas Gleixner <tglx@linutronix.de> - 2017-05-31 12:10 +0200
      [tip:irq/core] genirq: Warn when IRQ_NOAUTOEN is used with shared  interrupts tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-06-04 15:00 +0200

#1654084 — [patch 0/2] genirq: Handle NOAUTOEN interrupts correctly

This series addresses a few short comings in the interrupt core code
vs. interrupts marked NOAUTOEN and also paves the way for making sure that
irq chip callbacks like irq_startup/shutdown/enable/disable are only called
when an actual state change is required.

Thanks,

	tglx

[toc] | [next] | [standalone]

#1654089 — [patch 1/2] genirq: Handle NOAUTOEN interrupt setup proper

If an interrupt is marked NOAUTOEN then request_irq() installs the action,
but does not enable the interrupt via startup_irq().  The interrupt is
enabled via enable_irq() later from the driver. enable_irq() calls
irq_enable().

That means that for interrupts which have a irq_startup() callback this
callback is never invoked. Neither is irq_domain_activate_irq() invoked for
such interrupts.

If an interrupt depends on irq_startup() or irq_domain_activate_irq() then
the enable via irq_enable() is not enough.

Add a status flag IRQD_IRQ_STARTED_UP and use this to select the proper
mechanism in enable_irq(). Use the flag also to avoid pointless calls into
the low level functions.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/irq.h    |    6 ++++
 kernel/irq/chip.c      |   73 +++++++++++++++++++++++++++++++++++++------------
 kernel/irq/internals.h |    1 
 kernel/irq/manage.c    |    2 -
 4 files changed, 64 insertions(+), 18 deletions(-)

--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -216,6 +216,7 @@ enum {
 	IRQD_WAKEUP_ARMED		= (1 << 19),
 	IRQD_FORWARDED_TO_VCPU		= (1 << 20),
 	IRQD_AFFINITY_MANAGED		= (1 << 21),
+	IRQD_IRQ_STARTED_UP		= (1 << 22),
 };
 
 #define __irqd_to_state(d) ACCESS_PRIVATE((d)->common, state_use_accessors)
@@ -329,6 +330,11 @@ static inline void irqd_clr_activated(st
 	__irqd_to_state(d) &= ~IRQD_ACTIVATED;
 }
 
+static inline bool irqd_is_started_up(struct irq_data *d)
+{
+	return __irqd_to_state(d) & IRQD_IRQ_STARTED_UP;
+}
+
 #undef __irqd_to_state
 
 static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -185,37 +185,71 @@ static void irq_state_set_masked(struct
 	irqd_set(&desc->irq_data, IRQD_IRQ_MASKED);
 }
 
+static void irq_state_clr_started(struct irq_desc *desc)
+{
+	irqd_clear(&desc->irq_data, IRQD_IRQ_STARTED_UP);
+}
+
+static void irq_state_set_started(struct irq_desc *desc)
+{
+	irqd_set(&desc->irq_data, IRQD_IRQ_STARTED_UP);
+}
+
 int irq_startup(struct irq_desc *desc, bool resend)
 {
 	int ret = 0;
 
-	irq_state_clr_disabled(desc);
+	if (irqd_is_started_up(&desc->irq_data))
+		return 0;
+
 	desc->depth = 0;
 
 	irq_domain_activate_irq(&desc->irq_data);
 	if (desc->irq_data.chip->irq_startup) {
 		ret = desc->irq_data.chip->irq_startup(&desc->irq_data);
+		irq_state_clr_disabled(desc);
 		irq_state_clr_masked(desc);
 	} else {
 		irq_enable(desc);
 	}
+	irq_state_set_started(desc);
+
 	if (resend)
 		check_irq_resend(desc);
+
 	return ret;
 }
 
+static void __irq_disable(struct irq_desc *desc, bool mask);
+
 void irq_shutdown(struct irq_desc *desc)
 {
-	irq_state_set_disabled(desc);
-	desc->depth = 1;
-	if (desc->irq_data.chip->irq_shutdown)
-		desc->irq_data.chip->irq_shutdown(&desc->irq_data);
-	else if (desc->irq_data.chip->irq_disable)
-		desc->irq_data.chip->irq_disable(&desc->irq_data);
-	else
-		desc->irq_data.chip->irq_mask(&desc->irq_data);
+	if (irqd_is_started_up(&desc->irq_data)) {
+		desc->depth = 1;
+		if (desc->irq_data.chip->irq_shutdown) {
+			desc->irq_data.chip->irq_shutdown(&desc->irq_data);
+			irq_state_set_disabled(desc);
+			irq_state_set_masked(desc);
+		} else {
+			__irq_disable(desc, true);
+		}
+		irq_state_clr_started(desc);
+	}
+	/*
+	 * This must be called even if the interrupt was never started up,
+	 * because the activation can happen before the interrupt is
+	 * available for request/startup. It has it's own state tracking so
+	 * it's safe to call it unconditonally.
+	 */
 	irq_domain_deactivate_irq(&desc->irq_data);
-	irq_state_set_masked(desc);
+}
+
+void irq_enable_or_startup(struct irq_desc *desc)
+{
+	if (!irqd_is_started_up(&desc->irq_data))
+		irq_startup(desc, false);
+	else
+		irq_enable(desc);
 }
 
 void irq_enable(struct irq_desc *desc)
@@ -228,6 +262,17 @@ void irq_enable(struct irq_desc *desc)
 	irq_state_clr_masked(desc);
 }
 
+static void __irq_disable(struct irq_desc *desc, bool mask)
+{
+	irq_state_set_disabled(desc);
+	if (desc->irq_data.chip->irq_disable) {
+		desc->irq_data.chip->irq_disable(&desc->irq_data);
+		irq_state_set_masked(desc);
+	} else if (mask) {
+		mask_irq(desc);
+	}
+}
+
 /**
  * irq_disable - Mark interrupt disabled
  * @desc:	irq descriptor which should be disabled
@@ -250,13 +295,7 @@ void irq_enable(struct irq_desc *desc)
  */
 void irq_disable(struct irq_desc *desc)
 {
-	irq_state_set_disabled(desc);
-	if (desc->irq_data.chip->irq_disable) {
-		desc->irq_data.chip->irq_disable(&desc->irq_data);
-		irq_state_set_masked(desc);
-	} else if (irq_settings_disable_unlazy(desc)) {
-		mask_irq(desc);
-	}
+	__irq_disable(desc, irq_settings_disable_unlazy(desc));
 }
 
 void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu)
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -68,6 +68,7 @@ extern void __enable_irq(struct irq_desc
 
 extern int irq_startup(struct irq_desc *desc, bool resend);
 extern void irq_shutdown(struct irq_desc *desc);
+extern void irq_enable_or_startup(struct irq_desc *desc);
 extern void irq_enable(struct irq_desc *desc);
 extern void irq_disable(struct irq_desc *desc);
 extern void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu);
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -533,7 +533,7 @@ void __enable_irq(struct irq_desc *desc)
 			goto err_out;
 		/* Prevent probing on this irq: */
 		irq_settings_set_noprobe(desc);
-		irq_enable(desc);
+		irq_enable_or_startup(desc);
 		check_irq_resend(desc);
 		/* fall-through */
 	}

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

#1654259 — Re: [patch 1/2] genirq: Handle NOAUTOEN interrupt setup proper

Hi Thomas,

On 31/05/17 10:58, Thomas Gleixner wrote:
> If an interrupt is marked NOAUTOEN then request_irq() installs the action,
> but does not enable the interrupt via startup_irq().  The interrupt is
> enabled via enable_irq() later from the driver. enable_irq() calls
> irq_enable().
> 
> That means that for interrupts which have a irq_startup() callback this
> callback is never invoked. Neither is irq_domain_activate_irq() invoked for
> such interrupts.
> 
> If an interrupt depends on irq_startup() or irq_domain_activate_irq() then
> the enable via irq_enable() is not enough.
> 
> Add a status flag IRQD_IRQ_STARTED_UP and use this to select the proper
> mechanism in enable_irq(). Use the flag also to avoid pointless calls into
> the low level functions.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  include/linux/irq.h    |    6 ++++
>  kernel/irq/chip.c      |   73 +++++++++++++++++++++++++++++++++++++------------
>  kernel/irq/internals.h |    1 
>  kernel/irq/manage.c    |    2 -
>  4 files changed, 64 insertions(+), 18 deletions(-)
> 
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -216,6 +216,7 @@ enum {
>  	IRQD_WAKEUP_ARMED		= (1 << 19),
>  	IRQD_FORWARDED_TO_VCPU		= (1 << 20),
>  	IRQD_AFFINITY_MANAGED		= (1 << 21),
> +	IRQD_IRQ_STARTED_UP		= (1 << 22),
>  };
>  
>  #define __irqd_to_state(d) ACCESS_PRIVATE((d)->common, state_use_accessors)
> @@ -329,6 +330,11 @@ static inline void irqd_clr_activated(st
>  	__irqd_to_state(d) &= ~IRQD_ACTIVATED;
>  }
>  
> +static inline bool irqd_is_started_up(struct irq_data *d)

nit: since we have set/clr_started, consider making this is_started
(without the up)? Or add the _up to clr/set?

> +{
> +	return __irqd_to_state(d) & IRQD_IRQ_STARTED_UP;
> +}
> +
>  #undef __irqd_to_state
>  
>  static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -185,37 +185,71 @@ static void irq_state_set_masked(struct
>  	irqd_set(&desc->irq_data, IRQD_IRQ_MASKED);
>  }
>  
> +static void irq_state_clr_started(struct irq_desc *desc)
> +{
> +	irqd_clear(&desc->irq_data, IRQD_IRQ_STARTED_UP);
> +}
> +
> +static void irq_state_set_started(struct irq_desc *desc)
> +{
> +	irqd_set(&desc->irq_data, IRQD_IRQ_STARTED_UP);
> +}
> +
>  int irq_startup(struct irq_desc *desc, bool resend)
>  {
>  	int ret = 0;
>  
> -	irq_state_clr_disabled(desc);
> +	if (irqd_is_started_up(&desc->irq_data))
> +		return 0;
> +

How about:
	if (irqd_is_started_up(&desc->irq_data)) {
		irq_enable(desc);
		return 0;
	}

>  	desc->depth = 0;
>  
>  	irq_domain_activate_irq(&desc->irq_data);
>  	if (desc->irq_data.chip->irq_startup) {
>  		ret = desc->irq_data.chip->irq_startup(&desc->irq_data);
> +		irq_state_clr_disabled(desc);
>  		irq_state_clr_masked(desc);
>  	} else {
>  		irq_enable(desc);
>  	}
> +	irq_state_set_started(desc);
> +
>  	if (resend)
>  		check_irq_resend(desc);
> +
>  	return ret;
>  }
>  
> +static void __irq_disable(struct irq_desc *desc, bool mask);
> +
>  void irq_shutdown(struct irq_desc *desc)
>  {
> -	irq_state_set_disabled(desc);
> -	desc->depth = 1;
> -	if (desc->irq_data.chip->irq_shutdown)
> -		desc->irq_data.chip->irq_shutdown(&desc->irq_data);
> -	else if (desc->irq_data.chip->irq_disable)
> -		desc->irq_data.chip->irq_disable(&desc->irq_data);
> -	else
> -		desc->irq_data.chip->irq_mask(&desc->irq_data);
> +	if (irqd_is_started_up(&desc->irq_data)) {
> +		desc->depth = 1;
> +		if (desc->irq_data.chip->irq_shutdown) {
> +			desc->irq_data.chip->irq_shutdown(&desc->irq_data);
> +			irq_state_set_disabled(desc);
> +			irq_state_set_masked(desc);
> +		} else {
> +			__irq_disable(desc, true);
> +		}
> +		irq_state_clr_started(desc);
> +	}
> +	/*
> +	 * This must be called even if the interrupt was never started up,
> +	 * because the activation can happen before the interrupt is
> +	 * available for request/startup. It has it's own state tracking so
> +	 * it's safe to call it unconditonally.

                                unconditionally?

> +	 */
>  	irq_domain_deactivate_irq(&desc->irq_data);
> -	irq_state_set_masked(desc);
> +}
> +
> +void irq_enable_or_startup(struct irq_desc *desc)
> +{
> +	if (!irqd_is_started_up(&desc->irq_data))
> +		irq_startup(desc, false);
> +	else
> +		irq_enable(desc);

get rid of this new function...

>  }
>  
>  void irq_enable(struct irq_desc *desc)
> @@ -228,6 +262,17 @@ void irq_enable(struct irq_desc *desc)
>  	irq_state_clr_masked(desc);
>  }
>  
> +static void __irq_disable(struct irq_desc *desc, bool mask)
> +{
> +	irq_state_set_disabled(desc);
> +	if (desc->irq_data.chip->irq_disable) {
> +		desc->irq_data.chip->irq_disable(&desc->irq_data);
> +		irq_state_set_masked(desc);
> +	} else if (mask) {
> +		mask_irq(desc);
> +	}
> +}
> +
>  /**
>   * irq_disable - Mark interrupt disabled
>   * @desc:	irq descriptor which should be disabled
> @@ -250,13 +295,7 @@ void irq_enable(struct irq_desc *desc)
>   */
>  void irq_disable(struct irq_desc *desc)
>  {
> -	irq_state_set_disabled(desc);
> -	if (desc->irq_data.chip->irq_disable) {
> -		desc->irq_data.chip->irq_disable(&desc->irq_data);
> -		irq_state_set_masked(desc);
> -	} else if (irq_settings_disable_unlazy(desc)) {
> -		mask_irq(desc);
> -	}
> +	__irq_disable(desc, irq_settings_disable_unlazy(desc));
>  }
>  
>  void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu)
> --- a/kernel/irq/internals.h
> +++ b/kernel/irq/internals.h
> @@ -68,6 +68,7 @@ extern void __enable_irq(struct irq_desc
>  
>  extern int irq_startup(struct irq_desc *desc, bool resend);
>  extern void irq_shutdown(struct irq_desc *desc);
> +extern void irq_enable_or_startup(struct irq_desc *desc);
>  extern void irq_enable(struct irq_desc *desc);
>  extern void irq_disable(struct irq_desc *desc);
>  extern void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu);
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -533,7 +533,7 @@ void __enable_irq(struct irq_desc *desc)
>  			goto err_out;
>  		/* Prevent probing on this irq: */
>  		irq_settings_set_noprobe(desc);
> -		irq_enable(desc);
> +		irq_enable_or_startup(desc);

and make this irq_startup(desc, false)?

>  		check_irq_resend(desc);
>  		/* fall-through */
>  	}
> 
> 

Otherwise:

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

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

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

#1654344 — Re: [patch 1/2] genirq: Handle NOAUTOEN interrupt setup proper

On Wed, 31 May 2017, Marc Zyngier wrote:
> On 31/05/17 10:58, Thomas Gleixner wrote:
> >  #define __irqd_to_state(d) ACCESS_PRIVATE((d)->common, state_use_accessors)
> > @@ -329,6 +330,11 @@ static inline void irqd_clr_activated(st
> >  	__irqd_to_state(d) &= ~IRQD_ACTIVATED;
> >  }
> >  
> > +static inline bool irqd_is_started_up(struct irq_data *d)
> 
> nit: since we have set/clr_started, consider making this is_started
> (without the up)? Or add the _up to clr/set?

Sure.

> > +{
> > +	return __irqd_to_state(d) & IRQD_IRQ_STARTED_UP;
> > +}
> > +
> >  #undef __irqd_to_state
> >  
> >  static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
> > --- a/kernel/irq/chip.c
> > +++ b/kernel/irq/chip.c
> > @@ -185,37 +185,71 @@ static void irq_state_set_masked(struct
> >  	irqd_set(&desc->irq_data, IRQD_IRQ_MASKED);
> >  }
> >  
> > +static void irq_state_clr_started(struct irq_desc *desc)
> > +{
> > +	irqd_clear(&desc->irq_data, IRQD_IRQ_STARTED_UP);
> > +}
> > +
> > +static void irq_state_set_started(struct irq_desc *desc)
> > +{
> > +	irqd_set(&desc->irq_data, IRQD_IRQ_STARTED_UP);
> > +}
> > +
> >  int irq_startup(struct irq_desc *desc, bool resend)
> >  {
> >  	int ret = 0;
> >  
> > -	irq_state_clr_disabled(desc);
> > +	if (irqd_is_started_up(&desc->irq_data))
> > +		return 0;
> > +
> 
> How about:
> 	if (irqd_is_started_up(&desc->irq_data)) {
> 		irq_enable(desc);
> 		return 0;

Why would we do that? I rather have a WARN_ON() there as this should never
happen.

> > +	/*
> > +	 * This must be called even if the interrupt was never started up,
> > +	 * because the activation can happen before the interrupt is
> > +	 * available for request/startup. It has it's own state tracking so
> > +	 * it's safe to call it unconditonally.
> 
>                                 unconditionally?

Oops.

> >  extern int irq_startup(struct irq_desc *desc, bool resend);
> >  extern void irq_shutdown(struct irq_desc *desc);
> > +extern void irq_enable_or_startup(struct irq_desc *desc);
> >  extern void irq_enable(struct irq_desc *desc);
> >  extern void irq_disable(struct irq_desc *desc);
> >  extern void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu);
> > --- a/kernel/irq/manage.c
> > +++ b/kernel/irq/manage.c
> > @@ -533,7 +533,7 @@ void __enable_irq(struct irq_desc *desc)
> >  			goto err_out;
> >  		/* Prevent probing on this irq: */
> >  		irq_settings_set_noprobe(desc);
> > -		irq_enable(desc);
> > +		irq_enable_or_startup(desc);
> 
> and make this irq_startup(desc, false)?

Ah, now I see where you were heading with that irq_startup()
modification. Makes sense, but let me think about it.

Thanks,

	tglx

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

#1657064 — [tip:irq/core] genirq: Handle NOAUTOEN interrupt setup proper

Commit-ID:  201d7f47f34bd7cb19161d0426f13b141e381f30
Gitweb:     http://git.kernel.org/tip/201d7f47f34bd7cb19161d0426f13b141e381f30
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Wed, 31 May 2017 11:58:32 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sun, 4 Jun 2017 14:35:13 +0200

genirq: Handle NOAUTOEN interrupt setup proper

If an interrupt is marked NOAUTOEN then request_irq() installs the action,
but does not enable the interrupt via startup_irq().  The interrupt is
enabled via enable_irq() later from the driver. enable_irq() calls
irq_enable().

That means that for interrupts which have a irq_startup() callback this
callback is never invoked. Neither is irq_domain_activate_irq() invoked for
such interrupts.

If an interrupt depends on irq_startup() or irq_domain_activate_irq() then
the enable via irq_enable() is not enough.

Add a status flag IRQD_IRQ_STARTED_UP and use this to select the proper
mechanism in enable_irq(). Use the flag also to avoid pointless calls into
the low level functions.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Cc: dianders@chromium.org
Cc: jeffy <jeffy.chen@rock-chips.com>
Cc: Brian Norris <briannorris@chromium.org>
Cc: tfiga@chromium.org
Link: http://lkml.kernel.org/r/20170531100212.130986205@linutronix.de

---
 include/linux/irq.h |  6 +++++
 kernel/irq/chip.c   | 76 +++++++++++++++++++++++++++++++++++++----------------
 kernel/irq/manage.c | 12 ++++++---
 3 files changed, 69 insertions(+), 25 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index f887351..94d1ad6 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -216,6 +216,7 @@ enum {
 	IRQD_WAKEUP_ARMED		= (1 << 19),
 	IRQD_FORWARDED_TO_VCPU		= (1 << 20),
 	IRQD_AFFINITY_MANAGED		= (1 << 21),
+	IRQD_IRQ_STARTED		= (1 << 22),
 };
 
 #define __irqd_to_state(d) ACCESS_PRIVATE((d)->common, state_use_accessors)
@@ -329,6 +330,11 @@ static inline void irqd_clr_activated(struct irq_data *d)
 	__irqd_to_state(d) &= ~IRQD_ACTIVATED;
 }
 
+static inline bool irqd_is_started(struct irq_data *d)
+{
+	return __irqd_to_state(d) & IRQD_IRQ_STARTED;
+}
+
 #undef __irqd_to_state
 
 static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index c94da68..e0051d5 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -185,37 +185,64 @@ static void irq_state_set_masked(struct irq_desc *desc)
 	irqd_set(&desc->irq_data, IRQD_IRQ_MASKED);
 }
 
+static void irq_state_clr_started(struct irq_desc *desc)
+{
+	irqd_clear(&desc->irq_data, IRQD_IRQ_STARTED);
+}
+
+static void irq_state_set_started(struct irq_desc *desc)
+{
+	irqd_set(&desc->irq_data, IRQD_IRQ_STARTED);
+}
+
 int irq_startup(struct irq_desc *desc, bool resend)
 {
 	int ret = 0;
 
-	irq_state_clr_disabled(desc);
 	desc->depth = 0;
 
-	irq_domain_activate_irq(&desc->irq_data);
-	if (desc->irq_data.chip->irq_startup) {
-		ret = desc->irq_data.chip->irq_startup(&desc->irq_data);
-		irq_state_clr_masked(desc);
-	} else {
+	if (irqd_is_started(&desc->irq_data)) {
 		irq_enable(desc);
+	} else {
+		irq_domain_activate_irq(&desc->irq_data);
+		if (desc->irq_data.chip->irq_startup) {
+			ret = desc->irq_data.chip->irq_startup(&desc->irq_data);
+			irq_state_clr_disabled(desc);
+			irq_state_clr_masked(desc);
+		} else {
+			irq_enable(desc);
+		}
+		irq_state_set_started(desc);
 	}
+
 	if (resend)
 		check_irq_resend(desc);
+
 	return ret;
 }
 
+static void __irq_disable(struct irq_desc *desc, bool mask);
+
 void irq_shutdown(struct irq_desc *desc)
 {
-	irq_state_set_disabled(desc);
-	desc->depth = 1;
-	if (desc->irq_data.chip->irq_shutdown)
-		desc->irq_data.chip->irq_shutdown(&desc->irq_data);
-	else if (desc->irq_data.chip->irq_disable)
-		desc->irq_data.chip->irq_disable(&desc->irq_data);
-	else
-		desc->irq_data.chip->irq_mask(&desc->irq_data);
+	if (irqd_is_started(&desc->irq_data)) {
+		desc->depth = 1;
+		if (desc->irq_data.chip->irq_shutdown) {
+			desc->irq_data.chip->irq_shutdown(&desc->irq_data);
+			irq_state_set_disabled(desc);
+			irq_state_set_masked(desc);
+		} else {
+			__irq_disable(desc, true);
+		}
+		irq_state_clr_started(desc);
+	}
+	/*
+	 * This must be called even if the interrupt was never started up,
+	 * because the activation can happen before the interrupt is
+	 * available for request/startup. It has it's own state tracking so
+	 * it's safe to call it unconditionally.
+	 */
 	irq_domain_deactivate_irq(&desc->irq_data);
-	irq_state_set_masked(desc);
 }
 
 void irq_enable(struct irq_desc *desc)
@@ -228,6 +255,17 @@ void irq_enable(struct irq_desc *desc)
 	irq_state_clr_masked(desc);
 }
 
+static void __irq_disable(struct irq_desc *desc, bool mask)
+{
+	irq_state_set_disabled(desc);
+	if (desc->irq_data.chip->irq_disable) {
+		desc->irq_data.chip->irq_disable(&desc->irq_data);
+		irq_state_set_masked(desc);
+	} else if (mask) {
+		mask_irq(desc);
+	}
+}
+
 /**
  * irq_disable - Mark interrupt disabled
  * @desc:	irq descriptor which should be disabled
@@ -250,13 +288,7 @@ void irq_enable(struct irq_desc *desc)
  */
 void irq_disable(struct irq_desc *desc)
 {
-	irq_state_set_disabled(desc);
-	if (desc->irq_data.chip->irq_disable) {
-		desc->irq_data.chip->irq_disable(&desc->irq_data);
-		irq_state_set_masked(desc);
-	} else if (irq_settings_disable_unlazy(desc)) {
-		mask_irq(desc);
-	}
+	__irq_disable(desc, irq_settings_disable_unlazy(desc));
 }
 
 void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 070be98..5705610 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -533,9 +533,15 @@ void __enable_irq(struct irq_desc *desc)
 			goto err_out;
 		/* Prevent probing on this irq: */
 		irq_settings_set_noprobe(desc);
-		irq_enable(desc);
-		check_irq_resend(desc);
-		/* fall-through */
+		/*
+		 * Call irq_startup() not irq_enable() here because the
+		 * interrupt might be marked NOAUTOEN. So irq_startup()
+		 * needs to be invoked when it gets enabled the first
+		 * time. If it was already started up, then irq_startup()
+		 * will invoke irq_enable() under the hood.
+		 */
+		irq_startup(desc, true);
+		break;
 	}
 	default:
 		desc->depth--;

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

#1654091 — [patch 2/2] genirq: Warn when IRQ_NOAUTOEN is used with shared interrupts

Shared interrupts do not go well with disabling auto enable:

1) The sharing interrupt might request it while it's still disabled and
   then wait for interrupts forever.

2) The interrupt might have been requested by the driver sharing the line
   before IRQ_NOAUTOEN has been set. So the driver which expects that
   disabled state after calling request_irq() will not get what it wants.
   Even worse, when it calls enable_irq() later, it will trigger the
   unbalanced enable_irq() warning.

Reported-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/irq/chip.c   |    7 +++++++
 kernel/irq/manage.c |   12 ++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -942,6 +942,13 @@ void irq_modify_status(unsigned int irq,
 
 	if (!desc)
 		return;
+
+	/*
+	 * Warn when a driver sets the no autoenable flag on an already
+	 * active interrupt.
+	 */
+	WARN_ON_ONCE(!desc->depth && (set & IRQF_NOAUTOEN));
+
 	irq_settings_clr_and_set(desc, clr, set);
 
 	irqd_clear(&desc->irq_data, IRQD_NO_BALANCING | IRQD_PER_CPU |
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1328,11 +1328,19 @@ static int
 		if (new->flags & IRQF_ONESHOT)
 			desc->istate |= IRQS_ONESHOT;
 
-		if (irq_settings_can_autoenable(desc))
+		if (irq_settings_can_autoenable(desc)) {
 			irq_startup(desc, true);
-		else
+		} else {
+			/*
+			 * Shared interrupts do not go well with disabling
+			 * auto enable. The sharing interrupt might request
+			 * it while it's still disabled and then wait for
+			 * interrupts forever.
+			 */
+			WARN_ON_ONCE(new->flags & IRQF_SHARED);
 			/* Undo nested disables: */
 			desc->depth = 1;
+		}
 
 		/* Exclude IRQ from balancing if requested */
 		if (new->flags & IRQF_NOBALANCING) {

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

#1657065 — [tip:irq/core] genirq: Warn when IRQ_NOAUTOEN is used with shared interrupts

Commit-ID:  04c848d398797a626608ff48804d809ae6687163
Gitweb:     http://git.kernel.org/tip/04c848d398797a626608ff48804d809ae6687163
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Wed, 31 May 2017 11:58:33 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sun, 4 Jun 2017 14:38:41 +0200

genirq: Warn when IRQ_NOAUTOEN is used with shared interrupts

Shared interrupts do not go well with disabling auto enable:

1) The sharing interrupt might request it while it's still disabled and
   then wait for interrupts forever.

2) The interrupt might have been requested by the driver sharing the line
   before IRQ_NOAUTOEN has been set. So the driver which expects that
   disabled state after calling request_irq() will not get what it wants.
   Even worse, when it calls enable_irq() later, it will trigger the
   unbalanced enable_irq() warning.

Reported-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: dianders@chromium.org
Cc: jeffy <jeffy.chen@rock-chips.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: tfiga@chromium.org
Link: http://lkml.kernel.org/r/20170531100212.210682135@linutronix.de
---
 kernel/irq/chip.c   |  7 +++++++
 kernel/irq/manage.c | 12 ++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index e0051d5..bc1331f 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -935,6 +935,13 @@ void irq_modify_status(unsigned int irq, unsigned long clr, unsigned long set)
 
 	if (!desc)
 		return;
+
+	/*
+	 * Warn when a driver sets the no autoenable flag on an already
+	 * active interrupt.
+	 */
+	WARN_ON_ONCE(!desc->depth && (set & _IRQ_NOAUTOEN));
+
 	irq_settings_clr_and_set(desc, clr, set);
 
 	irqd_clear(&desc->irq_data, IRQD_NO_BALANCING | IRQD_PER_CPU |
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 5705610..49c37f1 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1334,11 +1334,19 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		if (new->flags & IRQF_ONESHOT)
 			desc->istate |= IRQS_ONESHOT;
 
-		if (irq_settings_can_autoenable(desc))
+		if (irq_settings_can_autoenable(desc)) {
 			irq_startup(desc, true);
-		else
+		} else {
+			/*
+			 * Shared interrupts do not go well with disabling
+			 * auto enable. The sharing interrupt might request
+			 * it while it's still disabled and then wait for
+			 * interrupts forever.
+			 */
+			WARN_ON_ONCE(new->flags & IRQF_SHARED);
 			/* Undo nested disables: */
 			desc->depth = 1;
+		}
 
 		/* Exclude IRQ from balancing if requested */
 		if (new->flags & IRQF_NOBALANCING) {

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web