[PATCH 1/3] irqchip: atmel-aic5: fix bug with mask/unmask

[PATCH 1/3] irqchip: atmel-aic5: fix bug with mask/unmask

[Ludovic Desroches]

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

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)
-- 
2.5.0

[PATCH 2/3] irqchip: atmel-aic5: fix variable naming

[Ludovic Desroches]

To avoid errors, use an explicit variable name when accessing the 'base'
generic chip.

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 drivers/irqchip/irq-atmel-aic5.c | 44 ++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c
index 6c5fd25..abff79e 100644
--- a/drivers/irqchip/irq-atmel-aic5.c
+++ b/drivers/irqchip/irq-atmel-aic5.c
@@ -71,15 +71,15 @@ static asmlinkage void __exception_irq_entry
 aic5_handle(struct pt_regs *regs)
 {
 	struct irq_domain_chip_generic *dgc = aic5_domain->gc;
-	struct irq_chip_generic *gc = dgc->gc[0];
+	struct irq_chip_generic *bgc = dgc->gc[0];
 	u32 irqnr;
 	u32 irqstat;
 
-	irqnr = irq_reg_readl(gc, AT91_AIC5_IVR);
-	irqstat = irq_reg_readl(gc, AT91_AIC5_ISR);
+	irqnr = irq_reg_readl(bgc, AT91_AIC5_IVR);
+	irqstat = irq_reg_readl(bgc, AT91_AIC5_ISR);
 
 	if (!irqstat)
-		irq_reg_writel(gc, 0, AT91_AIC5_EOICR);
+		irq_reg_writel(bgc, 0, AT91_AIC5_EOICR);
 	else
 		handle_domain_irq(aic5_domain, irqnr, regs);
 }
@@ -118,13 +118,13 @@ static int aic5_retrigger(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];
 
 	/* Enable interrupt on AIC5 */
-	irq_gc_lock(gc);
-	irq_reg_writel(gc, d->hwirq, AT91_AIC5_SSR);
-	irq_reg_writel(gc, 1, AT91_AIC5_ISCR);
-	irq_gc_unlock(gc);
+	irq_gc_lock(bgc);
+	irq_reg_writel(bgc, d->hwirq, AT91_AIC5_SSR);
+	irq_reg_writel(bgc, 1, AT91_AIC5_ISCR);
+	irq_gc_unlock(bgc);
 
 	return 0;
 }
@@ -133,17 +133,17 @@ static int aic5_set_type(struct irq_data *d, unsigned type)
 {
 	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];
 	unsigned int smr;
 	int ret;
 
-	irq_gc_lock(gc);
-	irq_reg_writel(gc, d->hwirq, AT91_AIC5_SSR);
-	smr = irq_reg_readl(gc, AT91_AIC5_SMR);
+	irq_gc_lock(bgc);
+	irq_reg_writel(bgc, d->hwirq, AT91_AIC5_SSR);
+	smr = irq_reg_readl(bgc, AT91_AIC5_SMR);
 	ret = aic_common_set_type(d, type, &smr);
 	if (!ret)
-		irq_reg_writel(gc, smr, AT91_AIC5_SMR);
-	irq_gc_unlock(gc);
+		irq_reg_writel(bgc, smr, AT91_AIC5_SMR);
+	irq_gc_unlock(bgc);
 
 	return ret;
 }
@@ -257,7 +257,7 @@ static int aic5_irq_domain_xlate(struct irq_domain *d,
 				 unsigned int *out_type)
 {
 	struct irq_domain_chip_generic *dgc = d->gc;
-	struct irq_chip_generic *gc;
+	struct irq_chip_generic *bgc;
 	unsigned smr;
 	int ret;
 
@@ -269,15 +269,15 @@ static int aic5_irq_domain_xlate(struct irq_domain *d,
 	if (ret)
 		return ret;
 
-	gc = dgc->gc[0];
+	bgc = dgc->gc[0];
 
-	irq_gc_lock(gc);
-	irq_reg_writel(gc, *out_hwirq, AT91_AIC5_SSR);
-	smr = irq_reg_readl(gc, AT91_AIC5_SMR);
+	irq_gc_lock(bgc);
+	irq_reg_writel(bgc, *out_hwirq, AT91_AIC5_SSR);
+	smr = irq_reg_readl(bgc, AT91_AIC5_SMR);
 	ret = aic_common_set_priority(intspec[2], &smr);
 	if (!ret)
-		irq_reg_writel(gc, intspec[2] | smr, AT91_AIC5_SMR);
-	irq_gc_unlock(gc);
+		irq_reg_writel(bgc, intspec[2] | smr, AT91_AIC5_SMR);
+	irq_gc_unlock(bgc);
 
 	return ret;
 }
-- 
2.5.0

[PATCH 3/3] irqchip: atmel-aic5: simplify base chip selection

[Ludovic Desroches]

Use irq_get_domain_generic_chip() to select the base chip.

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 drivers/irqchip/irq-atmel-aic5.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c
index abff79e..7264ec7 100644
--- a/drivers/irqchip/irq-atmel-aic5.c
+++ b/drivers/irqchip/irq-atmel-aic5.c
@@ -70,8 +70,7 @@ static struct irq_domain *aic5_domain;
 static asmlinkage void __exception_irq_entry
 aic5_handle(struct pt_regs *regs)
 {
-	struct irq_domain_chip_generic *dgc = aic5_domain->gc;
-	struct irq_chip_generic *bgc = dgc->gc[0];
+	struct irq_chip_generic *bgc = irq_get_domain_generic_chip(aic5_domain, 0);
 	u32 irqnr;
 	u32 irqstat;
 
@@ -87,8 +86,7 @@ aic5_handle(struct pt_regs *regs)
 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 *bgc = dgc->gc[0];
+	struct irq_chip_generic *bgc = irq_get_domain_generic_chip(domain, 0);
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
 
 	/* Disable interrupt on AIC5 */
@@ -102,8 +100,7 @@ static void aic5_mask(struct irq_data *d)
 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 *bgc = dgc->gc[0];
+	struct irq_chip_generic *bgc = irq_get_domain_generic_chip(domain, 0);
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
 
 	/* Enable interrupt on AIC5 */
@@ -117,8 +114,7 @@ static void aic5_unmask(struct irq_data *d)
 static int aic5_retrigger(struct irq_data *d)
 {
 	struct irq_domain *domain = d->domain;
-	struct irq_domain_chip_generic *dgc = domain->gc;
-	struct irq_chip_generic *bgc = dgc->gc[0];
+	struct irq_chip_generic *bgc = irq_get_domain_generic_chip(domain, 0);
 
 	/* Enable interrupt on AIC5 */
 	irq_gc_lock(bgc);
@@ -132,8 +128,7 @@ static int aic5_retrigger(struct irq_data *d)
 static int aic5_set_type(struct irq_data *d, unsigned type)
 {
 	struct irq_domain *domain = d->domain;
-	struct irq_domain_chip_generic *dgc = domain->gc;
-	struct irq_chip_generic *bgc = dgc->gc[0];
+	struct irq_chip_generic *bgc = irq_get_domain_generic_chip(domain, 0);
 	unsigned int smr;
 	int ret;
 
@@ -153,7 +148,7 @@ static void aic5_suspend(struct irq_data *d)
 {
 	struct irq_domain *domain = d->domain;
 	struct irq_domain_chip_generic *dgc = domain->gc;
-	struct irq_chip_generic *bgc = dgc->gc[0];
+	struct irq_chip_generic *bgc = irq_get_domain_generic_chip(domain, 0);
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
 	int i;
 	u32 mask;
@@ -177,7 +172,7 @@ static void aic5_resume(struct irq_data *d)
 {
 	struct irq_domain *domain = d->domain;
 	struct irq_domain_chip_generic *dgc = domain->gc;
-	struct irq_chip_generic *bgc = dgc->gc[0];
+	struct irq_chip_generic *bgc = irq_get_domain_generic_chip(domain, 0);
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
 	int i;
 	u32 mask;
@@ -201,7 +196,7 @@ static void aic5_pm_shutdown(struct irq_data *d)
 {
 	struct irq_domain *domain = d->domain;
 	struct irq_domain_chip_generic *dgc = domain->gc;
-	struct irq_chip_generic *bgc = dgc->gc[0];
+	struct irq_chip_generic *bgc = irq_get_domain_generic_chip(domain, 0);
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
 	int i;
 
@@ -256,12 +251,11 @@ static int aic5_irq_domain_xlate(struct irq_domain *d,
 				 irq_hw_number_t *out_hwirq,
 				 unsigned int *out_type)
 {
-	struct irq_domain_chip_generic *dgc = d->gc;
-	struct irq_chip_generic *bgc;
+	struct irq_chip_generic *bgc = irq_get_domain_generic_chip(d, 0);
 	unsigned smr;
 	int ret;
 
-	if (!dgc)
+	if (!bgc)
 		return -EINVAL;
 
 	ret = aic_common_irq_domain_xlate(d, ctrlr, intspec, intsize,
@@ -269,8 +263,6 @@ static int aic5_irq_domain_xlate(struct irq_domain *d,
 	if (ret)
 		return ret;
 
-	bgc = dgc->gc[0];
-
 	irq_gc_lock(bgc);
 	irq_reg_writel(bgc, *out_hwirq, AT91_AIC5_SSR);
 	smr = irq_reg_readl(bgc, AT91_AIC5_SMR);
-- 
2.5.0

[PATCH 1/3] irqchip: atmel-aic5: fix bug with mask/unmask

[Boris Brezillon]

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 at 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)



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 1/3] irqchip: atmel-aic5: fix bug with mask/unmask

[Nicolas Ferre]

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 at 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)
> 


-- 
Nicolas Ferre

[PATCH 2/3] irqchip: atmel-aic5: fix variable naming

[Nicolas Ferre]

Le 21/09/2015 15:46, Ludovic Desroches a ?crit :
> To avoid errors, use an explicit variable name when accessing the 'base'
> generic chip.
> 
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

> ---
>  drivers/irqchip/irq-atmel-aic5.c | 44 ++++++++++++++++++++--------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c
> index 6c5fd25..abff79e 100644
> --- a/drivers/irqchip/irq-atmel-aic5.c
> +++ b/drivers/irqchip/irq-atmel-aic5.c
> @@ -71,15 +71,15 @@ static asmlinkage void __exception_irq_entry
>  aic5_handle(struct pt_regs *regs)
>  {
>  	struct irq_domain_chip_generic *dgc = aic5_domain->gc;
> -	struct irq_chip_generic *gc = dgc->gc[0];
> +	struct irq_chip_generic *bgc = dgc->gc[0];
>  	u32 irqnr;
>  	u32 irqstat;
>  
> -	irqnr = irq_reg_readl(gc, AT91_AIC5_IVR);
> -	irqstat = irq_reg_readl(gc, AT91_AIC5_ISR);
> +	irqnr = irq_reg_readl(bgc, AT91_AIC5_IVR);
> +	irqstat = irq_reg_readl(bgc, AT91_AIC5_ISR);
>  
>  	if (!irqstat)
> -		irq_reg_writel(gc, 0, AT91_AIC5_EOICR);
> +		irq_reg_writel(bgc, 0, AT91_AIC5_EOICR);
>  	else
>  		handle_domain_irq(aic5_domain, irqnr, regs);
>  }
> @@ -118,13 +118,13 @@ static int aic5_retrigger(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];
>  
>  	/* Enable interrupt on AIC5 */
> -	irq_gc_lock(gc);
> -	irq_reg_writel(gc, d->hwirq, AT91_AIC5_SSR);
> -	irq_reg_writel(gc, 1, AT91_AIC5_ISCR);
> -	irq_gc_unlock(gc);
> +	irq_gc_lock(bgc);
> +	irq_reg_writel(bgc, d->hwirq, AT91_AIC5_SSR);
> +	irq_reg_writel(bgc, 1, AT91_AIC5_ISCR);
> +	irq_gc_unlock(bgc);
>  
>  	return 0;
>  }
> @@ -133,17 +133,17 @@ static int aic5_set_type(struct irq_data *d, unsigned type)
>  {
>  	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];
>  	unsigned int smr;
>  	int ret;
>  
> -	irq_gc_lock(gc);
> -	irq_reg_writel(gc, d->hwirq, AT91_AIC5_SSR);
> -	smr = irq_reg_readl(gc, AT91_AIC5_SMR);
> +	irq_gc_lock(bgc);
> +	irq_reg_writel(bgc, d->hwirq, AT91_AIC5_SSR);
> +	smr = irq_reg_readl(bgc, AT91_AIC5_SMR);
>  	ret = aic_common_set_type(d, type, &smr);
>  	if (!ret)
> -		irq_reg_writel(gc, smr, AT91_AIC5_SMR);
> -	irq_gc_unlock(gc);
> +		irq_reg_writel(bgc, smr, AT91_AIC5_SMR);
> +	irq_gc_unlock(bgc);
>  
>  	return ret;
>  }
> @@ -257,7 +257,7 @@ static int aic5_irq_domain_xlate(struct irq_domain *d,
>  				 unsigned int *out_type)
>  {
>  	struct irq_domain_chip_generic *dgc = d->gc;
> -	struct irq_chip_generic *gc;
> +	struct irq_chip_generic *bgc;
>  	unsigned smr;
>  	int ret;
>  
> @@ -269,15 +269,15 @@ static int aic5_irq_domain_xlate(struct irq_domain *d,
>  	if (ret)
>  		return ret;
>  
> -	gc = dgc->gc[0];
> +	bgc = dgc->gc[0];
>  
> -	irq_gc_lock(gc);
> -	irq_reg_writel(gc, *out_hwirq, AT91_AIC5_SSR);
> -	smr = irq_reg_readl(gc, AT91_AIC5_SMR);
> +	irq_gc_lock(bgc);
> +	irq_reg_writel(bgc, *out_hwirq, AT91_AIC5_SSR);
> +	smr = irq_reg_readl(bgc, AT91_AIC5_SMR);
>  	ret = aic_common_set_priority(intspec[2], &smr);
>  	if (!ret)
> -		irq_reg_writel(gc, intspec[2] | smr, AT91_AIC5_SMR);
> -	irq_gc_unlock(gc);
> +		irq_reg_writel(bgc, intspec[2] | smr, AT91_AIC5_SMR);
> +	irq_gc_unlock(bgc);
>  
>  	return ret;
>  }
> 


-- 
Nicolas Ferre

[PATCH 3/3] irqchip: atmel-aic5: simplify base chip selection

[Nicolas Ferre]

Le 21/09/2015 15:46, Ludovic Desroches a ?crit :
> Use irq_get_domain_generic_chip() to select the base chip.
> 
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

> ---
>  drivers/irqchip/irq-atmel-aic5.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c
> index abff79e..7264ec7 100644
> --- a/drivers/irqchip/irq-atmel-aic5.c
> +++ b/drivers/irqchip/irq-atmel-aic5.c
> @@ -70,8 +70,7 @@ static struct irq_domain *aic5_domain;
>  static asmlinkage void __exception_irq_entry
>  aic5_handle(struct pt_regs *regs)
>  {
> -	struct irq_domain_chip_generic *dgc = aic5_domain->gc;
> -	struct irq_chip_generic *bgc = dgc->gc[0];
> +	struct irq_chip_generic *bgc = irq_get_domain_generic_chip(aic5_domain, 0);
>  	u32 irqnr;
>  	u32 irqstat;
>  
> @@ -87,8 +86,7 @@ aic5_handle(struct pt_regs *regs)
>  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 *bgc = dgc->gc[0];
> +	struct irq_chip_generic *bgc = irq_get_domain_generic_chip(domain, 0);
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>  
>  	/* Disable interrupt on AIC5 */
> @@ -102,8 +100,7 @@ static void aic5_mask(struct irq_data *d)
>  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 *bgc = dgc->gc[0];
> +	struct irq_chip_generic *bgc = irq_get_domain_generic_chip(domain, 0);
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>  
>  	/* Enable interrupt on AIC5 */
> @@ -117,8 +114,7 @@ static void aic5_unmask(struct irq_data *d)
>  static int aic5_retrigger(struct irq_data *d)
>  {
>  	struct irq_domain *domain = d->domain;
> -	struct irq_domain_chip_generic *dgc = domain->gc;
> -	struct irq_chip_generic *bgc = dgc->gc[0];
> +	struct irq_chip_generic *bgc = irq_get_domain_generic_chip(domain, 0);
>  
>  	/* Enable interrupt on AIC5 */
>  	irq_gc_lock(bgc);
> @@ -132,8 +128,7 @@ static int aic5_retrigger(struct irq_data *d)
>  static int aic5_set_type(struct irq_data *d, unsigned type)
>  {
>  	struct irq_domain *domain = d->domain;
> -	struct irq_domain_chip_generic *dgc = domain->gc;
> -	struct irq_chip_generic *bgc = dgc->gc[0];
> +	struct irq_chip_generic *bgc = irq_get_domain_generic_chip(domain, 0);
>  	unsigned int smr;
>  	int ret;
>  
> @@ -153,7 +148,7 @@ static void aic5_suspend(struct irq_data *d)
>  {
>  	struct irq_domain *domain = d->domain;
>  	struct irq_domain_chip_generic *dgc = domain->gc;
> -	struct irq_chip_generic *bgc = dgc->gc[0];
> +	struct irq_chip_generic *bgc = irq_get_domain_generic_chip(domain, 0);
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>  	int i;
>  	u32 mask;
> @@ -177,7 +172,7 @@ static void aic5_resume(struct irq_data *d)
>  {
>  	struct irq_domain *domain = d->domain;
>  	struct irq_domain_chip_generic *dgc = domain->gc;
> -	struct irq_chip_generic *bgc = dgc->gc[0];
> +	struct irq_chip_generic *bgc = irq_get_domain_generic_chip(domain, 0);
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>  	int i;
>  	u32 mask;
> @@ -201,7 +196,7 @@ static void aic5_pm_shutdown(struct irq_data *d)
>  {
>  	struct irq_domain *domain = d->domain;
>  	struct irq_domain_chip_generic *dgc = domain->gc;
> -	struct irq_chip_generic *bgc = dgc->gc[0];
> +	struct irq_chip_generic *bgc = irq_get_domain_generic_chip(domain, 0);
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>  	int i;
>  
> @@ -256,12 +251,11 @@ static int aic5_irq_domain_xlate(struct irq_domain *d,
>  				 irq_hw_number_t *out_hwirq,
>  				 unsigned int *out_type)
>  {
> -	struct irq_domain_chip_generic *dgc = d->gc;
> -	struct irq_chip_generic *bgc;
> +	struct irq_chip_generic *bgc = irq_get_domain_generic_chip(d, 0);
>  	unsigned smr;
>  	int ret;
>  
> -	if (!dgc)
> +	if (!bgc)
>  		return -EINVAL;
>  
>  	ret = aic_common_irq_domain_xlate(d, ctrlr, intspec, intsize,
> @@ -269,8 +263,6 @@ static int aic5_irq_domain_xlate(struct irq_domain *d,
>  	if (ret)
>  		return ret;
>  
> -	bgc = dgc->gc[0];
> -
>  	irq_gc_lock(bgc);
>  	irq_reg_writel(bgc, *out_hwirq, AT91_AIC5_SSR);
>  	smr = irq_reg_readl(bgc, AT91_AIC5_SMR);
> 


-- 
Nicolas Ferre

[PATCH 1/3] irqchip: atmel-aic5: fix bug with mask/unmask

[Thomas Gleixner]

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

[tip:irq/urgent] irqchip/atmel-aic5: Use proper mask cache in mask/unmask()

[Thomas Gleixner]

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

[PATCH 1/3] irqchip: atmel-aic5: fix bug with mask/unmask

[Boris Brezillon]

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



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 1/3] irqchip: atmel-aic5: fix bug with mask/unmask

[Thomas Gleixner]

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

[PATCH 1/3] irqchip: atmel-aic5: fix bug with mask/unmask

[Ludovic Desroches]

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