genirq: Hierarchical irq domains and generic MSI interrupt code

From: Jiang Liu <[email protected]>
Index: tip/kernel/irq/chip.c
===================================================================

kernel/irq/chip.c +1 -0

@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/kernel_stat.h>
+#include <linux/irqdomain.h>
 
 #include <trace/events/irq.h>
 

kernel/irq/chip.c +1 -0

 	if (desc->irq_data.chip->irq_startup) {
 		ret = desc->irq_data.chip->irq_startup(&desc->irq_data);
 		irq_state_clr_masked(desc);

@@ -199,6 +201,7 @@ void irq_shutdown(struct irq_desc *desc)
 		desc->irq_data.chip->irq_disable(&desc->irq_data);
 	else
 		desc->irq_data.chip->irq_mask(&desc->irq_data);
+	irq_domain_deactivate_irq(&desc->irq_data);
 	irq_state_set_masked(desc);
 }
 

Hi Thomas, Jiang,

Quoted 13 lines · Thomas Gleixner

From: Jiang Liu <[email protected]>
Index: tip/kernel/irq/chip.c
===================================================================

kernel/irq/chip.c +1 -0

@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/kernel_stat.h>
+#include <linux/irqdomain.h>
 
 #include <trace/events/irq.h>
 

kernel/irq/chip.c int irq_startup(struct irq_desc *desc, b · line 182

Quoted 5 lines

kernel/irq/chip.c +1 -0

@@ -178,6 +179,7 @@ int irq_startup(struct irq_desc *desc, b
 	irq_state_clr_disabled(desc);
 	desc->depth = 0;
 
+	irq_domain_activate_irq(&desc->irq_data);

I'm not sure why this is needed, please help me out.. :)

Quoted 16 lines · Yun Wu (Abel)

Hi Thomas, Jiang,

Quoted 13 lines · Thomas Gleixner

From: Jiang Liu <[email protected]>
Index: tip/kernel/irq/chip.c
===================================================================

kernel/irq/chip.c +1 -0

@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/kernel_stat.h>
+#include <linux/irqdomain.h>
 
 #include <trace/events/irq.h>
 

Quoted 7 lines

kernel/irq/chip.c int irq_startup(struct irq_desc *desc, b · line 182

Quoted 5 lines

kernel/irq/chip.c +1 -0

@@ -178,6 +179,7 @@ int irq_startup(struct irq_desc *desc, b
 	irq_state_clr_disabled(desc);
 	desc->depth = 0;
 
+	irq_domain_activate_irq(&desc->irq_data);

I'm not sure why this is needed, please help me out.. :)

Because it makes the whole error handling in stacked allocations way simpler.

We allocate and reserve resources, but do not program the hardware up to the point where request_irq and therefor irq_startup() is invoked.

So when in the allocation/reservation phase any of the stack level fails we just have to undo the allocations/reservations and not any hardware settings.

That also solves the issue that depending on the stacking we might not be able to program the hardware during the allocation because all stack levels need to be allocated/reserved before we can figure out which hardware configuration we need for the various levels.

So we decided to postpone the actual hardware programming to the point where the interrupt actually gets used.

From: Jiang Liu <[email protected]>

/* Number of irqs reserved for a legacy isa controller */
#define NUM_ISA_INTERRUPTS 16

include/linux/irqdomain.h +0 -0

@@ -64,6 +66,16 @@ struct irq_domain_ops {
 	int (*xlate)(struct irq_domain *d, struct device_node *node,
 		     const u32 *intspec, unsigned int intsize,

include/linux/irqdomain.h +75 -0

+#endif
 };
 
 extern struct irq_domain_ops irq_generic_chip_ops;

@@ -77,6 +89,7 @@ struct irq_domain_chip_generic;
  * @ops: pointer to irq_domain methods
  * @host_data: private data pointer for use by owner.  Not touched by irq_domain
  *             core code.
+ * @flags: host per irq_domain flags
  *
  * Optional elements
  * @of_node: Pointer to device tree nodes associated with the irq_domain. Used

@@ -84,6 +97,7 @@ struct irq_domain_chip_generic;
  * @gc: Pointer to a list of generic chips. There is a helper function for
  *      setting up one or more generic chips for interrupt controllers
  *      drivers using the generic chip library which uses this pointer.
+ * @parent: Pointer to parent irq_domain to support hierarchy irq_domains
  *
  * Revmap data, used internally by irq_domain
  * @revmap_direct_max_irq: The largest hwirq that can be set for controllers that

@@ -97,10 +111,14 @@ struct irq_domain {
 	const char *name;
 	const struct irq_domain_ops *ops;
 	void *host_data;
+	unsigned int flags;
 
 	/* Optional data */
 	struct device_node *of_node;
 	struct irq_domain_chip_generic *gc;
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	struct irq_domain *parent;
+#endif
 
 	/* reverse map data. The linear map gets appended to the irq_domain */
 	irq_hw_number_t hwirq_max;

@@ -110,6 +128,9 @@ struct irq_domain {
 	unsigned int linear_revmap[];
 };
 
+#define	IRQ_DOMAIN_FLAG_HIERARCHY	0x1
+#define	IRQ_DOMAIN_FLAG_ARCH1		0x10000
+
 #ifdef CONFIG_IRQ_DOMAIN
 struct irq_domain *__irq_domain_add(struct device_node *of_node, int size,
 				    irq_hw_number_t hwirq_max, int direct_max,

@@ -220,8 +241,73 @@ int irq_domain_xlate_onetwocell(struct i
 			const u32 *intspec, unsigned int intsize,
 			irq_hw_number_t *out_hwirq, unsigned int *out_type);
 
+/* V2 interfaces to support hierarchy IRQ domains. */
+extern struct irq_data *irq_domain_get_irq_data(struct irq_domain *domain,
+						unsigned int virq);
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+extern int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
+				   unsigned int nr_irqs, int node, void *arg,
+				   bool realloc);
+extern void irq_domain_free_irqs(unsigned int virq, unsigned int nr_irqs);
+extern void irq_domain_activate_irq(struct irq_data *irq_data);
+extern void irq_domain_deactivate_irq(struct irq_data *irq_data);
+
+static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
+			unsigned int nr_irqs, int node, void *arg)
+{
+	return __irq_domain_alloc_irqs(domain, -1, nr_irqs, node, arg, false);
+}
+
+extern int irq_domain_set_hwirq_and_chip(struct irq_domain *domain,
+					 unsigned int virq,
+					 irq_hw_number_t hwirq,
+					 struct irq_chip *chip,
+					 void *chip_data);
+extern void irq_domain_reset_irq_data(struct irq_data *irq_data);
+extern void irq_domain_free_irqs_common(struct irq_domain *domain,
+					int virq, int nr_irqs);
+extern void irq_domain_free_irqs_top(struct irq_domain *domain,
+				     int virq, int nr_irqs);
+
+static inline int irq_domain_alloc_irqs_parent(struct irq_domain *domain,
+				int irq_base, unsigned int nr_irqs, void *arg)
+{
+	if (domain->parent && domain->parent->ops->alloc)
+		return domain->parent->ops->alloc(domain->parent, irq_base,
+						  nr_irqs, arg);
+	return -ENOSYS;
+}
+
+static inline void irq_domain_free_irqs_parent(struct irq_domain *domain,
+					int irq_base, unsigned int nr_irqs)
+{
+	if (domain->parent && domain->parent->ops->free)
+		domain->parent->ops->free(domain->parent, irq_base, nr_irqs);
+}
+
+static inline bool irq_domain_is_hierarchy(struct irq_domain *domain)
+{
+	return domain->flags & IRQ_DOMAIN_FLAG_HIERARCHY;
+}
+#else	/* CONFIG_IRQ_DOMAIN_HIERARCHY */
+static inline void irq_domain_activate_irq(struct irq_data *data) { }
+static inline void irq_domain_deactivate_irq(struct irq_data *data) { }
+static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
+			unsigned int nr_irqs, int node, void *arg)
+{
+	return -1;
+}
+
+static inline bool irq_domain_is_hierarchy(struct irq_domain *domain)
+{
+	return false;
+}
+#endif	/* CONFIG_IRQ_DOMAIN_HIERARCHY */
+
 #else /* CONFIG_IRQ_DOMAIN */
 static inline void irq_dispose_mapping(unsigned int virq) { }
+static inline void irq_domain_activate_irq(struct irq_data *data) { }
+static inline void irq_domain_deactivate_irq(struct irq_data *data) { }
 #endif /* !CONFIG_IRQ_DOMAIN */
 
 #endif /* _LINUX_IRQDOMAIN_H */

Index: tip/kernel/irq/Kconfig
===================================================================

kernel/irq/Kconfig +5 -0

@@ -55,6 +55,11 @@ config GENERIC_IRQ_CHIP
 config IRQ_DOMAIN
 	bool
 
+# Support for hierarchical irq domains
+config IRQ_DOMAIN_HIERARCHY
+	bool
+	select IRQ_DOMAIN
+
 config HANDLE_DOMAIN_IRQ
 	bool
 

Index: tip/kernel/irq/chip.c
===================================================================

kernel/irq/chip.c +3 -0

@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/kernel_stat.h>
+#include <linux/irqdomain.h>
 
 #include <trace/events/irq.h>
 

@@ -178,6 +179,7 @@ int irq_startup(struct irq_desc *desc, b
 	irq_state_clr_disabled(desc);
 	desc->depth = 0;
 
+	irq_domain_activate_irq(&desc->irq_data);
 	if (desc->irq_data.chip->irq_startup) {
 		ret = desc->irq_data.chip->irq_startup(&desc->irq_data);
 		irq_state_clr_masked(desc);

@@ -199,6 +201,7 @@ void irq_shutdown(struct irq_desc *desc)
 		desc->irq_data.chip->irq_disable(&desc->irq_data);
 	else
 		desc->irq_data.chip->irq_mask(&desc->irq_data);
+	irq_domain_deactivate_irq(&desc->irq_data);
 	irq_state_set_masked(desc);
 }
 

Index: tip/kernel/irq/irqdomain.c
===================================================================

kernel/irq/irqdomain.c +375 -17

@@ -23,6 +23,10 @@ static DEFINE_MUTEX(irq_domain_mutex);
 static DEFINE_MUTEX(revmap_trees_mutex);
 static struct irq_domain *irq_default_domain;
 
+static int irq_domain_alloc_descs(int virq, unsigned int nr_irqs,
+				  irq_hw_number_t hwirq, int node);
+static void irq_domain_check_hierarchy(struct irq_domain *domain);
+
 /**
  * __irq_domain_add() - Allocate a new irq_domain data structure
  * @of_node: optional device-tree node of the interrupt controller

@@ -30,7 +34,7 @@ static struct irq_domain *irq_default_do
  * @hwirq_max: Maximum number of interrupts supported by controller
  * @direct_max: Maximum value of direct maps; Use ~0 for no limit; 0 for no
  *              direct mapping
- * @ops: map/unmap domain callbacks
+ * @ops: domain callbacks
  * @host_data: Controller private data pointer
  *
  * Allocates and initialize and irq_domain structure.

@@ -56,6 +60,7 @@ struct irq_domain *__irq_domain_add(stru
 	domain->hwirq_max = hwirq_max;
 	domain->revmap_size = size;
 	domain->revmap_direct_max_irq = direct_max;
+	irq_domain_check_hierarchy(domain);
 
 	mutex_lock(&irq_domain_mutex);
 	list_add(&domain->link, &irq_domain_list);

@@ -109,7 +114,7 @@ EXPORT_SYMBOL_GPL(irq_domain_remove);
  * @first_irq: first number of irq block assigned to the domain,
  *	pass zero to assign irqs on-the-fly. If first_irq is non-zero, then
  *	pre-map all of the irqs in the domain to virqs starting at first_irq.
- * @ops: map/unmap domain callbacks
+ * @ops: domain callbacks
  * @host_data: Controller private data pointer
  *
  * Allocates an irq_domain, and optionally if first_irq is positive then also

@@ -174,10 +179,8 @@ struct irq_domain *irq_domain_add_legacy
 
 	domain = __irq_domain_add(of_node, first_hwirq + size,
 				  first_hwirq + size, 0, ops, host_data);
-	if (!domain)
-		return NULL;
-
-	irq_domain_associate_many(domain, first_irq, first_hwirq, size);
+	if (domain)
+		irq_domain_associate_many(domain, first_irq, first_hwirq, size);
 
 	return domain;
 }

@@ -388,7 +391,6 @@ EXPORT_SYMBOL_GPL(irq_create_direct_mapp
 unsigned int irq_create_mapping(struct irq_domain *domain,
 				irq_hw_number_t hwirq)
 {
-	unsigned int hint;
 	int virq;
 
 	pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);

@@ -410,12 +412,8 @@ unsigned int irq_create_mapping(struct i
 	}
 
 	/* 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_domain_alloc_descs(-1, 1, hwirq,
+				      of_node_to_nid(domain->of_node));
 	if (virq <= 0) {
 		pr_debug("-> virq allocation failed\n");
 		return 0;

@@ -471,7 +469,7 @@ unsigned int irq_create_of_mapping(struc
 	struct irq_domain *domain;
 	irq_hw_number_t hwirq;
 	unsigned int type = IRQ_TYPE_NONE;
-	unsigned int virq;
+	int virq;
 
 	domain = irq_data->np ? irq_find_host(irq_data->np) : irq_default_domain;
 	if (!domain) {

@@ -480,6 +478,11 @@ unsigned int irq_create_of_mapping(struc
 		return 0;
 	}
 
+	if (irq_domain_is_hierarchy(domain)) {
+		virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, irq_data);
+		return virq <= 0 ? 0 : virq;
+	}
+
 	/* If domain has no translation, then we assume interrupt line */
 	if (domain->ops->xlate == NULL)
 		hwirq = irq_data->args[0];

@@ -540,8 +543,8 @@ unsigned int irq_find_mapping(struct irq
 		return 0;
 
 	if (hwirq < domain->revmap_direct_max_irq) {
-		data = irq_get_irq_data(hwirq);
-		if (data && (data->domain == domain) && (data->hwirq == hwirq))
+		data = irq_domain_get_irq_data(domain, hwirq);
+		if (data && data->hwirq == hwirq)
 			return hwirq;
 	}
 

@@ -709,3 +712,359 @@ const struct irq_domain_ops irq_domain_s
 	.xlate = irq_domain_xlate_onetwocell,
 };
 EXPORT_SYMBOL_GPL(irq_domain_simple_ops);
+
+static int irq_domain_alloc_descs(int virq, unsigned int cnt,
+				  irq_hw_number_t hwirq, int node)
+{
+	unsigned int hint;
+
+	if (virq >= 0) {
+		virq = irq_alloc_descs(virq, virq, cnt, node);
+	} else {
+		hint = hwirq % nr_irqs;
+		if (hint == 0)
+			hint++;
+		virq = irq_alloc_descs_from(hint, cnt, node);
+		if (virq <= 0 && hint > 1)
+			virq = irq_alloc_descs_from(1, cnt, node);
+	}
+
+	return virq;
+}
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+static void irq_domain_insert_irq(int virq)
+{
+	struct irq_data *data;
+
+	for (data = irq_get_irq_data(virq); data; data = data->parent_data) {
+		struct irq_domain *domain = data->domain;
+		irq_hw_number_t hwirq = data->hwirq;
+
+		if (hwirq < domain->revmap_size) {
+			domain->linear_revmap[hwirq] = virq;
+		} else {
+			mutex_lock(&revmap_trees_mutex);
+			radix_tree_insert(&domain->revmap_tree, hwirq, data);
+			mutex_unlock(&revmap_trees_mutex);
+		}
+
+		/* If not already assigned, give the domain the chip's name */
+		if (!domain->name && data->chip)
+			domain->name = data->chip->name;
+	}
+
+	irq_clear_status_flags(virq, IRQ_NOREQUEST);
+}
+
+static void irq_domain_remove_irq(int virq)
+{
+	struct irq_data *data;
+
+	irq_set_status_flags(virq, IRQ_NOREQUEST);
+	irq_set_chip_and_handler(virq, NULL, NULL);
+	synchronize_irq(virq);
+	smp_mb();
+
+	for (data = irq_get_irq_data(virq); data; data = data->parent_data) {
+		struct irq_domain *domain = data->domain;
+		irq_hw_number_t hwirq = data->hwirq;
+
+		if (hwirq < domain->revmap_size) {
+			domain->linear_revmap[hwirq] = 0;
+		} else {
+			mutex_lock(&revmap_trees_mutex);
+			radix_tree_delete(&domain->revmap_tree, hwirq);
+			mutex_unlock(&revmap_trees_mutex);
+		}
+	}
+}
+
+static struct irq_data *irq_domain_insert_irq_data(struct irq_domain *domain,
+						   struct irq_data *child)
+{
+	struct irq_data *irq_data;
+
+	irq_data = kzalloc_node(sizeof(*irq_data), GFP_KERNEL, child->node);
+	if (irq_data) {
+		child->parent_data = irq_data;
+		irq_data->irq = child->irq;
+		irq_data->node = child->node;
+		irq_data->domain = domain;
+	}
+
+	return irq_data;
+}
+
+static void irq_domain_free_irq_data(unsigned int virq, unsigned int nr_irqs)
+{
+	int i;
+	struct irq_data *irq_data, *tmp;
+
+	for (i = 0; i < nr_irqs; i++) {
+		irq_data = irq_get_irq_data(virq + i);
+		tmp = irq_data->parent_data;
+		irq_data->parent_data = NULL;
+		irq_data->domain = NULL;
+
+		while (tmp) {
+			irq_data = tmp;
+			tmp = tmp->parent_data;
+			kfree(irq_data);
+		}
+	}
+}
+
+static int irq_domain_alloc_irq_data(struct irq_domain *domain,
+				     unsigned int virq, unsigned int nr_irqs)
+{
+	int i;
+	struct irq_data *irq_data;
+	struct irq_domain *parent;
+
+	/* The outermost irq_data is embedded in struct irq_desc */
+	for (i = 0; i < nr_irqs; i++) {
+		irq_data = irq_get_irq_data(virq + i);
+		irq_data->domain = domain;
+
+		for (parent = domain->parent; parent; parent = parent->parent) {
+			irq_data = irq_domain_insert_irq_data(parent, irq_data);
+			if (!irq_data) {
+				irq_domain_free_irq_data(virq, i + 1);
+				return -ENOMEM;
+			}
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * irq_domain_get_irq_data - Get irq_data associated with @virq and @domain
+ * @domain: domain to match
+ * @virq: IRQ number to get irq_data
+ */
+struct irq_data *irq_domain_get_irq_data(struct irq_domain *domain,
+					 unsigned int virq)
+{
+	struct irq_data *irq_data;
+
+	for (irq_data = irq_get_irq_data(virq); irq_data;
+	     irq_data = irq_data->parent_data)
+		if (irq_data->domain == domain)
+			return irq_data;
+
+	return NULL;
+}
+
+int irq_domain_set_hwirq_and_chip(struct irq_domain *domain, unsigned int virq,
+				  irq_hw_number_t hwirq, struct irq_chip *chip,
+				  void *chip_data)
+{
+	struct irq_data *irq_data = irq_domain_get_irq_data(domain, virq);
+
+	if (!irq_data)
+		return -ENOENT;
+
+	irq_data->hwirq = hwirq;
+	irq_data->chip = chip ? chip : &no_irq_chip;
+	irq_data->chip_data = chip_data;
+
+	return 0;
+}
+
+void irq_domain_reset_irq_data(struct irq_data *irq_data)
+{
+	irq_data->hwirq = 0;
+	irq_data->chip = &no_irq_chip;
+	irq_data->chip_data = NULL;
+}
+
+void irq_domain_free_irqs_common(struct irq_domain *domain, int virq,
+				 int nr_irqs)
+{
+	int i;
+	struct irq_data *irq_data;
+
+	for (i = 0; i < nr_irqs; i++) {
+		irq_data = irq_domain_get_irq_data(domain, virq + i);
+		if (irq_data)
+			irq_domain_reset_irq_data(irq_data);
+	}
+	irq_domain_free_irqs_parent(domain, virq, nr_irqs);
+}
+
+void irq_domain_free_irqs_top(struct irq_domain *domain, int virq,
+			      int nr_irqs)
+{
+	int i;
+
+	for (i = 0; i < nr_irqs; i++) {
+		irq_set_handler_data(virq + i, NULL);
+		irq_set_handler(virq + i, NULL);
+	}
+	irq_domain_free_irqs_common(domain, virq, nr_irqs);
+}
+
+/**
+ * __irq_domain_alloc_irqs - Allocate IRQs from domain
+ * @domain: domain to allocate from
+ * @irq_base: allocate specified IRQ nubmer if irq_base >= 0
+ * @nr_irqs: number of IRQs to allocate
+ * @node: NUMA node id for memory allocation
+ * @arg: domain specific argument
+ * @realloc: IRQ descriptors have already been allocated if true
+ *
+ * Allocate IRQ numbers and initialized all data structures to support
+ * hiearchy IRQ domains.
+ * Parameter @realloc is mainly to support legacy IRQs.
+ * Returns error code or allocated IRQ number
+ *
+ * The whole process to setup an IRQ has been split into two steps.
+ * The first step, __irq_domain_alloc_irqs(), is to allocate IRQ
+ * descriptor and required hardware resources. The second step,
+ * irq_domain_activate_irq(), is to program hardwares with preallocated
+ * resources. In this way, it's easier to rollback when failing to
+ * allocate resources.
+ */
+int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
+			    unsigned int nr_irqs, int node, void *arg,
+			    bool realloc)
+{
+	int i, ret, virq;
+
+	if (domain == NULL) {
+		domain = irq_default_domain;
+		if (WARN(!domain, "domain is NULL; cannot allocate IRQ\n"))
+			return -EINVAL;
+	}
+
+	if (!domain->ops->alloc) {
+		pr_debug("domain->ops->alloc() is NULL\n");
+		return -ENOSYS;
+	}
+
+	if (realloc && irq_base >= 0) {
+		virq = irq_base;
+	} else {
+		virq = irq_domain_alloc_descs(irq_base, nr_irqs, 0, node);
+		if (virq < 0) {
+			pr_debug("cannot allocate IRQ(base %d, count %d)\n",
+				 irq_base, nr_irqs);
+			return virq;
+		}
+	}
+
+	if (irq_domain_alloc_irq_data(domain, virq, nr_irqs)) {
+		pr_debug("cannot allocate memory for IRQ%d\n", virq);
+		ret = -ENOMEM;
+		goto out_free_desc;
+	}
+
+	mutex_lock(&irq_domain_mutex);
+	ret = domain->ops->alloc(domain, virq, nr_irqs, arg);
+	if (ret < 0) {
+		mutex_unlock(&irq_domain_mutex);
+		goto out_free_irq_data;
+	}
+	for (i = 0; i < nr_irqs; i++)
+		irq_domain_insert_irq(virq + i);
+	mutex_unlock(&irq_domain_mutex);
+
+	return virq;
+
+out_free_irq_data:
+	irq_domain_free_irq_data(virq, nr_irqs);
+out_free_desc:
+	irq_free_descs(virq, nr_irqs);
+	return ret;
+}
+
+/**
+ * irq_domain_free_irqs - Free IRQ number and associated data structures
+ * @virq: base IRQ number
+ * @nr_irqs: number of IRQs to free
+ */
+void irq_domain_free_irqs(unsigned int virq, unsigned int nr_irqs)
+{
+	int i;
+	struct irq_data *data = irq_get_irq_data(virq);
+
+	if (WARN(!data || !data->domain || !data->domain->ops->free,
+		 "NULL pointer, cannot free irq\n"))
+		return;
+
+	mutex_lock(&irq_domain_mutex);
+	for (i = 0; i < nr_irqs; i++)
+		irq_domain_remove_irq(virq + i);
+	data->domain->ops->free(data->domain, virq, nr_irqs);
+	mutex_unlock(&irq_domain_mutex);
+
+	irq_domain_free_irq_data(virq, nr_irqs);
+	irq_free_descs(virq, nr_irqs);
+}
+
+/**
+ * irq_domain_activate_irq - Call domain_ops->activate recursively to activate
+ *			     interrupt
+ * @irq_data: outermost irq_data associated with interrupt
+ *
+ * This is the second step to call domain_ops->activate to program interrupt
+ * controllers, so the interrupt could actually get delivered.
+ */
+void irq_domain_activate_irq(struct irq_data *irq_data)
+{
+	if (irq_data && irq_data->domain) {
+		struct irq_domain *domain = irq_data->domain;
+
+		if (irq_data->parent_data)
+			irq_domain_activate_irq(irq_data->parent_data);
+		if (domain->ops->activate)
+			domain->ops->activate(domain, irq_data);
+	}
+}
+
+/**
+ * irq_domain_deactivate_irq - Call domain_ops->deactivate recursively to
+ *			       deactivate interrupt
+ * @irq_data: outermost irq_data associated with interrupt
+ *
+ * It calls domain_ops->deactivate to program interrupt controllers to disable
+ * interrupt delivery.
+ */
+void irq_domain_deactivate_irq(struct irq_data *irq_data)
+{
+	if (irq_data && irq_data->domain) {
+		struct irq_domain *domain = irq_data->domain;
+
+		if (domain->ops->deactivate)
+			domain->ops->deactivate(domain, irq_data);
+		if (irq_data->parent_data)
+			irq_domain_deactivate_irq(irq_data->parent_data);
+	}
+}
+
+static void irq_domain_check_hierarchy(struct irq_domain *domain)
+{
+	/* Hierarchy irq_domains must implement callback alloc() */
+	if (domain->ops->alloc)
+		domain->flags |= IRQ_DOMAIN_FLAG_HIERARCHY;
+}
+#else	/* CONFIG_IRQ_DOMAIN_HIERARCHY */
+/**
+ * irq_domain_get_irq_data - Get irq_data associated with @virq and @domain
+ * @domain: domain to match
+ * @virq: IRQ number to get irq_data
+ */
+struct irq_data *irq_domain_get_irq_data(struct irq_domain *domain,
+					 unsigned int virq)
+{
+	struct irq_data *irq_data = irq_get_irq_data(virq);
+
+	return (irq_data && irq_data->domain == domain) ? irq_data : NULL;
+}
+
+static void irq_domain_check_hierarchy(struct irq_domain *domain)
+{
+}
+#endif	/* CONFIG_IRQ_DOMAIN_HIERARCHY */
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
.

Hi Thomas, Jiang,

Quoted 2 lines · Thomas Gleixner

From: Jiang Liu <[email protected]>

[...]

Quoted 5 lines

/* Number of irqs reserved for a legacy isa controller */
#define NUM_ISA_INTERRUPTS 16

include/linux/irqdomain.h +0 -0

@@ -64,6 +66,16 @@ struct irq_domain_ops {
 	int (*xlate)(struct irq_domain *d, struct device_node *node,
 		     const u32 *intspec, unsigned int intsize,

include/linux/irqdomain.h struct irq_domain_ops { · line 77

Quoted 10 lines

include/linux/irqdomain.h +9 -0

 		     unsigned long *out_hwirq, unsigned int *out_type);
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	/* extended V2 interfaces to support hierarchy irq_domains */
+	int (*alloc)(struct irq_domain *d, unsigned int virq,
+		     unsigned int nr_irqs, void *arg);
+	void (*free)(struct irq_domain *d, unsigned int virq,
+		     unsigned int nr_irqs);
+	void (*activate)(struct irq_domain *d, struct irq_data *irq_data);
+	void (*deactivate)(struct irq_domain *d, struct irq_data *irq_data);

What's the usage of the parameter domain reference in activate/deactivate? I think the purpose of the two callbacks is to activate/deactivate the irq_data->hwirq in irq_data->domain. If so, the first parameter @domain is required to be equal to irq_data->domain (which makes @domain useless). Besides, the main responsibility of interrupt domains is to manage mappings between hardware and linux interrupt numbers, so would it be better if move the two callbacks into struct irq_chip?

Quoted 22 lines · Yun Wu (Abel)

Hi Thomas, Jiang,

Quoted 2 lines · Thomas Gleixner

From: Jiang Liu <[email protected]>

[...]

Quoted 5 lines

/* Number of irqs reserved for a legacy isa controller */
#define NUM_ISA_INTERRUPTS 16

include/linux/irqdomain.h +0 -0

@@ -64,6 +66,16 @@ struct irq_domain_ops {
 	int (*xlate)(struct irq_domain *d, struct device_node *node,
 		     const u32 *intspec, unsigned int intsize,

include/linux/irqdomain.h struct irq_domain_ops { · line 77

Quoted 10 lines

include/linux/irqdomain.h +9 -0

 		     unsigned long *out_hwirq, unsigned int *out_type);
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	/* extended V2 interfaces to support hierarchy irq_domains */
+	int (*alloc)(struct irq_domain *d, unsigned int virq,
+		     unsigned int nr_irqs, void *arg);
+	void (*free)(struct irq_domain *d, unsigned int virq,
+		     unsigned int nr_irqs);
+	void (*activate)(struct irq_domain *d, struct irq_data *irq_data);
+	void (*deactivate)(struct irq_domain *d, struct irq_data *irq_data);

Quoted 7 lines

What's the usage of the parameter domain reference in activate/deactivate? I think the purpose of the two callbacks is to activate/deactivate the irq_data->hwirq in irq_data->domain. If so, the first parameter @domain is required to be equal to irq_data->domain (which makes @domain useless). Besides, the main responsibility of interrupt domains is to manage mappings between hardware and linux interrupt numbers, so would it be better if move the two callbacks into struct irq_chip?

No. It's not a function of the irq_chip to activate/deactivate a hierarchy. As I explained you before:

The existing irqdomain code maps between hardware and virtual interrupts and thereby activates the interrupt in hardware.

In the hierarchical case we do not touch the hardware in the allocation step, so we need to activate the allocated interrupt in the hardware before we can use it. And that's clearly a domain interface not a irq chip issue.

Quoted 34 lines · Thomas Gleixner

Quoted 22 lines · Yun Wu (Abel)

Hi Thomas, Jiang,

Quoted 2 lines · Thomas Gleixner

From: Jiang Liu <[email protected]>

[...]

Quoted 5 lines

/* Number of irqs reserved for a legacy isa controller */
#define NUM_ISA_INTERRUPTS 16

include/linux/irqdomain.h +0 -0

@@ -64,6 +66,16 @@ struct irq_domain_ops {
 	int (*xlate)(struct irq_domain *d, struct device_node *node,
 		     const u32 *intspec, unsigned int intsize,

include/linux/irqdomain.h struct irq_domain_ops { · line 77

Quoted 10 lines

include/linux/irqdomain.h +9 -0

 		     unsigned long *out_hwirq, unsigned int *out_type);
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	/* extended V2 interfaces to support hierarchy irq_domains */
+	int (*alloc)(struct irq_domain *d, unsigned int virq,
+		     unsigned int nr_irqs, void *arg);
+	void (*free)(struct irq_domain *d, unsigned int virq,
+		     unsigned int nr_irqs);
+	void (*activate)(struct irq_domain *d, struct irq_data *irq_data);
+	void (*deactivate)(struct irq_domain *d, struct irq_data *irq_data);

Quoted 7 lines

What's the usage of the parameter domain reference in activate/deactivate? I think the purpose of the two callbacks is to activate/deactivate the irq_data->hwirq in irq_data->domain. If so, the first parameter @domain is required to be equal to irq_data->domain (which makes @domain useless). Besides, the main responsibility of interrupt domains is to manage mappings between hardware and linux interrupt numbers, so would it be better if move the two callbacks into struct irq_chip?

No. It's not a function of the irq_chip to activate/deactivate a hierarchy. As I explained you before:

Quoted 8 lines

The existing irqdomain code maps between hardware and virtual interrupts and thereby activates the interrupt in hardware.

In the hierarchical case we do not touch the hardware in the allocation step, so we need to activate the allocated interrupt in the hardware before we can use it. And that's clearly a domain interface not a irq chip issue.

Makes sense, now the interrupt domain seems to be the best place. And when the @domain parameter can be really useful? I haven't see anyone using it so far.

From an object oriented point of view, we pass the object as the

Thanks,
Abel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 45 lines · Yun Wu (Abel)

Quoted 34 lines · Thomas Gleixner

Quoted 22 lines · Yun Wu (Abel)

Hi Thomas, Jiang,

Quoted 2 lines · Thomas Gleixner

From: Jiang Liu <[email protected]>

[...]

Quoted 5 lines

/* Number of irqs reserved for a legacy isa controller */
#define NUM_ISA_INTERRUPTS 16

include/linux/irqdomain.h +0 -0

@@ -64,6 +66,16 @@ struct irq_domain_ops {
 	int (*xlate)(struct irq_domain *d, struct device_node *node,
 		     const u32 *intspec, unsigned int intsize,

include/linux/irqdomain.h struct irq_domain_ops { · line 77

Quoted 10 lines

include/linux/irqdomain.h +9 -0

 		     unsigned long *out_hwirq, unsigned int *out_type);
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	/* extended V2 interfaces to support hierarchy irq_domains */
+	int (*alloc)(struct irq_domain *d, unsigned int virq,
+		     unsigned int nr_irqs, void *arg);
+	void (*free)(struct irq_domain *d, unsigned int virq,
+		     unsigned int nr_irqs);
+	void (*activate)(struct irq_domain *d, struct irq_data *irq_data);
+	void (*deactivate)(struct irq_domain *d, struct irq_data *irq_data);

Quoted 7 lines

What's the usage of the parameter domain reference in activate/deactivate? I think the purpose of the two callbacks is to activate/deactivate the irq_data->hwirq in irq_data->domain. If so, the first parameter @domain is required to be equal to irq_data->domain (which makes @domain useless). Besides, the main responsibility of interrupt domains is to manage mappings between hardware and linux interrupt numbers, so would it be better if move the two callbacks into struct irq_chip?

No. It's not a function of the irq_chip to activate/deactivate a hierarchy. As I explained you before:

Quoted 8 lines

The existing irqdomain code maps between hardware and virtual interrupts and thereby activates the interrupt in hardware.

In the hierarchical case we do not touch the hardware in the allocation step, so we need to activate the allocated interrupt in the hardware before we can use it. And that's clearly a domain interface not a irq chip issue.

Quoted 3 lines

Makes sense, now the interrupt domain seems to be the best place. And when the @domain parameter can be really useful? I haven't see anyone using it so far.

We will use it for IOAPIC on x86, as below:
void mp_irqdomain_deactivate(struct irq_domain *domain,
struct irq_data *irq_data)

{
        ioapic_mask_entry(mp_irqdomain_ioapic_idx(domain),
                          (int)irq_data->hwirq);
}

Quoted 1 line

From an object oriented point of view, we pass the object as the

first parameter. It's true that we could retrieve domain from irq_data->domain instead of explicitly passing it in, but that will cause irqdomain interfaces depends on irq_data, not sounds a good situation:)

Quoted 57 lines · Jiang Liu

Quoted 45 lines · Yun Wu (Abel)

Quoted 34 lines · Thomas Gleixner

Quoted 22 lines · Yun Wu (Abel)

Hi Thomas, Jiang,

Quoted 2 lines · Thomas Gleixner

From: Jiang Liu <[email protected]>

[...]

Quoted 5 lines

/* Number of irqs reserved for a legacy isa controller */
#define NUM_ISA_INTERRUPTS 16

include/linux/irqdomain.h +0 -0

@@ -64,6 +66,16 @@ struct irq_domain_ops {
 	int (*xlate)(struct irq_domain *d, struct device_node *node,
 		     const u32 *intspec, unsigned int intsize,

include/linux/irqdomain.h struct irq_domain_ops { · line 77

Quoted 10 lines

include/linux/irqdomain.h +9 -0

 		     unsigned long *out_hwirq, unsigned int *out_type);
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	/* extended V2 interfaces to support hierarchy irq_domains */
+	int (*alloc)(struct irq_domain *d, unsigned int virq,
+		     unsigned int nr_irqs, void *arg);
+	void (*free)(struct irq_domain *d, unsigned int virq,
+		     unsigned int nr_irqs);
+	void (*activate)(struct irq_domain *d, struct irq_data *irq_data);
+	void (*deactivate)(struct irq_domain *d, struct irq_data *irq_data);

Quoted 7 lines

What's the usage of the parameter domain reference in activate/deactivate? I think the purpose of the two callbacks is to activate/deactivate the irq_data->hwirq in irq_data->domain. If so, the first parameter @domain is required to be equal to irq_data->domain (which makes @domain useless). Besides, the main responsibility of interrupt domains is to manage mappings between hardware and linux interrupt numbers, so would it be better if move the two callbacks into struct irq_chip?

No. It's not a function of the irq_chip to activate/deactivate a hierarchy. As I explained you before:

Quoted 8 lines

The existing irqdomain code maps between hardware and virtual interrupts and thereby activates the interrupt in hardware.

In the hierarchical case we do not touch the hardware in the allocation step, so we need to activate the allocated interrupt in the hardware before we can use it. And that's clearly a domain interface not a irq chip issue.

Quoted 3 lines

Makes sense, now the interrupt domain seems to be the best place. And when the @domain parameter can be really useful? I haven't see anyone using it so far.

We will use it for IOAPIC on x86, as below:
void mp_irqdomain_deactivate(struct irq_domain *domain,
struct irq_data *irq_data)

{
        ioapic_mask_entry(mp_irqdomain_ioapic_idx(domain),
                          (int)irq_data->hwirq);
}

Quoted 5 lines

Quoted 1 line

From an object oriented point of view, we pass the object as the

first parameter. It's true that we could retrieve domain from irq_data->domain instead of explicitly passing it in, but that will cause irqdomain interfaces depends on irq_data, not sounds a good situation:)

Hi Gerry,

Is there any possibility that domain doesn't equal to irq_data->domain? I'm a little confused..

Quoted 67 lines · Yun Wu (Abel)

Quoted 57 lines · Jiang Liu

Quoted 45 lines · Yun Wu (Abel)

Quoted 34 lines · Thomas Gleixner

Quoted 22 lines · Yun Wu (Abel)

Hi Thomas, Jiang,

Quoted 2 lines · Thomas Gleixner

From: Jiang Liu <[email protected]>

[...]

Quoted 5 lines

/* Number of irqs reserved for a legacy isa controller */
#define NUM_ISA_INTERRUPTS 16

(quoted diff) +0 -0

@@ -64,6 +66,16 @@ struct irq_domain_ops {
 	int (*xlate)(struct irq_domain *d, struct device_node *node,
 		     const u32 *intspec, unsigned int intsize,

Review struct irq_domain_ops { · line 77

Quoted 10 lines

(quoted diff) +9 -0

 		     unsigned long *out_hwirq, unsigned int *out_type);
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	/* extended V2 interfaces to support hierarchy irq_domains */
+	int (*alloc)(struct irq_domain *d, unsigned int virq,
+		     unsigned int nr_irqs, void *arg);
+	void (*free)(struct irq_domain *d, unsigned int virq,
+		     unsigned int nr_irqs);
+	void (*activate)(struct irq_domain *d, struct irq_data *irq_data);
+	void (*deactivate)(struct irq_domain *d, struct irq_data *irq_data);

Quoted 7 lines

What's the usage of the parameter domain reference in activate/deactivate? I think the purpose of the two callbacks is to activate/deactivate the irq_data->hwirq in irq_data->domain. If so, the first parameter @domain is required to be equal to irq_data->domain (which makes @domain useless). Besides, the main responsibility of interrupt domains is to manage mappings between hardware and linux interrupt numbers, so would it be better if move the two callbacks into struct irq_chip?

No. It's not a function of the irq_chip to activate/deactivate a hierarchy. As I explained you before:

Quoted 8 lines

The existing irqdomain code maps between hardware and virtual interrupts and thereby activates the interrupt in hardware.

In the hierarchical case we do not touch the hardware in the allocation step, so we need to activate the allocated interrupt in the hardware before we can use it. And that's clearly a domain interface not a irq chip issue.

Quoted 3 lines

Makes sense, now the interrupt domain seems to be the best place. And when the @domain parameter can be really useful? I haven't see anyone using it so far.

We will use it for IOAPIC on x86, as below:
void mp_irqdomain_deactivate(struct irq_domain *domain,
struct irq_data *irq_data)

{
        ioapic_mask_entry(mp_irqdomain_ioapic_idx(domain),
                          (int)irq_data->hwirq);
}

Quoted 5 lines

Quoted 1 line

From an object oriented point of view, we pass the object as the

first parameter. It's true that we could retrieve domain from irq_data->domain instead of explicitly passing it in, but that will cause irqdomain interfaces depends on irq_data, not sounds a good situation:)

Hi Gerry,

Quoted 2 lines

Is there any possibility that domain doesn't equal to irq_data->domain? I'm a little confused..

Hi Yun,
Currently they are always the same, but we don't want irqdomain
interfaces make assumption of struct irq_data. If it will bring big performance improvement, we will try to kill the first parameter, otherwise we may prefer keeping irqdomain interfaces clear.

Quoted 5 lines · Thomas Gleixner

In the hierarchical case we do not touch the hardware in the allocation step, so we need to activate the allocated interrupt in the hardware before we can use it. And that's clearly a domain interface not a irq chip issue.

Makes sense, now the interrupt domain seems to be the best place. And when the @domain parameter can be really useful? I haven't see anyone using it so far.

Quoted 10 lines · Yun Wu (Abel)

Quoted 5 lines · Thomas Gleixner

In the hierarchical case we do not touch the hardware in the allocation step, so we need to activate the allocated interrupt in the hardware before we can use it. And that's clearly a domain interface not a irq chip issue.

Makes sense, now the interrupt domain seems to be the best place. And when the @domain parameter can be really useful? I haven't see anyone using it so far.

All irqdomain callbacks take the domain pointer as their first parameter. So for consistency sake we made it that way.

You can argue in circles about whether the domain argument could be removed. It's going to stay for now as it does not matter at all.

From: Jiang Liu <[email protected]>

Add callback irq_compose_msi_msg to struct irq_chip, which will be used to support stacked irqchip.

Signed-off-by Jiang Liu <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Cc Grant Likely <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Yingjoe Chen <[email protected]>

Cc Yijing Wang <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
 include/linux/irq.h |    5 +++++
 kernel/irq/chip.c   |   17 +++++++++++++++++
 2 files changed, 22 insertions(+)

include/linux/irq.h +5 -0

index 0adcbbbf2e87..536b7fc6c8f4 100644
@@ -29,6 +29,7 @@ struct seq_file;
 struct module;
 struct irq_desc;
 struct irq_data;
+struct msi_msg;
 typedef	void (*irq_flow_handler_t)(unsigned int irq,
 					    struct irq_desc *desc);
 typedef	void (*irq_preflow_handler_t)(struct irq_data *data);

@@ -320,6 +321,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
  *				any other callback related to this irq
  * @irq_release_resources:	optional to release resources acquired with
  *				irq_request_resources
+ * @irq_compose_msi_msg:	optional to compose message content for MSI
  * @flags:		chip specific flags
  */
 struct irq_chip {

@@ -356,6 +358,8 @@ struct irq_chip {
 	int		(*irq_request_resources)(struct irq_data *data);
 	void		(*irq_release_resources)(struct irq_data *data);
 
+	void		(*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
+
 	unsigned long	flags;
 };
 

@@ -443,6 +447,7 @@ extern void handle_percpu_devid_irq(unsigned int irq, struct irq_desc *desc);
 extern void handle_bad_irq(unsigned int irq, struct irq_desc *desc);
 extern void handle_nested_irq(unsigned int irq);
 
+extern int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg);
 #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
 extern void irq_chip_ack_parent(struct irq_data *data);
 extern int irq_chip_retrigger_hierarchy(struct irq_data *data);

kernel/irq/chip.c +7 -0

index 12f3e72449eb..8f362db17a8a 100644
@@ -867,3 +867,20 @@ int irq_chip_retrigger_hierarchy(struct irq_data *data)
 	return -ENOSYS;
 }
 #endif
+
+int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct irq_data *pos = NULL;
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	for (; data; data = data->parent_data)

Quoted 6 lines · Thomas Gleixner

kernel/irq/chip.c +6 -0

+int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct irq_data *pos = NULL;
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	for (; data; data = data->parent_data)

kernel/irq/chip.c int irq_chip_retrigger_hierarchy(struct irq_data *data) · line 886

Quoted 10 lines

kernel/irq/chip.c +10 -0

+#endif
+		if (data->chip && data->chip->irq_compose_msi_msg)
+			pos = data;
+	if (!pos)
+		return -ENOSYS;
+
+	pos->chip->irq_compose_msi_msg(pos, msg);
+
+	return 0;
+}

Adding message composing routine to struct irq_chip is OK to me, and it should be because it is interrupt controllers' duty to compose messages (so that they can parse the messages correctly without any pre-defined rules that endpoint devices absolutely need not to know).
However a problem comes out when deciding which parameters should be passed to

this routine. A message can associate with multiple interrupts, which makes me think composing messages for each interrupt is not that appropriate. And we can take a look at the new routine irq_chip_compose_msi_msg(). It is called by msi_domain_activate() which will be called by irq_domain_activate_irq() in irq_startup() for each interrupt descriptor, result in composing a message for each interrupt, right? (Unless requiring a judge on the parameter @data when implementing the irq_compose_msi_msg() callback that only compose message for the first entry of that message. But I really don't like that...)

Quoted 23 lines · Yun Wu (Abel)

Quoted 6 lines · Thomas Gleixner

kernel/irq/chip.c +6 -0

+int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct irq_data *pos = NULL;
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	for (; data; data = data->parent_data)

kernel/irq/chip.c int irq_chip_retrigger_hierarchy(struct irq_data *data) · line 886

Quoted 10 lines

kernel/irq/chip.c +10 -0

+#endif
+		if (data->chip && data->chip->irq_compose_msi_msg)
+			pos = data;
+	if (!pos)
+		return -ENOSYS;
+
+	pos->chip->irq_compose_msi_msg(pos, msg);
+
+	return 0;
+}

Adding message composing routine to struct irq_chip is OK to me, and it should be because it is interrupt controllers' duty to compose messages (so that they can parse the messages correctly without any pre-defined rules that endpoint devices absolutely need not to know).
However a problem comes out when deciding which parameters should be passed to

Quoted 8 lines

this routine. A message can associate with multiple interrupts, which makes me think composing messages for each interrupt is not that appropriate. And we can take a look at the new routine irq_chip_compose_msi_msg(). It is called by msi_domain_activate() which will be called by irq_domain_activate_irq() in irq_startup() for each interrupt descriptor, result in composing a message for each interrupt, right? (Unless requiring a judge on the parameter @data when implementing the irq_compose_msi_msg() callback that only compose message for the first entry of that message. But I really don't like that...)

No, that's not correct. You are looking at some random stale version of this. The current state of affairs is in

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/irqdomain

See also https://lkml.org/lkml/2014/11/17/764

In activate we write the message, which is the right point to do so.

Quoted 33 lines · Thomas Gleixner

Quoted 23 lines · Yun Wu (Abel)

Quoted 6 lines · Thomas Gleixner

kernel/irq/chip.c +6 -0

+int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct irq_data *pos = NULL;
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	for (; data; data = data->parent_data)

kernel/irq/chip.c int irq_chip_retrigger_hierarchy(struct irq_data *data) · line 886

Quoted 10 lines

kernel/irq/chip.c +10 -0

+#endif
+		if (data->chip && data->chip->irq_compose_msi_msg)
+			pos = data;
+	if (!pos)
+		return -ENOSYS;
+
+	pos->chip->irq_compose_msi_msg(pos, msg);
+
+	return 0;
+}

Adding message composing routine to struct irq_chip is OK to me, and it should be because it is interrupt controllers' duty to compose messages (so that they can parse the messages correctly without any pre-defined rules that endpoint devices absolutely need not to know).
However a problem comes out when deciding which parameters should be passed to

Quoted 8 lines

this routine. A message can associate with multiple interrupts, which makes me think composing messages for each interrupt is not that appropriate. And we can take a look at the new routine irq_chip_compose_msi_msg(). It is called by msi_domain_activate() which will be called by irq_domain_activate_irq() in irq_startup() for each interrupt descriptor, result in composing a message for each interrupt, right? (Unless requiring a judge on the parameter @data when implementing the irq_compose_msi_msg() callback that only compose message for the first entry of that message. But I really don't like that...)

Quoted 9 lines

No, that's not correct. You are looking at some random stale version of this. The current state of affairs is in

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/irqdomain

See also https://lkml.org/lkml/2014/11/17/764

In activate we write the message, which is the right point to do so.

I checked the current state, it seems to be the same.
Yes, the decision of postponing the actual hardware programming to the point where the interrupt actually gets used is right, but here above I was talking another thing.
As I mentioned, a message can associate with multiple interrupts. Enabling any of them will call irq_startup(). So if we don't want to compose or write messages repeatedly, we'd better require performing some checks before activating the interrupts.

Thanks,
Abel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 45 lines · Yun Wu (Abel)

Quoted 33 lines · Thomas Gleixner

Quoted 23 lines · Yun Wu (Abel)

Quoted 6 lines · Thomas Gleixner

kernel/irq/chip.c +6 -0

+int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct irq_data *pos = NULL;
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	for (; data; data = data->parent_data)

kernel/irq/chip.c int irq_chip_retrigger_hierarchy(struct irq_data *data) · line 886

Quoted 10 lines

kernel/irq/chip.c +10 -0

+#endif
+		if (data->chip && data->chip->irq_compose_msi_msg)
+			pos = data;
+	if (!pos)
+		return -ENOSYS;
+
+	pos->chip->irq_compose_msi_msg(pos, msg);
+
+	return 0;
+}

Adding message composing routine to struct irq_chip is OK to me, and it should be because it is interrupt controllers' duty to compose messages (so that they can parse the messages correctly without any pre-defined rules that endpoint devices absolutely need not to know).
However a problem comes out when deciding which parameters should be passed to

Quoted 8 lines

this routine. A message can associate with multiple interrupts, which makes me think composing messages for each interrupt is not that appropriate. And we can take a look at the new routine irq_chip_compose_msi_msg(). It is called by msi_domain_activate() which will be called by irq_domain_activate_irq() in irq_startup() for each interrupt descriptor, result in composing a message for each interrupt, right? (Unless requiring a judge on the parameter @data when implementing the irq_compose_msi_msg() callback that only compose message for the first entry of that message. But I really don't like that...)

Quoted 9 lines

No, that's not correct. You are looking at some random stale version of this. The current state of affairs is in

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/irqdomain

See also https://lkml.org/lkml/2014/11/17/764

In activate we write the message, which is the right point to do so.

Quoted 8 lines

I checked the current state, it seems to be the same.
Yes, the decision of postponing the actual hardware programming to the point where the interrupt actually gets used is right, but here above I was talking another thing.
As I mentioned, a message can associate with multiple interrupts. Enabling any of them will call irq_startup(). So if we don't want to compose or write messages repeatedly, we'd better require performing some checks before activating the interrupts.

Hi Yun,
Seems you are talking about the case of multiple MSI support.
Yes, we have special treatment for multiple MSI, which only writes PCI MSI registers when starting up the first MSI interrupt.
void pci_msi_domain_write_msg(struct irq_data *irq_data, struct msi_msg *msg)

{
        struct msi_desc *desc = irq_data->msi_desc;

        /*
         * For MSI-X desc->irq is always equal to irq_data->irq. For
         * MSI only the first interrupt of MULTI MSI passes the test.
         */
        if (desc->irq == irq_data->irq)
                __pci_write_msi_msg(desc, msg);
}

Quoted 60 lines · Jiang Liu

Quoted 45 lines · Yun Wu (Abel)

Quoted 33 lines · Thomas Gleixner

Quoted 23 lines · Yun Wu (Abel)

Quoted 6 lines · Thomas Gleixner

kernel/irq/chip.c +6 -0

+int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct irq_data *pos = NULL;
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	for (; data; data = data->parent_data)

kernel/irq/chip.c int irq_chip_retrigger_hierarchy(struct irq_data *data) · line 886

Quoted 10 lines

kernel/irq/chip.c +10 -0

+#endif
+		if (data->chip && data->chip->irq_compose_msi_msg)
+			pos = data;
+	if (!pos)
+		return -ENOSYS;
+
+	pos->chip->irq_compose_msi_msg(pos, msg);
+
+	return 0;
+}

Adding message composing routine to struct irq_chip is OK to me, and it should be because it is interrupt controllers' duty to compose messages (so that they can parse the messages correctly without any pre-defined rules that endpoint devices absolutely need not to know).
However a problem comes out when deciding which parameters should be passed to

Quoted 8 lines

this routine. A message can associate with multiple interrupts, which makes me think composing messages for each interrupt is not that appropriate. And we can take a look at the new routine irq_chip_compose_msi_msg(). It is called by msi_domain_activate() which will be called by irq_domain_activate_irq() in irq_startup() for each interrupt descriptor, result in composing a message for each interrupt, right? (Unless requiring a judge on the parameter @data when implementing the irq_compose_msi_msg() callback that only compose message for the first entry of that message. But I really don't like that...)

Quoted 9 lines

No, that's not correct. You are looking at some random stale version of this. The current state of affairs is in

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/irqdomain

See also https://lkml.org/lkml/2014/11/17/764

In activate we write the message, which is the right point to do so.

Quoted 8 lines

I checked the current state, it seems to be the same.
Yes, the decision of postponing the actual hardware programming to the point where the interrupt actually gets used is right, but here above I was talking another thing.
As I mentioned, a message can associate with multiple interrupts. Enabling any of them will call irq_startup(). So if we don't want to compose or write messages repeatedly, we'd better require performing some checks before activating the interrupts.

Hi Yun,
Seems you are talking about the case of multiple MSI support.
Yes, we have special treatment for multiple MSI, which only writes PCI MSI registers when starting up the first MSI interrupt.
void pci_msi_domain_write_msg(struct irq_data *irq_data, struct msi_msg *msg)

Quoted 10 lines

{
        struct msi_desc *desc = irq_data->msi_desc;

        /*
         * For MSI-X desc->irq is always equal to irq_data->irq. For
         * MSI only the first interrupt of MULTI MSI passes the test.
         */
        if (desc->irq == irq_data->irq)
                __pci_write_msi_msg(desc, msg);
}

Yes, I picked the case of multiple MSI support.
The check should also be performed when composing messages. That's why I don't like its parameters. The @data only indicates one interrupt, while I prefer doing compose/write in the unit of message descriptor.

Quoted 74 lines · Yun Wu (Abel)

Quoted 60 lines · Jiang Liu

Quoted 45 lines · Yun Wu (Abel)

Quoted 33 lines · Thomas Gleixner

Quoted 23 lines · Yun Wu (Abel)

Quoted 6 lines · Thomas Gleixner

(quoted diff) +6 -0

+int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct irq_data *pos = NULL;
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	for (; data; data = data->parent_data)

Review

Quoted 10 lines

(quoted diff) +10 -0

+#endif
+		if (data->chip && data->chip->irq_compose_msi_msg)
+			pos = data;
+	if (!pos)
+		return -ENOSYS;
+
+	pos->chip->irq_compose_msi_msg(pos, msg);
+
+	return 0;
+}

Adding message composing routine to struct irq_chip is OK to me, and it should be because it is interrupt controllers' duty to compose messages (so that they can parse the messages correctly without any pre-defined rules that endpoint devices absolutely need not to know).
However a problem comes out when deciding which parameters should be passed to

Quoted 8 lines

this routine. A message can associate with multiple interrupts, which makes me think composing messages for each interrupt is not that appropriate. And we can take a look at the new routine irq_chip_compose_msi_msg(). It is called by msi_domain_activate() which will be called by irq_domain_activate_irq() in irq_startup() for each interrupt descriptor, result in composing a message for each interrupt, right? (Unless requiring a judge on the parameter @data when implementing the irq_compose_msi_msg() callback that only compose message for the first entry of that message. But I really don't like that...)

Quoted 9 lines

No, that's not correct. You are looking at some random stale version of this. The current state of affairs is in

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/irqdomain

See also https://lkml.org/lkml/2014/11/17/764

In activate we write the message, which is the right point to do so.

Quoted 8 lines

I checked the current state, it seems to be the same.
Yes, the decision of postponing the actual hardware programming to the point where the interrupt actually gets used is right, but here above I was talking another thing.
As I mentioned, a message can associate with multiple interrupts. Enabling any of them will call irq_startup(). So if we don't want to compose or write messages repeatedly, we'd better require performing some checks before activating the interrupts.

Hi Yun,
Seems you are talking about the case of multiple MSI support.
Yes, we have special treatment for multiple MSI, which only writes PCI MSI registers when starting up the first MSI interrupt.
void pci_msi_domain_write_msg(struct irq_data *irq_data, struct msi_msg *msg)

Quoted 10 lines

{
        struct msi_desc *desc = irq_data->msi_desc;

        /*
         * For MSI-X desc->irq is always equal to irq_data->irq. For
         * MSI only the first interrupt of MULTI MSI passes the test.
         */
        if (desc->irq == irq_data->irq)
                __pci_write_msi_msg(desc, msg);
}

Quoted 4 lines

Yes, I picked the case of multiple MSI support.
The check should also be performed when composing messages. That's why I don't like its parameters. The @data only indicates one interrupt, while I prefer doing compose/write in the unit of message descriptor.

Hi Yun,
The common abstraction is that every message interrupt could be
controlled independently, so have compose_msi_msg()/write_msi_msg() per interrupt. MSI is abstracted as an special message signaled interrupt with hardware limitation where multiple interrupts sharing the same hardware registers. So we filter in pci_msi_domain_write_msg(). On the other handle, the generic MSI framework caches msi_msg in msi_desc, so we don't filter compose_msi_msg().

Quoted 79 lines · Jiang Liu

Quoted 74 lines · Yun Wu (Abel)

Quoted 60 lines · Jiang Liu

Quoted 45 lines · Yun Wu (Abel)

Quoted 33 lines · Thomas Gleixner

Quoted 23 lines · Yun Wu (Abel)

Quoted 6 lines · Thomas Gleixner

(quoted diff) +6 -0

+int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct irq_data *pos = NULL;
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	for (; data; data = data->parent_data)

Review

Quoted 10 lines

(quoted diff) +10 -0

+#endif
+		if (data->chip && data->chip->irq_compose_msi_msg)
+			pos = data;
+	if (!pos)
+		return -ENOSYS;
+
+	pos->chip->irq_compose_msi_msg(pos, msg);
+
+	return 0;
+}

Adding message composing routine to struct irq_chip is OK to me, and it should be because it is interrupt controllers' duty to compose messages (so that they can parse the messages correctly without any pre-defined rules that endpoint devices absolutely need not to know).
However a problem comes out when deciding which parameters should be passed to

Quoted 8 lines

this routine. A message can associate with multiple interrupts, which makes me think composing messages for each interrupt is not that appropriate. And we can take a look at the new routine irq_chip_compose_msi_msg(). It is called by msi_domain_activate() which will be called by irq_domain_activate_irq() in irq_startup() for each interrupt descriptor, result in composing a message for each interrupt, right? (Unless requiring a judge on the parameter @data when implementing the irq_compose_msi_msg() callback that only compose message for the first entry of that message. But I really don't like that...)

Quoted 9 lines

No, that's not correct. You are looking at some random stale version of this. The current state of affairs is in

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/irqdomain

See also https://lkml.org/lkml/2014/11/17/764

In activate we write the message, which is the right point to do so.

Quoted 8 lines

I checked the current state, it seems to be the same.
Yes, the decision of postponing the actual hardware programming to the point where the interrupt actually gets used is right, but here above I was talking another thing.
As I mentioned, a message can associate with multiple interrupts. Enabling any of them will call irq_startup(). So if we don't want to compose or write messages repeatedly, we'd better require performing some checks before activating the interrupts.

Hi Yun,
Seems you are talking about the case of multiple MSI support.
Yes, we have special treatment for multiple MSI, which only writes PCI MSI registers when starting up the first MSI interrupt.
void pci_msi_domain_write_msg(struct irq_data *irq_data, struct msi_msg *msg)

Quoted 10 lines

{
        struct msi_desc *desc = irq_data->msi_desc;

        /*
         * For MSI-X desc->irq is always equal to irq_data->irq. For
         * MSI only the first interrupt of MULTI MSI passes the test.
         */
        if (desc->irq == irq_data->irq)
                __pci_write_msi_msg(desc, msg);
}

Quoted 4 lines

Yes, I picked the case of multiple MSI support.
The check should also be performed when composing messages. That's why I don't like its parameters. The @data only indicates one interrupt, while I prefer doing compose/write in the unit of message descriptor.

Quoted 9 lines

Hi Yun,
The common abstraction is that every message interrupt could be
controlled independently, so have compose_msi_msg()/write_msi_msg() per interrupt. MSI is abstracted as an special message signaled interrupt with hardware limitation where multiple interrupts sharing the same hardware registers. So we filter in pci_msi_domain_write_msg(). On the other handle, the generic MSI framework caches msi_msg in msi_desc, so we don't filter compose_msi_msg().

It's true that every message interrupt could be controlled independently, I mean, by enable/disable/mask/unmask. But the message data & address are shared among the interrupts of that message.
Despite the detailed hardware implementation, MSI and MSI-X are the same thing in software view, that is a message related with several consecutive interrupts. And the core MSI infrastructure you want to build should not be based on any hardware assumptions.

Thanks,
Abel

Quoted 91 lines · Yun Wu (Abel)

Quoted 79 lines · Jiang Liu

Quoted 74 lines · Yun Wu (Abel)

Quoted 60 lines · Jiang Liu

Quoted 45 lines · Yun Wu (Abel)

Quoted 33 lines · Thomas Gleixner

Quoted 23 lines · Yun Wu (Abel)

Quoted 6 lines · Thomas Gleixner

(quoted diff) +6 -0

+int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct irq_data *pos = NULL;
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	for (; data; data = data->parent_data)

Review

Quoted 10 lines

(quoted diff) +10 -0

+#endif
+		if (data->chip && data->chip->irq_compose_msi_msg)
+			pos = data;
+	if (!pos)
+		return -ENOSYS;
+
+	pos->chip->irq_compose_msi_msg(pos, msg);
+
+	return 0;
+}

Adding message composing routine to struct irq_chip is OK to me, and it should be because it is interrupt controllers' duty to compose messages (so that they can parse the messages correctly without any pre-defined rules that endpoint devices absolutely need not to know).
However a problem comes out when deciding which parameters should be passed to

Quoted 8 lines

this routine. A message can associate with multiple interrupts, which makes me think composing messages for each interrupt is not that appropriate. And we can take a look at the new routine irq_chip_compose_msi_msg(). It is called by msi_domain_activate() which will be called by irq_domain_activate_irq() in irq_startup() for each interrupt descriptor, result in composing a message for each interrupt, right? (Unless requiring a judge on the parameter @data when implementing the irq_compose_msi_msg() callback that only compose message for the first entry of that message. But I really don't like that...)

Quoted 9 lines

No, that's not correct. You are looking at some random stale version of this. The current state of affairs is in

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/irqdomain

See also https://lkml.org/lkml/2014/11/17/764

In activate we write the message, which is the right point to do so.

Quoted 8 lines

I checked the current state, it seems to be the same.
Yes, the decision of postponing the actual hardware programming to the point where the interrupt actually gets used is right, but here above I was talking another thing.
As I mentioned, a message can associate with multiple interrupts. Enabling any of them will call irq_startup(). So if we don't want to compose or write messages repeatedly, we'd better require performing some checks before activating the interrupts.

Hi Yun,
Seems you are talking about the case of multiple MSI support.
Yes, we have special treatment for multiple MSI, which only writes PCI MSI registers when starting up the first MSI interrupt.
void pci_msi_domain_write_msg(struct irq_data *irq_data, struct msi_msg *msg)

Quoted 10 lines

{
        struct msi_desc *desc = irq_data->msi_desc;

        /*
         * For MSI-X desc->irq is always equal to irq_data->irq. For
         * MSI only the first interrupt of MULTI MSI passes the test.
         */
        if (desc->irq == irq_data->irq)
                __pci_write_msi_msg(desc, msg);
}

Quoted 4 lines

Yes, I picked the case of multiple MSI support.
The check should also be performed when composing messages. That's why I don't like its parameters. The @data only indicates one interrupt, while I prefer doing compose/write in the unit of message descriptor.

Quoted 9 lines

Hi Yun,
The common abstraction is that every message interrupt could be
controlled independently, so have compose_msi_msg()/write_msi_msg() per interrupt. MSI is abstracted as an special message signaled interrupt with hardware limitation where multiple interrupts sharing the same hardware registers. So we filter in pci_msi_domain_write_msg(). On the other handle, the generic MSI framework caches msi_msg in msi_desc, so we don't filter compose_msi_msg().

Quoted 7 lines

It's true that every message interrupt could be controlled independently, I mean, by enable/disable/mask/unmask. But the message data & address are shared among the interrupts of that message.
Despite the detailed hardware implementation, MSI and MSI-X are the same thing in software view, that is a message related with several consecutive interrupts. And the core MSI infrastructure you want to build should not be based on any hardware assumptions.

That's the key point. We abstract MSI as using a message to control an interrupt source instead of controlling several consecutive interrupts. PCI MSI is just a special case which controls a group of consecutive interrupts all together due to hardware limitation.

Quoted 98 lines · Jiang Liu

Quoted 97 lines · Yun Wu (Abel)

Quoted 78 lines · Jiang Liu

Quoted 73 lines · Yun Wu (Abel)

Quoted 59 lines · Jiang Liu

Quoted 44 lines · Yun Wu (Abel)

Quoted 32 lines · Thomas Gleixner

Quoted 22 lines · Yun Wu (Abel)

Quoted 5 lines · Thomas Gleixner

(quoted diff) +5 -0

+int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct irq_data *pos = NULL;
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY

Review

Quoted 10 lines

(quoted diff) +10 -0

+	for (; data; data = data->parent_data)
+#endif
+		if (data->chip && data->chip->irq_compose_msi_msg)
+			pos = data;>>>>>>>>> +	if (!pos)
+		return -ENOSYS;
+
+	pos->chip->irq_compose_msi_msg(pos, msg);
+
+	return 0;
+}

Adding message composing routine to struct irq_chip is OK to me, and it should be because it is interrupt controllers' duty to compose messages (so that they can parse the messages correctly without any pre-defined rules that endpoint devices absolutely need not to know).
However a problem comes out when deciding which parameters should be passed to

Quoted 8 lines

this routine. A message can associate with multiple interrupts, which makes me think composing messages for each interrupt is not that appropriate. And we can take a look at the new routine irq_chip_compose_msi_msg(). It is called by msi_domain_activate() which will be called by irq_domain_activate_irq() in irq_startup() for each interrupt descriptor, result in composing a message for each interrupt, right? (Unless requiring a judge on the parameter @data when implementing the irq_compose_msi_msg() callback that only compose message for the first entry of that message. But I really don't like that...)

Quoted 9 lines

No, that's not correct. You are looking at some random stale version of this. The current state of affairs is in

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/irqdomain

See also https://lkml.org/lkml/2014/11/17/764

In activate we write the message, which is the right point to do so.

Quoted 8 lines

I checked the current state, it seems to be the same.
Yes, the decision of postponing the actual hardware programming to the point where the interrupt actually gets used is right, but here above I was talking another thing.
As I mentioned, a message can associate with multiple interrupts. Enabling any of them will call irq_startup(). So if we don't want to compose or write messages repeatedly, we'd better require performing some checks before activating the interrupts.

Hi Yun,
Seems you are talking about the case of multiple MSI support.
Yes, we have special treatment for multiple MSI, which only writes PCI MSI registers when starting up the first MSI interrupt.
void pci_msi_domain_write_msg(struct irq_data *irq_data, struct msi_msg *msg)

Quoted 10 lines

{
        struct msi_desc *desc = irq_data->msi_desc;

        /*
         * For MSI-X desc->irq is always equal to irq_data->irq. For
         * MSI only the first interrupt of MULTI MSI passes the test.
         */
        if (desc->irq == irq_data->irq)
                __pci_write_msi_msg(desc, msg);
}

Quoted 4 lines

Yes, I picked the case of multiple MSI support.
The check should also be performed when composing messages. That's why I don't like its parameters. The @data only indicates one interrupt, while I prefer doing compose/write in the unit of message descriptor.

Quoted 9 lines

Hi Yun,
The common abstraction is that every message interrupt could be
controlled independently, so have compose_msi_msg()/write_msi_msg() per interrupt. MSI is abstracted as an special message signaled interrupt with hardware limitation where multiple interrupts sharing the same hardware registers. So we filter in pci_msi_domain_write_msg(). On the other handle, the generic MSI framework caches msi_msg in msi_desc, so we don't filter compose_msi_msg().

It's true that every message interrupt could be controlled independently, I mean, by enable/disable/mask/unmask. But the message data & address are shared among the interrupts of that message.
Despite the detailed hardware implementation, MSI and MSI-X are the same thing in software view, that is a message related with several consecutive interrupts. And the core MSI infrastructure you want to build should not be based on any hardware assumptions.

Quoted 5 lines

That's the key point. We abstract MSI as using a message to control an interrupt source instead of controlling several consecutive interrupts. PCI MSI is just a special case which controls a group of consecutive interrupts all together due to hardware limitation.

Oh, I see. We abstract it in different ways...
And sounds like you treat multiple MSI as a broken implementation?

plain text document attachment (rfc-part4-v1-02-17-genirq-introduce-helper-irq_domain_set_info-to-reduce-duplicated-code.patch)
From: Jiang Liu <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Cc Grant Likely <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Yingjoe Chen <[email protected]>

Cc Yijing Wang <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
 include/linux/irqdomain.h |    5 +++++
 kernel/irq/irqdomain.c    |   10 ++++++++++
 2 files changed, 15 insertions(+)

Index: tip/include/linux/irqdomain.h
===================================================================

include/linux/irqdomain.h +5 -0

@@ -33,6 +33,7 @@
 #define _LINUX_IRQDOMAIN_H
 
 #include <linux/types.h>
+#include <linux/irqhandler.h>
 #include <linux/radix-tree.h>
 
 struct device_node;

@@ -263,6 +264,10 @@ extern int irq_domain_set_hwirq_and_chip
 					 irq_hw_number_t hwirq,
 					 struct irq_chip *chip,
 					 void *chip_data);
+extern void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+				irq_hw_number_t hwirq, struct irq_chip *chip,
+				void *chip_data, irq_flow_handler_t handler,
+				void *handler_data, const char *handler_name);
 extern void irq_domain_reset_irq_data(struct irq_data *irq_data);
 extern void irq_domain_free_irqs_common(struct irq_domain *domain,
 					int virq, int nr_irqs);

Index: tip/kernel/irq/irqdomain.c
===================================================================

kernel/irq/irqdomain.c +0 -0

@@ -882,6 +882,16 @@ int irq_domain_set_hwirq_and_chip(struct
 	return 0;
 }
 

Quoted 8 lines · Thomas Gleixner

Index: tip/kernel/irq/irqdomain.c
===================================================================

kernel/irq/irqdomain.c +0 -0

@@ -882,6 +882,16 @@ int irq_domain_set_hwirq_and_chip(struct
 	return 0;
 }
 

kernel/irq/irqdomain.c int irq_domain_set_hwirq_and_chip(struct · line 894

Quoted 10 lines

kernel/irq/irqdomain.c +10 -0

+void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+			 irq_hw_number_t hwirq, struct irq_chip *chip,
+			 void *chip_data, irq_flow_handler_t handler,
+			 void *handler_data, const char *handler_name)
+{
+	irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
+	__irq_set_handler(virq, handler, 0, handler_name);
+	irq_set_handler_data(virq, handler_data);
+}
+

Hi,

While trying to use this function, I'm not sure about its interface. This function have 8 parameters but only save 3 function calls. After checking uses in Jiang's original patch, I think this make code harder to understand.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 22 lines · Yingjoe Chen

Quoted 8 lines · Thomas Gleixner

Index: tip/kernel/irq/irqdomain.c
===================================================================

kernel/irq/irqdomain.c +0 -0

@@ -882,6 +882,16 @@ int irq_domain_set_hwirq_and_chip(struct
 	return 0;
 }
 

kernel/irq/irqdomain.c int irq_domain_set_hwirq_and_chip(struct · line 894

Quoted 10 lines

kernel/irq/irqdomain.c +10 -0

+void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+			 irq_hw_number_t hwirq, struct irq_chip *chip,
+			 void *chip_data, irq_flow_handler_t handler,
+			 void *handler_data, const char *handler_name)
+{
+	irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
+	__irq_set_handler(virq, handler, 0, handler_name);
+	irq_set_handler_data(virq, handler_data);
+}
+

Hi,

Quoted 4 lines

While trying to use this function, I'm not sure about its interface. This function have 8 parameters but only save 3 function calls. After checking uses in Jiang's original patch, I think this make code harder to understand.

Hi Joe,

	It's true, but we also want to reduce duplicated code:(

From: Jiang Liu <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Cc Grant Likely <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Yingjoe Chen <[email protected]>

Cc Yijing Wang <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
 include/linux/irqdomain.h |    5 +++++
 kernel/irq/irqdomain.c    |   10 ++++++++++
 2 files changed, 15 insertions(+)

Index: tip/include/linux/irqdomain.h
===================================================================

include/linux/irqdomain.h +5 -0

@@ -33,6 +33,7 @@
 #define _LINUX_IRQDOMAIN_H
 
 #include <linux/types.h>
+#include <linux/irqhandler.h>
 #include <linux/radix-tree.h>
 
 struct device_node;

@@ -263,6 +264,10 @@ extern int irq_domain_set_hwirq_and_chip
 					 irq_hw_number_t hwirq,
 					 struct irq_chip *chip,
 					 void *chip_data);
+extern void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+				irq_hw_number_t hwirq, struct irq_chip *chip,
+				void *chip_data, irq_flow_handler_t handler,
+				void *handler_data, const char *handler_name);
 extern void irq_domain_reset_irq_data(struct irq_data *irq_data);
 extern void irq_domain_free_irqs_common(struct irq_domain *domain,
 					int virq, int nr_irqs);

Index: tip/kernel/irq/irqdomain.c
===================================================================

kernel/irq/irqdomain.c +0 -0

@@ -882,6 +882,16 @@ int irq_domain_set_hwirq_and_chip(struct
 	return 0;
 }
 

Quoted 45 lines · Thomas Gleixner

From: Jiang Liu <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Cc Grant Likely <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Yingjoe Chen <[email protected]>

Cc Yijing Wang <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
 include/linux/irqdomain.h |    5 +++++
 kernel/irq/irqdomain.c    |   10 ++++++++++
 2 files changed, 15 insertions(+)

Index: tip/include/linux/irqdomain.h
===================================================================

include/linux/irqdomain.h +5 -0

@@ -33,6 +33,7 @@
 #define _LINUX_IRQDOMAIN_H
 
 #include <linux/types.h>
+#include <linux/irqhandler.h>
 #include <linux/radix-tree.h>
 
 struct device_node;

@@ -263,6 +264,10 @@ extern int irq_domain_set_hwirq_and_chip
 					 irq_hw_number_t hwirq,
 					 struct irq_chip *chip,
 					 void *chip_data);
+extern void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+				irq_hw_number_t hwirq, struct irq_chip *chip,
+				void *chip_data, irq_flow_handler_t handler,
+				void *handler_data, const char *handler_name);
 extern void irq_domain_reset_irq_data(struct irq_data *irq_data);
 extern void irq_domain_free_irqs_common(struct irq_domain *domain,
 					int virq, int nr_irqs);

Index: tip/kernel/irq/irqdomain.c
===================================================================

kernel/irq/irqdomain.c +0 -0

@@ -882,6 +882,16 @@ int irq_domain_set_hwirq_and_chip(struct
 	return 0;
 }
 

kernel/irq/irqdomain.c int irq_domain_set_hwirq_and_chip(struct · line 894

Quoted 10 lines

kernel/irq/irqdomain.c +10 -0

+void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+			 irq_hw_number_t hwirq, struct irq_chip *chip,
+			 void *chip_data, irq_flow_handler_t handler,
+			 void *handler_data, const char *handler_name)
+{
+	irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
+	__irq_set_handler(virq, handler, 0, handler_name);
+	irq_set_handler_data(virq, handler_data);
+}
+

We still have the issue that, depending on where in the stack this is
called, this will succeed or fail: If this is called from the inner
irqchip, __irq_set_handler() will fail, as it will look at the outer domain as the (desc->irq_data.chip == &no_irq_chip) test fails (we haven't set the top level yet).

I have this very imperfect workaround in my tree:

kernel/irq/chip.c +10 -1

index d028b34..91e6515 100644
@@ -731,7 +731,16 @@ __irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
 	if (!handle) {
 		handle = handle_bad_irq;
 	} else {
-		if (WARN_ON(desc->irq_data.chip == &no_irq_chip))
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+		struct irq_data *irq_data = &desc->irq_data;
+		while (irq_data) {
+			if (irq_data->chip != &no_irq_chip)
+				break;
+			irq_data = irq_data->parent_data;
+		}
+#endif
+
+		if (WARN_ON(!irq_data || irq_data->chip == &no_irq_chip))
 			goto out;
 	}
 

Which translate into: If there is at least one irqchip in the domain, it will probably sort itself out. Not ideal. Any real solution to this problem?

GICv2 faces this exact problem, as some of its interrupts are used directly, and some others are used through the MSI domain. In the GIC driver, it is almost impossible to find out...

Thanks,

	M.

Quoted 91 lines · Marc Zyngier

Quoted 45 lines · Thomas Gleixner

From: Jiang Liu <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Cc Grant Likely <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Yingjoe Chen <[email protected]>

Cc Yijing Wang <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
 include/linux/irqdomain.h |    5 +++++
 kernel/irq/irqdomain.c    |   10 ++++++++++
 2 files changed, 15 insertions(+)

Index: tip/include/linux/irqdomain.h
===================================================================

include/linux/irqdomain.h +5 -0

@@ -33,6 +33,7 @@
 #define _LINUX_IRQDOMAIN_H
 
 #include <linux/types.h>
+#include <linux/irqhandler.h>
 #include <linux/radix-tree.h>
 
 struct device_node;

@@ -263,6 +264,10 @@ extern int irq_domain_set_hwirq_and_chip
 					 irq_hw_number_t hwirq,
 					 struct irq_chip *chip,
 					 void *chip_data);
+extern void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+				irq_hw_number_t hwirq, struct irq_chip *chip,
+				void *chip_data, irq_flow_handler_t handler,
+				void *handler_data, const char *handler_name);
 extern void irq_domain_reset_irq_data(struct irq_data *irq_data);
 extern void irq_domain_free_irqs_common(struct irq_domain *domain,
 					int virq, int nr_irqs);

Index: tip/kernel/irq/irqdomain.c
===================================================================

kernel/irq/irqdomain.c +0 -0

@@ -882,6 +882,16 @@ int irq_domain_set_hwirq_and_chip(struct
 	return 0;
 }
 

kernel/irq/irqdomain.c int irq_domain_set_hwirq_and_chip(struct · line 894

Quoted 10 lines

kernel/irq/irqdomain.c +10 -0

+void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+			 irq_hw_number_t hwirq, struct irq_chip *chip,
+			 void *chip_data, irq_flow_handler_t handler,
+			 void *handler_data, const char *handler_name)
+{
+	irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
+	__irq_set_handler(virq, handler, 0, handler_name);
+	irq_set_handler_data(virq, handler_data);
+}
+

We still have the issue that, depending on where in the stack this is
called, this will succeed or fail: If this is called from the inner
irqchip, __irq_set_handler() will fail, as it will look at the outer domain as the (desc->irq_data.chip == &no_irq_chip) test fails (we haven't set the top level yet).

I have this very imperfect workaround in my tree:

kernel/irq/chip.c +10 -1

index d028b34..91e6515 100644
@@ -731,7 +731,16 @@ __irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
 	if (!handle) {
 		handle = handle_bad_irq;
 	} else {
-		if (WARN_ON(desc->irq_data.chip == &no_irq_chip))
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+		struct irq_data *irq_data = &desc->irq_data;
+		while (irq_data) {
+			if (irq_data->chip != &no_irq_chip)
+				break;
+			irq_data = irq_data->parent_data;
+		}
+#endif
+
+		if (WARN_ON(!irq_data || irq_data->chip == &no_irq_chip))
 			goto out;
 	}
 

Which translate into: If there is at least one irqchip in the domain, it will probably sort itself out. Not ideal. Any real solution to this problem?

Quoted 3 lines

GICv2 faces this exact problem, as some of its interrupts are used directly, and some others are used through the MSI domain. In the GIC driver, it is almost impossible to find out...

Hi Marc,
I prefer the above solution to relax the warning conditions.
Changing the calling order in irq_domain_ops->alloc() looks a little strange, and other interrupt drivers may still run into the same issue.

Quoted 92 lines · Jiang Liu

Quoted 91 lines · Marc Zyngier

Quoted 45 lines · Thomas Gleixner

From: Jiang Liu <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Cc Grant Likely <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Yingjoe Chen <[email protected]>

Cc Yijing Wang <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
 include/linux/irqdomain.h |    5 +++++
 kernel/irq/irqdomain.c    |   10 ++++++++++
 2 files changed, 15 insertions(+)

Index: tip/include/linux/irqdomain.h
===================================================================

include/linux/irqdomain.h +5 -0

@@ -33,6 +33,7 @@
 #define _LINUX_IRQDOMAIN_H
 
 #include <linux/types.h>
+#include <linux/irqhandler.h>
 #include <linux/radix-tree.h>
 
 struct device_node;

@@ -263,6 +264,10 @@ extern int irq_domain_set_hwirq_and_chip
 					 irq_hw_number_t hwirq,
 					 struct irq_chip *chip,
 					 void *chip_data);
+extern void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+				irq_hw_number_t hwirq, struct irq_chip *chip,
+				void *chip_data, irq_flow_handler_t handler,
+				void *handler_data, const char *handler_name);
 extern void irq_domain_reset_irq_data(struct irq_data *irq_data);
 extern void irq_domain_free_irqs_common(struct irq_domain *domain,
 					int virq, int nr_irqs);

Index: tip/kernel/irq/irqdomain.c
===================================================================

kernel/irq/irqdomain.c +0 -0

@@ -882,6 +882,16 @@ int irq_domain_set_hwirq_and_chip(struct
 	return 0;
 }
 

kernel/irq/irqdomain.c int irq_domain_set_hwirq_and_chip(struct · line 894

Quoted 10 lines

kernel/irq/irqdomain.c +10 -0

+void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+			 irq_hw_number_t hwirq, struct irq_chip *chip,
+			 void *chip_data, irq_flow_handler_t handler,
+			 void *handler_data, const char *handler_name)
+{
+	irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
+	__irq_set_handler(virq, handler, 0, handler_name);
+	irq_set_handler_data(virq, handler_data);
+}
+

We still have the issue that, depending on where in the stack this is
called, this will succeed or fail: If this is called from the inner
irqchip, __irq_set_handler() will fail, as it will look at the outer domain as the (desc->irq_data.chip == &no_irq_chip) test fails (we haven't set the top level yet).

I have this very imperfect workaround in my tree:

kernel/irq/chip.c +10 -1

index d028b34..91e6515 100644
@@ -731,7 +731,16 @@ __irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
 	if (!handle) {
 		handle = handle_bad_irq;
 	} else {
-		if (WARN_ON(desc->irq_data.chip == &no_irq_chip))
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+		struct irq_data *irq_data = &desc->irq_data;
+		while (irq_data) {
+			if (irq_data->chip != &no_irq_chip)
+				break;
+			irq_data = irq_data->parent_data;
+		}
+#endif
+
+		if (WARN_ON(!irq_data || irq_data->chip == &no_irq_chip))
 			goto out;
 	}
 

Which translate into: If there is at least one irqchip in the domain, it will probably sort itself out. Not ideal. Any real solution to this problem?

Quoted 7 lines

Quoted 3 lines

GICv2 faces this exact problem, as some of its interrupts are used directly, and some others are used through the MSI domain. In the GIC driver, it is almost impossible to find out...

Hi Marc,
I prefer the above solution to relax the warning conditions.
Changing the calling order in irq_domain_ops->alloc() looks a little strange, and other interrupt drivers may still run into the same issue.

OK. Where do we from from there? Do you want a proper patch, or will you fold this into the existing code?

Thanks,

	M.

From: Jiang Liu <[email protected]>

Hi Thomas, Jiang,

Quoted 1 line · Thomas Gleixner

From: Jiang Liu <[email protected]>

[...]

kernel/irq/irqdomain.c int irq_domain_set_hwirq_and_chip(struct · line 893

Quoted 9 lines

kernel/irq/irqdomain.c +9 -0

+void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+			 irq_hw_number_t hwirq, struct irq_chip *chip,
+			 void *chip_data, irq_flow_handler_t handler,
+			 void *handler_data, const char *handler_name)
+{
+	irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
+	__irq_set_handler(virq, handler, 0, handler_name);
+	irq_set_handler_data(virq, handler_data);
+}

When stacked domain enabled, there will be a semantic shift to the linux interrupt identifiers. The @virq now delivers much more than before.
More specifically, now we need both @virq and @domain, rather than only @irq, to determine which irq_data we want to configure. And once we configure @irq without providing the exact domain, it means we are configuring all the domains related to that @irq. So I think this routine just messed all things up.

Quoted 5 lines · Yun Wu (Abel)

Hi Thomas, Jiang,

Quoted 1 line · Thomas Gleixner

From: Jiang Liu <[email protected]>

[...]

Quoted 16 lines

kernel/irq/irqdomain.c int irq_domain_set_hwirq_and_chip(struct · line 893

Quoted 9 lines

kernel/irq/irqdomain.c +9 -0

+void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+			 irq_hw_number_t hwirq, struct irq_chip *chip,
+			 void *chip_data, irq_flow_handler_t handler,
+			 void *handler_data, const char *handler_name)
+{
+	irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
+	__irq_set_handler(virq, handler, 0, handler_name);
+	irq_set_handler_data(virq, handler_data);
+}

When stacked domain enabled, there will be a semantic shift to the linux interrupt identifiers. The @virq now delivers much more than before.
More specifically, now we need both @virq and @domain, rather than only @irq, to determine which irq_data we want to configure. And once we configure @irq without providing the exact domain, it means we are configuring all the domains related to that @irq. So I think this routine just messed all things up.

You can mess up anything by using an interface in the wrong way. Open coding will not make that harder.

Quoted 24 lines · Thomas Gleixner

Quoted 21 lines · Yun Wu (Abel)

Hi Thomas, Jiang,

Quoted 1 line · Thomas Gleixner

From: Jiang Liu <[email protected]>

[...]

kernel/irq/irqdomain.c int irq_domain_set_hwirq_and_chip(struct · line 893

Quoted 9 lines

kernel/irq/irqdomain.c +9 -0

+void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+			 irq_hw_number_t hwirq, struct irq_chip *chip,
+			 void *chip_data, irq_flow_handler_t handler,
+			 void *handler_data, const char *handler_name)
+{
+	irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
+	__irq_set_handler(virq, handler, 0, handler_name);
+	irq_set_handler_data(virq, handler_data);
+}

When stacked domain enabled, there will be a semantic shift to the linux interrupt identifiers. The @virq now delivers much more than before.
More specifically, now we need both @virq and @domain, rather than only @irq, to determine which irq_data we want to configure. And once we configure @irq without providing the exact domain, it means we are configuring all the domains related to that @irq. So I think this routine just messed all things up.

Quoted 3 lines

You can mess up anything by using an interface in the wrong way. Open coding will not make that harder.

But what's the correct way to use this interface?

Thanks,
Abel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 26 lines · Yun Wu (Abel)

Quoted 24 lines · Thomas Gleixner

Quoted 21 lines · Yun Wu (Abel)

Hi Thomas, Jiang,

Quoted 1 line · Thomas Gleixner

From: Jiang Liu <[email protected]>

[...]

kernel/irq/irqdomain.c int irq_domain_set_hwirq_and_chip(struct · line 893

Quoted 9 lines

kernel/irq/irqdomain.c +9 -0

+void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+			 irq_hw_number_t hwirq, struct irq_chip *chip,
+			 void *chip_data, irq_flow_handler_t handler,
+			 void *handler_data, const char *handler_name)
+{
+	irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
+	__irq_set_handler(virq, handler, 0, handler_name);
+	irq_set_handler_data(virq, handler_data);
+}

When stacked domain enabled, there will be a semantic shift to the linux interrupt identifiers. The @virq now delivers much more than before.
More specifically, now we need both @virq and @domain, rather than only @irq, to determine which irq_data we want to configure. And once we configure @irq without providing the exact domain, it means we are configuring all the domains related to that @irq. So I think this routine just messed all things up.

Quoted 5 lines

Quoted 3 lines

You can mess up anything by using an interface in the wrong way. Open coding will not make that harder.

But what's the correct way to use this interface?

It's to be used by interrupt controller drivers to implement
hierarchy irqdomains.

Quoted 33 lines · Jiang Liu

Quoted 31 lines · Yun Wu (Abel)

Quoted 24 lines · Thomas Gleixner

Quoted 5 lines · Yun Wu (Abel)

Hi Thomas, Jiang,

Quoted 1 line · Thomas Gleixner

From: Jiang Liu <[email protected]>

[...]

Quoted 16 lines

kernel/irq/irqdomain.c int irq_domain_set_hwirq_and_chip(struct · line 893

Quoted 9 lines

kernel/irq/irqdomain.c +9 -0

+void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+			 irq_hw_number_t hwirq, struct irq_chip *chip,
+			 void *chip_data, irq_flow_handler_t handler,
+			 void *handler_data, const char *handler_name)
+{
+	irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
+	__irq_set_handler(virq, handler, 0, handler_name);
+	irq_set_handler_data(virq, handler_data);
+}

When stacked domain enabled, there will be a semantic shift to the linux interrupt identifiers. The @virq now delivers much more than before.
More specifically, now we need both @virq and @domain, rather than only @irq, to determine which irq_data we want to configure. And once we configure @irq without providing the exact domain, it means we are configuring all the domains related to that @irq. So I think this routine just messed all things up.

Quoted 3 lines

You can mess up anything by using an interface in the wrong way. Open coding will not make that harder.

But what's the correct way to use this interface?

Quoted 3 lines

It's to be used by interrupt controller drivers to implement
hierarchy irqdomains.

Each time an interrupt domain calls this, the previous (@handler, @handler_name, @handler_data) will be overrode. It's because the routines __irq_set_handler() and irq_set_handler_data() only configure top level (without Marc's fix which is not ideal). Is this what we really want to see?

Thanks,
Abel

From: Jiang Liu <[email protected]>

Introduce callback irq_chip.irq_write_msi_msg, so we can share common code among MSI alike interrupt controllers, such as HPET and DMAR.

Signed-off-by Jiang Liu <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Cc Grant Likely <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Yingjoe Chen <[email protected]>

Cc Yijing Wang <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
 include/linux/irq.h |    8 ++++++++
 1 file changed, 8 insertions(+)

Index: tip/include/linux/irq.h
===================================================================

include/linux/irq.h +1 -0

@@ -322,6 +322,7 @@ static inline irq_hw_number_t irqd_to_hw
  * @irq_release_resources:	optional to release resources acquired with
  *				irq_request_resources
  * @irq_compose_msi_msg:	optional to compose message content for MSI
+ * @irq_write_msi_msg:	optional to write message content for MSI
  * @flags:		chip specific flags
  */
 struct irq_chip {

include/linux/irq.h +6 -1

 	unsigned long	flags;
 };

@@ -453,6 +455,12 @@ extern void irq_chip_ack_parent(struct i
 extern int irq_chip_retrigger_hierarchy(struct irq_data *data);
 #endif
 
+static inline void irq_chip_write_msi_msg(struct irq_data *data,
+					  struct msi_msg *msg)
+{
+	data->chip->irq_write_msi_msg(data, msg);
+}
+
 /* Handling of unhandled and spurious interrupts: */
 extern void note_interrupt(unsigned int irq, struct irq_desc *desc,
 			   irqreturn_t action_ret);
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
.

include/linux/irq.h struct irq_chip { · line 363

Quoted 6 lines · Thomas Gleixner

struct irq_chip {

include/linux/irq.h +1 -0

@@ -359,6 +360,7 @@ struct irq_chip {
 	void		(*irq_release_resources)(struct irq_data *data);
 
 	void		(*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
+	void		(*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg);

Hmm... It's really weird.
I don't think it's the interrupt controllers' responsibility to write messages for all the endpoint devices since the methods of configuring message registers may different between these devices. And theoretically, the endpoint devices themselves should take the responsibility to configure their message registers. To say the least, the write_msg callback here still need to call some certain interfaces provided by the corresponding device.

There would be lots of ARM new devices capable of sending message based interrupts to interrupt controllers, does all the drivers of the devices need to expose a write_msg callback to interrupt controllers?

Quoted 16 lines · Yun Wu (Abel)

include/linux/irq.h struct irq_chip { · line 363

Quoted 6 lines · Thomas Gleixner

struct irq_chip {

include/linux/irq.h +1 -0

@@ -359,6 +360,7 @@ struct irq_chip {
 	void		(*irq_release_resources)(struct irq_data *data);
 
 	void		(*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
+	void		(*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg);

Hmm... It's really weird.
I don't think it's the interrupt controllers' responsibility to write messages for all the endpoint devices since the methods of configuring message registers may different between these devices. And theoretically, the endpoint devices themselves should take the responsibility to configure their message registers. To say the least, the write_msg callback here still need to call some certain interfaces provided by the corresponding device.

Quoted 4 lines

There would be lots of ARM new devices capable of sending message based interrupts to interrupt controllers, does all the drivers of the devices need to expose a write_msg callback to interrupt controllers?

Well, writing the message _IS_ part of the interrupt controller.

So in order to enable non PCI based MSI we want to have generic infrastructure with minimal per device/device class callbacks and of course you need to provide that callback for your special device.

We already have non PCI based MSI controllers in x86 today and we need to handle the whole stuff with tons of copied coded extra for each of those. So consolidating it into common infrastructure allows us to get rid of the pointless copied code and reduce the per device effort to the relevant hardware specific callbacks. irq_write_msi_msg being one of those.

Quoted 28 lines · Thomas Gleixner

Quoted 16 lines · Yun Wu (Abel)

include/linux/irq.h struct irq_chip { · line 363

Quoted 6 lines · Thomas Gleixner

struct irq_chip {

include/linux/irq.h +1 -0

@@ -359,6 +360,7 @@ struct irq_chip {
 	void		(*irq_release_resources)(struct irq_data *data);
 
 	void		(*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
+	void		(*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg);

Hmm... It's really weird.
I don't think it's the interrupt controllers' responsibility to write messages for all the endpoint devices since the methods of configuring message registers may different between these devices. And theoretically, the endpoint devices themselves should take the responsibility to configure their message registers. To say the least, the write_msg callback here still need to call some certain interfaces provided by the corresponding device.

Quoted 4 lines

There would be lots of ARM new devices capable of sending message based interrupts to interrupt controllers, does all the drivers of the devices need to expose a write_msg callback to interrupt controllers?

Well, writing the message _IS_ part of the interrupt controller.

So in order to enable non PCI based MSI we want to have generic infrastructure with minimal per device/device class callbacks and of course you need to provide that callback for your special device.

Quoted 7 lines

We already have non PCI based MSI controllers in x86 today and we need to handle the whole stuff with tons of copied coded extra for each of those. So consolidating it into common infrastructure allows us to get rid of the pointless copied code and reduce the per device effort to the relevant hardware specific callbacks. irq_write_msi_msg being one of those.

At least, we have the same goal.
I will illustrate my thoughts by an example.
The current code is something like:

Device A
========
void A_write_msg() { ... }

Group B
(a group of devices behave same on writing messages, i.e. PCI)
=======
void B_write_msg() { ... }

Controller
==========

irq_chip.irq_write_msi_msg () {
	if (A)
		A_write_msg();
	if (B)
		B_write_msg();
}

It's horrible when new devices come out, since we need to modify the controller part for each new device.
What I suggested is:

MSI Core
========
struct msi_ops { .write_msg, };
struct msi_desc { .msi_ops, };

write_msg() {
	X = get_dev();
	irq_chip.compose_msg(X);	// IRQ chips' responsibility
	X_msi_ops.write_msg();		// nothing to do with IRQ chips
}

Device A
========
void A_write_msg() { ... }
A_msi_ops.write_msg = A_write_msg;

Group B
=======
void B_write_msg() { ... }
B_msi_ops.write_msg = B_write_msg;

Please correct me if I misunderstood anything.

Thanks,
Abel

Quoted 85 lines · Yun Wu (Abel)

Quoted 28 lines · Thomas Gleixner

Quoted 16 lines · Yun Wu (Abel)

include/linux/irq.h struct irq_chip { · line 363

Quoted 6 lines · Thomas Gleixner

struct irq_chip {

include/linux/irq.h +1 -0

@@ -359,6 +360,7 @@ struct irq_chip {
 	void		(*irq_release_resources)(struct irq_data *data);
 
 	void		(*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
+	void		(*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg);

Hmm... It's really weird.
I don't think it's the interrupt controllers' responsibility to write messages for all the endpoint devices since the methods of configuring message registers may different between these devices. And theoretically, the endpoint devices themselves should take the responsibility to configure their message registers. To say the least, the write_msg callback here still need to call some certain interfaces provided by the corresponding device.

Quoted 4 lines

There would be lots of ARM new devices capable of sending message based interrupts to interrupt controllers, does all the drivers of the devices need to expose a write_msg callback to interrupt controllers?

Well, writing the message _IS_ part of the interrupt controller.

So in order to enable non PCI based MSI we want to have generic infrastructure with minimal per device/device class callbacks and of course you need to provide that callback for your special device.

Quoted 7 lines

We already have non PCI based MSI controllers in x86 today and we need to handle the whole stuff with tons of copied coded extra for each of those. So consolidating it into common infrastructure allows us to get rid of the pointless copied code and reduce the per device effort to the relevant hardware specific callbacks. irq_write_msi_msg being one of those.

At least, we have the same goal.
I will illustrate my thoughts by an example.
The current code is something like:

Device A
========
void A_write_msg() { ... }

Group B
(a group of devices behave same on writing messages, i.e. PCI)
=======
void B_write_msg() { ... }

Controller
==========

irq_chip.irq_write_msi_msg () {
	if (A)
		A_write_msg();
	if (B)
		B_write_msg();
}

It's horrible when new devices come out, since we need to modify the controller part for each new device.
What I suggested is:

MSI Core
========
struct msi_ops { .write_msg, };
struct msi_desc { .msi_ops, };

write_msg() {
	X = get_dev();
	irq_chip.compose_msg(X);	// IRQ chips' responsibility
	X_msi_ops.write_msg();		// nothing to do with IRQ chips
}

Device A
========
void A_write_msg() { ... }
A_msi_ops.write_msg = A_write_msg;

Group B
=======
void B_write_msg() { ... }
B_msi_ops.write_msg = B_write_msg;