[RESEND PATCH v2 0/3] Replace irq_gc_mask_disable_reg_and_ack

[RESEND PATCH v2 0/3] Replace irq_gc_mask_disable_reg_and_ack

[Marc Gonzalez]

Hello Thomas,

As we discussed at Kernel Recipes, I am re-sending the part of
Doug Berger's patch series that deals with a serious issue in
drivers/irqchip/irq-tango.c

The original name of the relevant patch series is:
[PATCH v2 0/6] Add support for BCM7271 style interrupt controller

Doug, I hope you won't mind me doing so.


Doug Berger (2):
  genirq: generic chip: add irq_gc_mask_disable_and_ack_set()
  genirq: generic chip: remove irq_gc_mask_disable_reg_and_ack()

Florian Fainelli (1):
  irqchip/tango: Use irq_gc_mask_disable_and_ack_set

 drivers/irqchip/irq-tango.c |  2 +-
 include/linux/irq.h         |  2 +-
 kernel/irq/generic-chip.c   | 15 ++++++++++++---
 3 files changed, 14 insertions(+), 5 deletions(-)



History of this patch series:
(v4 dropped the 3 patches included in this re-send)

[PATCH 0/6] Add support for BCM7271 style interrupt controller
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1439946.html

https://patchwork.kernel.org/patch/9831047/
https://patchwork.kernel.org/patch/9831045/
https://patchwork.kernel.org/patch/9831049/


[PATCH v2 0/6] Add support for BCM7271 style interrupt controller
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1449017.html

https://patchwork.kernel.org/patch/9853021/
https://patchwork.kernel.org/patch/9853035/
https://patchwork.kernel.org/patch/9853039/


[PATCH v4 0/3] Add support for BCM7271 style interrupt controller
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1493152.html

[RESEND PATCH v2 1/3] genirq: generic chip: add irq_gc_mask_disable_and_ack_set()

[Marc Gonzalez]

From: Doug Berger <opendmb@gmail.com>

The irq_gc_mask_disable_reg_and_ack() function name implies that it
provides the combined functions of irq_gc_mask_disable_reg() and
irq_gc_ack().  However, the implementation does not actually do
that since it writes the mask instead of the disable register. It
also does not maintain the mask cache which makes it inappropriate
to use with other masking functions.

In addition, commit 659fb32d1b67 ("genirq: replace irq_gc_ack() with
{set,clr}_bit variants (fwd)") effectively renamed irq_gc_ack() to
irq_gc_ack_set_bit() so this function probably should have also been
renamed at that time.

The generic chip code currently provides three functions for use
with the irq_mask member of the irq_chip structure and two functions
for use with the irq_ack member of the irq_chip structure. These
functions could be combined into six functions for use with the
irq_mask_ack member of the irq_chip structure.  However, since only
one of the combinations is currently used, only the function
irq_gc_mask_disable_and_ack_set() is added by this commit.

The '_reg' and '_bit' portions of the base function name were left
out of the new combined function name in an attempt to keep the
function name length manageable with the 80 character source code
line length while still allowing the distinct aspects of each
combination to be captured by the name.

If other combinations are desired in the future please add them to
the irq generic chip library at that time.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 include/linux/irq.h       |  1 +
 kernel/irq/generic-chip.c | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index d4728bf6a537..494d328f7051 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -1010,6 +1010,7 @@ void irq_gc_unmask_enable_reg(struct irq_data *d);
 void irq_gc_ack_set_bit(struct irq_data *d);
 void irq_gc_ack_clr_bit(struct irq_data *d);
 void irq_gc_mask_disable_reg_and_ack(struct irq_data *d);
+void irq_gc_mask_disable_and_ack_set(struct irq_data *d);
 void irq_gc_eoi(struct irq_data *d);
 int irq_gc_set_wake(struct irq_data *d, unsigned int on);
 
diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index 5270a54b9fa4..ec5fe9a0cb05 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -151,6 +151,31 @@ void irq_gc_mask_disable_reg_and_ack(struct irq_data *d)
 }
 
 /**
+ * irq_gc_mask_disable_and_ack_set - Mask and ack pending interrupt
+ * @d: irq_data
+ *
+ * This generic implementation of the irq_mask_ack method is for chips
+ * with separate enable/disable registers instead of a single mask
+ * register and where a pending interrupt is acknowledged by setting a
+ * bit.
+ *
+ * Note: This is the only permutation currently used.  Similar generic
+ * functions should be added here if other permutations are required.
+ */
+void irq_gc_mask_disable_and_ack_set(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	u32 mask = d->mask;
+
+	irq_gc_lock(gc);
+	irq_reg_writel(gc, mask, ct->regs.disable);
+	*ct->mask_cache &= ~mask;
+	irq_reg_writel(gc, mask, ct->regs.ack);
+	irq_gc_unlock(gc);
+}
+
+/**
  * irq_gc_eoi - EOI interrupt
  * @d: irq_data
  */
-- 
2.11.0

[RESEND PATCH v2 2/3] irqchip/tango: Use irq_gc_mask_disable_and_ack_set

[Marc Gonzalez]

From: Florian Fainelli <f.fainelli@gmail.com>

The only usage of the irq_gc_mask_disable_reg_and_ack() function
is by the Tango irqchip driver. This usage is replaced by the
irq_gc_mask_disable_and_ack_set() function since it provides the
intended functionality.

Fixes: 4bba66899ac6 ("irqchip/tango: Add support for Sigma Designs SMP86xx/SMP87xx interrupt controller")
Acked-by: Mans Rullgard <mans@mansr.com>
Acked-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/irqchip/irq-tango.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
index bdbb5c0ff7fe..0c085303a583 100644
--- a/drivers/irqchip/irq-tango.c
+++ b/drivers/irqchip/irq-tango.c
@@ -141,7 +141,7 @@ static void __init tangox_irq_init_chip(struct irq_chip_generic *gc,
 	for (i = 0; i < 2; i++) {
 		ct[i].chip.irq_ack = irq_gc_ack_set_bit;
 		ct[i].chip.irq_mask = irq_gc_mask_disable_reg;
-		ct[i].chip.irq_mask_ack = irq_gc_mask_disable_reg_and_ack;
+		ct[i].chip.irq_mask_ack = irq_gc_mask_disable_and_ack_set;
 		ct[i].chip.irq_unmask = irq_gc_unmask_enable_reg;
 		ct[i].chip.irq_set_type = tangox_irq_set_type;
 		ct[i].chip.name = gc->domain->name;
-- 
2.11.0

[RESEND PATCH v2 3/3] genirq: generic chip: remove irq_gc_mask_disable_reg_and_ack()

[Marc Gonzalez]

From: Doug Berger <opendmb@gmail.com>

Any usage of the irq_gc_mask_disable_reg_and_ack() function has
been replaced with the desired functionality.

The incorrect and ambiguously named function is removed here to
prevent accidental misuse.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 include/linux/irq.h       |  1 -
 kernel/irq/generic-chip.c | 16 ----------------
 2 files changed, 17 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 494d328f7051..5ad10948ea95 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -1009,7 +1009,6 @@ void irq_gc_mask_clr_bit(struct irq_data *d);
 void irq_gc_unmask_enable_reg(struct irq_data *d);
 void irq_gc_ack_set_bit(struct irq_data *d);
 void irq_gc_ack_clr_bit(struct irq_data *d);
-void irq_gc_mask_disable_reg_and_ack(struct irq_data *d);
 void irq_gc_mask_disable_and_ack_set(struct irq_data *d);
 void irq_gc_eoi(struct irq_data *d);
 int irq_gc_set_wake(struct irq_data *d, unsigned int on);
diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index ec5fe9a0cb05..c26c5bb6b491 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -135,22 +135,6 @@ void irq_gc_ack_clr_bit(struct irq_data *d)
 }
 
 /**
- * irq_gc_mask_disable_reg_and_ack - Mask and ack pending interrupt
- * @d: irq_data
- */
-void irq_gc_mask_disable_reg_and_ack(struct irq_data *d)
-{
-	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	struct irq_chip_type *ct = irq_data_get_chip_type(d);
-	u32 mask = d->mask;
-
-	irq_gc_lock(gc);
-	irq_reg_writel(gc, mask, ct->regs.mask);
-	irq_reg_writel(gc, mask, ct->regs.ack);
-	irq_gc_unlock(gc);
-}
-
-/**
  * irq_gc_mask_disable_and_ack_set - Mask and ack pending interrupt
  * @d: irq_data
  *
-- 
2.11.0

Re: [RESEND PATCH v2 0/3] Replace irq_gc_mask_disable_reg_and_ack

[Marc Gonzalez]

On 04/10/2017 14:24, Marc Gonzalez wrote:

> Hello Thomas,
> 
> As we discussed at Kernel Recipes, I am re-sending the part of
> Doug Berger's patch series that deals with a serious issue in
> drivers/irqchip/irq-tango.c
> 
> The original name of the relevant patch series is:
> [PATCH v2 0/6] Add support for BCM7271 style interrupt controller
> 
> Doug, I hope you won't mind me doing so.
> 
> 
> Doug Berger (2):
>   genirq: generic chip: add irq_gc_mask_disable_and_ack_set()
>   genirq: generic chip: remove irq_gc_mask_disable_reg_and_ack()
> 
> Florian Fainelli (1):
>   irqchip/tango: Use irq_gc_mask_disable_and_ack_set
> 
>  drivers/irqchip/irq-tango.c |  2 +-
>  include/linux/irq.h         |  2 +-
>  kernel/irq/generic-chip.c   | 15 ++++++++++++---
>  3 files changed, 14 insertions(+), 5 deletions(-)

Hello Thomas,

Is something preventing this series from being merged?

It's short, straight-forward, touches only 3 files, and more
importantly, fixes an actual bug in the irq-tango.c driver.

Hopefully, Greg can then back-port the change to stable kernels.

Regards.