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

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

[Joey Oravec]

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.

-joey

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

[Simon Guinot]

Hi Joey,

On Tue, Jul 19, 2011 at 03:32:21PM -0400, Joey Oravec wrote:
> 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.

The same mask cache (stored within struct irq_chip_generic) is shared
between all the irq_chip_type instances. As they can use different mask
registers, there is a problem.

We should probably move mask_cache into struct irq_chip_type.

Note that this issue only affect the Orion SoCs. On the other platform
using the generic interrupt chip API, a single irq_chip_type is defined.

Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110720/e4ea6982/attachment.sig>

[PATCH] genirq: move mask_cache into struct irq_chip_type

[Simon Guinot]

From: Simon Guinot <sguinot@lacie.com>

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 <joravec@drewtech.com>
Signed-off-by: Simon Guinot <sguinot@lacie.com>
---
 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(-)

diff --git a/arch/arm/plat-samsung/irq-vic-timer.c b/arch/arm/plat-samsung/irq-vic-timer.c
index f714d06..3bb0b26 100644
--- a/arch/arm/plat-samsung/irq-vic-timer.c
+++ b/arch/arm/plat-samsung/irq-vic-timer.c
@@ -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);
diff --git a/include/linux/irq.h b/include/linux/irq.h
index baa397e..5e5208e 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -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;
diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index 3a2cab4..e7d5c13 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -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)
-- 
1.7.5.1

[PATCH] genirq: move mask_cache into struct irq_chip_type

[Simon Guinot]

Hi Thomas, Nicolas, Lennert and Saeed,

On Fri, Jul 22, 2011 at 02:49:18AM +0200, Simon Guinot wrote:
> From: Simon Guinot <sguinot@lacie.com>
> 
> 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 <joravec@drewtech.com>
> Signed-off-by: Simon Guinot <sguinot@lacie.com>
> ---
>  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(-)

What do you think about this patch ?

Even if the issue is not critical, this should be fixed.

Regards,

Simon

> 
> diff --git a/arch/arm/plat-samsung/irq-vic-timer.c b/arch/arm/plat-samsung/irq-vic-timer.c
> index f714d06..3bb0b26 100644
> --- a/arch/arm/plat-samsung/irq-vic-timer.c
> +++ b/arch/arm/plat-samsung/irq-vic-timer.c
> @@ -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);
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index baa397e..5e5208e 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -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;
> diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
> index 3a2cab4..e7d5c13 100644
> --- a/kernel/irq/generic-chip.c
> +++ b/kernel/irq/generic-chip.c
> @@ -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)
> -- 
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110726/cd18a7b1/attachment-0001.sig>

[PATCH] genirq: move mask_cache into struct irq_chip_type

[saeed bishara]

On Fri, Jul 22, 2011 at 3:49 AM, Simon Guinot <simon@sequanux.org> wrote:
> From: Simon Guinot <sguinot@lacie.com>
>
> 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.
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.

saeed

[PATCH] genirq: move mask_cache into struct irq_chip_type

[Simon Guinot]

Hi Saeed,

On Tue, Jul 26, 2011 at 05:35:50PM +0300, saeed bishara wrote:
> On Fri, Jul 22, 2011 at 3:49 AM, Simon Guinot <simon@sequanux.org> wrote:
> > From: Simon Guinot <sguinot@lacie.com>
> >
> > 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.
> 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. What is the most
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.

Or we could find a way to make coexist this two mask cache policies
(even if I don't see how to do that cleanly) ?
Alternatively, we could allow to bypass this mask cache...

Regards,

Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110726/b92b8b8b/attachment.sig>

[PATCH] genirq: move mask_cache into struct irq_chip_type

[saeed bishara]

On Tue, Jul 26, 2011 at 6:39 PM, Simon Guinot <simon@sequanux.org> wrote:
> Hi Saeed,
>
> On Tue, Jul 26, 2011 at 05:35:50PM +0300, saeed bishara wrote:
>> On Fri, Jul 22, 2011 at 3:49 AM, Simon Guinot <simon@sequanux.org> wrote:
>> > From: Simon Guinot <sguinot@lacie.com>
>> >
>> > 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.
>> 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
> generic case for edge/level interrupts ? shared or separated mask
> registers ?
orion gpios use separate mask.
>
> 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

[PATCH] genirq: move mask_cache into struct irq_chip_type

[Gerlando Falauto]

Hi everyone,

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

On 07/27/2011 10:45 AM, saeed bishara wrote:
> On Tue, Jul 26, 2011 at 6:39 PM, Simon Guinot <simon@sequanux.org> wrote:
>> Hi Saeed,
>>
>> On Tue, Jul 26, 2011 at 05:35:50PM +0300, saeed bishara wrote:
>>> On Fri, Jul 22, 2011 at 3:49 AM, Simon Guinot <simon@sequanux.org> wrote:
>>>> From: Simon Guinot <sguinot@lacie.com>
>>>>
>>>> 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.
>>> 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
>> generic case for edge/level interrupts ? shared or separated mask
>> registers ?
> orion gpios use separate mask.
>>
>> 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

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?

Thanks a lot!
Gerlando

[PATCH] genirq: move mask_cache into struct irq_chip_type

[Simon Guinot]

On Mon, Mar 04, 2013 at 03:28:14PM +0100, Gerlando Falauto wrote:
> Hi everyone,
> 
> I know this issue is from two years ago...
> 
> On 07/27/2011 10:45 AM, saeed bishara wrote:
> >On Tue, Jul 26, 2011 at 6:39 PM, Simon Guinot <simon@sequanux.org> wrote:
> >>Hi Saeed,
> >>
> >>On Tue, Jul 26, 2011 at 05:35:50PM +0300, saeed bishara wrote:
> >>>On Fri, Jul 22, 2011 at 3:49 AM, Simon Guinot <simon@sequanux.org> wrote:
> >>>>From: Simon Guinot <sguinot@lacie.com>
> >>>>
> >>>>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.
> >>>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
> >>generic case for edge/level interrupts ? shared or separated mask
> >>registers ?
> >orion gpios use separate mask.
> >>
> >>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
> 
> 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.

Regards,

Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130304/0fabd282/attachment-0001.sig>

[PATCH] genirq: move mask_cache into struct irq_chip_type

[Gerlando Falauto]

Hi Simon,

On 03/04/2013 04:44 PM, Simon Guinot wrote:
> Hi Gerlando,
>
> AFAIK this issue is still here. I include the current Orion maintainers
> in the Cc list.

Thanks! And BTW, your patch solves the problem on my side. :-)

Regards,
Gerlando

[PATCH] genirq: move mask_cache into struct irq_chip_type

[Thomas Gleixner]

On Mon, 4 Mar 2013, Simon Guinot wrote:
> 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?

[PATCH] genirq: move mask_cache into struct irq_chip_type

[Simon Guinot]

On Tue, Mar 05, 2013 at 11:15:04AM +0100, Thomas Gleixner wrote:
> On Mon, 4 Mar 2013, Simon Guinot wrote:
> > 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?

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.

As I was comfortable with none of this solutions, I kind of let it lie.

Let me know you advice.

Simon

> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130306/014a76f1/attachment.sig>

[PATCH] genirq: move mask_cache into struct irq_chip_type

[Thomas Gleixner]

On Wed, 6 Mar 2013, Simon Guinot wrote:

> On Tue, Mar 05, 2013 at 11:15:04AM +0100, Thomas Gleixner wrote:
> > On Mon, 4 Mar 2013, Simon Guinot wrote:
> > > 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?
> 
> 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.
 
> 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.

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

> 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

[PATCH] genirq: move mask_cache into struct irq_chip_type

[Simon Guinot]

On Wed, Mar 06, 2013 at 04:19:30PM +0100, Thomas Gleixner wrote:
> On Wed, 6 Mar 2013, Simon Guinot wrote:
> 
> > On Tue, Mar 05, 2013 at 11:15:04AM +0100, Thomas Gleixner wrote:
> > > On Mon, 4 Mar 2013, Simon Guinot wrote:
> > > > 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?
> > 
> > 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.
>  
> > 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.
> 
> 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...

Simon

> 
> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130311/ea7b9c97/attachment.sig>

[PATCH] genirq: move mask_cache into struct irq_chip_type

[Thomas Gleixner]

On Mon, 11 Mar 2013, Simon Guinot wrote:
> On Wed, Mar 06, 2013 at 04:19:30PM +0100, Thomas Gleixner wrote:
> > 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...

No, it's because first of all the types have different flow handlers
and because they can have a different irq_chip due to different
callbacks while still having the necessarity to share a single
mask_cache.

Care to look at struct irq_chip_type as a whole and not just from the
POV of your particular chip incarnation?

Again. Here is the solution to the problem:

> > 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().

Stop arguing in circles and implement the 20 lines of patch already.

Thanks,

	tglx