[v3] of: Create asm-generic/of.h and provide default of_node_to_nid()

Submitted by Grant Likely on July 26, 2010, 10:04 p.m.

Details

Message ID 20100726220405.1550.63442.stgit@angua
State Not Applicable
Headers show

Commit Message

Grant Likely July 26, 2010, 10:04 p.m.
of_node_to_nid() is only relevant in a few architectures.  Don't force
everyone to implement it anyway.  This patch also adds asm-generic/of.h
which will be used to contain other overrideable symbols.

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

Changes in v3: don't use asm-generic, just keep macros in of.h
Changes in v2: address comments from sfr, add asm-generic/of.h

 arch/microblaze/include/asm/topology.h |   10 ----------
 arch/powerpc/include/asm/prom.h        |    5 +++++
 arch/powerpc/include/asm/topology.h    |    7 -------
 arch/sparc/include/asm/prom.h          |    1 +
 include/linux/of.h                     |    5 +++++
 5 files changed, 11 insertions(+), 17 deletions(-)

Comments

Grant Likely July 26, 2010, 10:05 p.m.
On Mon, Jul 26, 2010 at 4:04 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> of_node_to_nid() is only relevant in a few architectures.  Don't force
> everyone to implement it anyway.  This patch also adds asm-generic/of.h
> which will be used to contain other overrideable symbols.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

Oops.  The patch subject I will change before mergine.

g.

> ---
>
> Changes in v3: don't use asm-generic, just keep macros in of.h
> Changes in v2: address comments from sfr, add asm-generic/of.h
>
>  arch/microblaze/include/asm/topology.h |   10 ----------
>  arch/powerpc/include/asm/prom.h        |    5 +++++
>  arch/powerpc/include/asm/topology.h    |    7 -------
>  arch/sparc/include/asm/prom.h          |    1 +
>  include/linux/of.h                     |    5 +++++
>  5 files changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/arch/microblaze/include/asm/topology.h b/arch/microblaze/include/asm/topology.h
> index 96bcea5..5428f33 100644
> --- a/arch/microblaze/include/asm/topology.h
> +++ b/arch/microblaze/include/asm/topology.h
> @@ -1,11 +1 @@
>  #include <asm-generic/topology.h>
> -
> -#ifndef _ASM_MICROBLAZE_TOPOLOGY_H
> -#define _ASM_MICROBLAZE_TOPOLOGY_H
> -
> -struct device_node;
> -static inline int of_node_to_nid(struct device_node *device)
> -{
> -       return 0;
> -}
> -#endif /* _ASM_MICROBLAZE_TOPOLOGY_H */
> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
> index da7dd63..dca25a5 100644
> --- a/arch/powerpc/include/asm/prom.h
> +++ b/arch/powerpc/include/asm/prom.h
> @@ -103,6 +103,11 @@ struct device_node *of_find_next_cache_node(struct device_node *np);
>  /* Get the MAC address */
>  extern const void *of_get_mac_address(struct device_node *np);
>
> +#ifdef CONFIG_NUMA
> +extern int of_node_to_nid(struct device_node *device);
> +#define of_node_to_nid of_node_to_nid
> +#endif
> +
>  /**
>  * of_irq_map_pci - Resolve the interrupt for a PCI device
>  * @pdev:      the device whose interrupt is to be resolved
> diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
> index 32adf72..09dd38c 100644
> --- a/arch/powerpc/include/asm/topology.h
> +++ b/arch/powerpc/include/asm/topology.h
> @@ -41,8 +41,6 @@ static inline int cpu_to_node(int cpu)
>                               cpu_all_mask :                           \
>                               node_to_cpumask_map[node])
>
> -int of_node_to_nid(struct device_node *device);
> -
>  struct pci_bus;
>  #ifdef CONFIG_PCI
>  extern int pcibus_to_node(struct pci_bus *bus);
> @@ -94,11 +92,6 @@ extern void sysfs_remove_device_from_node(struct sys_device *dev, int nid);
>
>  #else
>
> -static inline int of_node_to_nid(struct device_node *device)
> -{
> -       return 0;
> -}
> -
>  static inline void dump_numa_cpu_topology(void) {}
>
>  static inline int sysfs_add_device_to_node(struct sys_device *dev, int nid)
> diff --git a/arch/sparc/include/asm/prom.h b/arch/sparc/include/asm/prom.h
> index c82a7da..73befa5 100644
> --- a/arch/sparc/include/asm/prom.h
> +++ b/arch/sparc/include/asm/prom.h
> @@ -43,6 +43,7 @@ extern int of_getintprop_default(struct device_node *np,
>  extern int of_find_in_proplist(const char *list, const char *match, int len);
>  #ifdef CONFIG_NUMA
>  extern int of_node_to_nid(struct device_node *dp);
> +#define of_node_to_nid of_node_to_nid
>  #else
>  #define of_node_to_nid(dp)     (-1)
>  #endif
> diff --git a/include/linux/of.h b/include/linux/of.h
> index b0756f3..cc936ca 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -146,6 +146,11 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size)
>
>  #define OF_BAD_ADDR    ((u64)-1)
>
> +#ifndef of_node_to_nid
> +static inline int of_node_to_nid(struct device_node *np) { return 0; }
> +#define of_node_to_nid of_node_to_nid
> +#endif
> +
>  extern struct device_node *of_find_node_by_name(struct device_node *from,
>        const char *name);
>  #define for_each_node_by_name(dn, name) \
>
>
Sam Ravnborg July 26, 2010, 10:26 p.m.
On Mon, Jul 26, 2010 at 04:04:55PM -0600, Grant Likely wrote:
> of_node_to_nid() is only relevant in a few architectures.  Don't force
> everyone to implement it anyway.  This patch also adds asm-generic/of.h
> which will be used to contain other overrideable symbols.
> 
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
> 
> Changes in v3: don't use asm-generic, just keep macros in of.h
> Changes in v2: address comments from sfr, add asm-generic/of.h

The use of asm-generic makes perfect sense.
This is how we usually deal with arch specific stuff.

With v3 of your patch we have a different result depending
on if we do:
#include <linux/of.h>

or we do:
#include <asm/prom.h>

This is undesireable.

I suggest to go back to v2 of your patch where you use asm-generic/of.h.

linux/of.h shall include asm/of.h
Then all archs shall have a of.h that may
include the asm-generic variant.


One patch to introduce of.h all over.
And a second patch to do the of_node_to_nid stuff would be appropriate.


> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
> index da7dd63..dca25a5 100644
> --- a/arch/powerpc/include/asm/prom.h
> +++ b/arch/powerpc/include/asm/prom.h
> @@ -103,6 +103,11 @@ struct device_node *of_find_next_cache_node(struct device_node *np);
>  /* Get the MAC address */
>  extern const void *of_get_mac_address(struct device_node *np);
>  

This shall go in asm/of.h
> +#ifdef CONFIG_NUMA
> +extern int of_node_to_nid(struct device_node *device);
> +#define of_node_to_nid of_node_to_nid

This define is used to tell asm-generic/of.h that the arch has
a local definition - OK.

> +#endif
> +
>  /**
>   * of_irq_map_pci - Resolve the interrupt for a PCI device
>   * @pdev:	the device whose interrupt is to be resolved

> diff --git a/include/linux/of.h b/include/linux/of.h
> index b0756f3..cc936ca 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -146,6 +146,11 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size)
>  
>  #define OF_BAD_ADDR	((u64)-1)
>  
> +#ifndef of_node_to_nid
> +static inline int of_node_to_nid(struct device_node *np) { return 0; }
> +#define of_node_to_nid of_node_to_nid

But I fail to see the purpose of this define.

	Sam
Grant Likely July 26, 2010, 10:42 p.m.
On Mon, Jul 26, 2010 at 4:26 PM, Sam Ravnborg <sam@ravnborg.org> wrote:
> On Mon, Jul 26, 2010 at 04:04:55PM -0600, Grant Likely wrote:
>> of_node_to_nid() is only relevant in a few architectures.  Don't force
>> everyone to implement it anyway.  This patch also adds asm-generic/of.h
>> which will be used to contain other overrideable symbols.
>>
>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>> ---
>>
>> Changes in v3: don't use asm-generic, just keep macros in of.h
>> Changes in v2: address comments from sfr, add asm-generic/of.h
>
> The use of asm-generic makes perfect sense.
> This is how we usually deal with arch specific stuff.
>
> With v3 of your patch we have a different result depending
> on if we do:
> #include <linux/of.h>
>
> or we do:
> #include <asm/prom.h>
>
> This is undesireable.

The patch does maintain consistency.  Including only asm/prom.h may
mean that of_node_to_nid is undefined, but it will never result in a
different definition.  linux/of.h includes asm/prom.h before doing the
#ifdef test.

> I suggest to go back to v2 of your patch where you use asm-generic/of.h.

Stephen suggested dropping asm-generic/of.h.  I'm happy to do it either way.

> linux/of.h shall include asm/of.h
> Then all archs shall have a of.h that may
> include the asm-generic variant.
>
>
> One patch to introduce of.h all over.
> And a second patch to do the of_node_to_nid stuff would be appropriate.
>
>
>> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
>> index da7dd63..dca25a5 100644
>> --- a/arch/powerpc/include/asm/prom.h
>> +++ b/arch/powerpc/include/asm/prom.h
>> @@ -103,6 +103,11 @@ struct device_node *of_find_next_cache_node(struct device_node *np);
>>  /* Get the MAC address */
>>  extern const void *of_get_mac_address(struct device_node *np);
>>
>
> This shall go in asm/of.h
>> +#ifdef CONFIG_NUMA
>> +extern int of_node_to_nid(struct device_node *device);
>> +#define of_node_to_nid of_node_to_nid
>
> This define is used to tell asm-generic/of.h that the arch has
> a local definition - OK.
>
>> +#endif
>> +
>>  /**
>>   * of_irq_map_pci - Resolve the interrupt for a PCI device
>>   * @pdev:    the device whose interrupt is to be resolved
>
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index b0756f3..cc936ca 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -146,6 +146,11 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size)
>>
>>  #define OF_BAD_ADDR  ((u64)-1)
>>
>> +#ifndef of_node_to_nid
>> +static inline int of_node_to_nid(struct device_node *np) { return 0; }
>> +#define of_node_to_nid of_node_to_nid
>
> But I fail to see the purpose of this define.

It protects against some later include file doing a #define
of_node_to_nid and thus resulting in an inconsistent definition.  If
some code tries to do this then the preprocessor will complain.  This
is the pattern that Stephen suggested.

g.
David Miller July 26, 2010, 10:48 p.m.
From: Grant Likely <grant.likely@secretlab.ca>
Date: Mon, 26 Jul 2010 16:42:20 -0600

> It protects against some later include file doing a #define
> of_node_to_nid and thus resulting in an inconsistent definition.  If
> some code tries to do this then the preprocessor will complain.  This
> is the pattern that Stephen suggested.

Also, it would be nice to unravel the "0" vs. "-1" default
inconsistency.
Grant Likely July 26, 2010, 10:51 p.m.
On Mon, Jul 26, 2010 at 4:48 PM, David Miller <davem@davemloft.net> wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
> Date: Mon, 26 Jul 2010 16:42:20 -0600
>
>> It protects against some later include file doing a #define
>> of_node_to_nid and thus resulting in an inconsistent definition.  If
>> some code tries to do this then the preprocessor will complain.  This
>> is the pattern that Stephen suggested.
>
> Also, it would be nice to unravel the "0" vs. "-1" default
> inconsistency.

Indeed.  I looked at it briefly, but it wasn't immediately clear what
the impact would be to switch powerpc over to -1, and it looked to me
like sparc depends on -1 to signify no node association.

g.
David Miller July 27, 2010, 1:38 a.m.
From: Grant Likely <grant.likely@secretlab.ca>
Date: Mon, 26 Jul 2010 16:51:01 -0600

> Indeed.  I looked at it briefly, but it wasn't immediately clear what
> the impact would be to switch powerpc over to -1, and it looked to me
> like sparc depends on -1 to signify no node association.

Kernel wide, NUMA interfaces tend to take "-1" to mean "any node."

I had looked over the powerpc cases, and besides 1 or 2 strange
locations the powerpc call sites were ready to handle -1.
Grant Likely July 27, 2010, 1:57 a.m.
On Mon, Jul 26, 2010 at 7:38 PM, David Miller <davem@davemloft.net> wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
> Date: Mon, 26 Jul 2010 16:51:01 -0600
>
>> Indeed.  I looked at it briefly, but it wasn't immediately clear what
>> the impact would be to switch powerpc over to -1, and it looked to me
>> like sparc depends on -1 to signify no node association.
>
> Kernel wide, NUMA interfaces tend to take "-1" to mean "any node."
>
> I had looked over the powerpc cases, and besides 1 or 2 strange
> locations the powerpc call sites were ready to handle -1.

Okay, well let me rework the patch to make -1 the default and let
powerpc override it.  A follow-on patch can fix up the powerpc usage
of 0.

g.
Arnd Bergmann July 27, 2010, 1:34 p.m.
On Tuesday 27 July 2010, Grant Likely wrote:
> > I suggest to go back to v2 of your patch where you use asm-generic/of.h.
> 
> Stephen suggested dropping asm-generic/of.h.  I'm happy to do it either way.

I don't mind adding stuff to asm-generic, but I think in this case it would
be easier to keep this in linux/of.h because there is nothing wrong with
all architectures including it.

Most files in asm-generic are there only for historical reasons, where some
architectures use them but others don't. IMHO we should use the include/linux
headers preferred for new stuff though.

	Arnd
Sam Ravnborg July 27, 2010, 4:52 p.m.
On Tue, Jul 27, 2010 at 03:34:01PM +0200, Arnd Bergmann wrote:
> On Tuesday 27 July 2010, Grant Likely wrote:
> > > I suggest to go back to v2 of your patch where you use asm-generic/of.h.
> > 
> > Stephen suggested dropping asm-generic/of.h.  I'm happy to do it either way.
> 
> I don't mind adding stuff to asm-generic, but I think in this case it would
> be easier to keep this in linux/of.h because there is nothing wrong with
> all architectures including it.
> 
> Most files in asm-generic are there only for historical reasons, where some
> architectures use them but others don't. IMHO we should use the include/linux
> headers preferred for new stuff though.

Hi Arnd.

Thanks for this explanation. Obviously my previous post
about asm-generic was wrong (as Grant already indicated).

	Sam

Patch hide | download patch | download mbox

diff --git a/arch/microblaze/include/asm/topology.h b/arch/microblaze/include/asm/topology.h
index 96bcea5..5428f33 100644
--- a/arch/microblaze/include/asm/topology.h
+++ b/arch/microblaze/include/asm/topology.h
@@ -1,11 +1 @@ 
 #include <asm-generic/topology.h>
-
-#ifndef _ASM_MICROBLAZE_TOPOLOGY_H
-#define _ASM_MICROBLAZE_TOPOLOGY_H
-
-struct device_node;
-static inline int of_node_to_nid(struct device_node *device)
-{
-	return 0;
-}
-#endif /* _ASM_MICROBLAZE_TOPOLOGY_H */
diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
index da7dd63..dca25a5 100644
--- a/arch/powerpc/include/asm/prom.h
+++ b/arch/powerpc/include/asm/prom.h
@@ -103,6 +103,11 @@  struct device_node *of_find_next_cache_node(struct device_node *np);
 /* Get the MAC address */
 extern const void *of_get_mac_address(struct device_node *np);
 
+#ifdef CONFIG_NUMA
+extern int of_node_to_nid(struct device_node *device);
+#define of_node_to_nid of_node_to_nid
+#endif
+
 /**
  * of_irq_map_pci - Resolve the interrupt for a PCI device
  * @pdev:	the device whose interrupt is to be resolved
diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
index 32adf72..09dd38c 100644
--- a/arch/powerpc/include/asm/topology.h
+++ b/arch/powerpc/include/asm/topology.h
@@ -41,8 +41,6 @@  static inline int cpu_to_node(int cpu)
 			       cpu_all_mask :				\
 			       node_to_cpumask_map[node])
 
-int of_node_to_nid(struct device_node *device);
-
 struct pci_bus;
 #ifdef CONFIG_PCI
 extern int pcibus_to_node(struct pci_bus *bus);
@@ -94,11 +92,6 @@  extern void sysfs_remove_device_from_node(struct sys_device *dev, int nid);
 
 #else
 
-static inline int of_node_to_nid(struct device_node *device)
-{
-	return 0;
-}
-
 static inline void dump_numa_cpu_topology(void) {}
 
 static inline int sysfs_add_device_to_node(struct sys_device *dev, int nid)
diff --git a/arch/sparc/include/asm/prom.h b/arch/sparc/include/asm/prom.h
index c82a7da..73befa5 100644
--- a/arch/sparc/include/asm/prom.h
+++ b/arch/sparc/include/asm/prom.h
@@ -43,6 +43,7 @@  extern int of_getintprop_default(struct device_node *np,
 extern int of_find_in_proplist(const char *list, const char *match, int len);
 #ifdef CONFIG_NUMA
 extern int of_node_to_nid(struct device_node *dp);
+#define of_node_to_nid of_node_to_nid
 #else
 #define of_node_to_nid(dp)	(-1)
 #endif
diff --git a/include/linux/of.h b/include/linux/of.h
index b0756f3..cc936ca 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -146,6 +146,11 @@  static inline unsigned long of_read_ulong(const __be32 *cell, int size)
 
 #define OF_BAD_ADDR	((u64)-1)
 
+#ifndef of_node_to_nid
+static inline int of_node_to_nid(struct device_node *np) { return 0; }
+#define of_node_to_nid of_node_to_nid
+#endif
+
 extern struct device_node *of_find_node_by_name(struct device_node *from,
 	const char *name);
 #define for_each_node_by_name(dn, name) \