[PATCH 1/2] Add irq_free_host() to free an irq_host

[PATCH 1/2] Add irq_free_host() to free an irq_host

[Michael Ellerman]

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

[PATCH 2/2] Fix irq_alloc_host() reference counting and callers

[Michael Ellerman]

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

Re: [PATCH 2/2] Fix irq_alloc_host() reference counting and callers

[Paul Mackerras]

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.

Re: [PATCH 2/2] Fix irq_alloc_host() reference counting and callers

[Michael Ellerman]

[-- 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 --]

Re: [PATCH 1/2] Add irq_free_host() to free an irq_host

[Benjamin Herrenschmidt]

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.

Re: [PATCH 1/2] Add irq_free_host() to free an irq_host

[Michael Ellerman]

[-- 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 --]