| Message ID | 1442843173-2390-1-git-send-email-ludovic.desroches@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ludovic, On Mon, 21 Sep 2015 15:46:04 +0200 Ludovic Desroches <ludovic.desroches@atmel.com> wrote: > When masking/unmasking interrupts, mask_cache is updated and used later > for suspend/resume. Unfortunately, it always was the mask_cache > associated with the first irq chip which was updated. So when performing > resume, only irqs 0-31 could be enabled and maybe not the good ones! > > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com> > Fixes: b1479ebb7720 ("irqchip: atmel-aic: Add atmel AIC/AIC5 drivers") > Cc: stable@vger.kernel.org #3.18 To the whole series Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com> Thanks, Boris > --- > > Sasha, > > This fix won't apply without conflicts because of irq_reg_writel changes. I > can provide you a fix for 3.18 if you need. > > Regards > > Ludovic > > > drivers/irqchip/irq-atmel-aic5.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c > index 9da9942..6c5fd25 100644 > --- a/drivers/irqchip/irq-atmel-aic5.c > +++ b/drivers/irqchip/irq-atmel-aic5.c > @@ -88,28 +88,30 @@ static void aic5_mask(struct irq_data *d) > { > struct irq_domain *domain = d->domain; > struct irq_domain_chip_generic *dgc = domain->gc; > - struct irq_chip_generic *gc = dgc->gc[0]; > + struct irq_chip_generic *bgc = dgc->gc[0]; > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > > /* Disable interrupt on AIC5 */ > - irq_gc_lock(gc); > + irq_gc_lock(bgc); > irq_reg_writel(gc, d->hwirq, AT91_AIC5_SSR); > irq_reg_writel(gc, 1, AT91_AIC5_IDCR); > gc->mask_cache &= ~d->mask; > - irq_gc_unlock(gc); > + irq_gc_unlock(bgc); > } > > static void aic5_unmask(struct irq_data *d) > { > struct irq_domain *domain = d->domain; > struct irq_domain_chip_generic *dgc = domain->gc; > - struct irq_chip_generic *gc = dgc->gc[0]; > + struct irq_chip_generic *bgc = dgc->gc[0]; > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > > /* Enable interrupt on AIC5 */ > - irq_gc_lock(gc); > + irq_gc_lock(bgc); > irq_reg_writel(gc, d->hwirq, AT91_AIC5_SSR); > irq_reg_writel(gc, 1, AT91_AIC5_IECR); > gc->mask_cache |= d->mask; > - irq_gc_unlock(gc); > + irq_gc_unlock(bgc); > } > > static int aic5_retrigger(struct irq_data *d)
Le 21/09/2015 15:46, Ludovic Desroches a écrit : > When masking/unmasking interrupts, mask_cache is updated and used later > for suspend/resume. Unfortunately, it always was the mask_cache > associated with the first irq chip which was updated. So when performing > resume, only irqs 0-31 could be enabled and maybe not the good ones! > > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com> > Fixes: b1479ebb7720 ("irqchip: atmel-aic: Add atmel AIC/AIC5 drivers") > Cc: stable@vger.kernel.org #3.18 Pretty important fix indeed! Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> Thomas, Jason, can we please have this series queued for the 4.3-rc phase? Bye, > --- > > Sasha, > > This fix won't apply without conflicts because of irq_reg_writel changes. I > can provide you a fix for 3.18 if you need. > > Regards > > Ludovic > > > drivers/irqchip/irq-atmel-aic5.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c > index 9da9942..6c5fd25 100644 > --- a/drivers/irqchip/irq-atmel-aic5.c > +++ b/drivers/irqchip/irq-atmel-aic5.c > @@ -88,28 +88,30 @@ static void aic5_mask(struct irq_data *d) > { > struct irq_domain *domain = d->domain; > struct irq_domain_chip_generic *dgc = domain->gc; > - struct irq_chip_generic *gc = dgc->gc[0]; > + struct irq_chip_generic *bgc = dgc->gc[0]; > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > > /* Disable interrupt on AIC5 */ > - irq_gc_lock(gc); > + irq_gc_lock(bgc); > irq_reg_writel(gc, d->hwirq, AT91_AIC5_SSR); > irq_reg_writel(gc, 1, AT91_AIC5_IDCR); > gc->mask_cache &= ~d->mask; > - irq_gc_unlock(gc); > + irq_gc_unlock(bgc); > } > > static void aic5_unmask(struct irq_data *d) > { > struct irq_domain *domain = d->domain; > struct irq_domain_chip_generic *dgc = domain->gc; > - struct irq_chip_generic *gc = dgc->gc[0]; > + struct irq_chip_generic *bgc = dgc->gc[0]; > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > > /* Enable interrupt on AIC5 */ > - irq_gc_lock(gc); > + irq_gc_lock(bgc); > irq_reg_writel(gc, d->hwirq, AT91_AIC5_SSR); > irq_reg_writel(gc, 1, AT91_AIC5_IECR); > gc->mask_cache |= d->mask; > - irq_gc_unlock(gc); > + irq_gc_unlock(bgc); > } > > static int aic5_retrigger(struct irq_data *d) >
On Mon, 21 Sep 2015, Ludovic Desroches wrote: > diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c > index 9da9942..6c5fd25 100644 > --- a/drivers/irqchip/irq-atmel-aic5.c > +++ b/drivers/irqchip/irq-atmel-aic5.c > @@ -88,28 +88,30 @@ static void aic5_mask(struct irq_data *d) > { > struct irq_domain *domain = d->domain; > struct irq_domain_chip_generic *dgc = domain->gc; > - struct irq_chip_generic *gc = dgc->gc[0]; > + struct irq_chip_generic *bgc = dgc->gc[0]; > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > > /* Disable interrupt on AIC5 */ > - irq_gc_lock(gc); > + irq_gc_lock(bgc); Why is this locking dgc->gc[0] and fiddling with some other generic chip? > irq_reg_writel(gc, d->hwirq, AT91_AIC5_SSR); > irq_reg_writel(gc, 1, AT91_AIC5_IDCR); Thanks, tglx
On Tue, 22 Sep 2015, tip-bot for Ludovic Desroches wrote: > Commit-ID: 478cc7b33413a4ad50852239d931d55e622f1cde > Gitweb: http://git.kernel.org/tip/478cc7b33413a4ad50852239d931d55e622f1cde FYI, I zapped that commit again after noticing that locking strangeness. Thanks, tglx
Hi Thomas, On Tue, 22 Sep 2015 12:27:08 +0200 (CEST) Thomas Gleixner <tglx@linutronix.de> wrote: > On Mon, 21 Sep 2015, Ludovic Desroches wrote: > > diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c > > index 9da9942..6c5fd25 100644 > > --- a/drivers/irqchip/irq-atmel-aic5.c > > +++ b/drivers/irqchip/irq-atmel-aic5.c > > @@ -88,28 +88,30 @@ static void aic5_mask(struct irq_data *d) > > { > > struct irq_domain *domain = d->domain; > > struct irq_domain_chip_generic *dgc = domain->gc; > > - struct irq_chip_generic *gc = dgc->gc[0]; > > + struct irq_chip_generic *bgc = dgc->gc[0]; > > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > > > > /* Disable interrupt on AIC5 */ > > - irq_gc_lock(gc); > > + irq_gc_lock(bgc); > > Why is this locking dgc->gc[0] and fiddling with some other generic > chip? Actually, we always access the same set of registers for all irqs of the domain, and thus need to take the same lock (I chose the one contained in the first generic irqchip, but I guess it could work with the others too, as long as we always take the same one) before accessing them because the configuration is done in two steps: 1/ specify the irq line you want to configure 2/ set the new configuration Regarding register accesses, all generic chips are configured to point to the same registers, so accessing them from the 'base' generic chip or from the generic chip attached to the irq_data struct is the same, though I agree that using bgc would add some consistency to the implementation. This leaves the mask_cache value, which should be updated on the generic chip attached to the irq_data (but since the lock is global to the whole domain, it should work fine). Please let us know if you see other problems, or have a better solution to fix the bug. Best Regards, Boris > > > irq_reg_writel(gc, d->hwirq, AT91_AIC5_SSR); > > irq_reg_writel(gc, 1, AT91_AIC5_IDCR); > > Thanks, > > tglx
On Tue, 22 Sep 2015, Boris Brezillon wrote: > On Tue, 22 Sep 2015 12:27:08 +0200 (CEST) > Thomas Gleixner <tglx@linutronix.de> wrote: > > Why is this locking dgc->gc[0] and fiddling with some other generic > > chip? > > Actually, we always access the same set of registers for all irqs of the > domain, and thus need to take the same lock (I chose the one contained > in the first generic irqchip, but I guess it could work with the others > too, as long as we always take the same one) before accessing them > because the configuration is done in two steps: > > 1/ specify the irq line you want to configure > 2/ set the new configuration > > Regarding register accesses, all generic chips are configured to > point to the same registers, so accessing them from the 'base' generic > chip or from the generic chip attached to the irq_data struct is the > same, though I agree that using bgc would add some consistency to the > implementation. Fair enough. It just deserves a comment for the casual reader. Thanks, tglx
On Tue, Sep 22, 2015 at 03:50:30PM +0200, Thomas Gleixner wrote: > On Tue, 22 Sep 2015, Boris Brezillon wrote: > > On Tue, 22 Sep 2015 12:27:08 +0200 (CEST) > > Thomas Gleixner <tglx@linutronix.de> wrote: > > > Why is this locking dgc->gc[0] and fiddling with some other generic > > > chip? > > > > Actually, we always access the same set of registers for all irqs of the > > domain, and thus need to take the same lock (I chose the one contained > > in the first generic irqchip, but I guess it could work with the others > > too, as long as we always take the same one) before accessing them > > because the configuration is done in two steps: > > > > 1/ specify the irq line you want to configure > > 2/ set the new configuration > > > > Regarding register accesses, all generic chips are configured to > > point to the same registers, so accessing them from the 'base' generic > > chip or from the generic chip attached to the irq_data struct is the > > same, though I agree that using bgc would add some consistency to the > > implementation. > > Fair enough. It just deserves a comment for the casual reader. > Thanks for the addition of the comment. Ludovic
diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c index 9da9942..6c5fd25 100644 --- a/drivers/irqchip/irq-atmel-aic5.c +++ b/drivers/irqchip/irq-atmel-aic5.c @@ -88,28 +88,30 @@ static void aic5_mask(struct irq_data *d) { struct irq_domain *domain = d->domain; struct irq_domain_chip_generic *dgc = domain->gc; - struct irq_chip_generic *gc = dgc->gc[0]; + struct irq_chip_generic *bgc = dgc->gc[0]; + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); /* Disable interrupt on AIC5 */ - irq_gc_lock(gc); + irq_gc_lock(bgc); irq_reg_writel(gc, d->hwirq, AT91_AIC5_SSR); irq_reg_writel(gc, 1, AT91_AIC5_IDCR); gc->mask_cache &= ~d->mask; - irq_gc_unlock(gc); + irq_gc_unlock(bgc); } static void aic5_unmask(struct irq_data *d) { struct irq_domain *domain = d->domain; struct irq_domain_chip_generic *dgc = domain->gc; - struct irq_chip_generic *gc = dgc->gc[0]; + struct irq_chip_generic *bgc = dgc->gc[0]; + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); /* Enable interrupt on AIC5 */ - irq_gc_lock(gc); + irq_gc_lock(bgc); irq_reg_writel(gc, d->hwirq, AT91_AIC5_SSR); irq_reg_writel(gc, 1, AT91_AIC5_IECR); gc->mask_cache |= d->mask; - irq_gc_unlock(gc); + irq_gc_unlock(bgc); } static int aic5_retrigger(struct irq_data *d)
When masking/unmasking interrupts, mask_cache is updated and used later for suspend/resume. Unfortunately, it always was the mask_cache associated with the first irq chip which was updated. So when performing resume, only irqs 0-31 could be enabled and maybe not the good ones! Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com> Fixes: b1479ebb7720 ("irqchip: atmel-aic: Add atmel AIC/AIC5 drivers") Cc: stable@vger.kernel.org #3.18 --- Sasha, This fix won't apply without conflicts because of irq_reg_writel changes. I can provide you a fix for 3.18 if you need. Regards Ludovic drivers/irqchip/irq-atmel-aic5.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)