| Message ID | 20201124061720.86766-6-aik@ozlabs.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | genirq/irqdomain: Add reference counting to IRQs | expand |
Alexey, On Tue, Nov 24 2020 at 17:17, Alexey Kardashevskiy wrote: > We want to make the irq_desc.kobj's release hook free associated resources > but we do not want to pollute the irqdesc code with domains. Can you please describe your changelog in factual ways without 'we and I and want'? See Documentation/process/ > This adds a free_irq hook which is called when the last reference to > the descriptor is dropped. > > The first user is mapped irqs. This potentially can break the existing > users; however they seem to do the right thing and call dispose once > per mapping. Q: How is this supposed to work with CONFIG_SPARSE_IRQ=n? A: Not at all. Also 'seem to do the right thing' is from the same quality as 'should not break stuff'. Either you have done a proper analysis or you did not. Aside of that how is anyone supposed to decode the subtle wreckage which is this going to cause if 'seem to do the right thing' turns out to be wrong? > -void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq) > +static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq) > { > struct irq_data *irq_data = irq_get_irq_data(irq); > irq_hw_number_t hwirq; > @@ -582,6 +582,13 @@ void irq_domain_associate_many(struct irq_domain *domain, unsigned int irq_base, > } > EXPORT_SYMBOL_GPL(irq_domain_associate_many); > > +static void irq_mapped_free_desc(struct irq_desc *desc) That function name is really misleading and badly chosen. The function is not about freeing the irq descriptor as the name suggests. It's called from that code in order to clean up the mapping. > +{ > + unsigned int virq = desc->irq_data.irq; > + > + irq_domain_disassociate(desc->irq_data.domain, virq); > +} > + > /** > * irq_create_direct_mapping() - Allocate an irq for direct mapping > * @domain: domain to allocate the irq for or NULL for default domain > @@ -638,6 +645,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain, > { > struct device_node *of_node; > int virq; > + struct irq_desc *desc; This code uses reverse fir tree ordering of variables struct device_node *of_node; struct irq_desc *desc; int virq; Why? Because it's simpler to read than the vertical camel case above. Thanks, tglx
On Mon, Nov 30 2020 at 23:18, Thomas Gleixner wrote: > On Tue, Nov 24 2020 at 17:17, Alexey Kardashevskiy wrote: >> We want to make the irq_desc.kobj's release hook free associated resources >> but we do not want to pollute the irqdesc code with domains. > > Can you please describe your changelog in factual ways without 'we and I > and want'? See Documentation/process/ And while we are at process. MAINTAINERS clearly says: IRQ DOMAINS (IRQ NUMBER MAPPING LIBRARY) T: git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/core IRQ SUBSYSTEM T: git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/core So why are these patches not applying against that git branch? Thanks, tglx
diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h index 5745491303e0..6d44cb6a20ad 100644 --- a/include/linux/irqdesc.h +++ b/include/linux/irqdesc.h @@ -57,6 +57,7 @@ struct irq_desc { struct irq_data irq_data; unsigned int __percpu *kstat_irqs; irq_flow_handler_t handle_irq; + irq_free_handler_t free_irq; struct irqaction *action; /* IRQ action list */ unsigned int status_use_accessors; unsigned int core_internal_state__do_not_mess_with_it; diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h index a353b93ddf9e..ccca87cd3d15 100644 --- a/include/linux/irqdomain.h +++ b/include/linux/irqdomain.h @@ -381,8 +381,6 @@ extern int irq_domain_associate(struct irq_domain *domain, unsigned int irq, extern void irq_domain_associate_many(struct irq_domain *domain, unsigned int irq_base, irq_hw_number_t hwirq_base, int count); -extern void irq_domain_disassociate(struct irq_domain *domain, - unsigned int irq); extern unsigned int irq_create_mapping(struct irq_domain *host, irq_hw_number_t hwirq); diff --git a/include/linux/irqhandler.h b/include/linux/irqhandler.h index c30f454a9518..3dbc2bb764f3 100644 --- a/include/linux/irqhandler.h +++ b/include/linux/irqhandler.h @@ -10,5 +10,6 @@ struct irq_desc; struct irq_data; typedef void (*irq_flow_handler_t)(struct irq_desc *desc); +typedef void (*irq_free_handler_t)(struct irq_desc *desc); #endif diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c index 75374b7944b5..071363da8688 100644 --- a/kernel/irq/irqdesc.c +++ b/kernel/irq/irqdesc.c @@ -427,6 +427,9 @@ static void irq_kobj_release(struct kobject *kobj) struct irq_desc *desc = container_of(kobj, struct irq_desc, kobj); unsigned int irq = desc->irq_data.irq; + if (desc->free_irq) + desc->free_irq(desc); + irq_remove_debugfs_entry(desc); unregister_irq_proc(irq, desc); diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index 805478f81d96..4779d912bb86 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -485,7 +485,7 @@ static void irq_domain_set_mapping(struct irq_domain *domain, } } -void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq) +static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq) { struct irq_data *irq_data = irq_get_irq_data(irq); irq_hw_number_t hwirq; @@ -582,6 +582,13 @@ void irq_domain_associate_many(struct irq_domain *domain, unsigned int irq_base, } EXPORT_SYMBOL_GPL(irq_domain_associate_many); +static void irq_mapped_free_desc(struct irq_desc *desc) +{ + unsigned int virq = desc->irq_data.irq; + + irq_domain_disassociate(desc->irq_data.domain, virq); +} + /** * irq_create_direct_mapping() - Allocate an irq for direct mapping * @domain: domain to allocate the irq for or NULL for default domain @@ -638,6 +645,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain, { struct device_node *of_node; int virq; + struct irq_desc *desc; pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq); @@ -674,6 +682,9 @@ 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(of_node), virq); + desc = irq_to_desc(virq); + desc->free_irq = irq_mapped_free_desc; + return virq; } EXPORT_SYMBOL_GPL(irq_create_mapping); @@ -865,7 +876,6 @@ void irq_dispose_mapping(unsigned int virq) if (irq_domain_is_hierarchy(domain)) { irq_domain_free_irqs(virq, 1); } else { - irq_domain_disassociate(domain, virq); irq_free_desc(virq); } }
We want to make the irq_desc.kobj's release hook free associated resources but we do not want to pollute the irqdesc code with domains. This adds a free_irq hook which is called when the last reference to the descriptor is dropped. The first user is mapped irqs. This potentially can break the existing users; however they seem to do the right thing and call dispose once per mapping. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- include/linux/irqdesc.h | 1 + include/linux/irqdomain.h | 2 -- include/linux/irqhandler.h | 1 + kernel/irq/irqdesc.c | 3 +++ kernel/irq/irqdomain.c | 14 ++++++++++++-- 5 files changed, 17 insertions(+), 4 deletions(-)