[PATCH 1/2] Introduce of_get_pci_dev_node()

[PATCH 1/2] Introduce of_get_pci_dev_node()

[Michael Ellerman]

Both powerpc and sparc have a routine, pci_device_to_OF_node(), which
returns the device_node associated with a given pci device.

The new routine, of_get_pci_dev_node(), performs exactly the same function
with the one exception that it returns a refcounted pointer to the
device_node, callers must then release the reference with of_node_put().

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
 drivers/of/Makefile |    1 +
 drivers/of/pci.c    |   24 ++++++++++++++++++++++++
 include/linux/of.h  |    3 +++
 3 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index ab9be5d..f49898c 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -1,2 +1,3 @@
 obj-y = base.o
 obj-$(CONFIG_OF_DEVICE) += device.o platform.o
+obj-$(CONFIG_PCI)	+= pci.o
diff --git a/drivers/of/pci.c b/drivers/of/pci.c
new file mode 100644
index 0000000..2319c22
--- /dev/null
+++ b/drivers/of/pci.c
@@ -0,0 +1,24 @@
+/*
+ * Copyright 2007 Michael Ellerman, IBM Corporation.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/of.h>
+#include <linux/pci.h>
+
+/**
+ * of_get_pci_dev_node - Get a device node from a pci_dev
+ * @pdev: pci_dev to get the node from
+ *
+ * Returns a node pointer with refcount incremented, use
+ * of_node_put() on it when done.
+ */
+struct device_node *of_get_pci_dev_node(struct pci_dev *pdev)
+{
+	return of_node_get(pci_device_to_OF_node(pdev));
+}
+EXPORT_SYMBOL(of_get_pci_dev_node);
diff --git a/include/linux/of.h b/include/linux/of.h
index 6df80e9..e5bfb13 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -57,4 +57,7 @@ extern const void *of_get_property(const struct device_node *node,
 extern int of_n_addr_cells(struct device_node *np);
 extern int of_n_size_cells(struct device_node *np);
 
+struct pci_dev;
+extern struct device_node *of_get_pci_dev_node(struct pci_dev *pdev);
+
 #endif /* _LINUX_OF_H */
-- 
1.5.1.3.g7a33b

[PATCH 2/2] Use of_get_pci_dev_node() in axon_msi.c

[Michael Ellerman]

Use of_get_pci_dev_node() in axon_msi.c. Switch to including <linux/of.h>
so we get the prototype.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
 arch/powerpc/platforms/cell/axon_msi.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/cell/axon_msi.c b/arch/powerpc/platforms/cell/axon_msi.c
index 095988f..034041c 100644
--- a/arch/powerpc/platforms/cell/axon_msi.c
+++ b/arch/powerpc/platforms/cell/axon_msi.c
@@ -14,10 +14,10 @@
 #include <linux/pci.h>
 #include <linux/msi.h>
 #include <linux/reboot.h>
+#include <linux/of.h>
 
 #include <asm/dcr.h>
 #include <asm/machdep.h>
-#include <asm/prom.h>
 
 
 /*
@@ -119,7 +119,7 @@ static struct axon_msic *find_msi_translator(struct pci_dev *dev)
 	const phandle *ph;
 	struct axon_msic *msic = NULL;
 
-	dn = of_node_get(pci_device_to_OF_node(dev));
+	dn = of_get_pci_dev_node(dev);
 	if (!dn) {
 		dev_dbg(&dev->dev, "axon_msi: no pci_dn found\n");
 		return NULL;
@@ -176,7 +176,7 @@ static int setup_msi_msg_address(struct pci_dev *dev, struct msi_msg *msg)
 	int len;
 	const u32 *prop;
 
-	dn = of_node_get(pci_device_to_OF_node(dev));
+	dn = of_get_pci_dev_node(dev);
 	if (!dn) {
 		dev_dbg(&dev->dev, "axon_msi: no pci_dn found\n");
 		return -ENODEV;
-- 
1.5.1.3.g7a33b

Re: [PATCH 2/2] Use of_get_pci_dev_node() in axon_msi.c

[David Miller]

From: Michael Ellerman <michael@ellerman.id.au>
Date: Wed, 17 Oct 2007 17:12:27 +1000 (EST)

> Use of_get_pci_dev_node() in axon_msi.c. Switch to including <linux/of.h>
> so we get the prototype.
> 
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>

I find it ironic that you add of_get_pci_dev_node() as a function
which gets the node and grabs a reference to it, and then the very
first usage you make of it doesn't drop the reference at all.

That reference grabbing aspect of the new interface is obviously very
useful! :-)

Kidding aside (I realize that in this case probably the driver never
unregisters and therefore the reference never needs to be released)
it's really much nicer to add facilities when you have patches in hand
that actually use them.

Re: [PATCH 2/2] Use of_get_pci_dev_node() in axon_msi.c

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 17 Oct 2007 04:22:29 -0700 (PDT)

> From: Michael Ellerman <michael@ellerman.id.au>
> Date: Wed, 17 Oct 2007 17:12:27 +1000 (EST)
> 
> > Use of_get_pci_dev_node() in axon_msi.c. Switch to including <linux/of.h>
> > so we get the prototype.
> > 
> > Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> 
> I find it ironic that you add of_get_pci_dev_node() as a function
> which gets the node and grabs a reference to it, and then the very
> first usage you make of it doesn't drop the reference at all.

Ignore this poor attempt at humor, I misread your patch :)

Re: [PATCH 2/2] Use of_get_pci_dev_node() in axon_msi.c

[Benjamin Herrenschmidt]

> I find it ironic that you add of_get_pci_dev_node() as a function
> which gets the node and grabs a reference to it, and then the very
> first usage you make of it doesn't drop the reference at all.
> 
> That reference grabbing aspect of the new interface is obviously very
> useful! :-)
> 
> Kidding aside (I realize that in this case probably the driver never
> unregisters and therefore the reference never needs to be released)
> it's really much nicer to add facilities when you have patches in hand
> that actually use them.

I think in this case, it's mostly a matter of consistency... pretty much
everything that returns a device_node grabs a reference... except
pci_device_to_OF_node :-)

I think Michael is trying to address that, and axon-msi happens to be
something he wrote so a good candidate for an initial conversion :-)

Cheers,
Ben.

Re: [PATCH 2/2] Use of_get_pci_dev_node() in axon_msi.c

[Linas Vepstas]

On Wed, Oct 17, 2007 at 05:12:27PM +1000, Michael Ellerman wrote:

> +struct device_node *of_get_pci_dev_node(struct pci_dev *pdev)
> +{
> +       return of_node_get(pci_device_to_OF_node(pdev));
> +}

[...]

> -	dn = of_node_get(pci_device_to_OF_node(dev));
> +	dn = of_get_pci_dev_node(dev);

Is this really useful or wise?

As a matter of personal taste, I find stuff like this clutters
and confuses my mind. I go to read new code, and I run across some
routine I haven't heard of before ... e.g. of_get_pci_dev_node(),
so now I have to look it up to see what it does.  A few minutes later, 
I realize that its just a pair of old freinds (of_node_get and 
pci_device_to_OF_node) and so now I have to make mental room for it.  

Tommorrow, or 3 days later, I'm again looking at of_get_pci_dev_node()
and I'm thinking "gee what did that thing do again??"

I don't much like this style, and I've been known to submit
patches that remove stuff like this ... 

--linas

Re: [PATCH 2/2] Use of_get_pci_dev_node() in axon_msi.c

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1025 bytes --]

On Wed, 2007-10-17 at 04:23 -0700, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Wed, 17 Oct 2007 04:22:29 -0700 (PDT)
> 
> > From: Michael Ellerman <michael@ellerman.id.au>
> > Date: Wed, 17 Oct 2007 17:12:27 +1000 (EST)
> > 
> > > Use of_get_pci_dev_node() in axon_msi.c. Switch to including <linux/of.h>
> > > so we get the prototype.
> > > 
> > > Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> > 
> > I find it ironic that you add of_get_pci_dev_node() as a function
> > which gets the node and grabs a reference to it, and then the very
> > first usage you make of it doesn't drop the reference at all.
> 
> Ignore this poor attempt at humor, I misread your patch :)

Well it _would_ have been funny :)

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 2/2] Use of_get_pci_dev_node() in axon_msi.c

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1535 bytes --]

On Wed, 2007-10-17 at 18:04 -0500, Linas Vepstas wrote:
> On Wed, Oct 17, 2007 at 05:12:27PM +1000, Michael Ellerman wrote:
> 
> > +struct device_node *of_get_pci_dev_node(struct pci_dev *pdev)
> > +{
> > +       return of_node_get(pci_device_to_OF_node(pdev));
> > +}
> 
> [...]
> 
> > -	dn = of_node_get(pci_device_to_OF_node(dev));
> > +	dn = of_get_pci_dev_node(dev);
> 
> Is this really useful or wise?

Yes, and yes.

> As a matter of personal taste, I find stuff like this clutters
> and confuses my mind. I go to read new code, and I run across some
> routine I haven't heard of before ... e.g. of_get_pci_dev_node(),
> so now I have to look it up to see what it does.  A few minutes later, 
> I realize that its just a pair of old freinds (of_node_get and 
> pci_device_to_OF_node) and so now I have to make mental room for it.  
> 
> Tommorrow, or 3 days later, I'm again looking at of_get_pci_dev_node()
> and I'm thinking "gee what did that thing do again??"

It does what pci_device_to_OF_node() does, but in the right way. 

The plan is to remove pci_device_to_OF_node() once all the callers have
been converted to properly handle the refcounting. When that happens you
can use the mental room it consumed for something else :)

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 2/2] Use of_get_pci_dev_node() in axon_msi.c

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1497 bytes --]

On Wed, 2007-10-17 at 21:36 +1000, Benjamin Herrenschmidt wrote:
> > I find it ironic that you add of_get_pci_dev_node() as a function
> > which gets the node and grabs a reference to it, and then the very
> > first usage you make of it doesn't drop the reference at all.
> > 
> > That reference grabbing aspect of the new interface is obviously very
> > useful! :-)
> > 
> > Kidding aside (I realize that in this case probably the driver never
> > unregisters and therefore the reference never needs to be released)
> > it's really much nicer to add facilities when you have patches in hand
> > that actually use them.
> 
> I think in this case, it's mostly a matter of consistency... pretty much
> everything that returns a device_node grabs a reference... except
> pci_device_to_OF_node :-)

Yeah, it's a matter of the API being error-prone in that most routines
take a reference for you, but this one doesn't.

> I think Michael is trying to address that, and axon-msi happens to be
> something he wrote so a good candidate for an initial conversion :-)

Yep, I wanted at least one user in tree with the patch. I plan to
convert other pci_device_to_OF_node() users to use the refcounted
version over time.

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 2/2] Use of_get_pci_dev_node() in axon_msi.c

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 668 bytes --]

On Wed, 17 Oct 2007 18:04:49 -0500 linas@austin.ibm.com (Linas Vepstas) wrote:
>
> Is this really useful or wise?

Yes, it is.  We are *replacing* an interface that does no ref counting
with one that does.

> As a matter of personal taste, I find stuff like this clutters

What has taste got to do with it?  And as for cluttering, you can page
out the previous interface and page in the new one.  :-)

> I don't much like this style, and I've been known to submit
> patches that remove stuff like this ... 

What "style" are you referring to?

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/2] Use of_get_pci_dev_node() in axon_msi.c

[Linas Vepstas]

On Thu, Oct 18, 2007 at 11:27:23AM +1000, Michael Ellerman wrote:
> 
> It does what pci_device_to_OF_node() does, but in the right way. 
> 
> The plan is to remove pci_device_to_OF_node() once all the callers have
> been converted to properly handle the refcounting. 

Oh. Yes. well, of course, then. Excellent reason. I didn't get 
that from the patch commit comments. So, FWIW:

Ack'ed-by: Linas Vepstas <linas@austin.ibm.com>

--linas

Re: [PATCH 2/2] Use of_get_pci_dev_node() in axon_msi.c

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1509 bytes --]


On Thu, 2007-10-18 at 14:09 -0500, Linas Vepstas wrote:
> On Thu, Oct 18, 2007 at 11:27:23AM +1000, Michael Ellerman wrote:
> > 
> > It does what pci_device_to_OF_node() does, but in the right way. 
> > 
> > The plan is to remove pci_device_to_OF_node() once all the callers have
> > been converted to properly handle the refcounting. 
> 
> Oh. Yes. well, of course, then. Excellent reason. I didn't get 
> that from the patch commit comments. So, FWIW:
> 
> Ack'ed-by: Linas Vepstas <linas@austin.ibm.com>

Thanks for the ACK. But on further consideration I'm going to NACK my
own patch :)

The reasoning being that a lot of the code that uses
pci_device_to_OF_node() only uses the device_node while it also holds a
reference to the pci_dev - so there's no possibility of the device_node
going away.

So Ben suggested what we really want is two routines,
of_get_pci_dev_node() and of_peek_pci_dev_node() - the former returning
a refcounted copy and the latter allowing you to "peek" at the
device_node as long as you own the pci_dev.

I'm not sure it's worth the churn really, so we should probably just
document that pci_device_to_OF_node() is contrary, and any users that
need a reference can take one explicitly.

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 2/2] Use of_get_pci_dev_node() in axon_msi.c

[Benjamin Herrenschmidt]

> So Ben suggested what we really want is two routines,
> of_get_pci_dev_node() and of_peek_pci_dev_node() - the former returning
> a refcounted copy and the latter allowing you to "peek" at the
> device_node as long as you own the pci_dev.
> 
> I'm not sure it's worth the churn really, so we should probably just
> document that pci_device_to_OF_node() is contrary, and any users that
> need a reference can take one explicitly.

Yeah, I pretty much made the same decision a couple of years ago which
is why it's still the way it is now :-)

Cheers,
Ben.