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

Groups > linux.kernel > #1560750 > unrolled thread

[PATCH] irqdomain: Avoid activating interrupts more than once

Back to article view | Back to linux.kernel

Contents

  [PATCH] irqdomain: Avoid activating interrupts more than once Marc Zyngier <marc.zyngier@arm.com> - 2017-01-17 17:10 +0100
    Re: [PATCH] irqdomain: Avoid activating interrupts more than once Marc Zyngier <marc.zyngier@arm.com> - 2017-01-25 16:00 +0100
      Re: [PATCH] irqdomain: Avoid activating interrupts more than once Thomas Gleixner <tglx@linutronix.de> - 2017-01-25 19:10 +0100
    [tip:irq/urgent] irqdomain: Avoid activating interrupts more than  once tip-bot for Marc Zyngier <tipbot@zytor.com> - 2017-01-30 15:50 +0100

#1560750 — [PATCH] irqdomain: Avoid activating interrupts more than once

Since commit f3b0946d629c ("genirq/msi: Make sure PCI MSIs are
activated early"), we can end-up activating a PCI/MSI twice (once
at allocation time, and once at startup time).

This is normally of no consequences, except that there is some
HW out there that may misbehave if activate is used more than once
(the GICv3 ITS, for example, uses the activate callback
to issue the MAPVI command, and the architecture spec says that
"If there is an existing mapping for the EventID-DeviceID
combination, behavior is UNPREDICTABLE").

While this could be worked around in each individual driver, it may
make more sense to tackle the issue at the core level. In order to
avoid getting in that situation, let's have a per-interrupt flag
to remember if we have already activated that interrupt or not.

Fixes: f3b0946d629c ("genirq/msi: Make sure PCI MSIs are activated early")
Reported-by: Andre Przywara <andre.przywara@arm.com>
Tested-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/linux/irq.h    | 17 +++++++++++++++++
 kernel/irq/irqdomain.c | 44 ++++++++++++++++++++++++++++++--------------
 2 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index e798755..39e3254 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -184,6 +184,7 @@ struct irq_data {
  *
  * IRQD_TRIGGER_MASK		- Mask for the trigger type bits
  * IRQD_SETAFFINITY_PENDING	- Affinity setting is pending
+ * IRQD_ACTIVATED		- Interrupt has already been activated
  * IRQD_NO_BALANCING		- Balancing disabled for this IRQ
  * IRQD_PER_CPU			- Interrupt is per cpu
  * IRQD_AFFINITY_SET		- Interrupt affinity was set
@@ -202,6 +203,7 @@ struct irq_data {
 enum {
 	IRQD_TRIGGER_MASK		= 0xf,
 	IRQD_SETAFFINITY_PENDING	= (1 <<  8),
+	IRQD_ACTIVATED			= (1 <<  9),
 	IRQD_NO_BALANCING		= (1 << 10),
 	IRQD_PER_CPU			= (1 << 11),
 	IRQD_AFFINITY_SET		= (1 << 12),
@@ -312,6 +314,21 @@ static inline bool irqd_affinity_is_managed(struct irq_data *d)
 	return __irqd_to_state(d) & IRQD_AFFINITY_MANAGED;
 }
 
+static inline bool irqd_is_activated(struct irq_data *d)
+{
+	return __irqd_to_state(d) & IRQD_ACTIVATED;
+}
+
+static inline void irqd_set_activated(struct irq_data *d)
+{
+	__irqd_to_state(d) |= IRQD_ACTIVATED;
+}
+
+static inline void irqd_clr_activated(struct irq_data *d)
+{
+	__irqd_to_state(d) &= ~IRQD_ACTIVATED;
+}
+
 #undef __irqd_to_state
 
 static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 8c0a0ae..b59e676 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1346,6 +1346,30 @@ void irq_domain_free_irqs_parent(struct irq_domain *domain,
 }
 EXPORT_SYMBOL_GPL(irq_domain_free_irqs_parent);
 
+static void __irq_domain_activate_irq(struct irq_data *irq_data)
+{
+	if (irq_data && irq_data->domain) {
+		struct irq_domain *domain = irq_data->domain;
+
+		if (irq_data->parent_data)
+			__irq_domain_activate_irq(irq_data->parent_data);
+		if (domain->ops->activate)
+			domain->ops->activate(domain, irq_data);
+	}
+}
+
+static void __irq_domain_deactivate_irq(struct irq_data *irq_data)
+{
+	if (irq_data && irq_data->domain) {
+		struct irq_domain *domain = irq_data->domain;
+
+		if (domain->ops->deactivate)
+			domain->ops->deactivate(domain, irq_data);
+		if (irq_data->parent_data)
+			__irq_domain_deactivate_irq(irq_data->parent_data);
+	}
+}
+
 /**
  * irq_domain_activate_irq - Call domain_ops->activate recursively to activate
  *			     interrupt
@@ -1356,13 +1380,9 @@ EXPORT_SYMBOL_GPL(irq_domain_free_irqs_parent);
  */
 void irq_domain_activate_irq(struct irq_data *irq_data)
 {
-	if (irq_data && irq_data->domain) {
-		struct irq_domain *domain = irq_data->domain;
-
-		if (irq_data->parent_data)
-			irq_domain_activate_irq(irq_data->parent_data);
-		if (domain->ops->activate)
-			domain->ops->activate(domain, irq_data);
+	if (!irqd_is_activated(irq_data)) {
+		__irq_domain_activate_irq(irq_data);
+		irqd_set_activated(irq_data);
 	}
 }
 
@@ -1376,13 +1396,9 @@ void irq_domain_activate_irq(struct irq_data *irq_data)
  */
 void irq_domain_deactivate_irq(struct irq_data *irq_data)
 {
-	if (irq_data && irq_data->domain) {
-		struct irq_domain *domain = irq_data->domain;
-
-		if (domain->ops->deactivate)
-			domain->ops->deactivate(domain, irq_data);
-		if (irq_data->parent_data)
-			irq_domain_deactivate_irq(irq_data->parent_data);
+	if (irqd_is_activated(irq_data)) {
+		__irq_domain_deactivate_irq(irq_data);
+		irqd_clr_activated(irq_data);
 	}
 }
 
-- 
2.1.4

[toc] | [next] | [standalone]

#1566658

Hi Thomas,

Any comment on that one?

Thanks,

	M.

On 17/01/17 16:00, Marc Zyngier wrote:
> Since commit f3b0946d629c ("genirq/msi: Make sure PCI MSIs are
> activated early"), we can end-up activating a PCI/MSI twice (once
> at allocation time, and once at startup time).
> 
> This is normally of no consequences, except that there is some
> HW out there that may misbehave if activate is used more than once
> (the GICv3 ITS, for example, uses the activate callback
> to issue the MAPVI command, and the architecture spec says that
> "If there is an existing mapping for the EventID-DeviceID
> combination, behavior is UNPREDICTABLE").
> 
> While this could be worked around in each individual driver, it may
> make more sense to tackle the issue at the core level. In order to
> avoid getting in that situation, let's have a per-interrupt flag
> to remember if we have already activated that interrupt or not.
> 
> Fixes: f3b0946d629c ("genirq/msi: Make sure PCI MSIs are activated early")
> Reported-by: Andre Przywara <andre.przywara@arm.com>
> Tested-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  include/linux/irq.h    | 17 +++++++++++++++++
>  kernel/irq/irqdomain.c | 44 ++++++++++++++++++++++++++++++--------------
>  2 files changed, 47 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index e798755..39e3254 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -184,6 +184,7 @@ struct irq_data {
>   *
>   * IRQD_TRIGGER_MASK		- Mask for the trigger type bits
>   * IRQD_SETAFFINITY_PENDING	- Affinity setting is pending
> + * IRQD_ACTIVATED		- Interrupt has already been activated
>   * IRQD_NO_BALANCING		- Balancing disabled for this IRQ
>   * IRQD_PER_CPU			- Interrupt is per cpu
>   * IRQD_AFFINITY_SET		- Interrupt affinity was set
> @@ -202,6 +203,7 @@ struct irq_data {
>  enum {
>  	IRQD_TRIGGER_MASK		= 0xf,
>  	IRQD_SETAFFINITY_PENDING	= (1 <<  8),
> +	IRQD_ACTIVATED			= (1 <<  9),
>  	IRQD_NO_BALANCING		= (1 << 10),
>  	IRQD_PER_CPU			= (1 << 11),
>  	IRQD_AFFINITY_SET		= (1 << 12),
> @@ -312,6 +314,21 @@ static inline bool irqd_affinity_is_managed(struct irq_data *d)
>  	return __irqd_to_state(d) & IRQD_AFFINITY_MANAGED;
>  }
>  
> +static inline bool irqd_is_activated(struct irq_data *d)
> +{
> +	return __irqd_to_state(d) & IRQD_ACTIVATED;
> +}
> +
> +static inline void irqd_set_activated(struct irq_data *d)
> +{
> +	__irqd_to_state(d) |= IRQD_ACTIVATED;
> +}
> +
> +static inline void irqd_clr_activated(struct irq_data *d)
> +{
> +	__irqd_to_state(d) &= ~IRQD_ACTIVATED;
> +}
> +
>  #undef __irqd_to_state
>  
>  static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 8c0a0ae..b59e676 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -1346,6 +1346,30 @@ void irq_domain_free_irqs_parent(struct irq_domain *domain,
>  }
>  EXPORT_SYMBOL_GPL(irq_domain_free_irqs_parent);
>  
> +static void __irq_domain_activate_irq(struct irq_data *irq_data)
> +{
> +	if (irq_data && irq_data->domain) {
> +		struct irq_domain *domain = irq_data->domain;
> +
> +		if (irq_data->parent_data)
> +			__irq_domain_activate_irq(irq_data->parent_data);
> +		if (domain->ops->activate)
> +			domain->ops->activate(domain, irq_data);
> +	}
> +}
> +
> +static void __irq_domain_deactivate_irq(struct irq_data *irq_data)
> +{
> +	if (irq_data && irq_data->domain) {
> +		struct irq_domain *domain = irq_data->domain;
> +
> +		if (domain->ops->deactivate)
> +			domain->ops->deactivate(domain, irq_data);
> +		if (irq_data->parent_data)
> +			__irq_domain_deactivate_irq(irq_data->parent_data);
> +	}
> +}
> +
>  /**
>   * irq_domain_activate_irq - Call domain_ops->activate recursively to activate
>   *			     interrupt
> @@ -1356,13 +1380,9 @@ EXPORT_SYMBOL_GPL(irq_domain_free_irqs_parent);
>   */
>  void irq_domain_activate_irq(struct irq_data *irq_data)
>  {
> -	if (irq_data && irq_data->domain) {
> -		struct irq_domain *domain = irq_data->domain;
> -
> -		if (irq_data->parent_data)
> -			irq_domain_activate_irq(irq_data->parent_data);
> -		if (domain->ops->activate)
> -			domain->ops->activate(domain, irq_data);
> +	if (!irqd_is_activated(irq_data)) {
> +		__irq_domain_activate_irq(irq_data);
> +		irqd_set_activated(irq_data);
>  	}
>  }
>  
> @@ -1376,13 +1396,9 @@ void irq_domain_activate_irq(struct irq_data *irq_data)
>   */
>  void irq_domain_deactivate_irq(struct irq_data *irq_data)
>  {
> -	if (irq_data && irq_data->domain) {
> -		struct irq_domain *domain = irq_data->domain;
> -
> -		if (domain->ops->deactivate)
> -			domain->ops->deactivate(domain, irq_data);
> -		if (irq_data->parent_data)
> -			irq_domain_deactivate_irq(irq_data->parent_data);
> +	if (irqd_is_activated(irq_data)) {
> +		__irq_domain_deactivate_irq(irq_data);
> +		irqd_clr_activated(irq_data);
>  	}
>  }
>  
> 


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

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

#1566804

On Wed, 25 Jan 2017, Marc Zyngier wrote:
> Hi Thomas,
> 
> Any comment on that one?

Yes, it got lost in my backlog. Thanks for the reminder.

     tglx

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

#1569795 — [tip:irq/urgent] irqdomain: Avoid activating interrupts more than once

Commit-ID:  08d85f3ea99f1eeafc4e8507936190e86a16ee8c
Gitweb:     http://git.kernel.org/tip/08d85f3ea99f1eeafc4e8507936190e86a16ee8c
Author:     Marc Zyngier <marc.zyngier@arm.com>
AuthorDate: Tue, 17 Jan 2017 16:00:48 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 30 Jan 2017 15:18:56 +0100

irqdomain: Avoid activating interrupts more than once

Since commit f3b0946d629c ("genirq/msi: Make sure PCI MSIs are
activated early"), we can end-up activating a PCI/MSI twice (once
at allocation time, and once at startup time).

This is normally of no consequences, except that there is some
HW out there that may misbehave if activate is used more than once
(the GICv3 ITS, for example, uses the activate callback
to issue the MAPVI command, and the architecture spec says that
"If there is an existing mapping for the EventID-DeviceID
combination, behavior is UNPREDICTABLE").

While this could be worked around in each individual driver, it may
make more sense to tackle the issue at the core level. In order to
avoid getting in that situation, let's have a per-interrupt flag
to remember if we have already activated that interrupt or not.

Fixes: f3b0946d629c ("genirq/msi: Make sure PCI MSIs are activated early")
Reported-and-tested-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Cc: stable@vger.kernel.org
Link: http://lkml.kernel.org/r/1484668848-24361-1-git-send-email-marc.zyngier@arm.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 include/linux/irq.h    | 17 +++++++++++++++++
 kernel/irq/irqdomain.c | 44 ++++++++++++++++++++++++++++++--------------
 2 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index e798755..39e3254 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -184,6 +184,7 @@ struct irq_data {
  *
  * IRQD_TRIGGER_MASK		- Mask for the trigger type bits
  * IRQD_SETAFFINITY_PENDING	- Affinity setting is pending
+ * IRQD_ACTIVATED		- Interrupt has already been activated
  * IRQD_NO_BALANCING		- Balancing disabled for this IRQ
  * IRQD_PER_CPU			- Interrupt is per cpu
  * IRQD_AFFINITY_SET		- Interrupt affinity was set
@@ -202,6 +203,7 @@ struct irq_data {
 enum {
 	IRQD_TRIGGER_MASK		= 0xf,
 	IRQD_SETAFFINITY_PENDING	= (1 <<  8),
+	IRQD_ACTIVATED			= (1 <<  9),
 	IRQD_NO_BALANCING		= (1 << 10),
 	IRQD_PER_CPU			= (1 << 11),
 	IRQD_AFFINITY_SET		= (1 << 12),
@@ -312,6 +314,21 @@ static inline bool irqd_affinity_is_managed(struct irq_data *d)
 	return __irqd_to_state(d) & IRQD_AFFINITY_MANAGED;
 }
 
+static inline bool irqd_is_activated(struct irq_data *d)
+{
+	return __irqd_to_state(d) & IRQD_ACTIVATED;
+}
+
+static inline void irqd_set_activated(struct irq_data *d)
+{
+	__irqd_to_state(d) |= IRQD_ACTIVATED;
+}
+
+static inline void irqd_clr_activated(struct irq_data *d)
+{
+	__irqd_to_state(d) &= ~IRQD_ACTIVATED;
+}
+
 #undef __irqd_to_state
 
 static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 8c0a0ae..b59e676 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1346,6 +1346,30 @@ void irq_domain_free_irqs_parent(struct irq_domain *domain,
 }
 EXPORT_SYMBOL_GPL(irq_domain_free_irqs_parent);
 
+static void __irq_domain_activate_irq(struct irq_data *irq_data)
+{
+	if (irq_data && irq_data->domain) {
+		struct irq_domain *domain = irq_data->domain;
+
+		if (irq_data->parent_data)
+			__irq_domain_activate_irq(irq_data->parent_data);
+		if (domain->ops->activate)
+			domain->ops->activate(domain, irq_data);
+	}
+}
+
+static void __irq_domain_deactivate_irq(struct irq_data *irq_data)
+{
+	if (irq_data && irq_data->domain) {
+		struct irq_domain *domain = irq_data->domain;
+
+		if (domain->ops->deactivate)
+			domain->ops->deactivate(domain, irq_data);
+		if (irq_data->parent_data)
+			__irq_domain_deactivate_irq(irq_data->parent_data);
+	}
+}
+
 /**
  * irq_domain_activate_irq - Call domain_ops->activate recursively to activate
  *			     interrupt
@@ -1356,13 +1380,9 @@ EXPORT_SYMBOL_GPL(irq_domain_free_irqs_parent);
  */
 void irq_domain_activate_irq(struct irq_data *irq_data)
 {
-	if (irq_data && irq_data->domain) {
-		struct irq_domain *domain = irq_data->domain;
-
-		if (irq_data->parent_data)
-			irq_domain_activate_irq(irq_data->parent_data);
-		if (domain->ops->activate)
-			domain->ops->activate(domain, irq_data);
+	if (!irqd_is_activated(irq_data)) {
+		__irq_domain_activate_irq(irq_data);
+		irqd_set_activated(irq_data);
 	}
 }
 
@@ -1376,13 +1396,9 @@ void irq_domain_activate_irq(struct irq_data *irq_data)
  */
 void irq_domain_deactivate_irq(struct irq_data *irq_data)
 {
-	if (irq_data && irq_data->domain) {
-		struct irq_domain *domain = irq_data->domain;
-
-		if (domain->ops->deactivate)
-			domain->ops->deactivate(domain, irq_data);
-		if (irq_data->parent_data)
-			irq_domain_deactivate_irq(irq_data->parent_data);
+	if (irqd_is_activated(irq_data)) {
+		__irq_domain_deactivate_irq(irq_data);
+		irqd_clr_activated(irq_data);
 	}
 }
 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web