plat-orion gpio regression for mixed level and edge sensitive IRQs

Orion maintainers,

I've found a problem on plat-orion when requesting IRQs on GPIO lines in the linux-3.0rc. This was introduced with the change to generic IRQ chip.

Processors in this family have separate mask registers for level and edge sensitive interrupts, but generic IRQ chip functions like irq_gc_mask_set_bit() are designed for chips with a single mask register. Test with the following sequence:

1. Register a level sensitive interrupt on pinA
2. Register an edge sensitive interrupt on pinB

During #2, the mask_cache (with pinA unmasked) gets written to the edge sensitive mask register. At the end of this call pinA is unmasked for edge sensitive interrupts. Now you can get into a state where an edge interrupt is asserted for pinA, but that edge interrupt never gets ack'ed since pinA is registered to handle level IRQs.

The older version of arch/arm/plat-orion/gpio.c does not have the problem because it treats the registers separately and doesn't use a cache. I've only tested this on MV78200 but any processor that uses plat-orion with separate registers will experience this problem.

From: Simon Guinot <[email protected]>

This fixes a regression introduced by e59347a
"arm: orion: Use generic irq chip".

The same interrupt mask cache (stored within struct irq_chip_generic) is shared between all the irq_chip_type instances. As each irq_chip_type can use a distinct mask register, share a single mask cache is not correct. This bug affects Orion SoCs, which have separate mask registers for edge and level interrupts.

This patch move mask_cache from struct irq_chip_generic into struct irq_chip_type. Note that the interrupt support for Samsung SoCs is also slightly affected.

Reported-by Joey Oravec <[email protected]>

Signed-off-by Simon Guinot <[email protected]>

---
 arch/arm/plat-samsung/irq-vic-timer.c |    6 +++-
 include/linux/irq.h                   |    4 +-
 kernel/irq/generic-chip.c             |   38 +++++++++++++++++++-------------
 3 files changed, 28 insertions(+), 20 deletions(-)

arch/arm/plat-samsung/irq-vic-timer.c +4 -2

index f714d06..3bb0b26 100644
@@ -31,9 +31,11 @@ static void s3c_irq_demux_vic_timer(unsigned int irq, struct irq_desc *desc)
 static void s3c_irq_timer_ack(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = gc->chip_types;
+
 	u32 mask = (1 << 5) << (d->irq - gc->irq_base);
 
-	irq_reg_writel(mask | gc->mask_cache, gc->reg_base);
+	irq_reg_writel(mask | ct->mask_cache, gc->reg_base);
 }
 
 /**

@@ -68,7 +70,7 @@ void __init s3c_init_vic_timer_irq(unsigned int num, unsigned int timer_irq)
 	irq_setup_generic_chip(s3c_tgc, IRQ_MSK(num), IRQ_GC_INIT_MASK_CACHE,
 			       IRQ_NOREQUEST | IRQ_NOPROBE, 0);
 	/* Clear the upper bits of the mask_cache*/
-	s3c_tgc->mask_cache &= 0x1f;
+	ct->mask_cache &= 0x1f;
 
 	for (i = 0; i < num; i++, timer_irq++) {
 		irq_set_chained_handler(pirq[i], s3c_irq_demux_vic_timer);

include/linux/irq.h +2 -2

index baa397e..5e5208e 100644
@@ -608,6 +608,7 @@ struct irq_chip_regs {
  * @regs:		Register offsets for this chip
  * @handler:		Flow handler associated with this chip
  * @type:		Chip can handle these flow types
+ * @mask_cache:		Cached mask register
  *
  * A irq_generic_chip can have several instances of irq_chip_type when
  * it requires different functions and register offsets for different

@@ -618,6 +619,7 @@ struct irq_chip_type {
 	struct irq_chip_regs	regs;
 	irq_flow_handler_t	handler;
 	u32			type;
+	u32			mask_cache;
 };
 
 /**

@@ -626,7 +628,6 @@ struct irq_chip_type {
  * @reg_base:		Register base address (virtual)
  * @irq_base:		Interrupt base nr for this chip
  * @irq_cnt:		Number of interrupts handled by this chip
- * @mask_cache:		Cached mask register
  * @type_cache:		Cached type register
  * @polarity_cache:	Cached polarity register
  * @wake_enabled:	Interrupt can wakeup from suspend

@@ -647,7 +648,6 @@ struct irq_chip_generic {
 	void __iomem		*reg_base;
 	unsigned int		irq_base;
 	unsigned int		irq_cnt;
-	u32			mask_cache;
 	u32			type_cache;
 	u32			polarity_cache;
 	u32			wake_enabled;

kernel/irq/generic-chip.c +22 -16

index 3a2cab4..e7d5c13 100644
@@ -15,9 +15,9 @@
 static LIST_HEAD(gc_list);
 static DEFINE_RAW_SPINLOCK(gc_lock);
 
-static inline struct irq_chip_regs *cur_regs(struct irq_data *d)
+static inline struct irq_chip_type *to_irq_chip_type(struct irq_data *d)
 {
-	return &container_of(d->chip, struct irq_chip_type, chip)->regs;
+	return container_of(d->chip, struct irq_chip_type, chip);
 }
 
 /**

@@ -38,11 +38,12 @@ void irq_gc_noop(struct irq_data *d)
 void irq_gc_mask_disable_reg(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = to_irq_chip_type(d);
 	u32 mask = 1 << (d->irq - gc->irq_base);
 
 	irq_gc_lock(gc);
-	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->disable);
-	gc->mask_cache &= ~mask;
+	irq_reg_writel(mask, gc->reg_base + ct->regs.disable);
+	ct->mask_cache &= ~mask;
 	irq_gc_unlock(gc);
 }
 

@@ -56,11 +57,12 @@ void irq_gc_mask_disable_reg(struct irq_data *d)
 void irq_gc_mask_set_bit(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = to_irq_chip_type(d);
 	u32 mask = 1 << (d->irq - gc->irq_base);
 
 	irq_gc_lock(gc);
-	gc->mask_cache |= mask;
-	irq_reg_writel(gc->mask_cache, gc->reg_base + cur_regs(d)->mask);
+	ct->mask_cache |= mask;
+	irq_reg_writel(ct->mask_cache, gc->reg_base + ct->regs.mask);
 	irq_gc_unlock(gc);
 }
 

@@ -74,11 +76,12 @@ void irq_gc_mask_set_bit(struct irq_data *d)
 void irq_gc_mask_clr_bit(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = to_irq_chip_type(d);
 	u32 mask = 1 << (d->irq - gc->irq_base);
 
 	irq_gc_lock(gc);
-	gc->mask_cache &= ~mask;
-	irq_reg_writel(gc->mask_cache, gc->reg_base + cur_regs(d)->mask);
+	ct->mask_cache &= ~mask;
+	irq_reg_writel(ct->mask_cache, gc->reg_base + ct->regs.mask);
 	irq_gc_unlock(gc);
 }
 

@@ -92,11 +95,12 @@ void irq_gc_mask_clr_bit(struct irq_data *d)
 void irq_gc_unmask_enable_reg(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = to_irq_chip_type(d);
 	u32 mask = 1 << (d->irq - gc->irq_base);
 
 	irq_gc_lock(gc);
-	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->enable);
-	gc->mask_cache |= mask;
+	irq_reg_writel(mask, gc->reg_base + ct->regs.enable);
+	ct->mask_cache |= mask;
 	irq_gc_unlock(gc);
 }
 

@@ -110,7 +114,7 @@ void irq_gc_ack_set_bit(struct irq_data *d)
 	u32 mask = 1 << (d->irq - gc->irq_base);
 
 	irq_gc_lock(gc);
-	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->ack);
+	irq_reg_writel(mask, gc->reg_base + to_irq_chip_type(d)->regs.ack);
 	irq_gc_unlock(gc);
 }
 

@@ -124,7 +128,7 @@ void irq_gc_ack_clr_bit(struct irq_data *d)
 	u32 mask = ~(1 << (d->irq - gc->irq_base));
 
 	irq_gc_lock(gc);
-	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->ack);
+	irq_reg_writel(mask, gc->reg_base + to_irq_chip_type(d)->regs.ack);
 	irq_gc_unlock(gc);
 }
 

@@ -138,8 +142,8 @@ void irq_gc_mask_disable_reg_and_ack(struct irq_data *d)
 	u32 mask = 1 << (d->irq - gc->irq_base);
 
 	irq_gc_lock(gc);
-	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->mask);
-	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->ack);
+	irq_reg_writel(mask, gc->reg_base + to_irq_chip_type(d)->regs.mask);
+	irq_reg_writel(mask, gc->reg_base + to_irq_chip_type(d)->regs.ack);
 	irq_gc_unlock(gc);
 }
 

@@ -153,7 +157,7 @@ void irq_gc_eoi(struct irq_data *d)
 	u32 mask = 1 << (d->irq - gc->irq_base);
 
 	irq_gc_lock(gc);
-	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->eoi);
+	irq_reg_writel(mask, gc->reg_base + to_irq_chip_type(d)->regs.eoi);
 	irq_gc_unlock(gc);
 }
 

@@ -243,7 +247,9 @@ void irq_setup_generic_chip(struct irq_chip_generic *gc, u32 msk,
 
 	/* Init mask cache ? */
 	if (flags & IRQ_GC_INIT_MASK_CACHE)
-		gc->mask_cache = irq_reg_readl(gc->reg_base + ct->regs.mask);
+		for (i = 0; i < gc->num_ct; i++)
+			ct[i].mask_cache =
+				irq_reg_readl(gc->reg_base + ct[i].regs.mask);
 
 	for (i = gc->irq_base; msk; msk >>= 1, i++) {
 		if (!msk & 0x01)

Signature · 7 lines

1.7.5.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Simon Guinot <[email protected]>

This fixes a regression introduced by e59347a
"arm: orion: Use generic irq chip".

The same interrupt mask cache (stored within struct irq_chip_generic) is shared between all the irq_chip_type instances. As each irq_chip_type can use a distinct mask register, share a single mask cache is not correct. This bug affects Orion SoCs, which have separate mask registers for edge and level interrupts.

This patch move mask_cache from struct irq_chip_generic into struct irq_chip_type. Note that the interrupt support for Samsung SoCs is also slightly affected.

Quoted 11 lines · Simon Guinot

From: Simon Guinot <[email protected]>

This fixes a regression introduced by e59347a
"arm: orion: Use generic irq chip".

The same interrupt mask cache (stored within struct irq_chip_generic) is shared between all the irq_chip_type instances. As each irq_chip_type can use a distinct mask register, share a single mask cache is not correct. This bug affects Orion SoCs, which have separate mask registers for edge and level interrupts.

Quoted 3 lines

This patch move mask_cache from struct irq_chip_generic into struct irq_chip_type. Note that the interrupt support for Samsung SoCs is also slightly affected.

The patch looks to fix the issue with orion, but it seems that it won't work for SoC with multiple irq_chip_type that use one mask register.

Hi Saeed,

Quoted 15 lines · saeed bishara

Quoted 14 lines · Simon Guinot

From: Simon Guinot <[email protected]>

This fixes a regression introduced by e59347a
"arm: orion: Use generic irq chip".

The same interrupt mask cache (stored within struct irq_chip_generic) is shared between all the irq_chip_type instances. As each irq_chip_type can use a distinct mask register, share a single mask cache is not correct. This bug affects Orion SoCs, which have separate mask registers for edge and level interrupts.

This patch move mask_cache from struct irq_chip_generic into struct irq_chip_type. Note that the interrupt support for Samsung SoCs is also slightly affected.

Quoted 3 lines

The patch looks to fix the issue with orion, but it seems that it won't work for SoC with multiple irq_chip_type that use one mask register.

Yes indeed, but does such SoCs exists ? I mean that the only supported SoC using multiple irq_chip_type (for now) is Orion.

generic case for edge/level interrupts ? shared or separated mask registers ?

If Orion is the specific case, maybe we could provide a dedicated irq_mask() handler instead of using the generic one. It would be a little sad.

Quoted 18 lines · Simon Guinot

Hi Saeed,

Quoted 15 lines · saeed bishara

Quoted 14 lines · Simon Guinot

From: Simon Guinot <[email protected]>

This fixes a regression introduced by e59347a
"arm: orion: Use generic irq chip".

The same interrupt mask cache (stored within struct irq_chip_generic) is shared between all the irq_chip_type instances. As each irq_chip_type can use a distinct mask register, share a single mask cache is not correct. This bug affects Orion SoCs, which have separate mask registers for edge and level interrupts.

This patch move mask_cache from struct irq_chip_generic into struct irq_chip_type. Note that the interrupt support for Samsung SoCs is also slightly affected.

Quoted 6 lines

Quoted 3 lines

The patch looks to fix the issue with orion, but it seems that it won't work for SoC with multiple irq_chip_type that use one mask register.

Yes indeed, but does such SoCs exists ? I mean that the only supported SoC using multiple irq_chip_type (for now) is Orion.

thats right, but as you code is generic, we should find some way to fix it or to prevent such devices to use this generic code.
What is the most

Quoted 2 lines

generic case for edge/level interrupts ? shared or separated mask registers ?

orion gpios use separate mask.

Quoted 4 lines

If Orion is the specific case, maybe we could provide a dedicated irq_mask() handler instead of using the generic one. It would be a little sad.

I personally prefer this method, along with getting rid off the multiple irq_chip_types, my main consideration in this case is performance.
the irq_gc_mask_clr_bit/irq_gc_ack_set_bit/irq_gc_unmask_enable_reg are critical since it get called for every (level) interrupt, and it would be great if you optimize those functions,
I think you better do the following:
1. use pre-calculated offsets instead using gc->reg_base + cur_regs(d)->ack.
2. put all hot variables (lock, mask/ack/eoi register offset) in the same cache line if possible.

regards
saeed

Hi everyone,

I know this issue is from two years ago...

Quoted 46 lines · saeed bishara

Quoted 18 lines · Simon Guinot

Hi Saeed,

Quoted 15 lines · saeed bishara

Quoted 14 lines · Simon Guinot

From: Simon Guinot <[email protected]>

This fixes a regression introduced by e59347a
"arm: orion: Use generic irq chip".

The same interrupt mask cache (stored within struct irq_chip_generic) is shared between all the irq_chip_type instances. As each irq_chip_type can use a distinct mask register, share a single mask cache is not correct. This bug affects Orion SoCs, which have separate mask registers for edge and level interrupts.

This patch move mask_cache from struct irq_chip_generic into struct irq_chip_type. Note that the interrupt support for Samsung SoCs is also slightly affected.

Quoted 6 lines

Quoted 3 lines

The patch looks to fix the issue with orion, but it seems that it won't work for SoC with multiple irq_chip_type that use one mask register.

Yes indeed, but does such SoCs exists ? I mean that the only supported SoC using multiple irq_chip_type (for now) is Orion.

thats right, but as you code is generic, we should find some way to fix it or to prevent such devices to use this generic code.
What is the most

Quoted 2 lines

generic case for edge/level interrupts ? shared or separated mask registers ?

orion gpios use separate mask.

Quoted 4 lines

If Orion is the specific case, maybe we could provide a dedicated irq_mask() handler instead of using the generic one. It would be a little sad.

I personally prefer this method, along with getting rid off the multiple irq_chip_types, my main consideration in this case is performance.
the irq_gc_mask_clr_bit/irq_gc_ack_set_bit/irq_gc_unmask_enable_reg are critical since it get called for every (level) interrupt, and it would be great if you optimize those functions,
I think you better do the following:
1. use pre-calculated offsets instead using gc->reg_base + cur_regs(d)->ack.
2. put all hot variables (lock, mask/ack/eoi register offset) in the same cache line if possible.

Quoted 2 lines

regards
saeed

I ran into the same problem (currently running 3.0.40)... but it looks like this patch was never applied anywhere.
I also quickly scanned the log between now and then and could not find any reference to this problem. Was a fix ever committed or we still have this regression?

Hi Gerlando,

AFAIK this issue is still here. I include the current Orion maintainers in the Cc list.

On Mon, Mar 04, 2013 at 03:28:14PM +0100, Gerlando Falauto wrote:

AFAIK this issue is still here. I include the current Orion maintainers in the Cc list.

Quoted 4 lines · Simon Guinot

On Mon, Mar 04, 2013 at 03:28:14PM +0100, Gerlando Falauto wrote:

AFAIK this issue is still here. I include the current Orion maintainers in the Cc list.

Can you please resend the patch?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 6 lines · Thomas Gleixner

Quoted 4 lines · Simon Guinot

On Mon, Mar 04, 2013 at 03:28:14PM +0100, Gerlando Falauto wrote:

AFAIK this issue is still here. I include the current Orion maintainers in the Cc list.

Quoted 1 line

Can you please resend the patch?

Hi Thomas,

Do you agree with the original patch concept: moving mask_cache from struct irq_chip_generic into struct irq_chip_type ?

This allows to handle the irq_chips which have separate mask registers for the edge and level interrupts. At the time of this patch, this was the only existing case.

Saeed objected that the patch was not correct because this was breaking "hypothetic" devices with multiple irq_chip_type and a single mask register.

The other option was to provide some dedicated irq_mask handlers for the Orion irq chip.

Quoted 7 lines · Simon Guinot

Quoted 6 lines · Thomas Gleixner

Quoted 4 lines · Simon Guinot

On Mon, Mar 04, 2013 at 03:28:14PM +0100, Gerlando Falauto wrote:

AFAIK this issue is still here. I include the current Orion maintainers in the Cc list.

Quoted 6 lines

Quoted 1 line

Can you please resend the patch?

Hi Thomas,

Do you agree with the original patch concept: moving mask_cache from struct irq_chip_generic into struct irq_chip_type ?

Not really. See below.

Quoted 4 lines

This allows to handle the irq_chips which have separate mask registers for the edge and level interrupts. At the time of this patch, this was the only existing case.

Quoted 3 lines

Saeed objected that the patch was not correct because this was breaking "hypothetic" devices with multiple irq_chip_type and a single mask register.

Not so hypothetic. There _are_ irq controllers out there which use the same mask register for both level and edge type irqs.

Quoted 3 lines

The other option was to provide some dedicated irq_mask handlers for the Orion irq chip.

I'd rather refactor the core code so it uses a pointer to the mask_cache. The default would be to let it point to gc->mask_cache and optionally let it point to ct->mask_cache. We'd need to store the flag in the gc struct so we can redirect the pointer to the ct->mask_cache in irq_setup_alt_chip().

Thanks,

tglx

Quoted 2 lines · Thomas Gleixner

Not so hypothetic. There _are_ irq controllers out there which use the same mask register for both level and edge type irqs.

Sure, but if the same mask register is used, then a single irq_chip_type should be able to handle both level and edge interrupts ? I mean, if one needs to register different irq_chip_type for edge and level interrupts, it is most likely because the registers are not the same...

Quoted 5 lines

I'd rather refactor the core code so it uses a pointer to the mask_cache. The default would be to let it point to gc->mask_cache and optionally let it point to ct->mask_cache. We'd need to store the flag in the gc struct so we can redirect the pointer to the ct->mask_cache in irq_setup_alt_chip().