Re: [PATCH 1/2] irqdomain: add support for creating a continous mapping

On Mon, 2014-11-03 at 17:18 +0100, Johannes Thumshirn wrote:

A MSI device may have multiple interrupts. That means that the
interrupts numbers should be continuos so that pdev->irq refers to the
first interrupt, pdev->irq + 1 to the second and so on.
This patch adds support for continuous allocation of virqs for a range
of hwirqs. The function is based on irq_create_mapping() but due to the
number argument there is very little in common now.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Johannes Thumshirn <redacted>
---
 include/linux/irqdomain.h | 10 ++++--
 kernel/irq/irqdomain.c    | 85 ++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 73 insertions(+), 22 deletions(-)

This is changing core kernel code and thus LKML should be CCed, as well
as Ben Herrenschmidt who is the maintainer of kernel/irq/irqdomain.c.

Also please respond to feedback in
http://patchwork.ozlabs.org/patch/322497/

Is it really necessary for the virqs to be contiguous?  How is the
availability of multiple MSIs communicated to the driver?  Is there an
example of a driver that currently uses multiple MSIs?

 /**
- * irq_create_mapping() - Map a hardware interrupt into linux irq space
+ * irq_create_mapping_block() - Map multiple hardware interrupts
  * @domain: domain owning this hardware interrupt or NULL for default domain
  * @hwirq: hardware irq number in that domain space
+ * @num: number of interrupts
+ *
+ * Maps a hwirq to a newly allocated virq. If num is greater than 1 then num
+ * hwirqs (hwirq ... hwirq + num - 1) will be mapped and virq will be
+ * continuous.
+ * Returns the first linux virq number.
  *
- * Only one mapping per hardware interrupt is permitted. Returns a linux
- * irq number.
  * If the sense/trigger is to be specified, set_irq_type() should be called
  * on the number returned from that call.
  */
-unsigned int irq_create_mapping(struct irq_domain *domain,
+unsigned int irq_create_mapping_block(struct irq_domain *domain,
 				irq_hw_number_t hwirq)
 {

Where is the num parameter?  How does this even build?

Please don't introduce new uses of NO_IRQ.  irq_find_mapping() returns
zero on failure.  Some architectures (e.g. ARM) define NO_IRQ as
something other than zero, which will cause this to break.

+			if (i == 0)
+				return irq_check_continuous_mapping(domain,
+								hwirq, num);
+			pr_err("irq: hwirq %ld has no mapping but hwirq %ld "
+				"maps to virq %d. This can't be a block\n",
+				hwirq, hwirq + i, virq);
+			return -EINVAL;
+		}
 	}

Explain how you'd get into this error state, and how you'd avoid doing
so.

+	node = of_node_to_nid(domain->of_node);
+
 	/* Allocate a virtual interrupt number */
 	hint = hwirq % nr_irqs;
 	if (hint == 0)
 		hint++;
-	virq = irq_alloc_desc_from(hint, of_node_to_nid(domain->of_node));
-	if (virq <= 0)
-		virq = irq_alloc_desc_from(1, of_node_to_nid(domain->of_node));
+	virq = irq_alloc_desc_from(hint, node);
+	if (virq <= 0 && hint != 1)
+		virq = irq_alloc_desc_from(1, node);

Factoring out node seems irrelevant to, and obscures, what you're doing
which is adding a chcek for hint.  Why is a hint value of 1 special?

You're also still allocating only one virq, unlike in
http://patchwork.ozlabs.org/patch/322497/

-Scott