| Message ID | 1442249047-21182-3-git-send-email-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Monday, September 14, 2015 05:44:01 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@arm.com> > --- > include/linux/irqdomain.h | 68 +++++++++++++++++++----------------- > kernel/irq/irqdomain.c | 89 ++++++++++++++++++++++++++++++++++------------- > 2 files changed, 101 insertions(+), 56 deletions(-) > > diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h > index f644fdb..ac7041b 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; I'm wondering if that may be something which isn't (void *), but a specific pointer type, so the compiler warns us when something suspicious is assigned to it? [Somewhat along the lines struct fwnode_handle is used elsewehere.] > enum irq_domain_bus_token bus_token; > struct irq_domain_chip_generic *gc; > #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY > @@ -161,70 +159,73 @@ enum { Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15/09/15 00:15, Rafael J. Wysocki wrote: > On Monday, September 14, 2015 05:44:01 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@arm.com> >> --- >> include/linux/irqdomain.h | 68 +++++++++++++++++++----------------- >> kernel/irq/irqdomain.c | 89 ++++++++++++++++++++++++++++++++++------------- >> 2 files changed, 101 insertions(+), 56 deletions(-) >> >> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h >> index f644fdb..ac7041b 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; > > I'm wondering if that may be something which isn't (void *), but a specific > pointer type, so the compiler warns us when something suspicious is assigned > to it? > > [Somewhat along the lines struct fwnode_handle is used elsewehere.] Yeah, I'm obviously being lazy ;-). More seriously, I'm trying hard to avoid anything that will require an additional memory allocation. Going from a device_node to a fwnode_handle-like structure requires such an allocation which is going to be a massive pain to retrofit - not impossible, but painful. What I'm currently aiming for is tagged pointers, where the two bottom bits indicate the type of the pointed object (see patch #3): - 00 indicates a device node - 01 indicates an IDR entry (shifted left by two bits) - 10 and 11 are currently unallocated, and one of them could be used to encode a fwnode_handle. It would be slightly easier to replace the (void *) with a union of the above types: union domain_token { struct device_node *of_node; struct fwnode_handle *fwnode; unsigned long id; }; That would remove the need for an extra allocation (at the cost of the above hack). We just need the accessors to strip the tag. Still need to repaint all the users of irq_domain_add_*, which is going to be amazingly invasive. Thoughts? M.
On 14.09.2015 18:44, 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@arm.com> > --- > include/linux/irqdomain.h | 68 +++++++++++++++++++----------------- > kernel/irq/irqdomain.c | 89 ++++++++++++++++++++++++++++++++++------------- > 2 files changed, 101 insertions(+), 56 deletions(-) > > diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h > index f644fdb..ac7041b 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); > @@ -292,7 +293,7 @@ extern void irq_domain_set_info(struct irq_domain *domain, unsigned int virq, > #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY > extern struct irq_domain *irq_domain_add_hierarchy(struct irq_domain *parent, > unsigned int flags, unsigned int size, > - struct device_node *node, > + void *domain_token, > const struct irq_domain_ops *ops, void *host_data); > extern int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base, > unsigned int nr_irqs, int node, void *arg, > @@ -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 79baaf8..a00e0ce 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)); While we are here, do you think it makes sense to abstract of_node_to_nid as well? Then we would really remove device_node dependency for irqdomain. Thanks, Tomasz > 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); > @@ -188,10 +210,12 @@ EXPORT_SYMBOL_GPL(irq_domain_add_legacy); > > /** > * irq_find_matching_host() - Locates a domain for a given device node > - * @node: device-tree node of the interrupt controller > + * @domain_token: global identifier of the interrupt controller. If this > + * is a valid kernel pointer, it is assumed to be a > + * struct device_node pointer. > * @bus_token: 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 +232,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 +364,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 +389,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 +431,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 +452,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 +468,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 +495,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 +627,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); > > @@ -755,7 +794,7 @@ static int irq_domain_alloc_descs(int virq, unsigned int cnt, > * @parent: Parent irq domain to associate with the new domain > * @flags: Irq domain flags associated to the domain > * @size: Size of the domain. See below > - * @node: Optional device-tree node of the interrupt controller > + * @domain_token: Optional global identifier of the interrupt controller > * @ops: Pointer to the interrupt domain callbacks > * @host_data: Controller private data pointer > * > @@ -768,16 +807,16 @@ static int irq_domain_alloc_descs(int virq, unsigned int cnt, > struct irq_domain *irq_domain_add_hierarchy(struct irq_domain *parent, > unsigned int flags, > unsigned int size, > - struct device_node *node, > + void *domain_token, > const struct irq_domain_ops *ops, > void *host_data) > { > struct irq_domain *domain; > > if (size) > - domain = irq_domain_add_linear(node, size, ops, host_data); > + domain = irq_domain_add_linear(domain_token, size, ops, host_data); > else > - domain = irq_domain_add_tree(node, ops, host_data); > + domain = irq_domain_add_tree(domain_token, ops, host_data); > if (domain) { > domain->parent = parent; > domain->flags |= flags; > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 15 Sep 2015, Tomasz Nowicki wrote: Can you folks please do proper quoting on reply? It's annoying to scroll down 250+ lines to find TWO lines of reply and then have another 250+ lines for nothing. Here is the real gist of your reply: > On 14.09.2015 18:44, Marc Zyngier wrote: > > -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)); > > While we are here, do you think it makes sense to abstract of_node_to_nid as > well? Then we would really remove device_node dependency for irqdomain. 15 lines of useful content versus 500+ lines of useless noise. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15.09.2015 14:04, Thomas Gleixner wrote: > On Tue, 15 Sep 2015, Tomasz Nowicki wrote: > > Can you folks please do proper quoting on reply? It's annoying to > scroll down 250+ lines to find TWO lines of reply and then have > another 250+ lines for nothing. > Yeah, good point! Sorry. Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15/09/15 11:58, Tomasz Nowicki wrote: > On 14.09.2015 18:44, 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@arm.com> >> --- >> include/linux/irqdomain.h | 68 +++++++++++++++++++----------------- >> kernel/irq/irqdomain.c | 89 ++++++++++++++++++++++++++++++++++------------- >> 2 files changed, 101 insertions(+), 56 deletions(-) >> [...] >> -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)); > > While we are here, do you think it makes sense to abstract > of_node_to_nid as well? Then we would really remove device_node > dependency for irqdomain. Not quite sure yet. I've decided to completely ignore the whole NUMA thing for the time being. This is a performance issue, not a functionality problem. Once we have something that actually makes sense, we can have a look. M.
On Tuesday, September 15, 2015 10:18:32 AM Marc Zyngier wrote: > On 15/09/15 00:15, Rafael J. Wysocki wrote: > > On Monday, September 14, 2015 05:44:01 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@arm.com> > >> --- > >> include/linux/irqdomain.h | 68 +++++++++++++++++++----------------- > >> kernel/irq/irqdomain.c | 89 ++++++++++++++++++++++++++++++++++------------- > >> 2 files changed, 101 insertions(+), 56 deletions(-) > >> > >> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h > >> index f644fdb..ac7041b 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; > > > > I'm wondering if that may be something which isn't (void *), but a specific > > pointer type, so the compiler warns us when something suspicious is assigned > > to it? > > > > [Somewhat along the lines struct fwnode_handle is used elsewehere.] > > Yeah, I'm obviously being lazy ;-). > > More seriously, I'm trying hard to avoid anything that will require an > additional memory allocation. Going from a device_node to a > fwnode_handle-like structure requires such an allocation which is going > to be a massive pain to retrofit - not impossible, but painful. > > What I'm currently aiming for is tagged pointers, where the two bottom > bits indicate the type of the pointed object (see patch #3): > - 00 indicates a device node > - 01 indicates an IDR entry (shifted left by two bits) > - 10 and 11 are currently unallocated, and one of them could be used to > encode a fwnode_handle. > > It would be slightly easier to replace the (void *) with a union of the > above types: > > union domain_token { > struct device_node *of_node; > struct fwnode_handle *fwnode; > unsigned long id; > }; > > That would remove the need for an extra allocation (at the cost of the > above hack). We just need the accessors to strip the tag. Still need to > repaint all the users of irq_domain_add_*, which is going to be > amazingly invasive. > > Thoughts? Well, I'm not sure this is worth the effort to be honest. I've just seen quite a few bugs where a pointer to something completely invalid have been silently passed via (void *) which often results in very interesting breakage (that is really hard to debug for that matter). Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16/09/15 02:53, Rafael J. Wysocki wrote: > On Tuesday, September 15, 2015 10:18:32 AM Marc Zyngier wrote: >> On 15/09/15 00:15, Rafael J. Wysocki wrote: >>> On Monday, September 14, 2015 05:44:01 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@arm.com> >>>> --- >>>> include/linux/irqdomain.h | 68 +++++++++++++++++++----------------- >>>> kernel/irq/irqdomain.c | 89 ++++++++++++++++++++++++++++++++++------------- >>>> 2 files changed, 101 insertions(+), 56 deletions(-) >>>> >>>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h >>>> index f644fdb..ac7041b 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; >>> >>> I'm wondering if that may be something which isn't (void *), but a specific >>> pointer type, so the compiler warns us when something suspicious is assigned >>> to it? >>> >>> [Somewhat along the lines struct fwnode_handle is used elsewehere.] >> >> Yeah, I'm obviously being lazy ;-). >> >> More seriously, I'm trying hard to avoid anything that will require an >> additional memory allocation. Going from a device_node to a >> fwnode_handle-like structure requires such an allocation which is going >> to be a massive pain to retrofit - not impossible, but painful. >> >> What I'm currently aiming for is tagged pointers, where the two bottom >> bits indicate the type of the pointed object (see patch #3): >> - 00 indicates a device node >> - 01 indicates an IDR entry (shifted left by two bits) >> - 10 and 11 are currently unallocated, and one of them could be used to >> encode a fwnode_handle. >> >> It would be slightly easier to replace the (void *) with a union of the >> above types: >> >> union domain_token { >> struct device_node *of_node; >> struct fwnode_handle *fwnode; >> unsigned long id; >> }; >> >> That would remove the need for an extra allocation (at the cost of the >> above hack). We just need the accessors to strip the tag. Still need to >> repaint all the users of irq_domain_add_*, which is going to be >> amazingly invasive. >> >> Thoughts? > > Well, I'm not sure this is worth the effort to be honest. > > I've just seen quite a few bugs where a pointer to something completely invalid > have been silently passed via (void *) which often results in very interesting > breakage (that is really hard to debug for that matter). I actually tried to prototype this yesterday, and ended up in hell. The main issue is the point where the generic irqdomain code meets the DT subsystem (which is basically any interrupt controller, including those being ACPI driven). The domain_token to of_node path is dead easy, but you cannot do the reverse conversion, so this would have to spread around like a cancer. I gave up. At this stage, I see two options: sticking with (void *) with the risk of breakage and subtle bugs which we all love to track down (not!), or do a major U-turn and make device_node a strict requirement for domain lookup. After the above experiment, I now see some value in actually keeping device_node around, using it as the token, and allocating it when required (the typical case being those interrupt controllers that are both DT and ACPI). I'll play with it a bit more. Thanks, M.
On Wed, 16 Sep 2015, Marc Zyngier wrote: > On 16/09/15 02:53, Rafael J. Wysocki wrote: > > I've just seen quite a few bugs where a pointer to something completely invalid > > have been silently passed via (void *) which often results in very interesting > > breakage (that is really hard to debug for that matter). > > I actually tried to prototype this yesterday, and ended up in hell. The > main issue is the point where the generic irqdomain code meets the DT > subsystem (which is basically any interrupt controller, including those > being ACPI driven). The domain_token to of_node path is dead easy, but > you cannot do the reverse conversion, so this would have to spread > around like a cancer. I gave up. > > At this stage, I see two options: sticking with (void *) with the risk > of breakage and subtle bugs which we all love to track down (not!), or > do a major U-turn and make device_node a strict requirement for domain > lookup. > > After the above experiment, I now see some value in actually keeping > device_node around, using it as the token, and allocating it when > required (the typical case being those interrupt controllers that are > both DT and ACPI). I'll play with it a bit more. Right. We already have the fwnode_type in struct device_node, so we can reuse it. If you care about the size of the struct, then you can make some of the struct members conditional on CONFIG_OF. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, September 16, 2015 08:49:27 AM Marc Zyngier wrote: > On 16/09/15 02:53, Rafael J. Wysocki wrote: > > On Tuesday, September 15, 2015 10:18:32 AM Marc Zyngier wrote: > >> On 15/09/15 00:15, Rafael J. Wysocki wrote: > >>> On Monday, September 14, 2015 05:44:01 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@arm.com> > >>>> --- > >>>> include/linux/irqdomain.h | 68 +++++++++++++++++++----------------- > >>>> kernel/irq/irqdomain.c | 89 ++++++++++++++++++++++++++++++++++------------- > >>>> 2 files changed, 101 insertions(+), 56 deletions(-) > >>>> > >>>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h > >>>> index f644fdb..ac7041b 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; > >>> > >>> I'm wondering if that may be something which isn't (void *), but a specific > >>> pointer type, so the compiler warns us when something suspicious is assigned > >>> to it? > >>> > >>> [Somewhat along the lines struct fwnode_handle is used elsewehere.] > >> > >> Yeah, I'm obviously being lazy ;-). > >> > >> More seriously, I'm trying hard to avoid anything that will require an > >> additional memory allocation. Going from a device_node to a > >> fwnode_handle-like structure requires such an allocation which is going > >> to be a massive pain to retrofit - not impossible, but painful. > >> > >> What I'm currently aiming for is tagged pointers, where the two bottom > >> bits indicate the type of the pointed object (see patch #3): > >> - 00 indicates a device node > >> - 01 indicates an IDR entry (shifted left by two bits) > >> - 10 and 11 are currently unallocated, and one of them could be used to > >> encode a fwnode_handle. > >> > >> It would be slightly easier to replace the (void *) with a union of the > >> above types: > >> > >> union domain_token { > >> struct device_node *of_node; > >> struct fwnode_handle *fwnode; > >> unsigned long id; > >> }; > >> > >> That would remove the need for an extra allocation (at the cost of the > >> above hack). We just need the accessors to strip the tag. Still need to > >> repaint all the users of irq_domain_add_*, which is going to be > >> amazingly invasive. > >> > >> Thoughts? > > > > Well, I'm not sure this is worth the effort to be honest. > > > > I've just seen quite a few bugs where a pointer to something completely invalid > > have been silently passed via (void *) which often results in very interesting > > breakage (that is really hard to debug for that matter). > > I actually tried to prototype this yesterday, and ended up in hell. The > main issue is the point where the generic irqdomain code meets the DT > subsystem (which is basically any interrupt controller, including those > being ACPI driven). The domain_token to of_node path is dead easy, but > you cannot do the reverse conversion, so this would have to spread > around like a cancer. I gave up. If you replaced the struct device_node pointer with a struct fwnode_handle one, you can get from there to the original with to_of_node() easily and backwards too. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h index f644fdb..ac7041b 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); @@ -292,7 +293,7 @@ extern void irq_domain_set_info(struct irq_domain *domain, unsigned int virq, #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY extern struct irq_domain *irq_domain_add_hierarchy(struct irq_domain *parent, unsigned int flags, unsigned int size, - struct device_node *node, + void *domain_token, const struct irq_domain_ops *ops, void *host_data); extern int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base, unsigned int nr_irqs, int node, void *arg, @@ -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 79baaf8..a00e0ce 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); @@ -188,10 +210,12 @@ EXPORT_SYMBOL_GPL(irq_domain_add_legacy); /** * irq_find_matching_host() - Locates a domain for a given device node - * @node: device-tree node of the interrupt controller + * @domain_token: global identifier of the interrupt controller. If this + * is a valid kernel pointer, it is assumed to be a + * struct device_node pointer. * @bus_token: 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 +232,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 +364,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 +389,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 +431,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 +452,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 +468,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 +495,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 +627,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); @@ -755,7 +794,7 @@ static int irq_domain_alloc_descs(int virq, unsigned int cnt, * @parent: Parent irq domain to associate with the new domain * @flags: Irq domain flags associated to the domain * @size: Size of the domain. See below - * @node: Optional device-tree node of the interrupt controller + * @domain_token: Optional global identifier of the interrupt controller * @ops: Pointer to the interrupt domain callbacks * @host_data: Controller private data pointer * @@ -768,16 +807,16 @@ static int irq_domain_alloc_descs(int virq, unsigned int cnt, struct irq_domain *irq_domain_add_hierarchy(struct irq_domain *parent, unsigned int flags, unsigned int size, - struct device_node *node, + void *domain_token, const struct irq_domain_ops *ops, void *host_data) { struct irq_domain *domain; if (size) - domain = irq_domain_add_linear(node, size, ops, host_data); + domain = irq_domain_add_linear(domain_token, size, ops, host_data); else - domain = irq_domain_add_tree(node, ops, host_data); + domain = irq_domain_add_tree(domain_token, ops, host_data); if (domain) { domain->parent = parent; domain->flags |= flags;
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@arm.com> --- include/linux/irqdomain.h | 68 +++++++++++++++++++----------------- kernel/irq/irqdomain.c | 89 ++++++++++++++++++++++++++++++++++------------- 2 files changed, 101 insertions(+), 56 deletions(-)