* [PATCH 1/2] Add irq_free_host() to free an irq_host @ 2008-05-08 4:23 Michael Ellerman 2008-05-08 4:23 ` [PATCH 2/2] Fix irq_alloc_host() reference counting and callers Michael Ellerman 2008-05-15 6:55 ` [PATCH 1/2] Add irq_free_host() to free an irq_host Benjamin Herrenschmidt 0 siblings, 2 replies; 6+ messages in thread From: Michael Ellerman @ 2008-05-08 4:23 UTC (permalink / raw) To: linuxppc-dev Currently there is no irq_free_host() routine, and that's more or less OK - because all you need to do is free it. However the next patch needs to introduce additional tear down so we'll need a proper free routine. Signed-off-by: Michael Ellerman <michael@ellerman.id.au> --- arch/powerpc/kernel/irq.c | 15 ++++++++------- include/asm-powerpc/irq.h | 8 ++++++++ 2 files changed, 16 insertions(+), 7 deletions(-) ps. This is 27 material. diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c index 2f73f70..8ba8fbf 100644 --- a/arch/powerpc/kernel/irq.c +++ b/arch/powerpc/kernel/irq.c @@ -442,6 +442,13 @@ static int default_irq_host_match(struct irq_host *h, struct device_node *np) return h->of_node != NULL && h->of_node == np; } +void irq_free_host(struct irq_host *host) +{ + /* If it's still very early in boot we can't free, oh well. */ + if (mem_init_done) + kfree(host); +} + struct irq_host *irq_alloc_host(struct device_node *of_node, unsigned int revmap_type, unsigned int revmap_arg, @@ -478,13 +485,7 @@ struct irq_host *irq_alloc_host(struct device_node *of_node, if (revmap_type == IRQ_HOST_MAP_LEGACY) { if (irq_map[0].host != NULL) { spin_unlock_irqrestore(&irq_big_lock, flags); - /* If we are early boot, we can't free the structure, - * too bad... - * this will be fixed once slab is made available early - * instead of the current cruft - */ - if (mem_init_done) - kfree(host); + irq_free_host(host); return NULL; } irq_map[0].host = host; diff --git a/include/asm-powerpc/irq.h b/include/asm-powerpc/irq.h index 5089deb..7d9974a 100644 --- a/include/asm-powerpc/irq.h +++ b/include/asm-powerpc/irq.h @@ -165,6 +165,14 @@ extern struct irq_host *irq_alloc_host(struct device_node *of_node, struct irq_host_ops *ops, irq_hw_number_t inval_irq); +/** + * irq_free_host - Free an irq_host data structure + * @host: The irq_host to free. + * + * Does any required cleanup and then frees the irq_host. + */ +extern void irq_free_host(struct irq_host *host); + /** * irq_find_host - Locates a host for a given device node -- 1.5.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] Fix irq_alloc_host() reference counting and callers 2008-05-08 4:23 [PATCH 1/2] Add irq_free_host() to free an irq_host Michael Ellerman @ 2008-05-08 4:23 ` Michael Ellerman 2008-05-13 10:58 ` Paul Mackerras 2008-05-15 6:55 ` [PATCH 1/2] Add irq_free_host() to free an irq_host Benjamin Herrenschmidt 1 sibling, 1 reply; 6+ messages in thread From: Michael Ellerman @ 2008-05-08 4:23 UTC (permalink / raw) To: linuxppc-dev When I changed irq_alloc_host() to take an of_node (52964f87c64e6c6ea671b5bf3030fb1494090a48: "Add an optional device_node pointer to the irq_host"), I botched the reference counting semantics. Stephen pointed out that it's irq_alloc_host()'s business if it needs to take an additional reference to the device_node, the caller shouldn't need to care. To make that clean we also need a free routine, so the caller doesn't have to deal with dropping the reference. Luckily we now have an irq_free_host() where we can drop the reference - so we can make the reference counting internal to irq_alloc_host()/irq_free_host(). Signed-off-by: Michael Ellerman <michael@ellerman.id.au> --- arch/powerpc/kernel/irq.c | 4 +++- arch/powerpc/platforms/cell/axon_msi.c | 4 ++-- arch/powerpc/platforms/cell/spider-pic.c | 2 +- arch/powerpc/sysdev/cpm1.c | 2 +- arch/powerpc/sysdev/cpm2_pic.c | 2 +- arch/powerpc/sysdev/i8259.c | 2 +- arch/powerpc/sysdev/ipic.c | 8 +++----- arch/powerpc/sysdev/mpic.c | 6 ++---- arch/powerpc/sysdev/qe_lib/qe_ic.c | 6 ++---- arch/powerpc/sysdev/tsi108_pci.c | 3 +-- arch/powerpc/sysdev/uic.c | 6 ++---- 11 files changed, 19 insertions(+), 26 deletions(-) diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c index 8ba8fbf..0cb2cf3 100644 --- a/arch/powerpc/kernel/irq.c +++ b/arch/powerpc/kernel/irq.c @@ -444,6 +444,8 @@ static int default_irq_host_match(struct irq_host *h, struct device_node *np) void irq_free_host(struct irq_host *host) { + of_node_put(host->of_node); + /* If it's still very early in boot we can't free, oh well. */ if (mem_init_done) kfree(host); @@ -472,7 +474,7 @@ struct irq_host *irq_alloc_host(struct device_node *of_node, host->revmap_type = revmap_type; host->inval_irq = inval_irq; host->ops = ops; - host->of_node = of_node; + host->of_node = of_node_get(of_node); if (host->ops->match == NULL) host->ops->match = default_irq_host_match; diff --git a/arch/powerpc/platforms/cell/axon_msi.c b/arch/powerpc/platforms/cell/axon_msi.c index 8b055bc..45bddbc 100644 --- a/arch/powerpc/platforms/cell/axon_msi.c +++ b/arch/powerpc/platforms/cell/axon_msi.c @@ -358,7 +358,7 @@ static int axon_msi_probe(struct of_device *device, goto out_free_msic; } - msic->irq_host = irq_alloc_host(of_node_get(dn), IRQ_HOST_MAP_NOMAP, + msic->irq_host = irq_alloc_host(dn, IRQ_HOST_MAP_NOMAP, NR_IRQS, &msic_host_ops, 0); if (!msic->irq_host) { printk(KERN_ERR "axon_msi: couldn't allocate irq_host for %s\n", @@ -400,7 +400,7 @@ static int axon_msi_probe(struct of_device *device, return 0; out_free_host: - kfree(msic->irq_host); + irq_free_host(msic->irq_host); out_free_fifo: dma_free_coherent(&device->dev, MSIC_FIFO_SIZE_BYTES, msic->fifo_virt, msic->fifo_phys); diff --git a/arch/powerpc/platforms/cell/spider-pic.c b/arch/powerpc/platforms/cell/spider-pic.c index 3f4b4ae..4e56556 100644 --- a/arch/powerpc/platforms/cell/spider-pic.c +++ b/arch/powerpc/platforms/cell/spider-pic.c @@ -300,7 +300,7 @@ static void __init spider_init_one(struct device_node *of_node, int chip, panic("spider_pic: can't map registers !"); /* Allocate a host */ - pic->host = irq_alloc_host(of_node_get(of_node), IRQ_HOST_MAP_LINEAR, + pic->host = irq_alloc_host(of_node, IRQ_HOST_MAP_LINEAR, SPIDER_SRC_COUNT, &spider_host_ops, SPIDER_IRQ_INVALID); if (pic->host == NULL) diff --git a/arch/powerpc/sysdev/cpm1.c b/arch/powerpc/sysdev/cpm1.c index 58292a0..661df42 100644 --- a/arch/powerpc/sysdev/cpm1.c +++ b/arch/powerpc/sysdev/cpm1.c @@ -159,7 +159,7 @@ unsigned int cpm_pic_init(void) out_be32(&cpic_reg->cpic_cimr, 0); - cpm_pic_host = irq_alloc_host(of_node_get(np), IRQ_HOST_MAP_LINEAR, + cpm_pic_host = irq_alloc_host(np, IRQ_HOST_MAP_LINEAR, 64, &cpm_pic_host_ops, 64); if (cpm_pic_host == NULL) { printk(KERN_ERR "CPM2 PIC: failed to allocate irq host!\n"); diff --git a/arch/powerpc/sysdev/cpm2_pic.c b/arch/powerpc/sysdev/cpm2_pic.c index 5fe65b2..b16ca3e 100644 --- a/arch/powerpc/sysdev/cpm2_pic.c +++ b/arch/powerpc/sysdev/cpm2_pic.c @@ -266,7 +266,7 @@ void cpm2_pic_init(struct device_node *node) out_be32(&cpm2_intctl->ic_scprrl, 0x05309770); /* create a legacy host */ - cpm2_pic_host = irq_alloc_host(of_node_get(node), IRQ_HOST_MAP_LINEAR, + cpm2_pic_host = irq_alloc_host(node, IRQ_HOST_MAP_LINEAR, 64, &cpm2_pic_host_ops, 64); if (cpm2_pic_host == NULL) { printk(KERN_ERR "CPM2 PIC: failed to allocate irq host!\n"); diff --git a/arch/powerpc/sysdev/i8259.c b/arch/powerpc/sysdev/i8259.c index 216c0f5..a96584a 100644 --- a/arch/powerpc/sysdev/i8259.c +++ b/arch/powerpc/sysdev/i8259.c @@ -276,7 +276,7 @@ void i8259_init(struct device_node *node, unsigned long intack_addr) spin_unlock_irqrestore(&i8259_lock, flags); /* create a legacy host */ - i8259_host = irq_alloc_host(of_node_get(node), IRQ_HOST_MAP_LEGACY, + i8259_host = irq_alloc_host(node, IRQ_HOST_MAP_LEGACY, 0, &i8259_host_ops, 0); if (i8259_host == NULL) { printk(KERN_ERR "i8259: failed to allocate irq host !\n"); diff --git a/arch/powerpc/sysdev/ipic.c b/arch/powerpc/sysdev/ipic.c index 0f2dfb0..80ebba3 100644 --- a/arch/powerpc/sysdev/ipic.c +++ b/arch/powerpc/sysdev/ipic.c @@ -731,17 +731,15 @@ struct ipic * __init ipic_init(struct device_node *node, unsigned int flags) memset(ipic, 0, sizeof(struct ipic)); - ipic->irqhost = irq_alloc_host(of_node_get(node), IRQ_HOST_MAP_LINEAR, + ipic->irqhost = irq_alloc_host(node, IRQ_HOST_MAP_LINEAR, NR_IPIC_INTS, &ipic_host_ops, 0); - if (ipic->irqhost == NULL) { - of_node_put(node); + if (ipic->irqhost == NULL) return NULL; - } ret = of_address_to_resource(node, 0, &res); if (ret) { - of_node_put(node); + irq_free_host(ipic->irqhost); return NULL; } diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c index 8619f2a..7df0252 100644 --- a/arch/powerpc/sysdev/mpic.c +++ b/arch/powerpc/sysdev/mpic.c @@ -1016,13 +1016,11 @@ struct mpic * __init mpic_alloc(struct device_node *node, memset(mpic, 0, sizeof(struct mpic)); mpic->name = name; - mpic->irqhost = irq_alloc_host(of_node_get(node), IRQ_HOST_MAP_LINEAR, + mpic->irqhost = irq_alloc_host(node, IRQ_HOST_MAP_LINEAR, isu_size, &mpic_host_ops, flags & MPIC_LARGE_VECTORS ? 2048 : 256); - if (mpic->irqhost == NULL) { - of_node_put(node); + if (mpic->irqhost == NULL) return NULL; - } mpic->irqhost->host_data = mpic; mpic->hc_irq = mpic_irq_chip; diff --git a/arch/powerpc/sysdev/qe_lib/qe_ic.c b/arch/powerpc/sysdev/qe_lib/qe_ic.c index f59444d..b6f3d1c 100644 --- a/arch/powerpc/sysdev/qe_lib/qe_ic.c +++ b/arch/powerpc/sysdev/qe_lib/qe_ic.c @@ -335,12 +335,10 @@ void __init qe_ic_init(struct device_node *node, unsigned int flags, memset(qe_ic, 0, sizeof(struct qe_ic)); - qe_ic->irqhost = irq_alloc_host(of_node_get(node), IRQ_HOST_MAP_LINEAR, + qe_ic->irqhost = irq_alloc_host(node, IRQ_HOST_MAP_LINEAR, NR_QE_IC_INTS, &qe_ic_host_ops, 0); - if (qe_ic->irqhost == NULL) { - of_node_put(node); + if (qe_ic->irqhost == NULL) return; - } ret = of_address_to_resource(node, 0, &res); if (ret) diff --git a/arch/powerpc/sysdev/tsi108_pci.c b/arch/powerpc/sysdev/tsi108_pci.c index ac1a72d..24e1f5a 100644 --- a/arch/powerpc/sysdev/tsi108_pci.c +++ b/arch/powerpc/sysdev/tsi108_pci.c @@ -426,11 +426,10 @@ void __init tsi108_pci_int_init(struct device_node *node) { DBG("Tsi108_pci_int_init: initializing PCI interrupts\n"); - pci_irq_host = irq_alloc_host(of_node_get(node), IRQ_HOST_MAP_LEGACY, + pci_irq_host = irq_alloc_host(node, IRQ_HOST_MAP_LEGACY, 0, &pci_irq_host_ops, 0); if (pci_irq_host == NULL) { printk(KERN_ERR "pci_irq_host: failed to allocate irq host !\n"); - of_node_put(node); return; } diff --git a/arch/powerpc/sysdev/uic.c b/arch/powerpc/sysdev/uic.c index 625b275..d35405c 100644 --- a/arch/powerpc/sysdev/uic.c +++ b/arch/powerpc/sysdev/uic.c @@ -280,12 +280,10 @@ static struct uic * __init uic_init_one(struct device_node *node) } uic->dcrbase = *dcrreg; - uic->irqhost = irq_alloc_host(of_node_get(node), IRQ_HOST_MAP_LINEAR, + uic->irqhost = irq_alloc_host(node, IRQ_HOST_MAP_LINEAR, NR_UIC_INTS, &uic_host_ops, -1); - if (! uic->irqhost) { - of_node_put(node); + if (! uic->irqhost) return NULL; /* FIXME: panic? */ - } uic->irqhost->host_data = uic; -- 1.5.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] Fix irq_alloc_host() reference counting and callers 2008-05-08 4:23 ` [PATCH 2/2] Fix irq_alloc_host() reference counting and callers Michael Ellerman @ 2008-05-13 10:58 ` Paul Mackerras 2008-05-13 11:49 ` Michael Ellerman 0 siblings, 1 reply; 6+ messages in thread From: Paul Mackerras @ 2008-05-13 10:58 UTC (permalink / raw) To: Michael Ellerman; +Cc: linuxppc-dev Michael Ellerman writes: > When I changed irq_alloc_host() to take an of_node > (52964f87c64e6c6ea671b5bf3030fb1494090a48: "Add an > optional device_node pointer to the irq_host"), > I botched the reference counting semantics. > > Stephen pointed out that it's irq_alloc_host()'s > business if it needs to take an additional reference > to the device_node, the caller shouldn't need to care. > To make that clean we also need a free routine, so > the caller doesn't have to deal with dropping the > reference. > > Luckily we now have an irq_free_host() where we can > drop the reference - so we can make the reference > counting internal to irq_alloc_host()/irq_free_host(). Is this needed for 2.6.26? What's the worst possible effect of this bug? Paul. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] Fix irq_alloc_host() reference counting and callers 2008-05-13 10:58 ` Paul Mackerras @ 2008-05-13 11:49 ` Michael Ellerman 0 siblings, 0 replies; 6+ messages in thread From: Michael Ellerman @ 2008-05-13 11:49 UTC (permalink / raw) To: Paul Mackerras; +Cc: linuxppc-dev [-- Attachment #1: Type: text/plain, Size: 1268 bytes --] On Tue, 2008-05-13 at 20:58 +1000, Paul Mackerras wrote: > Michael Ellerman writes: > > > When I changed irq_alloc_host() to take an of_node > > (52964f87c64e6c6ea671b5bf3030fb1494090a48: "Add an > > optional device_node pointer to the irq_host"), > > I botched the reference counting semantics. > > > > Stephen pointed out that it's irq_alloc_host()'s > > business if it needs to take an additional reference > > to the device_node, the caller shouldn't need to care. > > To make that clean we also need a free routine, so > > the caller doesn't have to deal with dropping the > > reference. > > > > Luckily we now have an irq_free_host() where we can > > drop the reference - so we can make the reference > > counting internal to irq_alloc_host()/irq_free_host(). > > Is this needed for 2.6.26? No. > What's the worst possible effect of this bug? Only nodes having an incorrect reference count - in either direction. But pseries is fine, so there should be no real bug. cheers -- Michael Ellerman OzLabs, IBM Australia Development Lab wwweb: http://michael.ellerman.id.au phone: +61 2 6212 1183 (tie line 70 21183) We do not inherit the earth from our ancestors, we borrow it from our children. - S.M.A.R.T Person [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] Add irq_free_host() to free an irq_host 2008-05-08 4:23 [PATCH 1/2] Add irq_free_host() to free an irq_host Michael Ellerman 2008-05-08 4:23 ` [PATCH 2/2] Fix irq_alloc_host() reference counting and callers Michael Ellerman @ 2008-05-15 6:55 ` Benjamin Herrenschmidt 2008-05-15 7:00 ` Michael Ellerman 1 sibling, 1 reply; 6+ messages in thread From: Benjamin Herrenschmidt @ 2008-05-15 6:55 UTC (permalink / raw) To: Michael Ellerman; +Cc: linuxppc-dev On Thu, 2008-05-08 at 14:23 +1000, Michael Ellerman wrote: > +void irq_free_host(struct irq_host *host) > +{ > + /* If it's still very early in boot we can't free, oh well. */ > + if (mem_init_done) > + kfree(host); > +} Hrm... that means that a host that was allocated before mem_init_done and freed later will call kfree on memory obtained from bootmem... no good. In which case do we need to free and irq host other than failure in irq_alloc_host ? Cheers, Ben. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] Add irq_free_host() to free an irq_host 2008-05-15 6:55 ` [PATCH 1/2] Add irq_free_host() to free an irq_host Benjamin Herrenschmidt @ 2008-05-15 7:00 ` Michael Ellerman 0 siblings, 0 replies; 6+ messages in thread From: Michael Ellerman @ 2008-05-15 7:00 UTC (permalink / raw) To: benh; +Cc: linuxppc-dev [-- Attachment #1: Type: text/plain, Size: 1295 bytes --] On Wed, 2008-05-14 at 23:55 -0700, Benjamin Herrenschmidt wrote: > On Thu, 2008-05-08 at 14:23 +1000, Michael Ellerman wrote: > > +void irq_free_host(struct irq_host *host) > > +{ > > + /* If it's still very early in boot we can't free, oh well. */ > > + if (mem_init_done) > > + kfree(host); > > +} > > Hrm... that means that a host that was allocated before mem_init_done > and freed later will call kfree on memory obtained from bootmem... no > good. God damn mem_init_done crap .. gah you're right. > In which case do we need to free and irq host other than failure in > irq_alloc_host ? Well most code either doesn't check the return value, or panics. There's two callers at the moment (after my patch) of irq_free_host(), axon_msi.c and ipic.c - they both call free from the same function as alloc so they're safe. We could just do the free in irq_alloc_host()'s error path and have irq_free_host() not actually free anything, just drop the reference count. What a mess. cheers -- Michael Ellerman OzLabs, IBM Australia Development Lab wwweb: http://michael.ellerman.id.au phone: +61 2 6212 1183 (tie line 70 21183) We do not inherit the earth from our ancestors, we borrow it from our children. - S.M.A.R.T Person [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-05-15 7:00 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-05-08 4:23 [PATCH 1/2] Add irq_free_host() to free an irq_host Michael Ellerman 2008-05-08 4:23 ` [PATCH 2/2] Fix irq_alloc_host() reference counting and callers Michael Ellerman 2008-05-13 10:58 ` Paul Mackerras 2008-05-13 11:49 ` Michael Ellerman 2008-05-15 6:55 ` [PATCH 1/2] Add irq_free_host() to free an irq_host Benjamin Herrenschmidt 2008-05-15 7:00 ` Michael Ellerman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).