Re: [PATCH v2 2/7] genirq: irqdomain: Remove irqdomain dependency on struct device_node

[Hanjun Guo]

On 07/23/2015 09:05 PM, Marc Zyngier wrote:
> struct device_node is very much DT specific, and the original authors
> of the irqdomain subsystem recognized that tie, and went as far as
> mentionning that this could be replaced by some "void *token",
> should another firmware infrastructure be using it.
>
> As we move ACPI on arm64 towards this model too, it makes a lot of sense
> to perform that particular move.
>
> We replace "struct device_node *of_node" with "void *domain_token", which
> is a benign enough transformation. A non DT user of irqdomain can now
> identify its domains using this pointer.
>
> Also, in order to prevent the introduction of sideband type information,
> only DT is allowed to store a valid kernel pointer in domain_token
> (a pointer that passes the virt_addr_valid() test will be considered
> as a valid device_node).
>
> non-DT users that wish to store valid pointers in domain_token are
> required to use another structure such as an IDR.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>

Reviewed-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>

Thanks
Hanjun

> ---
>   include/linux/irqdomain.h | 66 ++++++++++++++++++++++------------------
>   kernel/irq/irqdomain.c    | 77 +++++++++++++++++++++++++++++++++++------------
>   2 files changed, 93 insertions(+), 50 deletions(-)
>
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index f644fdb..7fd998d 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -17,16 +17,14 @@
>    * model). It's the domain callbacks that are responsible for setting the
>    * irq_chip on a given irq_desc after it's been mapped.
>    *
> - * The host code and data structures are agnostic to whether or not
> - * we use an open firmware device-tree. We do have references to struct
> - * device_node in two places: in irq_find_host() to find the host matching
> - * a given interrupt controller node, and of course as an argument to its
> - * counterpart domain->ops->match() callback. However, those are treated as
> - * generic pointers by the core and the fact that it's actually a device-node
> - * pointer is purely a convention between callers and implementation. This
> - * code could thus be used on other architectures by replacing those two
> - * by some sort of arch-specific void * "token" used to identify interrupt
> - * controllers.
> + * The host code and data structures are agnostic to whether or not we
> + * use an open firmware device-tree. Domains can be assigned a
> + * "void *domain_token" identifier, which is assumed to represent a
> + * "struct device_node" if the pointer is a valid virtual address.
> + *
> + * Otherwise, the value is assumed to be an opaque identifier. Should
> + * a pointer to a non "struct device_node" be required, another data
> + * structure should be used to indirect it (an IDR for example).
>    */
>
>   #ifndef _LINUX_IRQDOMAIN_H
> @@ -108,8 +106,8 @@ struct irq_domain_chip_generic;
>    * @flags: host per irq_domain flags
>    *
>    * Optional elements
> - * @of_node: Pointer to device tree nodes associated with the irq_domain. Used
> - *           when decoding device tree interrupt specifiers.
> + * @domain_token: optional firmware-specific identifier for
> + *                the interrupt controller
>    * @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.
> @@ -130,7 +128,7 @@ struct irq_domain {
>   	unsigned int flags;
>
>   	/* Optional data */
> -	struct device_node *of_node;
> +	void *domain_token;
>   	enum irq_domain_bus_token bus_token;
>   	struct irq_domain_chip_generic *gc;
>   #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
> @@ -161,70 +159,73 @@ enum {
>   	IRQ_DOMAIN_FLAG_NONCORE		= (1 << 16),
>   };
>
> +#ifdef CONFIG_IRQ_DOMAIN
> +extern struct device_node *irq_domain_token_to_of_node(void *domain_token);
> +
>   static inline struct device_node *irq_domain_get_of_node(struct irq_domain *d)
>   {
> -	return d->of_node;
> +	return irq_domain_token_to_of_node(d->domain_token);
>   }
>
> -#ifdef CONFIG_IRQ_DOMAIN
> -struct irq_domain *__irq_domain_add(struct device_node *of_node, int size,
> +struct irq_domain *__irq_domain_add(void *domain_token, int size,
>   				    irq_hw_number_t hwirq_max, int direct_max,
>   				    const struct irq_domain_ops *ops,
>   				    void *host_data);
> -struct irq_domain *irq_domain_add_simple(struct device_node *of_node,
> +struct irq_domain *irq_domain_add_simple(void *domain_token,
>   					 unsigned int size,
>   					 unsigned int first_irq,
>   					 const struct irq_domain_ops *ops,
>   					 void *host_data);
> -struct irq_domain *irq_domain_add_legacy(struct device_node *of_node,
> +struct irq_domain *irq_domain_add_legacy(void *domain_token,
>   					 unsigned int size,
>   					 unsigned int first_irq,
>   					 irq_hw_number_t first_hwirq,
>   					 const struct irq_domain_ops *ops,
>   					 void *host_data);
> -extern struct irq_domain *irq_find_matching_host(struct device_node *node,
> +extern struct irq_domain *irq_find_matching_host(void *domain_token,
>   						 enum irq_domain_bus_token bus_token);
>   extern void irq_set_default_host(struct irq_domain *host);
>
> -static inline struct irq_domain *irq_find_host(struct device_node *node)
> +static inline struct irq_domain *irq_find_host(void *domain_token)
>   {
> -	return irq_find_matching_host(node, DOMAIN_BUS_ANY);
> +	return irq_find_matching_host(domain_token, DOMAIN_BUS_ANY);
>   }
>
>   /**
>    * irq_domain_add_linear() - Allocate and register a linear revmap irq_domain.
> - * @of_node: pointer to interrupt controller's device tree node.
> + * @domain_token: optional firmware-specific identifier for
> + *                the interrupt controller
>    * @size: Number of interrupts in the domain.
>    * @ops: map/unmap domain callbacks
>    * @host_data: Controller private data pointer
>    */
> -static inline struct irq_domain *irq_domain_add_linear(struct device_node *of_node,
> +static inline struct irq_domain *irq_domain_add_linear(void *domain_token,
>   					 unsigned int size,
>   					 const struct irq_domain_ops *ops,
>   					 void *host_data)
>   {
> -	return __irq_domain_add(of_node, size, size, 0, ops, host_data);
> +	return __irq_domain_add(domain_token, size, size, 0, ops, host_data);
>   }
> -static inline struct irq_domain *irq_domain_add_nomap(struct device_node *of_node,
> +static inline struct irq_domain *irq_domain_add_nomap(void *domain_token,
>   					 unsigned int max_irq,
>   					 const struct irq_domain_ops *ops,
>   					 void *host_data)
>   {
> -	return __irq_domain_add(of_node, 0, max_irq, max_irq, ops, host_data);
> +	return __irq_domain_add(domain_token, 0, max_irq, max_irq, ops, host_data);
>   }
>   static inline struct irq_domain *irq_domain_add_legacy_isa(
> -				struct device_node *of_node,
> +				void *domain_token,
>   				const struct irq_domain_ops *ops,
>   				void *host_data)
>   {
> -	return irq_domain_add_legacy(of_node, NUM_ISA_INTERRUPTS, 0, 0, ops,
> +	return irq_domain_add_legacy(domain_token, NUM_ISA_INTERRUPTS, 0, 0, ops,
>   				     host_data);
>   }
> -static inline struct irq_domain *irq_domain_add_tree(struct device_node *of_node,
> +static inline struct irq_domain *irq_domain_add_tree(void *domain_token,
>   					 const struct irq_domain_ops *ops,
>   					 void *host_data)
>   {
> -	return __irq_domain_add(of_node, 0, ~0, 0, ops, host_data);
> +	return __irq_domain_add(domain_token, 0, ~0, 0, ops, host_data);
>   }
>
>   extern void irq_domain_remove(struct irq_domain *host);
> @@ -347,6 +348,11 @@ static inline bool irq_domain_is_hierarchy(struct irq_domain *domain)
>   #endif	/* CONFIG_IRQ_DOMAIN_HIERARCHY */
>
>   #else /* CONFIG_IRQ_DOMAIN */
> +static inline struct device_node *irq_domain_get_of_node(struct irq_domain *d)
> +{
> +	return d->domain_token;
> +}
> +
>   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) { }
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 995d217..27f4ec7 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -6,6 +6,7 @@
>   #include <linux/irq.h>
>   #include <linux/irqdesc.h>
>   #include <linux/irqdomain.h>
> +#include <linux/mm.h>
>   #include <linux/module.h>
>   #include <linux/mutex.h>
>   #include <linux/of.h>
> @@ -27,9 +28,24 @@ 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);
>
> +struct device_node *irq_domain_token_to_of_node(void *domain_token)
> +{
> +	/*
> +	 * Assume that anything represented by a valid kernel address
> +	 * is a device_node. Anything else must be a "small integer",
> +	 * and indirected by some other structure (an IDR, for
> +	 * example) if a pointer is required.
> +	 */
> +	if (virt_addr_valid(domain_token))
> +		return domain_token;
> +
> +	return NULL;
> +}
> +
>   /**
>    * __irq_domain_add() - Allocate a new irq_domain data structure
> - * @of_node: optional device-tree node of the interrupt controller
> + * @domain_token: optional firmware-specific identifier for
> + *                the interrupt controller
>    * @size: Size of linear map; 0 for radix mapping only
>    * @hwirq_max: Maximum number of interrupts supported by controller
>    * @direct_max: Maximum value of direct maps; Use ~0 for no limit; 0 for no
> @@ -40,13 +56,15 @@ static void irq_domain_check_hierarchy(struct irq_domain *domain);
>    * Allocates and initialize and irq_domain structure.
>    * Returns pointer to IRQ domain, or NULL on failure.
>    */
> -struct irq_domain *__irq_domain_add(struct device_node *of_node, int size,
> +struct irq_domain *__irq_domain_add(void *domain_token, int size,
>   				    irq_hw_number_t hwirq_max, int direct_max,
>   				    const struct irq_domain_ops *ops,
>   				    void *host_data)
>   {
> +	struct device_node *of_node;
>   	struct irq_domain *domain;
>
> +	of_node = irq_domain_token_to_of_node(domain_token);
>   	domain = kzalloc_node(sizeof(*domain) + (sizeof(unsigned int) * size),
>   			      GFP_KERNEL, of_node_to_nid(of_node));
>   	if (WARN_ON(!domain))
> @@ -56,7 +74,7 @@ struct irq_domain *__irq_domain_add(struct device_node *of_node, int size,
>   	INIT_RADIX_TREE(&domain->revmap_tree, GFP_KERNEL);
>   	domain->ops = ops;
>   	domain->host_data = host_data;
> -	domain->of_node = of_node_get(of_node);
> +	domain->domain_token = of_node_get(of_node) ?: domain_token;
>   	domain->hwirq_max = hwirq_max;
>   	domain->revmap_size = size;
>   	domain->revmap_direct_max_irq = direct_max;
> @@ -102,14 +120,15 @@ void irq_domain_remove(struct irq_domain *domain)
>
>   	pr_debug("Removed domain %s\n", domain->name);
>
> -	of_node_put(domain->of_node);
> +	of_node_put(irq_domain_get_of_node(domain));
>   	kfree(domain);
>   }
>   EXPORT_SYMBOL_GPL(irq_domain_remove);
>
>   /**
>    * irq_domain_add_simple() - Register an irq_domain and optionally map a range of irqs
> - * @of_node: pointer to interrupt controller's device tree node.
> + * @domain_token: optional firmware-specific identifier for
> + *                the interrupt controller
>    * @size: total number of irqs in mapping
>    * @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
> @@ -125,18 +144,20 @@ EXPORT_SYMBOL_GPL(irq_domain_remove);
>    * irqs get mapped dynamically on the fly. However, if the controller requires
>    * static virq assignments (non-DT boot) then it will set that up correctly.
>    */
> -struct irq_domain *irq_domain_add_simple(struct device_node *of_node,
> +struct irq_domain *irq_domain_add_simple(void *domain_token,
>   					 unsigned int size,
>   					 unsigned int first_irq,
>   					 const struct irq_domain_ops *ops,
>   					 void *host_data)
>   {
> +	struct device_node *of_node;
>   	struct irq_domain *domain;
>
> -	domain = __irq_domain_add(of_node, size, size, 0, ops, host_data);
> +	domain = __irq_domain_add(domain_token, size, size, 0, ops, host_data);
>   	if (!domain)
>   		return NULL;
>
> +	of_node = irq_domain_token_to_of_node(domain_token);
>   	if (first_irq > 0) {
>   		if (IS_ENABLED(CONFIG_SPARSE_IRQ)) {
>   			/* attempt to allocated irq_descs */
> @@ -155,7 +176,8 @@ EXPORT_SYMBOL_GPL(irq_domain_add_simple);
>
>   /**
>    * irq_domain_add_legacy() - Allocate and register a legacy revmap irq_domain.
> - * @of_node: pointer to interrupt controller's device tree node.
> + * @domain_token: optional firmware-specific identifier for
> + *                the interrupt controller
>    * @size: total number of irqs in legacy mapping
>    * @first_irq: first number of irq block assigned to the domain
>    * @first_hwirq: first hwirq number to use for the translation. Should normally
> @@ -168,7 +190,7 @@ EXPORT_SYMBOL_GPL(irq_domain_add_simple);
>    * for all legacy interrupts except 0 (which is always the invalid irq for
>    * a legacy controller).
>    */
> -struct irq_domain *irq_domain_add_legacy(struct device_node *of_node,
> +struct irq_domain *irq_domain_add_legacy(void *domain_token,
>   					 unsigned int size,
>   					 unsigned int first_irq,
>   					 irq_hw_number_t first_hwirq,
> @@ -177,7 +199,7 @@ struct irq_domain *irq_domain_add_legacy(struct device_node *of_node,
>   {
>   	struct irq_domain *domain;
>
> -	domain = __irq_domain_add(of_node, first_hwirq + size,
> +	domain = __irq_domain_add(domain_token, first_hwirq + size,
>   				  first_hwirq + size, 0, ops, host_data);
>   	if (domain)
>   		irq_domain_associate_many(domain, first_irq, first_hwirq, size);
> @@ -191,7 +213,7 @@ EXPORT_SYMBOL_GPL(irq_domain_add_legacy);
>    * @node: device-tree node of the interrupt controller
>    * @data: domain-specific data
>    */
> -struct irq_domain *irq_find_matching_host(struct device_node *node,
> +struct irq_domain *irq_find_matching_host(void *domain_token,
>   					  enum irq_domain_bus_token bus_token)
>   {
>   	struct irq_domain *h, *found = NULL;
> @@ -208,12 +230,16 @@ struct irq_domain *irq_find_matching_host(struct device_node *node,
>   	 */
>   	mutex_lock(&irq_domain_mutex);
>   	list_for_each_entry(h, &irq_domain_list, link) {
> -		if (h->ops->match)
> +		if (h->ops->match) {
> +			struct device_node *node;
> +			node = irq_domain_get_of_node(h);
>   			rc = h->ops->match(h, node, bus_token);
> -		else
> -			rc = ((h->of_node != NULL) && (h->of_node == node) &&
> +		} else {
> +			rc = ((h->domain_token != NULL) &&
> +			      (h->domain_token == domain_token) &&
>   			      ((bus_token == DOMAIN_BUS_ANY) ||
>   			       (h->bus_token == bus_token)));
> +		}
>
>   		if (rc) {
>   			found = h;
> @@ -336,10 +362,12 @@ EXPORT_SYMBOL_GPL(irq_domain_associate);
>   void irq_domain_associate_many(struct irq_domain *domain, unsigned int irq_base,
>   			       irq_hw_number_t hwirq_base, int count)
>   {
> +	struct device_node *of_node;
>   	int i;
>
> +	of_node = irq_domain_get_of_node(domain);
>   	pr_debug("%s(%s, irqbase=%i, hwbase=%i, count=%i)\n", __func__,
> -		of_node_full_name(domain->of_node), irq_base, (int)hwirq_base, count);
> +		of_node_full_name(of_node), irq_base, (int)hwirq_base, count);
>
>   	for (i = 0; i < count; i++) {
>   		irq_domain_associate(domain, irq_base + i, hwirq_base + i);
> @@ -359,12 +387,14 @@ EXPORT_SYMBOL_GPL(irq_domain_associate_many);
>    */
>   unsigned int irq_create_direct_mapping(struct irq_domain *domain)
>   {
> +	struct device_node *of_node;
>   	unsigned int virq;
>
>   	if (domain == NULL)
>   		domain = irq_default_domain;
>
> -	virq = irq_alloc_desc_from(1, of_node_to_nid(domain->of_node));
> +	of_node = irq_domain_get_of_node(domain);
> +	virq = irq_alloc_desc_from(1, of_node_to_nid(of_node));
>   	if (!virq) {
>   		pr_debug("create_direct virq allocation failed\n");
>   		return 0;
> @@ -399,6 +429,7 @@ EXPORT_SYMBOL_GPL(irq_create_direct_mapping);
>   unsigned int irq_create_mapping(struct irq_domain *domain,
>   				irq_hw_number_t hwirq)
>   {
> +	struct device_node *of_node;
>   	int virq;
>
>   	pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
> @@ -419,9 +450,11 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>   		return virq;
>   	}
>
> +	of_node = irq_domain_get_of_node(domain);
> +
>   	/* Allocate a virtual interrupt number */
>   	virq = irq_domain_alloc_descs(-1, 1, hwirq,
> -				      of_node_to_nid(domain->of_node));
> +				      of_node_to_nid(of_node));
>   	if (virq <= 0) {
>   		pr_debug("-> virq allocation failed\n");
>   		return 0;
> @@ -433,7 +466,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>   	}
>
>   	pr_debug("irq %lu on domain %s mapped to virtual irq %u\n",
> -		hwirq, of_node_full_name(domain->of_node), virq);
> +		hwirq, of_node_full_name(of_node), virq);
>
>   	return virq;
>   }
> @@ -460,10 +493,12 @@ EXPORT_SYMBOL_GPL(irq_create_mapping);
>   int irq_create_strict_mappings(struct irq_domain *domain, unsigned int irq_base,
>   			       irq_hw_number_t hwirq_base, int count)
>   {
> +	struct device_node *of_node;
>   	int ret;
>
> +	of_node = irq_domain_get_of_node(domain);
>   	ret = irq_alloc_descs(irq_base, irq_base, count,
> -			      of_node_to_nid(domain->of_node));
> +			      of_node_to_nid(of_node));
>   	if (unlikely(ret < 0))
>   		return ret;
>
> @@ -590,14 +625,16 @@ static int virq_debug_show(struct seq_file *m, void *private)
>   		   "name", "mapped", "linear-max", "direct-max", "devtree-node");
>   	mutex_lock(&irq_domain_mutex);
>   	list_for_each_entry(domain, &irq_domain_list, link) {
> +		struct device_node *of_node;
>   		int count = 0;
> +		of_node = irq_domain_token_get_node(domain);
>   		radix_tree_for_each_slot(slot, &domain->revmap_tree, &iter, 0)
>   			count++;
>   		seq_printf(m, "%c%-16s  %6u  %10u  %10u  %s\n",
>   			   domain == irq_default_domain ? '*' : ' ', domain->name,
>   			   domain->revmap_size + count, domain->revmap_size,
>   			   domain->revmap_direct_max_irq,
> -			   domain->of_node ? of_node_full_name(domain->of_node) : "");
> +			   of_node ? of_node_full_name(of_node) : "");
>   	}
>   	mutex_unlock(&irq_domain_mutex);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/