Patchwork [1/2,v2] of: move phandle/ihandle into types.h and export to userspace

login
register
mail settings
Submitter Andres Salomon
Date Oct. 8, 2010, 7:43 a.m.
Message ID <20101008004354.13b5b254@debxo>
Download mbox | patch
Permalink /patch/67129/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Andres Salomon - Oct. 8, 2010, 7:43 a.m.
We need phandle for some exported sparc headers; of.h isn't an
exported header, and it would be silly to export it when all we
really need is one or two types from it.  Also, later patches
use phandle in structs that are exported to userspace, so export
a __kernel_phandle type.

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 include/linux/of.h    |    3 ---
 include/linux/types.h |    7 +++++++
 2 files changed, 7 insertions(+), 3 deletions(-)
Grant Likely - Oct. 8, 2010, 5:12 p.m.
On Fri, Oct 08, 2010 at 12:43:54AM -0700, Andres Salomon wrote:
> 
> We need phandle for some exported sparc headers; of.h isn't an
> exported header, and it would be silly to export it when all we
> really need is one or two types from it.  Also, later patches
> use phandle in structs that are exported to userspace, so export
> a __kernel_phandle type.
> 
> Signed-off-by: Andres Salomon <dilinger@queued.net>

Looks okay to me.  I'm build testing it now on powerpc and
microblaze.  If davem agrees, then I'll pick it up into my
devicetree-next branch.

Question though; since sparc userspace is the only user of this,
should the types.h header avoid exposing it on non-sparc?

g.

> ---
>  include/linux/of.h    |    3 ---
>  include/linux/types.h |    7 +++++++
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index cad7cf0..db184dc 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -25,9 +25,6 @@
>  
>  #ifdef CONFIG_OF
>  
> -typedef u32 phandle;
> -typedef u32 ihandle;
> -
>  struct property {
>  	char	*name;
>  	int	length;
> diff --git a/include/linux/types.h b/include/linux/types.h
> index 01a082f..67a034a 100644
> --- a/include/linux/types.h
> +++ b/include/linux/types.h
> @@ -178,10 +178,17 @@ typedef __u64 __bitwise __be64;
>  typedef __u16 __bitwise __sum16;
>  typedef __u32 __bitwise __wsum;
>  
> +/* Basic openboot/openfirmware types */
> +typedef __u32 __kernel_phandle;
> +typedef __u32 __kernel_ihandle;
> +
>  #ifdef __KERNEL__
>  typedef unsigned __bitwise__ gfp_t;
>  typedef unsigned __bitwise__ fmode_t;
>  
> +typedef __kernel_phandle phandle;
> +typedef __kernel_ihandle ihandle;
> +
>  #ifdef CONFIG_PHYS_ADDR_T_64BIT
>  typedef u64 phys_addr_t;
>  #else
> -- 
> 1.5.6.5
> 
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 8, 2010, 5:17 p.m.
From: Grant Likely <grant.likely@secretlab.ca>
Date: Fri, 8 Oct 2010 11:12:10 -0600

> On Fri, Oct 08, 2010 at 12:43:54AM -0700, Andres Salomon wrote:
>> 
>> We need phandle for some exported sparc headers; of.h isn't an
>> exported header, and it would be silly to export it when all we
>> really need is one or two types from it.  Also, later patches
>> use phandle in structs that are exported to userspace, so export
>> a __kernel_phandle type.
>> 
>> Signed-off-by: Andres Salomon <dilinger@queued.net>
> 
> Looks okay to me.  I'm build testing it now on powerpc and
> microblaze.  If davem agrees, then I'll pick it up into my
> devicetree-next branch.
> 
> Question though; since sparc userspace is the only user of this,
> should the types.h header avoid exposing it on non-sparc?

I can't think of anything in Sparc userspace that needs the
phandle_t type.

In fact I was a bit confused when you asked Andres to expose
it to userspace.

What I remember from a previous thread is that some other platforms
use the type in their DT device-tree building tools or whatever, but
they define phandle_t for themselves I remember specifically
mentioning sparc has no use for this currently.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grant Likely - Oct. 8, 2010, 5:36 p.m.
On Fri, Oct 08, 2010 at 10:17:59AM -0700, David Miller wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
> Date: Fri, 8 Oct 2010 11:12:10 -0600
> 
> > On Fri, Oct 08, 2010 at 12:43:54AM -0700, Andres Salomon wrote:
> >> 
> >> We need phandle for some exported sparc headers; of.h isn't an
> >> exported header, and it would be silly to export it when all we
> >> really need is one or two types from it.  Also, later patches
> >> use phandle in structs that are exported to userspace, so export
> >> a __kernel_phandle type.
> >> 
> >> Signed-off-by: Andres Salomon <dilinger@queued.net>
> > 
> > Looks okay to me.  I'm build testing it now on powerpc and
> > microblaze.  If davem agrees, then I'll pick it up into my
> > devicetree-next branch.
> > 
> > Question though; since sparc userspace is the only user of this,
> > should the types.h header avoid exposing it on non-sparc?
> 
> I can't think of anything in Sparc userspace that needs the
> phandle_t type.
> 
> In fact I was a bit confused when you asked Andres to expose
> it to userspace.
> 
> What I remember from a previous thread is that some other platforms
> use the type in their DT device-tree building tools or whatever, but
> they define phandle_t for themselves I remember specifically
> mentioning sparc has no use for this currently.

Weird.  Yeah, no other platforms expect to get a phandle type
definition from the kernel headers.  The only thing driving this
conversation is from arch/sparc/include/asm/Kbuild:

header-y += openprom.h

I don't have any background on the history of that file or why it
needs to be exported to userspace.  There seems to be a lot of stuff
in there; but I cannot see the *_funcs structures having any utility
outside of the kernel.  Perhaps they should be moved out of
asm/openprom.h?

Here are the two affected structures:

$ git grep linux_nodeops arch/sparc/include
arch/sparc/include/asm/openprom.h:      struct linux_nodeops *pv_nodeops;
arch/sparc/include/asm/openprom.h:struct linux_nodeops {
arch/sparc/include/asm/oplib_32.h:extern struct linux_nodeops *prom_nodeops;
$ git grep linux_dev_v2_funcs arch/sparc/include
arch/sparc/include/asm/openprom.h:struct linux_dev_v2_funcs {
arch/sparc/include/asm/openprom.h:      struct linux_dev_v2_funcs pv_v2devops;
(devicetree/next) ~/hacking/linux-2.6$ 

And linux_romvec references linux_nodeops:
$ git grep linux_romvec arch/sparc/include
arch/sparc/include/asm/openprom.h:struct linux_romvec {
arch/sparc/include/asm/oplib_32.h:extern struct linux_romvec *romvec;
arch/sparc/include/asm/oplib_32.h:extern void prom_init(struct linux_romvec *rom_ptr);

oplib_32.h is not exported.

g.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 8, 2010, 5:45 p.m.
From: Grant Likely <grant.likely@secretlab.ca>
Date: Fri, 8 Oct 2010 11:36:50 -0600

> Weird.  Yeah, no other platforms expect to get a phandle type
> definition from the kernel headers.  The only thing driving this
> conversation is from arch/sparc/include/asm/Kbuild:
> 
> header-y += openprom.h

It was probably at one point for the sake of asm/openpromio.h but
that header has no dependencies on openprom.h

The only hit I can find in google code search, for non-kernel code, is
the SILO bootloader.

But that tree includes it's own copy of include/asm/openprom.h so
the actual kernel copy isn't even used.

I'd say we can stop exporting that header and also therefore not
worry about making phandle_t visible to userspace.


--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grant Likely - Oct. 8, 2010, 6:27 p.m.
On Fri, Oct 08, 2010 at 10:45:57AM -0700, David Miller wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
> Date: Fri, 8 Oct 2010 11:36:50 -0600
> 
> > Weird.  Yeah, no other platforms expect to get a phandle type
> > definition from the kernel headers.  The only thing driving this
> > conversation is from arch/sparc/include/asm/Kbuild:
> > 
> > header-y += openprom.h
> 
> It was probably at one point for the sake of asm/openpromio.h but
> that header has no dependencies on openprom.h
> 
> The only hit I can find in google code search, for non-kernel code, is
> the SILO bootloader.
> 
> But that tree includes it's own copy of include/asm/openprom.h so
> the actual kernel copy isn't even used.
> 
> I'd say we can stop exporting that header and also therefore not
> worry about making phandle_t visible to userspace.

Yay!  That simplifies everything, and I believe it also means that the
phandle/ihandle definitions can remain where they currently are in
linux/of.h

Andres, can you post an updated series that includes removing
openprom.h from the header export list?

Thanks,
g.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andres Salomon - Oct. 8, 2010, 6:38 p.m.
On Fri, 8 Oct 2010 12:27:45 -0600
Grant Likely <grant.likely@secretlab.ca> wrote:

> On Fri, Oct 08, 2010 at 10:45:57AM -0700, David Miller wrote:
> > From: Grant Likely <grant.likely@secretlab.ca>
> > Date: Fri, 8 Oct 2010 11:36:50 -0600
> > 
> > > Weird.  Yeah, no other platforms expect to get a phandle type
> > > definition from the kernel headers.  The only thing driving this
> > > conversation is from arch/sparc/include/asm/Kbuild:
> > > 
> > > header-y += openprom.h
> > 
> > It was probably at one point for the sake of asm/openpromio.h but
> > that header has no dependencies on openprom.h
> > 
> > The only hit I can find in google code search, for non-kernel code,
> > is the SILO bootloader.
> > 
> > But that tree includes it's own copy of include/asm/openprom.h so
> > the actual kernel copy isn't even used.
> > 
> > I'd say we can stop exporting that header and also therefore not
> > worry about making phandle_t visible to userspace.
> 
> Yay!  That simplifies everything, and I believe it also means that the
> phandle/ihandle definitions can remain where they currently are in
> linux/of.h
> 
> Andres, can you post an updated series that includes removing
> openprom.h from the header export list?
> 

Sure, I'll just need to rework/retest the build.  I've already done
that with the openprom.h-removal patch (which was just sent).

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/of.h b/include/linux/of.h
index cad7cf0..db184dc 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -25,9 +25,6 @@ 
 
 #ifdef CONFIG_OF
 
-typedef u32 phandle;
-typedef u32 ihandle;
-
 struct property {
 	char	*name;
 	int	length;
diff --git a/include/linux/types.h b/include/linux/types.h
index 01a082f..67a034a 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -178,10 +178,17 @@  typedef __u64 __bitwise __be64;
 typedef __u16 __bitwise __sum16;
 typedef __u32 __bitwise __wsum;
 
+/* Basic openboot/openfirmware types */
+typedef __u32 __kernel_phandle;
+typedef __u32 __kernel_ihandle;
+
 #ifdef __KERNEL__
 typedef unsigned __bitwise__ gfp_t;
 typedef unsigned __bitwise__ fmode_t;
 
+typedef __kernel_phandle phandle;
+typedef __kernel_ihandle ihandle;
+
 #ifdef CONFIG_PHYS_ADDR_T_64BIT
 typedef u64 phys_addr_t;
 #else