| Message ID | 20100726220405.1550.63442.stgit@angua (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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) \ > >
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
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.
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.
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.
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.
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.
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
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
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) \
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(-)