[PATCH v2 0/6] Add support for BCM7271 style interrupt controller

[PATCH v2 0/6] Add support for BCM7271 style interrupt controller

[Doug Berger]

This patch set extends the functionality of the irq-brcmstb-l2 interrupt
controller driver to cover a hardware variant first introduced in the
BCM7271 SoC.  The main difference between this variant and the block
found in earlier brcmstb SoCs is that this variant only supports level
sensitive interrupts and therefore does not latch the interrupt state
based on edges.  Since there is no longer a need to ack interrupts with
a register write to clear the latch the register map has been changed.

Therefore the change to add support for the new hardware block is to
abstract the register accesses to accommodate different maps and to
identify the block with a new device-tree compatible string.

I also took the opportunity to make some small efficiency enhancements
to the driver.  One of these was to make use of the slightly more
efficient irq_mask_ack method.  However, I discovered that the defined
irq_gc_mask_disable_reg_and_ack() generic irq function was insufficient
for my needs.  The first three commits of this set are intended to be a
correction of the existing generic irq implementation to provide a
function that can be used by interrupt controller drivers for their
irq_mask_ack method when disable/enable registers are used for masking
and interrupts are acknowledged by setting a bit in an ack register.

I believe these first three commits should be added to the irq/core
repository and possibly stable branches. The remaining commits should be
added to the irqchip/core repository but I have included the complete
set here for improved context since the irqchip patches are dependent on
the irq patches.  This entire set is therefore based on the irq/core
master branch.  Please let me know if you would like a different
packaging.

If the changes to genirq are not acceptable I can implement the
irq_mask_ask method locally in the irq-brcmstb-l2 driver and submit
that on its own.

Changes in v2:

- removed unused permutations of irq_mask_ack methods
- added Reviewed-by and Acked-by responses from first submission

Doug Berger (5):
  genirq: generic chip: add irq_gc_mask_disable_and_ack_set()
  genirq: generic chip: remove irq_gc_mask_disable_reg_and_ack()
  irqchip: brcmstb-l2: Remove some processing from the handler
  irqchip: brcmstb-l2: Abstract register accesses
  irqchip: brcmstb-l2: Add support for the BCM7271 L2 controller

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

 .../bindings/interrupt-controller/brcm,l2-intc.txt |   3 +-
 drivers/irqchip/irq-brcmstb-l2.c                   | 145 ++++++++++++++-------
 drivers/irqchip/irq-tango.c                        |   2 +-
 include/linux/irq.h                                |   2 +-
 kernel/irq/generic-chip.c                          |  15 ++-
 5 files changed, 114 insertions(+), 53 deletions(-)

-- 
2.13.0

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

[Doug Berger]

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 00db35b61e9e..5b27f65c47d0 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -1003,6 +1003,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 f7086b78ad6e..7f61b6e9f5ca 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.13.0

[PATCH v2 2/6] irqchip/tango: Use irq_gc_mask_disable_and_ack_set

[Doug Berger]

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")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Acked-by: Mans Rullgard <mans@mansr.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.13.0

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

[Doug Berger]

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 5b27f65c47d0..c73e2eca4abd 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -1002,7 +1002,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 7f61b6e9f5ca..49ea3347719e 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.13.0

[PATCH v2 4/6] irqchip: brcmstb-l2: Remove some processing from the handler

[Doug Berger]

Saving the generic chip pointer in the brcmstb_l2_intc_data prevents
the need to call irq_get_domain_generic_chip().  Also don't need to
save parent_irq and base there since local variables in the
brcmstb_l2_intc_of_init() function are just as good.

The handle_edge_irq flow or chained_irq_enter takes care of the
acknowledgement of the interrupt so it is redundant to clear it in
brcmstb_l2_intc_irq_handle().

irq_linear_revmap() is a fast path equivalent of irq_find_mapping()
that is appropriate to use for domain controllers of this type.

Defining irq_mask_ack is slightly more efficient than just
implementing irq_mask and irq_ack seperately.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/irqchip/irq-brcmstb-l2.c | 46 +++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/irqchip/irq-brcmstb-l2.c b/drivers/irqchip/irq-brcmstb-l2.c
index bddf169c4b37..977ae55d47d4 100644
--- a/drivers/irqchip/irq-brcmstb-l2.c
+++ b/drivers/irqchip/irq-brcmstb-l2.c
@@ -1,7 +1,7 @@
 /*
  * Generic Broadcom Set Top Box Level 2 Interrupt controller driver
  *
- * Copyright (C) 2014 Broadcom Corporation
+ * Copyright (C) 2014-2017 Broadcom
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -41,9 +41,8 @@
 
 /* L2 intc private data structure */
 struct brcmstb_l2_intc_data {
-	int parent_irq;
-	void __iomem *base;
 	struct irq_domain *domain;
+	struct irq_chip_generic *gc;
 	bool can_wake;
 	u32 saved_mask; /* for suspend/resume */
 };
@@ -51,15 +50,14 @@ struct brcmstb_l2_intc_data {
 static void brcmstb_l2_intc_irq_handle(struct irq_desc *desc)
 {
 	struct brcmstb_l2_intc_data *b = irq_desc_get_handler_data(desc);
-	struct irq_chip_generic *gc = irq_get_domain_generic_chip(b->domain, 0);
 	struct irq_chip *chip = irq_desc_get_chip(desc);
 	unsigned int irq;
 	u32 status;
 
 	chained_irq_enter(chip, desc);
 
-	status = irq_reg_readl(gc, CPU_STATUS) &
-		~(irq_reg_readl(gc, CPU_MASK_STATUS));
+	status = irq_reg_readl(b->gc, CPU_STATUS) &
+		~(irq_reg_readl(b->gc, CPU_MASK_STATUS));
 
 	if (status == 0) {
 		raw_spin_lock(&desc->lock);
@@ -70,10 +68,8 @@ static void brcmstb_l2_intc_irq_handle(struct irq_desc *desc)
 
 	do {
 		irq = ffs(status) - 1;
-		/* ack at our level */
-		irq_reg_writel(gc, 1 << irq, CPU_CLEAR);
 		status &= ~(1 << irq);
-		generic_handle_irq(irq_find_mapping(b->domain, irq));
+		generic_handle_irq(irq_linear_revmap(b->domain, irq));
 	} while (status);
 out:
 	chained_irq_exit(chip, desc);
@@ -116,32 +112,33 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
 {
 	unsigned int clr = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN;
 	struct brcmstb_l2_intc_data *data;
-	struct irq_chip_generic *gc;
 	struct irq_chip_type *ct;
 	int ret;
 	unsigned int flags;
+	int parent_irq;
+	void __iomem *base;
 
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	data->base = of_iomap(np, 0);
-	if (!data->base) {
+	base = of_iomap(np, 0);
+	if (!base) {
 		pr_err("failed to remap intc L2 registers\n");
 		ret = -ENOMEM;
 		goto out_free;
 	}
 
 	/* Disable all interrupts by default */
-	writel(0xffffffff, data->base + CPU_MASK_SET);
+	writel(0xffffffff, base + CPU_MASK_SET);
 
 	/* Wakeup interrupts may be retained from S5 (cold boot) */
 	data->can_wake = of_property_read_bool(np, "brcm,irq-can-wake");
 	if (!data->can_wake)
-		writel(0xffffffff, data->base + CPU_CLEAR);
+		writel(0xffffffff, base + CPU_CLEAR);
 
-	data->parent_irq = irq_of_parse_and_map(np, 0);
-	if (!data->parent_irq) {
+	parent_irq = irq_of_parse_and_map(np, 0);
+	if (!parent_irq) {
 		pr_err("failed to find parent interrupt\n");
 		ret = -EINVAL;
 		goto out_unmap;
@@ -170,18 +167,19 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
 	}
 
 	/* Set the IRQ chaining logic */
-	irq_set_chained_handler_and_data(data->parent_irq,
+	irq_set_chained_handler_and_data(parent_irq,
 					 brcmstb_l2_intc_irq_handle, data);
 
-	gc = irq_get_domain_generic_chip(data->domain, 0);
-	gc->reg_base = data->base;
-	gc->private = data;
-	ct = gc->chip_types;
+	data->gc = irq_get_domain_generic_chip(data->domain, 0);
+	data->gc->reg_base = base;
+	data->gc->private = data;
+	ct = data->gc->chip_types;
 
 	ct->chip.irq_ack = irq_gc_ack_set_bit;
 	ct->regs.ack = CPU_CLEAR;
 
 	ct->chip.irq_mask = irq_gc_mask_disable_reg;
+	ct->chip.irq_mask_ack = irq_gc_mask_disable_and_ack_set;
 	ct->regs.disable = CPU_MASK_SET;
 
 	ct->chip.irq_unmask = irq_gc_unmask_enable_reg;
@@ -194,19 +192,19 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
 		/* This IRQ chip can wake the system, set all child interrupts
 		 * in wake_enabled mask
 		 */
-		gc->wake_enabled = 0xffffffff;
+		data->gc->wake_enabled = 0xffffffff;
 		ct->chip.irq_set_wake = irq_gc_set_wake;
 	}
 
 	pr_info("registered L2 intc (mem: 0x%p, parent irq: %d)\n",
-			data->base, data->parent_irq);
+			base, parent_irq);
 
 	return 0;
 
 out_free_domain:
 	irq_domain_remove(data->domain);
 out_unmap:
-	iounmap(data->base);
+	iounmap(base);
 out_free:
 	kfree(data);
 	return ret;
-- 
2.13.0

[PATCH v2 5/6] irqchip: brcmstb-l2: Abstract register accesses

[Doug Berger]

Added register block offsets to the brcmstb_l2_intc_data structure
for the status and mask registers to support reading the active
interupts in an abstracted way.  It seems like an irq_chip method
should have been provided for this, but it's not there yet.

Abstracted the implementation of the handler, suspend, and resume
functions to not use any hard coded register offsets.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/irqchip/irq-brcmstb-l2.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/irqchip/irq-brcmstb-l2.c b/drivers/irqchip/irq-brcmstb-l2.c
index 977ae55d47d4..ce3850530e2b 100644
--- a/drivers/irqchip/irq-brcmstb-l2.c
+++ b/drivers/irqchip/irq-brcmstb-l2.c
@@ -43,6 +43,8 @@
 struct brcmstb_l2_intc_data {
 	struct irq_domain *domain;
 	struct irq_chip_generic *gc;
+	int status_offset;
+	int mask_offset;
 	bool can_wake;
 	u32 saved_mask; /* for suspend/resume */
 };
@@ -56,8 +58,8 @@ static void brcmstb_l2_intc_irq_handle(struct irq_desc *desc)
 
 	chained_irq_enter(chip, desc);
 
-	status = irq_reg_readl(b->gc, CPU_STATUS) &
-		~(irq_reg_readl(b->gc, CPU_MASK_STATUS));
+	status = irq_reg_readl(b->gc, b->status_offset) &
+		~(irq_reg_readl(b->gc, b->mask_offset));
 
 	if (status == 0) {
 		raw_spin_lock(&desc->lock);
@@ -78,16 +80,17 @@ static void brcmstb_l2_intc_irq_handle(struct irq_desc *desc)
 static void brcmstb_l2_intc_suspend(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);
 	struct brcmstb_l2_intc_data *b = gc->private;
 
 	irq_gc_lock(gc);
 	/* Save the current mask */
-	b->saved_mask = irq_reg_readl(gc, CPU_MASK_STATUS);
+	b->saved_mask = irq_reg_readl(gc, ct->regs.mask);
 
 	if (b->can_wake) {
 		/* Program the wakeup mask */
-		irq_reg_writel(gc, ~gc->wake_active, CPU_MASK_SET);
-		irq_reg_writel(gc, gc->wake_active, CPU_MASK_CLEAR);
+		irq_reg_writel(gc, ~gc->wake_active, ct->regs.disable);
+		irq_reg_writel(gc, gc->wake_active, ct->regs.enable);
 	}
 	irq_gc_unlock(gc);
 }
@@ -95,15 +98,19 @@ static void brcmstb_l2_intc_suspend(struct irq_data *d)
 static void brcmstb_l2_intc_resume(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);
 	struct brcmstb_l2_intc_data *b = gc->private;
 
 	irq_gc_lock(gc);
-	/* Clear unmasked non-wakeup interrupts */
-	irq_reg_writel(gc, ~b->saved_mask & ~gc->wake_active, CPU_CLEAR);
+	if (ct->chip.irq_ack != irq_gc_noop) {
+		/* Clear unmasked non-wakeup interrupts */
+		irq_reg_writel(gc, ~b->saved_mask & ~gc->wake_active,
+				ct->regs.ack);
+	}
 
 	/* Restore the saved mask */
-	irq_reg_writel(gc, b->saved_mask, CPU_MASK_SET);
-	irq_reg_writel(gc, ~b->saved_mask, CPU_MASK_CLEAR);
+	irq_reg_writel(gc, b->saved_mask, ct->regs.disable);
+	irq_reg_writel(gc, ~b->saved_mask, ct->regs.enable);
 	irq_gc_unlock(gc);
 }
 
@@ -173,6 +180,9 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
 	data->gc = irq_get_domain_generic_chip(data->domain, 0);
 	data->gc->reg_base = base;
 	data->gc->private = data;
+	data->status_offset = CPU_STATUS;
+	data->mask_offset = CPU_MASK_STATUS;
+
 	ct = data->gc->chip_types;
 
 	ct->chip.irq_ack = irq_gc_ack_set_bit;
@@ -181,6 +191,7 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
 	ct->chip.irq_mask = irq_gc_mask_disable_reg;
 	ct->chip.irq_mask_ack = irq_gc_mask_disable_and_ack_set;
 	ct->regs.disable = CPU_MASK_SET;
+	ct->regs.mask = CPU_MASK_STATUS;
 
 	ct->chip.irq_unmask = irq_gc_unmask_enable_reg;
 	ct->regs.enable = CPU_MASK_CLEAR;
-- 
2.13.0

[PATCH v2 6/6] irqchip: brcmstb-l2: Add support for the BCM7271 L2 controller

[Doug Berger]

Add the initialization of the generic irq chip for the BCM7271 L2
interrupt controller.  This controller only supports level
interrupts and uses the "brcm,bcm7271-l2-intc" compatibility
string.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 .../bindings/interrupt-controller/brcm,l2-intc.txt |  3 +-
 drivers/irqchip/irq-brcmstb-l2.c                   | 86 ++++++++++++++++------
 2 files changed, 66 insertions(+), 23 deletions(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/brcm,l2-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/brcm,l2-intc.txt
index 448273a30a11..36df06c5c567 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/brcm,l2-intc.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/brcm,l2-intc.txt
@@ -2,7 +2,8 @@ Broadcom Generic Level 2 Interrupt Controller
 
 Required properties:
 
-- compatible: should be "brcm,l2-intc"
+- compatible: should be "brcm,l2-intc" for latched interrupt controllers
+              should be "brcm,bcm7271-l2-intc" for level interrupt controllers
 - reg: specifies the base physical address and size of the registers
 - interrupt-controller: identifies the node as an interrupt controller
 - #interrupt-cells: specifies the number of cells needed to encode an
diff --git a/drivers/irqchip/irq-brcmstb-l2.c b/drivers/irqchip/irq-brcmstb-l2.c
index ce3850530e2b..f77e6c9530dc 100644
--- a/drivers/irqchip/irq-brcmstb-l2.c
+++ b/drivers/irqchip/irq-brcmstb-l2.c
@@ -31,13 +31,34 @@
 #include <linux/irqchip.h>
 #include <linux/irqchip/chained_irq.h>
 
-/* Register offsets in the L2 interrupt controller */
-#define CPU_STATUS	0x00
-#define CPU_SET		0x04
-#define CPU_CLEAR	0x08
-#define CPU_MASK_STATUS	0x0c
-#define CPU_MASK_SET	0x10
-#define CPU_MASK_CLEAR	0x14
+struct brcmstb_intc_init_params {
+	irq_flow_handler_t handler;
+	int cpu_status;
+	int cpu_clear;
+	int cpu_mask_status;
+	int cpu_mask_set;
+	int cpu_mask_clear;
+};
+
+/* Register offsets in the L2 latched interrupt controller */
+static const struct brcmstb_intc_init_params l2_edge_intc_init = {
+	.handler		= handle_edge_irq,
+	.cpu_status		= 0x00,
+	.cpu_clear		= 0x08,
+	.cpu_mask_status	= 0x0c,
+	.cpu_mask_set		= 0x10,
+	.cpu_mask_clear		= 0x14
+};
+
+/* Register offsets in the L2 level interrupt controller */
+static const struct brcmstb_intc_init_params l2_lvl_intc_init = {
+	.handler		= handle_level_irq,
+	.cpu_status		= 0x00,
+	.cpu_clear		= -1, /* Register not present */
+	.cpu_mask_status	= 0x04,
+	.cpu_mask_set		= 0x08,
+	.cpu_mask_clear		= 0x0C
+};
 
 /* L2 intc private data structure */
 struct brcmstb_l2_intc_data {
@@ -102,7 +123,7 @@ static void brcmstb_l2_intc_resume(struct irq_data *d)
 	struct brcmstb_l2_intc_data *b = gc->private;
 
 	irq_gc_lock(gc);
-	if (ct->chip.irq_ack != irq_gc_noop) {
+	if (ct->chip.irq_ack) {
 		/* Clear unmasked non-wakeup interrupts */
 		irq_reg_writel(gc, ~b->saved_mask & ~gc->wake_active,
 				ct->regs.ack);
@@ -115,7 +136,9 @@ static void brcmstb_l2_intc_resume(struct irq_data *d)
 }
 
 static int __init brcmstb_l2_intc_of_init(struct device_node *np,
-					  struct device_node *parent)
+					  struct device_node *parent,
+					  const struct brcmstb_intc_init_params
+					  *init_params)
 {
 	unsigned int clr = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN;
 	struct brcmstb_l2_intc_data *data;
@@ -137,12 +160,12 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
 	}
 
 	/* Disable all interrupts by default */
-	writel(0xffffffff, base + CPU_MASK_SET);
+	writel(0xffffffff, base + init_params->cpu_mask_set);
 
 	/* Wakeup interrupts may be retained from S5 (cold boot) */
 	data->can_wake = of_property_read_bool(np, "brcm,irq-can-wake");
-	if (!data->can_wake)
-		writel(0xffffffff, base + CPU_CLEAR);
+	if (!data->can_wake && (init_params->cpu_clear >= 0))
+		writel(0xffffffff, base + init_params->cpu_clear);
 
 	parent_irq = irq_of_parse_and_map(np, 0);
 	if (!parent_irq) {
@@ -167,7 +190,7 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
 
 	/* Allocate a single Generic IRQ chip for this node */
 	ret = irq_alloc_domain_generic_chips(data->domain, 32, 1,
-				np->full_name, handle_edge_irq, clr, 0, flags);
+			np->full_name, init_params->handler, clr, 0, flags);
 	if (ret) {
 		pr_err("failed to allocate generic irq chip\n");
 		goto out_free_domain;
@@ -180,21 +203,26 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
 	data->gc = irq_get_domain_generic_chip(data->domain, 0);
 	data->gc->reg_base = base;
 	data->gc->private = data;
-	data->status_offset = CPU_STATUS;
-	data->mask_offset = CPU_MASK_STATUS;
+	data->status_offset = init_params->cpu_status;
+	data->mask_offset = init_params->cpu_mask_status;
 
 	ct = data->gc->chip_types;
 
-	ct->chip.irq_ack = irq_gc_ack_set_bit;
-	ct->regs.ack = CPU_CLEAR;
+	if (init_params->cpu_clear >= 0) {
+		ct->regs.ack = init_params->cpu_clear;
+		ct->chip.irq_ack = irq_gc_ack_set_bit;
+		ct->chip.irq_mask_ack = irq_gc_mask_disable_and_ack_set;
+	} else {
+		/* No Ack - but still slightly more efficient to define this */
+		ct->chip.irq_mask_ack = irq_gc_mask_disable_reg;
+	}
 
 	ct->chip.irq_mask = irq_gc_mask_disable_reg;
-	ct->chip.irq_mask_ack = irq_gc_mask_disable_and_ack_set;
-	ct->regs.disable = CPU_MASK_SET;
-	ct->regs.mask = CPU_MASK_STATUS;
+	ct->regs.disable = init_params->cpu_mask_set;
+	ct->regs.mask = init_params->cpu_mask_status;
 
 	ct->chip.irq_unmask = irq_gc_unmask_enable_reg;
-	ct->regs.enable = CPU_MASK_CLEAR;
+	ct->regs.enable = init_params->cpu_mask_clear;
 
 	ct->chip.irq_suspend = brcmstb_l2_intc_suspend;
 	ct->chip.irq_resume = brcmstb_l2_intc_resume;
@@ -220,4 +248,18 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
 	kfree(data);
 	return ret;
 }
-IRQCHIP_DECLARE(brcmstb_l2_intc, "brcm,l2-intc", brcmstb_l2_intc_of_init);
+
+int __init brcmstb_l2_edge_intc_of_init(struct device_node *np,
+	struct device_node *parent)
+{
+	return brcmstb_l2_intc_of_init(np, parent, &l2_edge_intc_init);
+}
+IRQCHIP_DECLARE(brcmstb_l2_intc, "brcm,l2-intc", brcmstb_l2_edge_intc_of_init);
+
+int __init brcmstb_l2_lvl_intc_of_init(struct device_node *np,
+	struct device_node *parent)
+{
+	return brcmstb_l2_intc_of_init(np, parent, &l2_lvl_intc_init);
+}
+IRQCHIP_DECLARE(bcm7271_l2_intc, "brcm,bcm7271-l2-intc",
+	brcmstb_l2_lvl_intc_of_init);
-- 
2.13.0

Re: [PATCH v2 2/6] irqchip/tango: Use irq_gc_mask_disable_and_ack_set

[Marc Gonzalez]

[ Trimming CC list ]

On 19/07/2017 21:07, Doug Berger wrote:

> 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")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> Acked-by: Mans Rullgard <mans@mansr.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;
> 

(I had a look only at patches 1 and 2)

irq_gc_mask_disable_reg() =
	irq_reg_writel(gc, mask, ct->regs.disable);
	*ct->mask_cache &= ~mask;

irq_gc_ack_set_bit() =
	irq_reg_writel(gc, mask, ct->regs.ack);


irq_gc_mask_disable_reg_and_ack() =
	irq_reg_writel(gc, mask, ct->regs.mask);  // regs.mask not defined for tango driver
	irq_reg_writel(gc, mask, ct->regs.ack);

It will try to write at offset 0, which is a read-only register
so no disaster, but interrupt is not disabled. I wonder why
everything doesn't blow up...

irq_gc_mask_disable_and_ack_set() =
	irq_reg_writel(gc, mask, ct->regs.disable);
	*ct->mask_cache &= ~mask;
	irq_reg_writel(gc, mask, ct->regs.ack);


Your change makes sense.

But note that mask_ack_irq() is defined as:

static inline void mask_ack_irq(struct irq_desc *desc)
{
	if (desc->irq_data.chip->irq_mask_ack) {
		desc->irq_data.chip->irq_mask_ack(&desc->irq_data);
		irq_state_set_masked(desc);
	} else {
		mask_irq(desc);
		if (desc->irq_data.chip->irq_ack)
			desc->irq_data.chip->irq_ack(&desc->irq_data);
	}
}

So IIUC, if we don't define a irq_mask_ack() callback,
it will just call irq_mask() and irq_ack() which will
do the right thing.

So an alternative is simply to delete the assignment
of ct[i].chip.irq_mask_ack. What do you think?

diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
index bdbb5c0ff7fe..825085cdab99 100644
--- a/drivers/irqchip/irq-tango.c
+++ b/drivers/irqchip/irq-tango.c
@@ -141,7 +141,6 @@ 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_unmask = irq_gc_unmask_enable_reg;
                ct[i].chip.irq_set_type = tangox_irq_set_type;
                ct[i].chip.name = gc->domain->name;


One thing I'm not sure about is why we need to ack for level
interrupts... We should be able to get away with defining ack()
only for edge interrupts... I'm confused. If I try to do that,
I get:

[    1.430547] irq 20: nobody cared (try booting with the "irqpoll" option)
[    1.437283] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.13.0-rc1 #148
[    1.443754] Hardware name: Sigma Tango DT
[    1.447812] [<c010e890>] (unwind_backtrace) from [<c010b01c>] (show_stack+0x10/0x14)
[    1.455598] [<c010b01c>] (show_stack) from [<c04a3fc4>] (dump_stack+0x84/0x98)
[    1.462864] [<c04a3fc4>] (dump_stack) from [<c0159d80>] (__report_bad_irq+0x28/0xcc)
[    1.470650] [<c0159d80>] (__report_bad_irq) from [<c015a188>] (note_interrupt+0x28c/0x2dc)
[    1.478959] [<c015a188>] (note_interrupt) from [<c0157688>] (handle_irq_event_percpu+0x4c/0x58)
[    1.487702] [<c0157688>] (handle_irq_event_percpu) from [<c01576cc>] (handle_irq_event+0x38/0x5c)
[    1.496621] [<c01576cc>] (handle_irq_event) from [<c015aa84>] (handle_level_irq+0xa8/0x124)
[    1.505017] [<c015aa84>] (handle_level_irq) from [<c01568bc>] (generic_handle_irq+0x24/0x34)
[    1.513501] [<c01568bc>] (generic_handle_irq) from [<c02f9dc0>] (tangox_dispatch_irqs+0x4c/0x58)
[    1.522333] [<c02f9dc0>] (tangox_dispatch_irqs) from [<c02f9e18>] (tangox_irq_handler+0x4c/0xb0)
[    1.531163] [<c02f9e18>] (tangox_irq_handler) from [<c01568bc>] (generic_handle_irq+0x24/0x34)
[    1.539819] [<c01568bc>] (generic_handle_irq) from [<c0156df4>] (__handle_domain_irq+0x5c/0xb4)
[    1.548562] [<c0156df4>] (__handle_domain_irq) from [<c0101450>] (gic_handle_irq+0x48/0x8c)
[    1.556956] [<c0101450>] (gic_handle_irq) from [<c010bb4c>] (__irq_svc+0x6c/0xa8)


Regards.

Re: [PATCH v2 2/6] irqchip/tango: Use irq_gc_mask_disable_and_ack_set

[Doug Berger]

On 07/24/2017 09:40 AM, Marc Gonzalez wrote:
> [ Trimming CC list ]
> 
> On 19/07/2017 21:07, Doug Berger wrote:
> 
>> 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")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> Acked-by: Mans Rullgard <mans@mansr.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;
>>
> 
> (I had a look only at patches 1 and 2)
> 
> irq_gc_mask_disable_reg() =
> 	irq_reg_writel(gc, mask, ct->regs.disable);
> 	*ct->mask_cache &= ~mask;
> 
> irq_gc_ack_set_bit() =
> 	irq_reg_writel(gc, mask, ct->regs.ack);
> 
> 
> irq_gc_mask_disable_reg_and_ack() =
> 	irq_reg_writel(gc, mask, ct->regs.mask);  // regs.mask not defined for tango driver
> 	irq_reg_writel(gc, mask, ct->regs.ack);
> 
> It will try to write at offset 0, which is a read-only register
> so no disaster, but interrupt is not disabled. I wonder why
> everything doesn't blow up...
> 
> irq_gc_mask_disable_and_ack_set() =
> 	irq_reg_writel(gc, mask, ct->regs.disable);
> 	*ct->mask_cache &= ~mask;
> 	irq_reg_writel(gc, mask, ct->regs.ack);
> 
> 
> Your change makes sense.
> 
> But note that mask_ack_irq() is defined as:
> 
> static inline void mask_ack_irq(struct irq_desc *desc)
> {
> 	if (desc->irq_data.chip->irq_mask_ack) {
> 		desc->irq_data.chip->irq_mask_ack(&desc->irq_data);
> 		irq_state_set_masked(desc);
> 	} else {
> 		mask_irq(desc);
> 		if (desc->irq_data.chip->irq_ack)
> 			desc->irq_data.chip->irq_ack(&desc->irq_data);
> 	}
> }
> 
> So IIUC, if we don't define a irq_mask_ack() callback,
> it will just call irq_mask() and irq_ack() which will
> do the right thing.
> 
> So an alternative is simply to delete the assignment
> of ct[i].chip.irq_mask_ack. What do you think?
> 

Yes, you understand correctly.  The irq_mask_ack method is entirely
optional and I assume that is why this issue went undetected for so
long; however, it is slightly more efficient to combine the functions
(even if the ack is unnecessary) which is why I chose to do so for my
changes to the irqchip-brcmstb-l2 driver where I first discovered this
issue.  How much value the improved efficiency has is certainly
debatable, but interrupt handling is one area where people might care
about such a small difference.  As the irqchip-tango driver maintainer
you are welcome to decide whether or not the irq_mask_ack method makes
sense to you.

I wanted to use the method for the irqchip-brcmstb-l2 driver which is
based on the irq generic chip implementation.  I discovered that the
irq_gc_mask_disable_reg_and_ack() function was not the function I needed
despite the name and that it appeared not to be the function the
irqchip-tango driver needed as well.  My desire here was to correct that
error and provide a standard set of generic implementations of
irq_mask_ack so that other drivers could avoid a similar mistake in an
attempt to provide a service to the community.  That was pared down at
Thomas Gleixner's request to correcting just the one implementation that
I wanted to use.

> diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
> index bdbb5c0ff7fe..825085cdab99 100644
> --- a/drivers/irqchip/irq-tango.c
> +++ b/drivers/irqchip/irq-tango.c
> @@ -141,7 +141,6 @@ 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_unmask = irq_gc_unmask_enable_reg;
>                 ct[i].chip.irq_set_type = tangox_irq_set_type;
>                 ct[i].chip.name = gc->domain->name;
> 
> 
> One thing I'm not sure about is why we need to ack for level
> interrupts... We should be able to get away with defining ack()
> only for edge interrupts... I'm confused. If I try to do that,
> I get:
> 
> [    1.430547] irq 20: nobody cared (try booting with the "irqpoll" option)
> [    1.437283] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.13.0-rc1 #148
> [    1.443754] Hardware name: Sigma Tango DT
> [    1.447812] [<c010e890>] (unwind_backtrace) from [<c010b01c>] (show_stack+0x10/0x14)
> [    1.455598] [<c010b01c>] (show_stack) from [<c04a3fc4>] (dump_stack+0x84/0x98)
> [    1.462864] [<c04a3fc4>] (dump_stack) from [<c0159d80>] (__report_bad_irq+0x28/0xcc)
> [    1.470650] [<c0159d80>] (__report_bad_irq) from [<c015a188>] (note_interrupt+0x28c/0x2dc)
> [    1.478959] [<c015a188>] (note_interrupt) from [<c0157688>] (handle_irq_event_percpu+0x4c/0x58)
> [    1.487702] [<c0157688>] (handle_irq_event_percpu) from [<c01576cc>] (handle_irq_event+0x38/0x5c)
> [    1.496621] [<c01576cc>] (handle_irq_event) from [<c015aa84>] (handle_level_irq+0xa8/0x124)
> [    1.505017] [<c015aa84>] (handle_level_irq) from [<c01568bc>] (generic_handle_irq+0x24/0x34)
> [    1.513501] [<c01568bc>] (generic_handle_irq) from [<c02f9dc0>] (tangox_dispatch_irqs+0x4c/0x58)
> [    1.522333] [<c02f9dc0>] (tangox_dispatch_irqs) from [<c02f9e18>] (tangox_irq_handler+0x4c/0xb0)
> [    1.531163] [<c02f9e18>] (tangox_irq_handler) from [<c01568bc>] (generic_handle_irq+0x24/0x34)
> [    1.539819] [<c01568bc>] (generic_handle_irq) from [<c0156df4>] (__handle_domain_irq+0x5c/0xb4)
> [    1.548562] [<c0156df4>] (__handle_domain_irq) from [<c0101450>] (gic_handle_irq+0x48/0x8c)
> [    1.556956] [<c0101450>] (gic_handle_irq) from [<c010bb4c>] (__irq_svc+0x6c/0xa8)
> 

Whether an interrupt requires acknowledgement is a function of the
design of the interrupt controller hardware.  As you say, in principle
it should not be necessary to acknowledge a level sensitive interrupt.
In fact, that is the fundamental difference between the new hardware
implementation of the brcmstb-l2 interrupt controller in the BCM7271
device and previous versions (i.e. it only supports level interrupts and
therefore does not require acknowledgement).  However, it is not unusual
for the interrupt controller designers to latch the state of triggered
interrupts especially on controllers that can be configured for
different types of interrupt triggers (e.g. rising edge, falling edge,
active low, ...) and so it becomes necessary to acknowledge level
interrupts on such controllers to clear that latched state.

I don't know the requirements of the tango interrupt controller hardware.

> 
> Regards.
> 

Thanks,
    Doug

[PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback

[Marc Gonzalez]

irq_gc_mask_disable_reg_and_ack() is not equivalent to
irq_gc_mask_disable_reg() and irq_gc_ack_set_bit().

Leave the irq_mask_ack callback undefined, and let the irqchip
framework use irq_mask and irq_ack instead.

Reported-by: Doug Berger <opendmb@gmail.com>
Fixes: 4bba66899ac6 ("irqchip/tango: Add support for Sigma Designs SMP86xx/SMP87xx interrupt controller")
Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
Cc: stable@vger.kernel.org
---
As discussed previously, it is acceptable for tango to rely
on mask_ack_irq() doing the right thing(TM).
---
 drivers/irqchip/irq-tango.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
index bdbb5c0ff7fe..825085cdab99 100644
--- a/drivers/irqchip/irq-tango.c
+++ b/drivers/irqchip/irq-tango.c
@@ -141,7 +141,6 @@ 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_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

Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback

[Måns Rullgård]

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> irq_gc_mask_disable_reg_and_ack() is not equivalent to
> irq_gc_mask_disable_reg() and irq_gc_ack_set_bit().
>
> Leave the irq_mask_ack callback undefined, and let the irqchip
> framework use irq_mask and irq_ack instead.
>
> Reported-by: Doug Berger <opendmb@gmail.com>
> Fixes: 4bba66899ac6 ("irqchip/tango: Add support for Sigma Designs SMP86xx/SMP87xx interrupt controller")
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> Cc: stable@vger.kernel.org
> ---
> As discussed previously, it is acceptable for tango to rely
> on mask_ack_irq() doing the right thing(TM).
> ---
>  drivers/irqchip/irq-tango.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
> index bdbb5c0ff7fe..825085cdab99 100644
> --- a/drivers/irqchip/irq-tango.c
> +++ b/drivers/irqchip/irq-tango.c
> @@ -141,7 +141,6 @@ 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_unmask = irq_gc_unmask_enable_reg;
>  		ct[i].chip.irq_set_type = tangox_irq_set_type;
>  		ct[i].chip.name = gc->domain->name;
> -- 

What happened to the patch adding the proper combined function?

-- 
Måns Rullgård

Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback

[Marc Gonzalez]

On 25/07/2017 15:16, Måns Rullgård wrote:

> What happened to the patch adding the proper combined function?

It appears you're not CCed on v2.

https://patchwork.kernel.org/patch/9859799/

Doug wrote:
> Yes, you understand correctly.  The irq_mask_ack method is entirely
> optional and I assume that is why this issue went undetected for so
> long; however, it is slightly more efficient to combine the functions
> (even if the ack is unnecessary) which is why I chose to do so for my
> changes to the irqchip-brcmstb-l2 driver where I first discovered this
> issue.  How much value the improved efficiency has is certainly
> debatable, but interrupt handling is one area where people might care
> about such a small difference.  As the irqchip-tango driver maintainer
> you are welcome to decide whether or not the irq_mask_ack method makes
> sense to you.

My preference goes to leaving the irq_mask_ack callback undefined,
and let the irqchip framework use irq_mask and irq_ack instead.

Regards.

Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback

[Måns Rullgård]

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> On 25/07/2017 15:16, Måns Rullgård wrote:
>
>> What happened to the patch adding the proper combined function?
>
> It appears you're not CCed on v2.
>
> https://patchwork.kernel.org/patch/9859799/
>
> Doug wrote:
>> Yes, you understand correctly.  The irq_mask_ack method is entirely
>> optional and I assume that is why this issue went undetected for so
>> long; however, it is slightly more efficient to combine the functions
>> (even if the ack is unnecessary) which is why I chose to do so for my
>> changes to the irqchip-brcmstb-l2 driver where I first discovered this
>> issue.  How much value the improved efficiency has is certainly
>> debatable, but interrupt handling is one area where people might care
>> about such a small difference.  As the irqchip-tango driver maintainer
>> you are welcome to decide whether or not the irq_mask_ack method makes
>> sense to you.
>
> My preference goes to leaving the irq_mask_ack callback undefined,
> and let the irqchip framework use irq_mask and irq_ack instead.

Why would you prefer the less efficient way?

-- 
Måns Rullgård

Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback

[Marc Gonzalez]

On 25/07/2017 15:08, Marc Gonzalez wrote:

> irq_gc_mask_disable_reg_and_ack() is not equivalent to
> irq_gc_mask_disable_reg() and irq_gc_ack_set_bit().
> 
> Leave the irq_mask_ack callback undefined, and let the irqchip
> framework use irq_mask and irq_ack instead.
> 
> Reported-by: Doug Berger <opendmb@gmail.com>
> Fixes: 4bba66899ac6 ("irqchip/tango: Add support for Sigma Designs SMP86xx/SMP87xx interrupt controller")
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> Cc: stable@vger.kernel.org

FWIW, the lockup reported in the thread below disappears
once the patch is applied.

https://lkml.org/lkml/2016/10/21/709

Regards.

Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback

[Florian Fainelli]

On 07/25/2017 06:29 AM, Måns Rullgård wrote:
> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
> 
>> On 25/07/2017 15:16, Måns Rullgård wrote:
>>
>>> What happened to the patch adding the proper combined function?
>>
>> It appears you're not CCed on v2.
>>
>> https://patchwork.kernel.org/patch/9859799/
>>
>> Doug wrote:
>>> Yes, you understand correctly.  The irq_mask_ack method is entirely
>>> optional and I assume that is why this issue went undetected for so
>>> long; however, it is slightly more efficient to combine the functions
>>> (even if the ack is unnecessary) which is why I chose to do so for my
>>> changes to the irqchip-brcmstb-l2 driver where I first discovered this
>>> issue.  How much value the improved efficiency has is certainly
>>> debatable, but interrupt handling is one area where people might care
>>> about such a small difference.  As the irqchip-tango driver maintainer
>>> you are welcome to decide whether or not the irq_mask_ack method makes
>>> sense to you.
>>
>> My preference goes to leaving the irq_mask_ack callback undefined,
>> and let the irqchip framework use irq_mask and irq_ack instead.
> 
> Why would you prefer the less efficient way?
> 

Same question here, that does not really make sense to me.

The whole point of this patch series is to have a set of efficient and
bugfree (or nearly) helper functions that drivers can rely on, are you
saying that somehow using irq_mask_and_ack is exposing a bug in the
tango irqchip driver and using the separate functions does not expose
this bug?
-- 
Florian

Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback

[Måns Rullgård]

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

> On 07/25/2017 06:29 AM, Måns Rullgård wrote:
>> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
>> 
>>> On 25/07/2017 15:16, Måns Rullgård wrote:
>>>
>>>> What happened to the patch adding the proper combined function?
>>>
>>> It appears you're not CCed on v2.
>>>
>>> https://patchwork.kernel.org/patch/9859799/
>>>
>>> Doug wrote:
>>>> Yes, you understand correctly.  The irq_mask_ack method is entirely
>>>> optional and I assume that is why this issue went undetected for so
>>>> long; however, it is slightly more efficient to combine the functions
>>>> (even if the ack is unnecessary) which is why I chose to do so for my
>>>> changes to the irqchip-brcmstb-l2 driver where I first discovered this
>>>> issue.  How much value the improved efficiency has is certainly
>>>> debatable, but interrupt handling is one area where people might care
>>>> about such a small difference.  As the irqchip-tango driver maintainer
>>>> you are welcome to decide whether or not the irq_mask_ack method makes
>>>> sense to you.
>>>
>>> My preference goes to leaving the irq_mask_ack callback undefined,
>>> and let the irqchip framework use irq_mask and irq_ack instead.
>> 
>> Why would you prefer the less efficient way?
>> 
>
> Same question here, that does not really make sense to me.
>
> The whole point of this patch series is to have a set of efficient and
> bugfree (or nearly) helper functions that drivers can rely on, are you
> saying that somehow using irq_mask_and_ack is exposing a bug in the
> tango irqchip driver and using the separate functions does not expose
> this bug?

There is currently a bug in that the function used doesn't do what its
name implies which can't be good.  Using the separate mask and ack
functions obviously works, but combining them saves a lock/unlock
sequence.  The correct combined function has already been written, so I
see no reason not to use it.

-- 
Måns Rullgård

Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback

[Florian Fainelli]

On 07/26/2017 12:13 PM, Måns Rullgård wrote:
> Florian Fainelli <f.fainelli@gmail.com> writes:
> 
>> On 07/25/2017 06:29 AM, Måns Rullgård wrote:
>>> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
>>>
>>>> On 25/07/2017 15:16, Måns Rullgård wrote:
>>>>
>>>>> What happened to the patch adding the proper combined function?
>>>>
>>>> It appears you're not CCed on v2.
>>>>
>>>> https://patchwork.kernel.org/patch/9859799/
>>>>
>>>> Doug wrote:
>>>>> Yes, you understand correctly.  The irq_mask_ack method is entirely
>>>>> optional and I assume that is why this issue went undetected for so
>>>>> long; however, it is slightly more efficient to combine the functions
>>>>> (even if the ack is unnecessary) which is why I chose to do so for my
>>>>> changes to the irqchip-brcmstb-l2 driver where I first discovered this
>>>>> issue.  How much value the improved efficiency has is certainly
>>>>> debatable, but interrupt handling is one area where people might care
>>>>> about such a small difference.  As the irqchip-tango driver maintainer
>>>>> you are welcome to decide whether or not the irq_mask_ack method makes
>>>>> sense to you.
>>>>
>>>> My preference goes to leaving the irq_mask_ack callback undefined,
>>>> and let the irqchip framework use irq_mask and irq_ack instead.
>>>
>>> Why would you prefer the less efficient way?
>>>
>>
>> Same question here, that does not really make sense to me.
>>
>> The whole point of this patch series is to have a set of efficient and
>> bugfree (or nearly) helper functions that drivers can rely on, are you
>> saying that somehow using irq_mask_and_ack is exposing a bug in the
>> tango irqchip driver and using the separate functions does not expose
>> this bug?
> 
> There is currently a bug in that the function used doesn't do what its
> name implies which can't be good.  Using the separate mask and ack
> functions obviously works, but combining them saves a lock/unlock
> sequence.  The correct combined function has already been written, so I
> see no reason not to use it.

Marc/Mason, are you intending to get this patch accepted in order to
provide a quick bugfix targeting earlier kernels with the tango irqchip
driver or is this how you think the correct fix for the tango irqchip
driver is as opposed to using Doug's fix?
-- 
Florian

Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback

[Marc Gonzalez]

On 27/07/2017 20:17, Florian Fainelli wrote:

> On 07/26/2017 12:13 PM, Måns Rullgård wrote:
>
>> Florian Fainelli writes:
>>
>>> On 07/25/2017 06:29 AM, Måns Rullgård wrote:
>>>
>>>> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
>>>>
>>>>> On 25/07/2017 15:16, Måns Rullgård wrote:
>>>>>
>>>>>> What happened to the patch adding the proper combined function?
>>>>>
>>>>> It appears you're not CCed on v2.
>>>>>
>>>>> https://patchwork.kernel.org/patch/9859799/
>>>>>
>>>>> Doug wrote:
>>>>>> Yes, you understand correctly.  The irq_mask_ack method is entirely
>>>>>> optional and I assume that is why this issue went undetected for so
>>>>>> long; however, it is slightly more efficient to combine the functions
>>>>>> (even if the ack is unnecessary) which is why I chose to do so for my
>>>>>> changes to the irqchip-brcmstb-l2 driver where I first discovered this
>>>>>> issue.  How much value the improved efficiency has is certainly
>>>>>> debatable, but interrupt handling is one area where people might care
>>>>>> about such a small difference.  As the irqchip-tango driver maintainer
>>>>>> you are welcome to decide whether or not the irq_mask_ack method makes
>>>>>> sense to you.
>>>>>
>>>>> My preference goes to leaving the irq_mask_ack callback undefined,
>>>>> and let the irqchip framework use irq_mask and irq_ack instead.
>>>>
>>>> Why would you prefer the less efficient way?
>>>>
>>>
>>> Same question here, that does not really make sense to me.
>>>
>>> The whole point of this patch series is to have a set of efficient and
>>> bugfree (or nearly) helper functions that drivers can rely on, are you
>>> saying that somehow using irq_mask_and_ack is exposing a bug in the
>>> tango irqchip driver and using the separate functions does not expose
>>> this bug?
>>
>> There is currently a bug in that the function used doesn't do what its
>> name implies which can't be good.  Using the separate mask and ack
>> functions obviously works, but combining them saves a lock/unlock
>> sequence.  The correct combined function has already been written, so I
>> see no reason not to use it.
> 
> Marc/Mason, are you intending to get this patch accepted in order to
> provide a quick bugfix targeting earlier kernels with the tango irqchip
> driver or is this how you think the correct fix for the tango irqchip
> driver is as opposed to using Doug's fix?

Hello Florian,

I am extremely grateful for you and Doug bringing the defect to
my attention, as it was indeed causing an issue which I had not
found the time to investigate.

The reason I proposed an alternate patch is that
1) Doug didn't seem to mind, 2) simpler code leads to fewer bugs
and less maintenance IME, and 3) I didn't see many drivers using
the irq_mask_ack() callback (9 out of 86) with a few misusing it,
by defining irq_mask = irq_mask_ack.

As you point out, my patch might be slightly easier to backport
than Doug's (TBH, I hadn't considered that aspect until you
mentioned it).

Has anyone ever quantified the performance improvement of
mask_ack over mask + ack?

Regards.

Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback

[Marc Zyngier]

On 28/07/17 15:06, Marc Gonzalez wrote:
> On 27/07/2017 20:17, Florian Fainelli wrote:
> 
>> On 07/26/2017 12:13 PM, Måns Rullgård wrote:
>>
>>> Florian Fainelli writes:
>>>
>>>> On 07/25/2017 06:29 AM, Måns Rullgård wrote:
>>>>
>>>>> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
>>>>>
>>>>>> On 25/07/2017 15:16, Måns Rullgård wrote:
>>>>>>
>>>>>>> What happened to the patch adding the proper combined function?
>>>>>>
>>>>>> It appears you're not CCed on v2.
>>>>>>
>>>>>> https://patchwork.kernel.org/patch/9859799/
>>>>>>
>>>>>> Doug wrote:
>>>>>>> Yes, you understand correctly.  The irq_mask_ack method is entirely
>>>>>>> optional and I assume that is why this issue went undetected for so
>>>>>>> long; however, it is slightly more efficient to combine the functions
>>>>>>> (even if the ack is unnecessary) which is why I chose to do so for my
>>>>>>> changes to the irqchip-brcmstb-l2 driver where I first discovered this
>>>>>>> issue.  How much value the improved efficiency has is certainly
>>>>>>> debatable, but interrupt handling is one area where people might care
>>>>>>> about such a small difference.  As the irqchip-tango driver maintainer
>>>>>>> you are welcome to decide whether or not the irq_mask_ack method makes
>>>>>>> sense to you.
>>>>>>
>>>>>> My preference goes to leaving the irq_mask_ack callback undefined,
>>>>>> and let the irqchip framework use irq_mask and irq_ack instead.
>>>>>
>>>>> Why would you prefer the less efficient way?
>>>>>
>>>>
>>>> Same question here, that does not really make sense to me.
>>>>
>>>> The whole point of this patch series is to have a set of efficient and
>>>> bugfree (or nearly) helper functions that drivers can rely on, are you
>>>> saying that somehow using irq_mask_and_ack is exposing a bug in the
>>>> tango irqchip driver and using the separate functions does not expose
>>>> this bug?
>>>
>>> There is currently a bug in that the function used doesn't do what its
>>> name implies which can't be good.  Using the separate mask and ack
>>> functions obviously works, but combining them saves a lock/unlock
>>> sequence.  The correct combined function has already been written, so I
>>> see no reason not to use it.
>>
>> Marc/Mason, are you intending to get this patch accepted in order to
>> provide a quick bugfix targeting earlier kernels with the tango irqchip
>> driver or is this how you think the correct fix for the tango irqchip
>> driver is as opposed to using Doug's fix?
> 
> Hello Florian,
> 
> I am extremely grateful for you and Doug bringing the defect to
> my attention, as it was indeed causing an issue which I had not
> found the time to investigate.
> 
> The reason I proposed an alternate patch is that
> 1) Doug didn't seem to mind, 2) simpler code leads to fewer bugs
> and less maintenance IME, and 3) I didn't see many drivers using
> the irq_mask_ack() callback (9 out of 86) with a few misusing it,
> by defining irq_mask = irq_mask_ack.
> 
> As you point out, my patch might be slightly easier to backport
> than Doug's (TBH, I hadn't considered that aspect until you
> mentioned it).
> 
> Has anyone ever quantified the performance improvement of
> mask_ack over mask + ack?

Aren't you the one who is in position of measuring this effect on the
actual HW that uses this?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback

[Florian Fainelli]

On 08/07/2017 05:56 AM, Marc Zyngier wrote:
> On 28/07/17 15:06, Marc Gonzalez wrote:
>> On 27/07/2017 20:17, Florian Fainelli wrote:
>>
>>> On 07/26/2017 12:13 PM, Måns Rullgård wrote:
>>>
>>>> Florian Fainelli writes:
>>>>
>>>>> On 07/25/2017 06:29 AM, Måns Rullgård wrote:
>>>>>
>>>>>> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
>>>>>>
>>>>>>> On 25/07/2017 15:16, Måns Rullgård wrote:
>>>>>>>
>>>>>>>> What happened to the patch adding the proper combined function?
>>>>>>>
>>>>>>> It appears you're not CCed on v2.
>>>>>>>
>>>>>>> https://patchwork.kernel.org/patch/9859799/
>>>>>>>
>>>>>>> Doug wrote:
>>>>>>>> Yes, you understand correctly.  The irq_mask_ack method is entirely
>>>>>>>> optional and I assume that is why this issue went undetected for so
>>>>>>>> long; however, it is slightly more efficient to combine the functions
>>>>>>>> (even if the ack is unnecessary) which is why I chose to do so for my
>>>>>>>> changes to the irqchip-brcmstb-l2 driver where I first discovered this
>>>>>>>> issue.  How much value the improved efficiency has is certainly
>>>>>>>> debatable, but interrupt handling is one area where people might care
>>>>>>>> about such a small difference.  As the irqchip-tango driver maintainer
>>>>>>>> you are welcome to decide whether or not the irq_mask_ack method makes
>>>>>>>> sense to you.
>>>>>>>
>>>>>>> My preference goes to leaving the irq_mask_ack callback undefined,
>>>>>>> and let the irqchip framework use irq_mask and irq_ack instead.
>>>>>>
>>>>>> Why would you prefer the less efficient way?
>>>>>>
>>>>>
>>>>> Same question here, that does not really make sense to me.
>>>>>
>>>>> The whole point of this patch series is to have a set of efficient and
>>>>> bugfree (or nearly) helper functions that drivers can rely on, are you
>>>>> saying that somehow using irq_mask_and_ack is exposing a bug in the
>>>>> tango irqchip driver and using the separate functions does not expose
>>>>> this bug?
>>>>
>>>> There is currently a bug in that the function used doesn't do what its
>>>> name implies which can't be good.  Using the separate mask and ack
>>>> functions obviously works, but combining them saves a lock/unlock
>>>> sequence.  The correct combined function has already been written, so I
>>>> see no reason not to use it.
>>>
>>> Marc/Mason, are you intending to get this patch accepted in order to
>>> provide a quick bugfix targeting earlier kernels with the tango irqchip
>>> driver or is this how you think the correct fix for the tango irqchip
>>> driver is as opposed to using Doug's fix?
>>
>> Hello Florian,
>>
>> I am extremely grateful for you and Doug bringing the defect to
>> my attention, as it was indeed causing an issue which I had not
>> found the time to investigate.
>>
>> The reason I proposed an alternate patch is that
>> 1) Doug didn't seem to mind, 2) simpler code leads to fewer bugs
>> and less maintenance IME, and 3) I didn't see many drivers using
>> the irq_mask_ack() callback (9 out of 86) with a few misusing it,
>> by defining irq_mask = irq_mask_ack.
>>
>> As you point out, my patch might be slightly easier to backport
>> than Doug's (TBH, I hadn't considered that aspect until you
>> mentioned it).
>>
>> Has anyone ever quantified the performance improvement of
>> mask_ack over mask + ack?
> 
> Aren't you the one who is in position of measuring this effect on the
> actual HW that uses this?

What do we do with this patch series to move forward? Can we get Doug's
changes queued up for 4.14?
-- 
Florian

Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback

[Måns Rullgård]

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

> What do we do with this patch series to move forward? Can we get Doug's
> changes queued up for 4.14?

My opinion is that the correct combined function should be added and the
tango driver updated to use it.  Patches already exist, so what are we
waiting for?

-- 
Måns Rullgård

Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback

[Marc Gonzalez]

On 07/08/2017 14:56, Marc Zyngier wrote:

> On 28/07/17 15:06, Marc Gonzalez wrote:
>
>> On 27/07/2017 20:17, Florian Fainelli wrote:
>>
>>> On 07/26/2017 12:13 PM, Måns Rullgård wrote:
>>>
>>>> Florian Fainelli writes:
>>>>
>>>>> On 07/25/2017 06:29 AM, Måns Rullgård wrote:
>>>>>
>>>>>> Marc Gonzalez writes:
>>>>>>
>>>>>>> On 25/07/2017 15:16, Måns Rullgård wrote:
>>>>>>>
>>>>>>>> What happened to the patch adding the proper combined function?
>>>>>>>
>>>>>>> It appears you're not CCed on v2.
>>>>>>>
>>>>>>> https://patchwork.kernel.org/patch/9859799/
>>>>>>>
>>>>>>> Doug wrote:
>>>>>>>> Yes, you understand correctly.  The irq_mask_ack method is entirely
>>>>>>>> optional and I assume that is why this issue went undetected for so
>>>>>>>> long; however, it is slightly more efficient to combine the functions
>>>>>>>> (even if the ack is unnecessary) which is why I chose to do so for my
>>>>>>>> changes to the irqchip-brcmstb-l2 driver where I first discovered this
>>>>>>>> issue.  How much value the improved efficiency has is certainly
>>>>>>>> debatable, but interrupt handling is one area where people might care
>>>>>>>> about such a small difference.  As the irqchip-tango driver maintainer
>>>>>>>> you are welcome to decide whether or not the irq_mask_ack method makes
>>>>>>>> sense to you.
>>>>>>>
>>>>>>> My preference goes to leaving the irq_mask_ack callback undefined,
>>>>>>> and let the irqchip framework use irq_mask and irq_ack instead.
>>>>>>
>>>>>> Why would you prefer the less efficient way?
>>>>>
>>>>> Same question here, that does not really make sense to me.
>>>>>
>>>>> The whole point of this patch series is to have a set of efficient and
>>>>> bugfree (or nearly) helper functions that drivers can rely on, are you
>>>>> saying that somehow using irq_mask_and_ack is exposing a bug in the
>>>>> tango irqchip driver and using the separate functions does not expose
>>>>> this bug?
>>>>
>>>> There is currently a bug in that the function used doesn't do what its
>>>> name implies which can't be good.  Using the separate mask and ack
>>>> functions obviously works, but combining them saves a lock/unlock
>>>> sequence.  The correct combined function has already been written, so I
>>>> see no reason not to use it.
>>>
>>> Marc/Mason, are you intending to get this patch accepted in order to
>>> provide a quick bugfix targeting earlier kernels with the tango irqchip
>>> driver or is this how you think the correct fix for the tango irqchip
>>> driver is as opposed to using Doug's fix?
>>
>> Hello Florian,
>>
>> I am extremely grateful for you and Doug bringing the defect to
>> my attention, as it was indeed causing an issue which I had not
>> found the time to investigate.
>>
>> The reason I proposed an alternate patch is that
>> 1) Doug didn't seem to mind, 2) simpler code leads to fewer bugs
>> and less maintenance IME, and 3) I didn't see many drivers using
>> the irq_mask_ack() callback (9 out of 86) with a few misusing it,
>> by defining irq_mask = irq_mask_ack.
>>
>> As you point out, my patch might be slightly easier to backport
>> than Doug's (TBH, I hadn't considered that aspect until you
>> mentioned it).
>>
>> Has anyone ever quantified the performance improvement of
>> mask_ack over mask + ack?
> 
> Aren't you the one who is in position of measuring this effect on the
> actual HW that uses this?

Using separate mask and ack functions (i.e. my patch)

# iperf3 -c 172.27.64.110 -t 20
Connecting to host 172.27.64.110, port 5201
[  4] local 172.27.64.1 port 40868 connected to 172.27.64.110 port 5201
[ ID] Interval           Transfer     Bandwidth       Retr  Cwnd
[  4]   0.00-1.00   sec   106 MBytes   888 Mbits/sec   18    324 KBytes       
[  4]   1.00-2.00   sec   106 MBytes   885 Mbits/sec    0    361 KBytes       
[  4]   2.00-3.00   sec   105 MBytes   883 Mbits/sec    4    279 KBytes       
[  4]   3.00-4.00   sec   106 MBytes   890 Mbits/sec    0    300 KBytes       
[  4]   4.00-5.00   sec   106 MBytes   887 Mbits/sec    0    310 KBytes       
[  4]   5.00-6.00   sec   105 MBytes   883 Mbits/sec    0    315 KBytes       
[  4]   6.00-7.00   sec   105 MBytes   885 Mbits/sec    0    321 KBytes       
[  4]   7.00-8.00   sec   105 MBytes   880 Mbits/sec    0    325 KBytes       
[  4]   8.00-9.00   sec   106 MBytes   888 Mbits/sec    0    329 KBytes       
[  4]   9.00-10.00  sec   106 MBytes   886 Mbits/sec    0    335 KBytes       
[  4]  10.00-11.00  sec   105 MBytes   885 Mbits/sec    0    351 KBytes       
[  4]  11.00-12.00  sec   106 MBytes   887 Mbits/sec    1    276 KBytes       
[  4]  12.00-13.00  sec   106 MBytes   885 Mbits/sec    0    321 KBytes       
[  4]  13.00-14.00  sec   105 MBytes   883 Mbits/sec    0    349 KBytes       
[  4]  14.00-15.00  sec   106 MBytes   890 Mbits/sec    0    366 KBytes       
[  4]  15.00-16.00  sec   106 MBytes   888 Mbits/sec    2    286 KBytes       
[  4]  16.00-17.00  sec   105 MBytes   884 Mbits/sec    0    303 KBytes       
[  4]  17.00-18.00  sec   105 MBytes   883 Mbits/sec    0    311 KBytes       
[  4]  18.00-19.00  sec   105 MBytes   880 Mbits/sec    0    315 KBytes       
[  4]  19.00-20.00  sec   106 MBytes   890 Mbits/sec    0    321 KBytes       
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bandwidth       Retr
[  4]   0.00-20.00  sec  2.06 GBytes   885 Mbits/sec   25             sender


Using combined mask and ack functions (i.e. Doug's patch)

# iperf3 -c 172.27.64.110 -t 20
Connecting to host 172.27.64.110, port 5201
[  4] local 172.27.64.1 port 41235 connected to 172.27.64.110 port 5201
[ ID] Interval           Transfer     Bandwidth       Retr  Cwnd
[  4]   0.00-1.00   sec   107 MBytes   897 Mbits/sec   60    324 KBytes       
[  4]   1.00-2.00   sec   107 MBytes   898 Mbits/sec    0    361 KBytes       
[  4]   2.00-3.00   sec   107 MBytes   898 Mbits/sec   39    194 KBytes       
[  4]   3.00-4.00   sec   107 MBytes   895 Mbits/sec    0    214 KBytes       
[  4]   4.00-5.00   sec   107 MBytes   901 Mbits/sec    0    223 KBytes       
[  4]   5.00-6.00   sec   108 MBytes   902 Mbits/sec    0    230 KBytes       
[  4]   6.00-7.00   sec   107 MBytes   895 Mbits/sec    0    242 KBytes       
[  4]   7.00-8.00   sec   107 MBytes   901 Mbits/sec    0    253 KBytes       
[  4]   8.00-9.00   sec   107 MBytes   899 Mbits/sec    0    264 KBytes       
[  4]   9.00-10.00  sec   108 MBytes   903 Mbits/sec    0    276 KBytes       
[  4]  10.00-11.00  sec   108 MBytes   902 Mbits/sec    0    286 KBytes       
[  4]  11.00-12.00  sec   107 MBytes   899 Mbits/sec    0    300 KBytes       
[  4]  12.00-13.00  sec   107 MBytes   898 Mbits/sec   33    247 KBytes       
[  4]  13.00-14.00  sec   107 MBytes   900 Mbits/sec    0    294 KBytes       
[  4]  14.00-15.00  sec   107 MBytes   900 Mbits/sec    0    325 KBytes       
[  4]  15.00-16.00  sec   107 MBytes   899 Mbits/sec    0    342 KBytes       
[  4]  16.00-17.00  sec   107 MBytes   898 Mbits/sec    0    351 KBytes       
[  4]  17.00-18.00  sec   108 MBytes   902 Mbits/sec    0    355 KBytes       
[  4]  18.00-19.00  sec   107 MBytes   901 Mbits/sec    0    359 KBytes       
[  4]  19.00-20.00  sec   108 MBytes   903 Mbits/sec   32    255 KBytes       
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bandwidth       Retr
[  4]   0.00-20.00  sec  2.09 GBytes   900 Mbits/sec  164             sender


Ergo, it seems that the performance improvement of the combined
implementation is approximately 1.5% for a load generating ~80k
interrupts per second.

I suppose a 1.5% improvement for free should not be ignored.
Therefore, I rescind my v3 patch.

Doug/Florian, thanks again for fixing the tango driver.

Regards.

Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback

[Marc Gonzalez]

On 21/08/2017 15:25, Marc Gonzalez wrote:

> Using separate mask and ack functions (i.e. my patch)
> 
> # iperf3 -c 172.27.64.110 -t 20
> Connecting to host 172.27.64.110, port 5201
> [  4] local 172.27.64.1 port 40868 connected to 172.27.64.110 port 5201
> [ ID] Interval           Transfer     Bandwidth       Retr  Cwnd
> [  4]   0.00-1.00   sec   106 MBytes   888 Mbits/sec   18    324 KBytes
> [  4]   1.00-2.00   sec   106 MBytes   885 Mbits/sec    0    361 KBytes
> [  4]   2.00-3.00   sec   105 MBytes   883 Mbits/sec    4    279 KBytes
> [  4]   3.00-4.00   sec   106 MBytes   890 Mbits/sec    0    300 KBytes
> [  4]   4.00-5.00   sec   106 MBytes   887 Mbits/sec    0    310 KBytes
> [  4]   5.00-6.00   sec   105 MBytes   883 Mbits/sec    0    315 KBytes
> [  4]   6.00-7.00   sec   105 MBytes   885 Mbits/sec    0    321 KBytes
> [  4]   7.00-8.00   sec   105 MBytes   880 Mbits/sec    0    325 KBytes
> [  4]   8.00-9.00   sec   106 MBytes   888 Mbits/sec    0    329 KBytes
> [  4]   9.00-10.00  sec   106 MBytes   886 Mbits/sec    0    335 KBytes
> [  4]  10.00-11.00  sec   105 MBytes   885 Mbits/sec    0    351 KBytes
> [  4]  11.00-12.00  sec   106 MBytes   887 Mbits/sec    1    276 KBytes
> [  4]  12.00-13.00  sec   106 MBytes   885 Mbits/sec    0    321 KBytes
> [  4]  13.00-14.00  sec   105 MBytes   883 Mbits/sec    0    349 KBytes
> [  4]  14.00-15.00  sec   106 MBytes   890 Mbits/sec    0    366 KBytes
> [  4]  15.00-16.00  sec   106 MBytes   888 Mbits/sec    2    286 KBytes
> [  4]  16.00-17.00  sec   105 MBytes   884 Mbits/sec    0    303 KBytes
> [  4]  17.00-18.00  sec   105 MBytes   883 Mbits/sec    0    311 KBytes
> [  4]  18.00-19.00  sec   105 MBytes   880 Mbits/sec    0    315 KBytes
> [  4]  19.00-20.00  sec   106 MBytes   890 Mbits/sec    0    321 KBytes
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bandwidth       Retr
> [  4]   0.00-20.00  sec  2.06 GBytes   885 Mbits/sec   25             sender
> 
> 
> Using combined mask and ack functions (i.e. Doug's patch)
> 
> # iperf3 -c 172.27.64.110 -t 20
> Connecting to host 172.27.64.110, port 5201
> [  4] local 172.27.64.1 port 41235 connected to 172.27.64.110 port 5201
> [ ID] Interval           Transfer     Bandwidth       Retr  Cwnd
> [  4]   0.00-1.00   sec   107 MBytes   897 Mbits/sec   60    324 KBytes
> [  4]   1.00-2.00   sec   107 MBytes   898 Mbits/sec    0    361 KBytes
> [  4]   2.00-3.00   sec   107 MBytes   898 Mbits/sec   39    194 KBytes
> [  4]   3.00-4.00   sec   107 MBytes   895 Mbits/sec    0    214 KBytes
> [  4]   4.00-5.00   sec   107 MBytes   901 Mbits/sec    0    223 KBytes
> [  4]   5.00-6.00   sec   108 MBytes   902 Mbits/sec    0    230 KBytes
> [  4]   6.00-7.00   sec   107 MBytes   895 Mbits/sec    0    242 KBytes
> [  4]   7.00-8.00   sec   107 MBytes   901 Mbits/sec    0    253 KBytes
> [  4]   8.00-9.00   sec   107 MBytes   899 Mbits/sec    0    264 KBytes
> [  4]   9.00-10.00  sec   108 MBytes   903 Mbits/sec    0    276 KBytes
> [  4]  10.00-11.00  sec   108 MBytes   902 Mbits/sec    0    286 KBytes
> [  4]  11.00-12.00  sec   107 MBytes   899 Mbits/sec    0    300 KBytes
> [  4]  12.00-13.00  sec   107 MBytes   898 Mbits/sec   33    247 KBytes
> [  4]  13.00-14.00  sec   107 MBytes   900 Mbits/sec    0    294 KBytes
> [  4]  14.00-15.00  sec   107 MBytes   900 Mbits/sec    0    325 KBytes
> [  4]  15.00-16.00  sec   107 MBytes   899 Mbits/sec    0    342 KBytes
> [  4]  16.00-17.00  sec   107 MBytes   898 Mbits/sec    0    351 KBytes
> [  4]  17.00-18.00  sec   108 MBytes   902 Mbits/sec    0    355 KBytes
> [  4]  18.00-19.00  sec   107 MBytes   901 Mbits/sec    0    359 KBytes
> [  4]  19.00-20.00  sec   108 MBytes   903 Mbits/sec   32    255 KBytes
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bandwidth       Retr
> [  4]   0.00-20.00  sec  2.09 GBytes   900 Mbits/sec  164             sender
> 
> 
> Ergo, it seems that the performance improvement of the combined
> implementation is approximately 1.5% for a load generating ~80k
> interrupts per second.

Hello irqchip maintainers,

As mentioned upthread, there is a bug in drivers/irqchip/irq-tango.c
caused by the unexpected implementation of irq_gc_mask_disable_reg_and_ack()

That bug can be fixed either by using an appropriate irq_mask_ack callback,
or by not defining an irq_mask_ack callback at all. The first option provides
~1.5% more throughput than the second, for a typical use-case.

Whichever option you favor, can we fix this bug in current and stable branches?
(The fix was submitted two months ago.)

Regards.