* plat-orion gpio regression for mixed level and edge sensitive IRQs @ 2011-07-19 19:32 Joey Oravec 2011-07-20 23:45 ` Simon Guinot 0 siblings, 1 reply; 15+ messages in thread From: Joey Oravec @ 2011-07-19 19:32 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* plat-orion gpio regression for mixed level and edge sensitive IRQs 2011-07-19 19:32 plat-orion gpio regression for mixed level and edge sensitive IRQs Joey Oravec @ 2011-07-20 23:45 ` Simon Guinot 2011-07-22 0:49 ` [PATCH] genirq: move mask_cache into struct irq_chip_type Simon Guinot 0 siblings, 1 reply; 15+ messages in thread From: Simon Guinot @ 2011-07-20 23:45 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] genirq: move mask_cache into struct irq_chip_type 2011-07-20 23:45 ` Simon Guinot @ 2011-07-22 0:49 ` Simon Guinot 2011-07-26 14:11 ` Simon Guinot 2011-07-26 14:35 ` saeed bishara 0 siblings, 2 replies; 15+ messages in thread From: Simon Guinot @ 2011-07-22 0:49 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH] genirq: move mask_cache into struct irq_chip_type 2011-07-22 0:49 ` [PATCH] genirq: move mask_cache into struct irq_chip_type Simon Guinot @ 2011-07-26 14:11 ` Simon Guinot 2011-07-26 14:35 ` saeed bishara 1 sibling, 0 replies; 15+ messages in thread From: Simon Guinot @ 2011-07-26 14:11 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] genirq: move mask_cache into struct irq_chip_type 2011-07-22 0:49 ` [PATCH] genirq: move mask_cache into struct irq_chip_type Simon Guinot 2011-07-26 14:11 ` Simon Guinot @ 2011-07-26 14:35 ` saeed bishara 2011-07-26 15:39 ` Simon Guinot 1 sibling, 1 reply; 15+ messages in thread From: saeed bishara @ 2011-07-26 14:35 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] genirq: move mask_cache into struct irq_chip_type 2011-07-26 14:35 ` saeed bishara @ 2011-07-26 15:39 ` Simon Guinot 2011-07-27 8:45 ` saeed bishara 0 siblings, 1 reply; 15+ messages in thread From: Simon Guinot @ 2011-07-26 15:39 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] genirq: move mask_cache into struct irq_chip_type 2011-07-26 15:39 ` Simon Guinot @ 2011-07-27 8:45 ` saeed bishara 2013-03-04 14:28 ` Gerlando Falauto 0 siblings, 1 reply; 15+ messages in thread From: saeed bishara @ 2011-07-27 8:45 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] genirq: move mask_cache into struct irq_chip_type 2011-07-27 8:45 ` saeed bishara @ 2013-03-04 14:28 ` Gerlando Falauto 2013-03-04 15:44 ` Simon Guinot 0 siblings, 1 reply; 15+ messages in thread From: Gerlando Falauto @ 2013-03-04 14:28 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] genirq: move mask_cache into struct irq_chip_type 2013-03-04 14:28 ` Gerlando Falauto @ 2013-03-04 15:44 ` Simon Guinot 2013-03-04 17:20 ` Gerlando Falauto 2013-03-05 10:15 ` Thomas Gleixner 0 siblings, 2 replies; 15+ messages in thread From: Simon Guinot @ 2013-03-04 15:44 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] genirq: move mask_cache into struct irq_chip_type 2013-03-04 15:44 ` Simon Guinot @ 2013-03-04 17:20 ` Gerlando Falauto 2013-03-05 10:15 ` Thomas Gleixner 1 sibling, 0 replies; 15+ messages in thread From: Gerlando Falauto @ 2013-03-04 17:20 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] genirq: move mask_cache into struct irq_chip_type 2013-03-04 15:44 ` Simon Guinot 2013-03-04 17:20 ` Gerlando Falauto @ 2013-03-05 10:15 ` Thomas Gleixner 2013-03-06 13:29 ` Simon Guinot 1 sibling, 1 reply; 15+ messages in thread From: Thomas Gleixner @ 2013-03-05 10:15 UTC (permalink / raw) To: linux-arm-kernel 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? ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] genirq: move mask_cache into struct irq_chip_type 2013-03-05 10:15 ` Thomas Gleixner @ 2013-03-06 13:29 ` Simon Guinot 2013-03-06 15:19 ` Thomas Gleixner 0 siblings, 1 reply; 15+ messages in thread From: Simon Guinot @ 2013-03-06 13:29 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] genirq: move mask_cache into struct irq_chip_type 2013-03-06 13:29 ` Simon Guinot @ 2013-03-06 15:19 ` Thomas Gleixner 2013-03-11 15:40 ` Simon Guinot 0 siblings, 1 reply; 15+ messages in thread From: Thomas Gleixner @ 2013-03-06 15:19 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] genirq: move mask_cache into struct irq_chip_type 2013-03-06 15:19 ` Thomas Gleixner @ 2013-03-11 15:40 ` Simon Guinot 2013-03-11 21:08 ` Thomas Gleixner 0 siblings, 1 reply; 15+ messages in thread From: Simon Guinot @ 2013-03-11 15:40 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] genirq: move mask_cache into struct irq_chip_type 2013-03-11 15:40 ` Simon Guinot @ 2013-03-11 21:08 ` Thomas Gleixner 0 siblings, 0 replies; 15+ messages in thread From: Thomas Gleixner @ 2013-03-11 21:08 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2013-03-11 21:08 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-07-19 19:32 plat-orion gpio regression for mixed level and edge sensitive IRQs Joey Oravec 2011-07-20 23:45 ` Simon Guinot 2011-07-22 0:49 ` [PATCH] genirq: move mask_cache into struct irq_chip_type Simon Guinot 2011-07-26 14:11 ` Simon Guinot 2011-07-26 14:35 ` saeed bishara 2011-07-26 15:39 ` Simon Guinot 2011-07-27 8:45 ` saeed bishara 2013-03-04 14:28 ` Gerlando Falauto 2013-03-04 15:44 ` Simon Guinot 2013-03-04 17:20 ` Gerlando Falauto 2013-03-05 10:15 ` Thomas Gleixner 2013-03-06 13:29 ` Simon Guinot 2013-03-06 15:19 ` Thomas Gleixner 2013-03-11 15:40 ` Simon Guinot 2013-03-11 21:08 ` Thomas Gleixner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).