[PATCH v2 0/2] genirq: Make irqchip flags work with stacked irq domains

[PATCH v2 0/2] genirq: Make irqchip flags work with stacked irq domains

[Marc Zyngier]

With the landing of stacked irq domains in 3.19, we have ended up in a
situation where we have a stack of IRQ controllers, each with their
set of flags, but the core code is only able to look at the top-most,
which is not very helpful. This small series is trying to fix this.

The first patch converts all access to desc->irq_data.chip->flags to
using an accessor, without changing anything else. The second patch
adds some logic to combine these flags as we allocate the interrupts,
ultimately storing the resulting set as part of the irq_desc
structure.

We end-up with a configuration where the flags can either be located
in the irq_chip structure (non stacked case), or in the irq_desc
(stacked case). While this isn't really ideal, this gives at least the
right level of information to the rest of the IRQ framework, and is
only computed at allocation time, instead of parsing the irqchip stack
at runtime.

Based on 3.19-rc3, and available at:
git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git irq/stacked-irqchip-flags

>From v1 [1]:
- fix one last occurence of desc->irq_data.chip->flags, spotted by Jiang

[1]: https://lkml.org/lkml/2015/1/8/387

Marc Zyngier (2):
  genirq: Abstract access to irq_chip flags
  genirq: Allow irq_desc to carry the union of stacked irq_chip flags

 include/linux/irq.h     |  4 ++++
 include/linux/irqdesc.h | 12 ++++++++++++
 kernel/irq/chip.c       | 10 +++++-----
 kernel/irq/irqdesc.c    |  3 +++
 kernel/irq/irqdomain.c  | 20 +++++++++++++++++++-
 kernel/irq/manage.c     |  8 ++++----
 kernel/irq/pm.c         |  2 +-
 7 files changed, 48 insertions(+), 11 deletions(-)

-- 
2.1.4

[PATCH v2 1/2] genirq: Abstract access to irq_chip flags

[Marc Zyngier]

In order to safely migrate to a cumulative set of flags, start by
abstracting the way we look at these flags. There is otherwise no
change in semantics here.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/linux/irqdesc.h |  5 +++++
 kernel/irq/chip.c       | 10 +++++-----
 kernel/irq/manage.c     |  8 ++++----
 kernel/irq/pm.c         |  2 +-
 4 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index faf433a..32d9fff 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -107,6 +107,11 @@ static inline void *irq_desc_get_chip_data(struct irq_desc *desc)
 	return desc->irq_data.chip_data;
 }
 
+static inline unsigned long irq_desc_get_chip_flags(struct irq_desc *desc)
+{
+	return desc->irq_data.chip->flags;
+}
+
 static inline void *irq_desc_get_handler_data(struct irq_desc *desc)
 {
 	return desc->irq_data.handler_data;
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 6f1c7a5..d8ffb36 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -287,7 +287,7 @@ void unmask_threaded_irq(struct irq_desc *desc)
 {
 	struct irq_chip *chip = desc->irq_data.chip;
 
-	if (chip->flags & IRQCHIP_EOI_THREADED)
+	if (irq_desc_get_chip_flags(desc) & IRQCHIP_EOI_THREADED)
 		chip->irq_eoi(&desc->irq_data);
 
 	if (chip->irq_unmask) {
@@ -489,7 +489,7 @@ static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
 	    irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) {
 		chip->irq_eoi(&desc->irq_data);
 		unmask_irq(desc);
-	} else if (!(chip->flags & IRQCHIP_EOI_THREADED)) {
+	} else if (!(irq_desc_get_chip_flags(desc) & IRQCHIP_EOI_THREADED)) {
 		chip->irq_eoi(&desc->irq_data);
 	}
 }
@@ -538,7 +538,7 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
 	raw_spin_unlock(&desc->lock);
 	return;
 out:
-	if (!(chip->flags & IRQCHIP_EOI_IF_HANDLED))
+	if (!(irq_desc_get_chip_flags(desc) & IRQCHIP_EOI_IF_HANDLED))
 		chip->irq_eoi(&desc->irq_data);
 	raw_spin_unlock(&desc->lock);
 }
@@ -836,7 +836,7 @@ void irq_cpu_online(void)
 
 		chip = irq_data_get_irq_chip(&desc->irq_data);
 		if (chip && chip->irq_cpu_online &&
-		    (!(chip->flags & IRQCHIP_ONOFFLINE_ENABLED) ||
+		    (!(irq_desc_get_chip_flags(desc) & IRQCHIP_ONOFFLINE_ENABLED) ||
 		     !irqd_irq_disabled(&desc->irq_data)))
 			chip->irq_cpu_online(&desc->irq_data);
 
@@ -866,7 +866,7 @@ void irq_cpu_offline(void)
 
 		chip = irq_data_get_irq_chip(&desc->irq_data);
 		if (chip && chip->irq_cpu_offline &&
-		    (!(chip->flags & IRQCHIP_ONOFFLINE_ENABLED) ||
+		    (!(irq_desc_get_chip_flags(desc) & IRQCHIP_ONOFFLINE_ENABLED) ||
 		     !irqd_irq_disabled(&desc->irq_data)))
 			chip->irq_cpu_offline(&desc->irq_data);
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 8069237..be7ce78 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -491,7 +491,7 @@ static int set_irq_wake_real(unsigned int irq, unsigned int on)
 	struct irq_desc *desc = irq_to_desc(irq);
 	int ret = -ENXIO;
 
-	if (irq_desc_get_chip(desc)->flags &  IRQCHIP_SKIP_SET_WAKE)
+	if (irq_desc_get_chip_flags(desc) &  IRQCHIP_SKIP_SET_WAKE)
 		return 0;
 
 	if (desc->irq_data.chip->irq_set_wake)
@@ -589,7 +589,7 @@ int __irq_set_trigger(struct irq_desc *desc, unsigned int irq,
 
 	flags &= IRQ_TYPE_SENSE_MASK;
 
-	if (chip->flags & IRQCHIP_SET_TYPE_MASKED) {
+	if (irq_desc_get_chip_flags(desc) & IRQCHIP_SET_TYPE_MASKED) {
 		if (!irqd_irq_masked(&desc->irq_data))
 			mask_irq(desc);
 		if (!irqd_irq_disabled(&desc->irq_data))
@@ -1043,7 +1043,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	 * chip flags, so we can avoid the unmask dance at the end of
 	 * the threaded handler for those.
 	 */
-	if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)
+	if (irq_desc_get_chip_flags(desc) & IRQCHIP_ONESHOT_SAFE)
 		new->flags &= ~IRQF_ONESHOT;
 
 	/*
@@ -1121,7 +1121,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		new->thread_mask = 1 << ffz(thread_mask);
 
 	} else if (new->handler == irq_default_primary_handler &&
-		   !(desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)) {
+		   !(irq_desc_get_chip_flags(desc) & IRQCHIP_ONESHOT_SAFE)) {
 		/*
 		 * The interrupt was requested with handler = NULL, so
 		 * we use the default primary handler for it. But it
diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index 3ca5325..8ed029d 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -88,7 +88,7 @@ static bool suspend_device_irq(struct irq_desc *desc, int irq)
 	 * chip level. The chip implementation indicates that with
 	 * IRQCHIP_MASK_ON_SUSPEND.
 	 */
-	if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND)
+	if (irq_desc_get_chip_flags(desc) & IRQCHIP_MASK_ON_SUSPEND)
 		mask_irq(desc);
 	return true;
 }
-- 
2.1.4

[PATCH v2 2/2] genirq: Allow irq_desc to carry the union of stacked irq_chip flags

[Marc Zyngier]

The current infrastructure for stacked domains doesn't propagate
irq_chip flags, and as we only look at the top-level irq_chip,
we may miss a number of critical flags.

This patch accumulates the flags into a new set, stored at the
irq_desc level, with an additional flag to indicate that this is
a stack of irqchip. The accessor is updated to return the right one.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/linux/irq.h     |  4 ++++
 include/linux/irqdesc.h |  7 +++++++
 kernel/irq/irqdesc.c    |  3 +++
 kernel/irq/irqdomain.c  | 20 +++++++++++++++++++-
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index d09ec7a..cb956f6 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -377,6 +377,7 @@ struct irq_chip {
  * IRQCHIP_SKIP_SET_WAKE:	Skip chip.irq_set_wake(), for this irq chip
  * IRQCHIP_ONESHOT_SAFE:	One shot does not require mask/unmask
  * IRQCHIP_EOI_THREADED:	Chip requires eoi() on unmask in threaded mode
+ * IRQCHIP_STACKED_CHIPS:	Pseudo flag for stacked irq chips
  */
 enum {
 	IRQCHIP_SET_TYPE_MASKED		= (1 <<  0),
@@ -386,6 +387,9 @@ enum {
 	IRQCHIP_SKIP_SET_WAKE		= (1 <<  4),
 	IRQCHIP_ONESHOT_SAFE		= (1 <<  5),
 	IRQCHIP_EOI_THREADED		= (1 <<  6),
+
+	/* The following is only valid as part of irq_desc.chip_flags */
+	IRQCHIP_STACKED_CHIPS		= (1 << 31),
 };
 
 /* This include will go away once we isolated irq_desc usage to core code */
diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index 32d9fff..f35f192 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -50,6 +50,9 @@ struct irq_desc {
 	struct irq_data		irq_data;
 	unsigned int __percpu	*kstat_irqs;
 	irq_flow_handler_t	handle_irq;
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+	unsigned long		chip_flags;	/* Union of all the irqchips */
+#endif
 #ifdef CONFIG_IRQ_PREFLOW_FASTEOI
 	irq_preflow_handler_t	preflow_handler;
 #endif
@@ -109,6 +112,10 @@ static inline void *irq_desc_get_chip_data(struct irq_desc *desc)
 
 static inline unsigned long irq_desc_get_chip_flags(struct irq_desc *desc)
 {
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+	if (desc->chip_flags & IRQCHIP_STACKED_CHIPS)
+		return desc->chip_flags;
+#endif
 	return desc->irq_data.chip->flags;
 }
 
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 99793b9..b916ea4 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -84,6 +84,9 @@ static void desc_set_defaults(unsigned int irq, struct irq_desc *desc, int node,
 	irq_settings_clr_and_set(desc, ~0, _IRQ_DEFAULT_INIT_FLAGS);
 	irqd_set(&desc->irq_data, IRQD_IRQ_DISABLED);
 	desc->handle_irq = handle_bad_irq;
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+	desc->chip_flags = 0;
+#endif
 	desc->depth = 1;
 	desc->irq_count = 0;
 	desc->irqs_unhandled = 0;
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 7fac311..3d0e366 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1013,6 +1013,17 @@ static void irq_domain_free_irqs_recursive(struct irq_domain *domain,
 	}
 }
 
+static void irq_desc_update_chip_flags(struct irq_domain *domain,
+				       unsigned int virq)
+{
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+	struct irq_desc *desc = irq_to_desc(virq);
+	struct irq_data *data = irq_domain_get_irq_data(domain, virq);
+
+	desc->chip_flags |= data->chip->flags | IRQCHIP_STACKED_CHIPS;
+#endif
+}
+
 static int irq_domain_alloc_irqs_recursive(struct irq_domain *domain,
 					   unsigned int irq_base,
 					   unsigned int nr_irqs, void *arg)
@@ -1025,8 +1036,15 @@ static int irq_domain_alloc_irqs_recursive(struct irq_domain *domain,
 	if (recursive)
 		ret = irq_domain_alloc_irqs_recursive(parent, irq_base,
 						      nr_irqs, arg);
-	if (ret >= 0)
+	if (ret >= 0) {
 		ret = domain->ops->alloc(domain, irq_base, nr_irqs, arg);
+		if (ret >= 0) {
+			int i;
+
+			for (i = 0; i < nr_irqs; i++)
+				irq_desc_update_chip_flags(domain, irq_base + i);
+		}
+	}
 	if (ret < 0 && recursive)
 		irq_domain_free_irqs_recursive(parent, irq_base, nr_irqs);
 
-- 
2.1.4

Re: [PATCH v2 2/2] genirq: Allow irq_desc to carry the union of stacked irq_chip flags

[Thomas Gleixner]

On Wed, 14 Jan 2015, Marc Zyngier wrote:

> The current infrastructure for stacked domains doesn't propagate
> irq_chip flags, and as we only look at the top-level irq_chip,
> we may miss a number of critical flags.
> 
> This patch accumulates the flags into a new set, stored at the
> irq_desc level, with an additional flag to indicate that this is
> a stack of irqchip. The accessor is updated to return the right one.

>  static inline unsigned long irq_desc_get_chip_flags(struct irq_desc *desc)
>  {
> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> +	if (desc->chip_flags & IRQCHIP_STACKED_CHIPS)
> +		return desc->chip_flags;
> +#endif
>  	return desc->irq_data.chip->flags;

We can avoid the extra conditional if we just make the accumulated
flags unconditional and collect them even for the !hierarchy case.

Also this patch is missing that chips can be swapped at runtime either
via the normal interfaces or via __irq_set_chip_handler_name_locked().

This needs to be addressed otherwise we might end up looking at the
wrong flags.

Thanks,

	tglx

Re: [PATCH v2 2/2] genirq: Allow irq_desc to carry the union of stacked irq_chip flags

[Jiang Liu]

On 2015/1/23 23:55, Thomas Gleixner wrote:
> On Wed, 14 Jan 2015, Marc Zyngier wrote:
> 
>> The current infrastructure for stacked domains doesn't propagate
>> irq_chip flags, and as we only look at the top-level irq_chip,
>> we may miss a number of critical flags.
>>
>> This patch accumulates the flags into a new set, stored at the
>> irq_desc level, with an additional flag to indicate that this is
>> a stack of irqchip. The accessor is updated to return the right one.
> 
>>  static inline unsigned long irq_desc_get_chip_flags(struct irq_desc *desc)
>>  {
>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>> +	if (desc->chip_flags & IRQCHIP_STACKED_CHIPS)
>> +		return desc->chip_flags;
>> +#endif
>>  	return desc->irq_data.chip->flags;
> 
> We can avoid the extra conditional if we just make the accumulated
> flags unconditional and collect them even for the !hierarchy case.
> 
> Also this patch is missing that chips can be swapped at runtime either
> via the normal interfaces or via __irq_set_chip_handler_name_locked().
> 
> This needs to be addressed otherwise we might end up looking at the
> wrong flags.
I'm splitting irq_data into irq_common_data and irq_data, seems
we could host flags by using irq_common_data instead of irq_desc.

> 
> Thanks,
> 
> 	tglx
>