Patchwork [11/11] of: unify phandle name in struct device_node

login
register
mail settings
Submitter Grant Likely
Date Nov. 24, 2009, 8:20 a.m.
Message ID <20091124081957.6216.74456.stgit@angua>
Download mbox | patch
Permalink /patch/39179/
State Superseded
Headers show

Comments

Grant Likely - Nov. 24, 2009, 8:20 a.m.
In struct device_node, the phandle is named 'linux_phandle' for PowerPC
and MicroBlaze, and 'node' for SPARC.  There is no good reason for the
David Miller - Nov. 24, 2009, 5:37 p.m.
From: Grant Likely <grant.likely@secretlab.ca>
Date: Tue, 24 Nov 2009 01:20:05 -0700

> In struct device_node, the phandle is named 'linux_phandle' for PowerPC
> and MicroBlaze, and 'node' for SPARC.  There is no good reason for the
> difference, it is just an artifact of the code diverging over a couple
> of years.  This patch renames .node to .linux_phandle for SPARC.

I know it's just a name, but there is no reason to put "linux" in the
member name.  It's the real device phandle value from OpenFirmware not
something invented by Linux's OF layer.

PowerPC uses this for something different, it records the information
here using the special "linux,phandle" and "ibm,phandle" properties.

See unflatten_dt_node() in arch/powerpc/kernel/prom.c before your
changes.
Grant Likely - Nov. 24, 2009, 8:33 p.m.
On Tue, Nov 24, 2009 at 10:37 AM, David Miller <davem@davemloft.net> wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
> Date: Tue, 24 Nov 2009 01:20:05 -0700
>
>> In struct device_node, the phandle is named 'linux_phandle' for PowerPC
>> and MicroBlaze, and 'node' for SPARC.  There is no good reason for the
>> difference, it is just an artifact of the code diverging over a couple
>> of years.  This patch renames .node to .linux_phandle for SPARC.
>
> I know it's just a name, but there is no reason to put "linux" in the
> member name.  It's the real device phandle value from OpenFirmware not
> something invented by Linux's OF layer.

I agree, and I also considered renaming it to simply 'phandle' or to
change the powerpc code back to using .node everywhere (more on .node
usage in powerpc below).  I didn't want to use .node because .node is
pretty generic, and is used in other places for different things.
Basically I wanted to avoid confusion.  In particular, .node points to
a device_node, not a phandle, in struct of_device.  Changing all
references to simply 'phandle' seems to be the best, but it does
require changes to more locations in the code.  In the end I decided
on some constructive lazyness by settling for the already-used
.linux_phandle so that I would need to touch as many files, but still
retain a unique name that is easy to find.  If you prefer, I can
change it all to simply '.phandle'.

> PowerPC uses this for something different, it records the information
> here using the special "linux,phandle" and "ibm,phandle" properties.

Agreed, the phandle isn't a real phandle when using the flat tree.
However, though the source of the phandle value is different, the use
case is identical, so it make sense to stuff it into the same member.
Merging them lets me unify more code.  For example,
of_find_node_by_phandle().

> See unflatten_dt_node() in arch/powerpc/kernel/prom.c before your
> changes.

Yup, I looked at this.  The unflatten code stuffs both linux,phandle
and ibm,phandle into .linux_phandle, and linux,phandle also gets
stuffed into .node.  But all the users except one use the
.linux_phandle, not .node, and that remaining user *I think* can
safely use .linux_phandle too.

Thanks for the review,
g.
Benjamin Herrenschmidt - Nov. 24, 2009, 9:06 p.m.
On Tue, 2009-11-24 at 09:37 -0800, David Miller wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
> Date: Tue, 24 Nov 2009 01:20:05 -0700
> 
> > In struct device_node, the phandle is named 'linux_phandle' for PowerPC
> > and MicroBlaze, and 'node' for SPARC.  There is no good reason for the
> > difference, it is just an artifact of the code diverging over a couple
> > of years.  This patch renames .node to .linux_phandle for SPARC.
> 
> I know it's just a name, but there is no reason to put "linux" in the
> member name.  It's the real device phandle value from OpenFirmware not
> something invented by Linux's OF layer.
> 
> PowerPC uses this for something different, it records the information
> here using the special "linux,phandle" and "ibm,phandle" properties.
> 
> See unflatten_dt_node() in arch/powerpc/kernel/prom.c before your
> changes.

Right, this comes from a subtle problem with our firmwares on pSeries.

We have a phandle from OF. But some devices, especially virtual devices,
also have an "ibm,phandle" which may or may not be the same afaik.

In addition, when the hypervisor hotplugs some devices, it sends us some
new device-tree bits to add. Those don't have a phandle in the formal
sense afaik, but -do- have an ibm,phandle property.

I think we can always just use ibm,phandle instead of the OF one when
the former is present, they should be non overlapping, but of course,
any chance in that area will require plenty of testing on some of those
machines.

Cheers,
Ben.
David Miller - Nov. 24, 2009, 9:10 p.m.
From: Grant Likely <grant.likely@secretlab.ca>
Date: Tue, 24 Nov 2009 13:33:22 -0700

> If you prefer, I can change it all to simply '.phandle'.

Please do.
Grant Likely - Nov. 24, 2009, 9:39 p.m.
On Tue, Nov 24, 2009 at 2:06 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Tue, 2009-11-24 at 09:37 -0800, David Miller wrote:
>> From: Grant Likely <grant.likely@secretlab.ca>
>> Date: Tue, 24 Nov 2009 01:20:05 -0700
>>
>> > In struct device_node, the phandle is named 'linux_phandle' for PowerPC
>> > and MicroBlaze, and 'node' for SPARC.  There is no good reason for the
>> > difference, it is just an artifact of the code diverging over a couple
>> > of years.  This patch renames .node to .linux_phandle for SPARC.
>>
>> I know it's just a name, but there is no reason to put "linux" in the
>> member name.  It's the real device phandle value from OpenFirmware not
>> something invented by Linux's OF layer.
>>
>> PowerPC uses this for something different, it records the information
>> here using the special "linux,phandle" and "ibm,phandle" properties.
>>
>> See unflatten_dt_node() in arch/powerpc/kernel/prom.c before your
>> changes.
>
> Right, this comes from a subtle problem with our firmwares on pSeries.
>
> We have a phandle from OF. But some devices, especially virtual devices,
> also have an "ibm,phandle" which may or may not be the same afaik.
>
> In addition, when the hypervisor hotplugs some devices, it sends us some
> new device-tree bits to add. Those don't have a phandle in the formal
> sense afaik, but -do- have an ibm,phandle property.
>
> I think we can always just use ibm,phandle instead of the OF one when
> the former is present, they should be non overlapping, but of course,
> any chance in that area will require plenty of testing on some of those
> machines.

If pseries is the troublesome platform, not powermac, then I'm pretty
confident this change is okay.  I cannot find any other users of .node
other than arch/powerpc/platforms/powermac/pfunc_core.c, but I'll
double check with an allmodconfig and an allyesconfig.  This patch
shouldn't change the behaviour of linux_phandle at all.

g.

Patch

difference, it is just an artifact of the code diverging over a couple
of years.  This patch renames .node to .linux_phandle for SPARC.

Note: the .node also existed in PowerPC/MicroBlaze, but the only user
seems to be arch/powerpc/platforms/powermac/pfunc_core.c.  It doesn't
look like the assignment between .linux_phandle and .node is
significantly different enough to warrant the separate code paths
unless ibm,phandle properties actually appear in Apple device trees.

I think it is safe to eliminate the old .node property and use
linux_phandle everywhere.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 arch/powerpc/platforms/powermac/pfunc_core.c |    2 +-
 arch/sparc/kernel/devices.c                  |    2 +-
 arch/sparc/kernel/of_device_32.c             |    2 +-
 arch/sparc/kernel/of_device_64.c             |    2 +-
 arch/sparc/kernel/prom_common.c              |    8 ++++----
 arch/sparc/kernel/smp_64.c                   |    2 +-
 drivers/of/fdt.c                             |    3 +--
 drivers/sbus/char/openprom.c                 |   10 +++++-----
 drivers/video/aty/atyfb_base.c               |    2 +-
 include/linux/of.h                           |    3 ---
 10 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/arch/powerpc/platforms/powermac/pfunc_core.c b/arch/powerpc/platforms/powermac/pfunc_core.c
index 96d5ce5..2004b19 100644
--- a/arch/powerpc/platforms/powermac/pfunc_core.c
+++ b/arch/powerpc/platforms/powermac/pfunc_core.c
@@ -842,7 +842,7 @@  struct pmf_function *__pmf_find_function(struct device_node *target,
 	list_for_each_entry(func, &dev->functions, link) {
 		if (name && strcmp(name, func->name))
 			continue;
-		if (func->phandle && target->node != func->phandle)
+		if (func->phandle && target->linux_phandle != func->phandle)
 			continue;
 		if ((func->flags & flags) == 0)
 			continue;
diff --git a/arch/sparc/kernel/devices.c b/arch/sparc/kernel/devices.c
index b171ae8..2196e71 100644
--- a/arch/sparc/kernel/devices.c
+++ b/arch/sparc/kernel/devices.c
@@ -59,7 +59,7 @@  static int __cpu_find_by(int (*compare)(int, int, void *), void *compare_arg,
 
 	cur_inst = 0;
 	for_each_node_by_type(dp, "cpu") {
-		int err = check_cpu_node(dp->node, &cur_inst,
+		int err = check_cpu_node(dp->linux_phandle, &cur_inst,
 					 compare, compare_arg,
 					 prom_node, mid);
 		if (!err) {
diff --git a/arch/sparc/kernel/of_device_32.c b/arch/sparc/kernel/of_device_32.c
index 4c26eb5..27f4c2e 100644
--- a/arch/sparc/kernel/of_device_32.c
+++ b/arch/sparc/kernel/of_device_32.c
@@ -433,7 +433,7 @@  build_resources:
 	if (!parent)
 		dev_set_name(&op->dev, "root");
 	else
-		dev_set_name(&op->dev, "%08x", dp->node);
+		dev_set_name(&op->dev, "%08x", dp->linux_phandle);
 
 	if (of_device_register(op)) {
 		printk("%s: Could not register of device.\n",
diff --git a/arch/sparc/kernel/of_device_64.c b/arch/sparc/kernel/of_device_64.c
index 881947e..9a6245d 100644
--- a/arch/sparc/kernel/of_device_64.c
+++ b/arch/sparc/kernel/of_device_64.c
@@ -666,7 +666,7 @@  static struct of_device * __init scan_one_device(struct device_node *dp,
 	if (!parent)
 		dev_set_name(&op->dev, "root");
 	else
-		dev_set_name(&op->dev, "%08x", dp->node);
+		dev_set_name(&op->dev, "%08x", dp->linux_phandle);
 
 	if (of_device_register(op)) {
 		printk("%s: Could not register of device.\n",
diff --git a/arch/sparc/kernel/prom_common.c b/arch/sparc/kernel/prom_common.c
index d80a65d..2a3e302 100644
--- a/arch/sparc/kernel/prom_common.c
+++ b/arch/sparc/kernel/prom_common.c
@@ -42,7 +42,7 @@  struct device_node *of_find_node_by_phandle(phandle handle)
 	struct device_node *np;
 
 	for (np = allnodes; np; np = np->allnext)
-		if (np->node == handle)
+		if (np->linux_phandle == handle)
 			break;
 
 	return np;
@@ -89,7 +89,7 @@  int of_set_property(struct device_node *dp, const char *name, void *val, int len
 			void *old_val = prop->value;
 			int ret;
 
-			ret = prom_setprop(dp->node, name, val, len);
+			ret = prom_setprop(dp->linux_phandle, name, val, len);
 
 			err = -EINVAL;
 			if (ret >= 0) {
@@ -236,7 +236,7 @@  static struct device_node * __init prom_create_node(phandle node,
 
 	dp->name = get_one_property(node, "name");
 	dp->type = get_one_property(node, "device_type");
-	dp->node = node;
+	dp->linux_phandle = node;
 
 	dp->properties = build_prop_list(node);
 
@@ -313,7 +313,7 @@  void __init prom_build_devicetree(void)
 
 	nextp = &allnodes->allnext;
 	allnodes->child = prom_build_tree(allnodes,
-					  prom_getchild(allnodes->node),
+					  prom_getchild(allnodes->linux_phandle),
 					  &nextp);
 	of_console_init();
 
diff --git a/arch/sparc/kernel/smp_64.c b/arch/sparc/kernel/smp_64.c
index aa36223..9222700 100644
--- a/arch/sparc/kernel/smp_64.c
+++ b/arch/sparc/kernel/smp_64.c
@@ -370,7 +370,7 @@  static int __cpuinit smp_boot_one_cpu(unsigned int cpu)
 	} else {
 		struct device_node *dp = of_find_node_by_cpuid(cpu);
 
-		prom_startcpu(dp->node, entry, cookie);
+		prom_startcpu(dp->linux_phandle, entry, cookie);
 	}
 
 	for (timeout = 0; timeout < 50000; timeout++) {
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 6164781..07ae161 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -315,9 +315,8 @@  unsigned long __init unflatten_dt_node(unsigned long mem,
 					__alignof__(struct property));
 		if (allnextpp) {
 			if (strcmp(pname, "linux,phandle") == 0) {
-				np->node = *((u32 *)*p);
 				if (np->linux_phandle == 0)
-					np->linux_phandle = np->node;
+					np->linux_phandle = *((u32 *)*p);
 			}
 			if (strcmp(pname, "ibm,phandle") == 0)
 				np->linux_phandle = *((u32 *)*p);
diff --git a/drivers/sbus/char/openprom.c b/drivers/sbus/char/openprom.c
index 75ac19b..a776e65 100644
--- a/drivers/sbus/char/openprom.c
+++ b/drivers/sbus/char/openprom.c
@@ -233,7 +233,7 @@  static int opromnext(void __user *argp, unsigned int cmd, struct device_node *dp
 
 	ph = 0;
 	if (dp)
-		ph = dp->node;
+		ph = dp->linux_phandle;
 
 	data->current_node = dp;
 	*((int *) op->oprom_array) = ph;
@@ -256,7 +256,7 @@  static int oprompci2node(void __user *argp, struct device_node *dp, struct openp
 
 		dp = pci_device_to_OF_node(pdev);
 		data->current_node = dp;
-		*((int *)op->oprom_array) = dp->node;
+		*((int *)op->oprom_array) = dp->linux_phandle;
 		op->oprom_size = sizeof(int);
 		err = copyout(argp, op, bufsize + sizeof(int));
 
@@ -273,7 +273,7 @@  static int oprompath2node(void __user *argp, struct device_node *dp, struct open
 
 	dp = of_find_node_by_path(op->oprom_array);
 	if (dp)
-		ph = dp->node;
+		ph = dp->linux_phandle;
 	data->current_node = dp;
 	*((int *)op->oprom_array) = ph;
 	op->oprom_size = sizeof(int);
@@ -540,7 +540,7 @@  static int opiocgetnext(unsigned int cmd, void __user *argp)
 		}
 	}
 	if (dp)
-		nd = dp->node;
+		nd = dp->linux_phandle;
 	if (copy_to_user(argp, &nd, sizeof(phandle)))
 		return -EFAULT;
 
@@ -570,7 +570,7 @@  static int openprom_bsd_ioctl(struct inode * inode, struct file * file,
 	case OPIOCGETOPTNODE:
 		BUILD_BUG_ON(sizeof(phandle) != sizeof(int));
 
-		if (copy_to_user(argp, &options_node->node, sizeof(phandle)))
+		if (copy_to_user(argp, &options_node->linux_phandle, sizeof(phandle)))
 			return -EFAULT;
 
 		return 0;
diff --git a/drivers/video/aty/atyfb_base.c b/drivers/video/aty/atyfb_base.c
index 913b4a4..8357927 100644
--- a/drivers/video/aty/atyfb_base.c
+++ b/drivers/video/aty/atyfb_base.c
@@ -3104,7 +3104,7 @@  static int __devinit atyfb_setup_sparc(struct pci_dev *pdev,
 	}
 
 	dp = pci_device_to_OF_node(pdev);
-	if (node == dp->node) {
+	if (node == dp->linux_phandle) {
 		struct fb_var_screeninfo *var = &default_var;
 		unsigned int N, P, Q, M, T, R;
 		u32 v_total, h_total;
diff --git a/include/linux/of.h b/include/linux/of.h
index 0a51742..0ddd985 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -39,10 +39,7 @@  struct of_irq_controller;
 struct device_node {
 	const char *name;
 	const char *type;
-	phandle	node;
-#if !defined(CONFIG_SPARC)
 	phandle linux_phandle;
-#endif
 	char	*full_name;
 
 	struct	property *properties;