[PATCH] enable RTAS /proc for PowerPC/CHRP platform

[PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Nicolas DET]

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

This patch enables RTAS /proc support for PowerPC 32bits / CHRP platform.

A new entry is created (/proc/ppc/rtas/) which contains the RTAS nodes. 
Theses nodes and the code are the exact same than with the ppc64 
architecture.

This patch has been succefully applied on the kernel 2.6.18.1 and tested 
on bPlan's OpenFirmware.

Signed-off-by: Nicolas DET <nd@bplan-gmbh.de>

[-- Attachment #2: rtas-proc_ppc32.patch --]
[-- Type: text/plain, Size: 2707 bytes --]

--- a/arch/powerpc/kernel/rtas-proc.c	2006-10-14 05:34:03.000000000 +0200
+++ b/arch/powerpc/kernel/rtas-proc.c	2006-10-16 10:46:16.000000000 +0200
@@ -253,43 +253,70 @@ static void get_location_code(struct seq
 static void check_location_string(struct seq_file *m, char *c);
 static void check_location(struct seq_file *m, char *c);
 
+#ifdef CONFIG_PPC64
+#define PROCRTAS_ROOT "ppc64"
+#else
+#define PROCRTAS_ROOT "ppc"
+static int __init proc_ppc32_create(void)
+{
+        struct proc_dir_entry *root;
+
+        root = proc_mkdir(PROCRTAS_ROOT , NULL);
+        if (!root)
+                return -1;
+
+        if (!proc_mkdir("rtas", root))
+                return -1;
+
+        if (!proc_symlink("rtas", NULL, PROCRTAS_ROOT "/rtas"))
+                return -1;
+
+        return 0;
+}
+#endif
+
 static int __init proc_rtas_init(void)
 {
 	struct proc_dir_entry *entry;
 
-	if (!machine_is(pseries))
+	if ( ! ( machine_is(pseries) || machine_is(chrp) ) )
 		return -ENODEV;
 
 	rtas_node = of_find_node_by_name(NULL, "rtas");
 	if (rtas_node == NULL)
 		return -ENODEV;
 
-	entry = create_proc_entry("ppc64/rtas/progress", S_IRUGO|S_IWUSR, NULL);
+#ifndef CONFIG_PPC64
+	if (proc_ppc32_create() != 0)
+		return -ENODEV;
+#endif
+
+	entry = create_proc_entry(PROCRTAS_ROOT "/rtas/progress", S_IRUGO|S_IWUSR, NULL);
 	if (entry)
 		entry->proc_fops = &ppc_rtas_progress_operations;
 
-	entry = create_proc_entry("ppc64/rtas/clock", S_IRUGO|S_IWUSR, NULL);
+	entry = create_proc_entry(PROCRTAS_ROOT "/rtas/clock", S_IRUGO|S_IWUSR, NULL);
 	if (entry)
 		entry->proc_fops = &ppc_rtas_clock_operations;
 
-	entry = create_proc_entry("ppc64/rtas/poweron", S_IWUSR|S_IRUGO, NULL);
+	entry = create_proc_entry(PROCRTAS_ROOT "/rtas/poweron", S_IWUSR|S_IRUGO, NULL);
 	if (entry)
 		entry->proc_fops = &ppc_rtas_poweron_operations;
 
-	entry = create_proc_entry("ppc64/rtas/sensors", S_IRUGO, NULL);
+	entry = create_proc_entry(PROCRTAS_ROOT "/rtas/sensors", S_IRUGO, NULL);
 	if (entry)
 		entry->proc_fops = &ppc_rtas_sensors_operations;
 
-	entry = create_proc_entry("ppc64/rtas/frequency", S_IWUSR|S_IRUGO,
+	entry = create_proc_entry(PROCRTAS_ROOT "/rtas/frequency", S_IWUSR|S_IRUGO,
 				  NULL);
 	if (entry)
 		entry->proc_fops = &ppc_rtas_tone_freq_operations;
 
-	entry = create_proc_entry("ppc64/rtas/volume", S_IWUSR|S_IRUGO, NULL);
+	entry = create_proc_entry(PROCRTAS_ROOT "/rtas/volume", S_IWUSR|S_IRUGO, NULL);
 	if (entry)
 		entry->proc_fops = &ppc_rtas_tone_volume_operations;
 
-	entry = create_proc_entry("ppc64/rtas/rmo_buffer", S_IRUSR, NULL);
+	entry = create_proc_entry(PROCRTAS_ROOT "/rtas/rmo_buffer", S_IRUSR, NULL);
 	if (entry)
 		entry->proc_fops = &ppc_rtas_rmo_buf_ops;
 

[-- Attachment #3: nd.vcf --]
[-- Type: text/x-vcard, Size: 249 bytes --]

begin:vcard
fn:Nicolas DET ( bplan GmbH )
n:DET;Nicolas
org:bplan GmbH
adr:;;;;;;Germany
email;internet:nd@bplan-gmbh.de
title:Software Entwicklung
tel;work:+49 6171 9187 - 31
x-mozilla-html:FALSE
url:http://www.bplan-gmbh.de
version:2.1
end:vcard

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Christoph Hellwig]

> --- a/arch/powerpc/kernel/rtas-proc.c	2006-10-14 05:34:03.000000000 +0200
> +++ b/arch/powerpc/kernel/rtas-proc.c	2006-10-16 10:46:16.000000000 +0200
> @@ -253,43 +253,70 @@ static void get_location_code(struct seq
>  static void check_location_string(struct seq_file *m, char *c);
>  static void check_location(struct seq_file *m, char *c);
>  
> +#ifdef CONFIG_PPC64
> +#define PROCRTAS_ROOT "ppc64"
> +#else
> +#define PROCRTAS_ROOT "ppc"

Please don't do any pathname changes.  Even if ppc64 isn't correct it's
what applications expect and what we should provide for a coherent user
interface.

> -	if (!machine_is(pseries))
> +	if ( ! ( machine_is(pseries) || machine_is(chrp) ) )
>  		return -ENODEV;

This should be the only change you need, and it should follow kernel
coding style, aka:

	if (!machine_is(pseries) && !machine_is(chrp))
		return -ENODEV;

>  	rtas_node = of_find_node_by_name(NULL, "rtas");
>  	if (rtas_node == NULL)
>  		return -ENODEV;

And given this check I wonder why we need the platform check at all.  It
should be safe to just remove it.

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Arnd Bergmann]

On Tuesday 17 October 2006 15:22, Christoph Hellwig wrote:
> > =A0=A0=A0=A0=A0=A0rtas_node =3D of_find_node_by_name(NULL, "rtas");
> > =A0=A0=A0=A0=A0=A0if (rtas_node =3D=3D NULL)
> > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0return -ENODEV;
>=20
> And given this check I wonder why we need the platform check at all. =A0It
> should be safe to just remove it.
>=20

One difference would be that it triggers on machines running SLOF (QS20,=20
some JS20/JS21) and maybe some older Macs, which is probably a good
thing.

I wonder if it should be a little stricter though:

> 	rtas_node =3D of_find_node_by_path("/rtas");
>	if (!rtas_node)
>		return -ENODEV;

In case there is a node called "rtas" somewhere else.

	Arnd <><

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Benjamin Herrenschmidt]

On Wed, 2006-10-18 at 00:22 +0200, Arnd Bergmann wrote:
> On Tuesday 17 October 2006 15:22, Christoph Hellwig wrote:
> > >       rtas_node = of_find_node_by_name(NULL, "rtas");
> > >       if (rtas_node == NULL)
> > >               return -ENODEV;
> > 
> > And given this check I wonder why we need the platform check at all.  It
> > should be safe to just remove it.
> > 
> 
> One difference would be that it triggers on machines running SLOF (QS20, 
> some JS20/JS21) and maybe some older Macs, which is probably a good
> thing.

Older macs may have bits of RTAS, but it doesn't work. I'd rather still
blacklist machine_is(powermac).

> I wonder if it should be a little stricter though:
> 
> > 	rtas_node = of_find_node_by_path("/rtas");
> >	if (!rtas_node)
> >		return -ENODEV;
> 
> In case there is a node called "rtas" somewhere else.

Yeah, just in case...

Ben.

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Nicolas DET]

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

Arnd Bergmann wrote:
 > On Tuesday 17 October 2006 15:22, Christoph Hellwig wrote:
 >>>       rtas_node = of_find_node_by_name(NULL, "rtas");
 >>>       if (rtas_node == NULL)
 >>>               return -ENODEV;
 >> And given this check I wonder why we need the platform check at all.  It
 >> should be safe to just remove it.
 >>

 > One difference would be that it triggers on machines running SLOF 
(QS20, some JS20/JS21) and maybe some older Macs, which is probably a good
 > thing.
 >
 > I wonder if it should be a little stricter though:
 >
 >>     rtas_node = of_find_node_by_path("/rtas");
 >>     if (!rtas_node)
 >>         return -ENODEV;
 >
 > In case there is a node called "rtas" somewhere else.
 >
 >     Arnd <><
 >

Maybe we could check for the RTAS revision. The CHRP manual define this 
properties 'rtas-version' in /rtas/. This should be 1 for current 
implementation.

You can find here a new patch. It does not create the /proc/rtas and the 
symlink /proc/ppc64/rtas in  arch/powerpc/kernel/proc_ppc64.c anymore 
but rather create the entry (/proc/rtas) in 
arch/powerpc/kernel/rtas-proc.c and only the ppc64/ratas link for 64bit 
machine.

This has been tested on our PowerPC 32bit machine. It would be nice if 
PowerPC64 developer could reveiw and test it.

I stay ready to test upcomming patch if required.

Regards


[-- Attachment #2: rtas_proc.patch --]
[-- Type: text/plain, Size: 2984 bytes --]

diff -uprN a/arch/powerpc/kernel/proc_ppc64.c b/arch/powerpc/kernel/proc_ppc64.c
--- a/arch/powerpc/kernel/proc_ppc64.c	2006-10-14 05:34:03.000000000 +0200
+++ b/arch/powerpc/kernel/proc_ppc64.c	2006-10-20 08:32:20.000000000 +0200
@@ -51,15 +51,6 @@ static int __init proc_ppc64_create(void
 	if (!root)
 		return 1;
 
-	if (!of_find_node_by_path("/rtas"))
-		return 0;
-
-	if (!proc_mkdir("rtas", root))
-		return 1;
-
-	if (!proc_symlink("rtas", NULL, "ppc64/rtas"))
-		return 1;
-
 	return 0;
 }
 core_initcall(proc_ppc64_create);
diff -uprN a/arch/powerpc/kernel/rtas-proc.c b/arch/powerpc/kernel/rtas-proc.c
--- a/arch/powerpc/kernel/rtas-proc.c	2006-10-14 05:34:03.000000000 +0200
+++ b/arch/powerpc/kernel/rtas-proc.c	2006-10-20 08:22:57.000000000 +0200
@@ -253,43 +253,60 @@ static void get_location_code(struct seq
 static void check_location_string(struct seq_file *m, char *c);
 static void check_location(struct seq_file *m, char *c);
 
+static int __init proc_rtas_create(void)
+{
+        struct proc_dir_entry *root;
+
+        root = proc_mkdir("rtas" , NULL);
+        if (!root)
+                return -1;
+
+
+#ifdef CONFIG_PPC64
+        if (!proc_symlink("rtas", NULL, "ppc64/rtas"))
+                return -1;
+#endif
+
+        return 0;
+}
+
 static int __init proc_rtas_init(void)
 {
 	struct proc_dir_entry *entry;
 
-	if (!machine_is(pseries))
-		return -ENODEV;
-
 	rtas_node = of_find_node_by_name(NULL, "rtas");
 	if (rtas_node == NULL)
 		return -ENODEV;
 
-	entry = create_proc_entry("ppc64/rtas/progress", S_IRUGO|S_IWUSR, NULL);
+	if (proc_rtas_create() != 0)
+		return -ENODEV;
+
+	entry = create_proc_entry("/rtas/progress", S_IRUGO|S_IWUSR, NULL);
 	if (entry)
 		entry->proc_fops = &ppc_rtas_progress_operations;
 
-	entry = create_proc_entry("ppc64/rtas/clock", S_IRUGO|S_IWUSR, NULL);
+	entry = create_proc_entry("/rtas/clock", S_IRUGO|S_IWUSR, NULL);
 	if (entry)
 		entry->proc_fops = &ppc_rtas_clock_operations;
 
-	entry = create_proc_entry("ppc64/rtas/poweron", S_IWUSR|S_IRUGO, NULL);
+	entry = create_proc_entry("/rtas/poweron", S_IWUSR|S_IRUGO, NULL);
 	if (entry)
 		entry->proc_fops = &ppc_rtas_poweron_operations;
 
-	entry = create_proc_entry("ppc64/rtas/sensors", S_IRUGO, NULL);
+	entry = create_proc_entry("/rtas/sensors", S_IRUGO, NULL);
 	if (entry)
 		entry->proc_fops = &ppc_rtas_sensors_operations;
 
-	entry = create_proc_entry("ppc64/rtas/frequency", S_IWUSR|S_IRUGO,
+	entry = create_proc_entry("/rtas/frequency", S_IWUSR|S_IRUGO,
 				  NULL);
 	if (entry)
 		entry->proc_fops = &ppc_rtas_tone_freq_operations;
 
-	entry = create_proc_entry("ppc64/rtas/volume", S_IWUSR|S_IRUGO, NULL);
+	entry = create_proc_entry("/rtas/volume", S_IWUSR|S_IRUGO, NULL);
 	if (entry)
 		entry->proc_fops = &ppc_rtas_tone_volume_operations;
 
-	entry = create_proc_entry("ppc64/rtas/rmo_buffer", S_IRUSR, NULL);
+	entry = create_proc_entry("/rtas/rmo_buffer", S_IRUSR, NULL);
 	if (entry)
 		entry->proc_fops = &ppc_rtas_rmo_buf_ops;
 

[-- Attachment #3: nd.vcf --]
[-- Type: text/x-vcard, Size: 249 bytes --]

begin:vcard
fn:Nicolas DET ( bplan GmbH )
n:DET;Nicolas
org:bplan GmbH
adr:;;;;;;Germany
email;internet:nd@bplan-gmbh.de
title:Software Entwicklung
tel;work:+49 6171 9187 - 31
x-mozilla-html:FALSE
url:http://www.bplan-gmbh.de
version:2.1
end:vcard

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Nicolas DET]

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

Christoph Hellwig wrote:
 >> --- a/arch/powerpc/kernel/rtas-proc.c    2006-10-14 
05:34:03.000000000 +0200
 >> +++ b/arch/powerpc/kernel/rtas-proc.c    2006-10-16 
10:46:16.000000000 +0200
 >> @@ -253,43 +253,70 @@ static void get_location_code(struct seq
 >>  static void check_location_string(struct seq_file *m, char *c);
 >>  static void check_location(struct seq_file *m, char *c);
 >>
 >> +#ifdef CONFIG_PPC64
 >> +#define PROCRTAS_ROOT "ppc64"
 >> +#else
 >> +#define PROCRTAS_ROOT "ppc"
 >
 > Please don't do any pathname changes.  Even if ppc64 isn't correct it's
 > what applications expect and what we should provide for a coherent user
 > interface.

Humm, ok.
However, in this case 'ppc' (could be 32 or 64 as it is not specified) 
is more generic than 'ppc64'.

 > This should be the only change you need, and it should follow kernel
 > coding style, aka:
 >
 >     if (!machine_is(pseries) && !machine_is(chrp))
 >         return -ENODEV;
 >
 >>      rtas_node = of_find_node_by_name(NULL, "rtas");
 >>      if (rtas_node == NULL)
 >>          return -ENODEV;
 >
 > And given this check I wonder why we need the platform check at all.  It
 > should be safe to just remove it.
 >
 >

Indeed, however I can only test on CHRP. I'll remove the check in the 
upcomming patch.

The patch also include a small code to create the /proc/ppc/rtas entry. 
Should this be done here, or somewhere in arch/powerpc/chrp/setup.c ?

I will re submit a patch as soon as I can.

[-- Attachment #2: nd.vcf --]
[-- Type: text/x-vcard, Size: 249 bytes --]

begin:vcard
fn:Nicolas DET ( bplan GmbH )
n:DET;Nicolas
org:bplan GmbH
adr:;;;;;;Germany
email;internet:nd@bplan-gmbh.de
title:Software Entwicklung
tel;work:+49 6171 9187 - 31
x-mozilla-html:FALSE
url:http://www.bplan-gmbh.de
version:2.1
end:vcard

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Sven Luther]

On Wed, Oct 18, 2006 at 07:51:52AM +0200, Nicolas DET wrote:
> Christoph Hellwig wrote:
> >> --- a/arch/powerpc/kernel/rtas-proc.c    2006-10-14 
> 05:34:03.000000000 +0200
> >> +++ b/arch/powerpc/kernel/rtas-proc.c    2006-10-16 
> 10:46:16.000000000 +0200
> >> @@ -253,43 +253,70 @@ static void get_location_code(struct seq
> >>  static void check_location_string(struct seq_file *m, char *c);
> >>  static void check_location(struct seq_file *m, char *c);
> >>
> >> +#ifdef CONFIG_PPC64
> >> +#define PROCRTAS_ROOT "ppc64"
> >> +#else
> >> +#define PROCRTAS_ROOT "ppc"
> >
> > Please don't do any pathname changes.  Even if ppc64 isn't correct it's
> > what applications expect and what we should provide for a coherent user
> > interface.
> 
> Humm, ok.
> However, in this case 'ppc' (could be 32 or 64 as it is not specified) 
> is more generic than 'ppc64'.

Christoph, Nicolas, ...

What about doing the logical thing, and switching it to "powerpc" like the
recent arch change, or more logically, to "power" like the current rebranding
of the whole thing ?

I suppose we can also provide some symlinking or similar for compatibility ?
Or move the whole stuff under /sys/.../power/rtas ? 

Friendly,

Sven Luther

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Michael Ellerman]

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

On Wed, 2006-10-18 at 07:51 +0200, Nicolas DET wrote:
> Christoph Hellwig wrote:
>  >> --- a/arch/powerpc/kernel/rtas-proc.c    2006-10-14 
> 05:34:03.000000000 +0200
>  >> +++ b/arch/powerpc/kernel/rtas-proc.c    2006-10-16 
> 10:46:16.000000000 +0200
>  >> @@ -253,43 +253,70 @@ static void get_location_code(struct seq
>  >>  static void check_location_string(struct seq_file *m, char *c);
>  >>  static void check_location(struct seq_file *m, char *c);
>  >>
>  >> +#ifdef CONFIG_PPC64
>  >> +#define PROCRTAS_ROOT "ppc64"
>  >> +#else
>  >> +#define PROCRTAS_ROOT "ppc"
>  >
>  > Please don't do any pathname changes.  Even if ppc64 isn't correct it's
>  > what applications expect and what we should provide for a coherent user
>  > interface.
> 
> Humm, ok.
> However, in this case 'ppc' (could be 32 or 64 as it is not specified) 
> is more generic than 'ppc64'.

But it's called '/proc/ppc64' right now on lots of machines, so you
can't go changing it.

>  > This should be the only change you need, and it should follow kernel
>  > coding style, aka:
>  >
>  >     if (!machine_is(pseries) && !machine_is(chrp))
>  >         return -ENODEV;
>  >
>  >>      rtas_node = of_find_node_by_name(NULL, "rtas");
>  >>      if (rtas_node == NULL)
>  >>          return -ENODEV;
>  >
>  > And given this check I wonder why we need the platform check at all.  It
>  > should be safe to just remove it.
>  >
>  >
> 
> Indeed, however I can only test on CHRP. I'll remove the check in the 
> upcomming patch.

That should be fine AFAICT, you should probably just check that each of
the proc routines checks for errors - ie. just because you have an
"/rtas" node doesn't mean you necessarily have "/rtas/set-indicator" or
whatever.

> The patch also include a small code to create the /proc/ppc/rtas entry. 
> Should this be done here, or somewhere in arch/powerpc/chrp/setup.c ?

That code is almost entirely the same as proc_ppc64_create(), so I think
you should try and merge them - we want to minimise the number of
foo_ppc64() and foo_ppc32() routines we have.

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: 191 bytes --]

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Nicolas DET]

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

Michael Ellerman wrote:

 >>  >> +#ifdef CONFIG_PPC64
 >>  >> +#define PROCRTAS_ROOT "ppc64"
 >>  >> +#else
 >>  >> +#define PROCRTAS_ROOT "ppc"
 >>  >
 >>  > Please don't do any pathname changes.  Even if ppc64 isn't 
correct it's
 >>  > what applications expect and what we should provide for a 
coherent user
 >>  > interface.
 >>
 >> Humm, ok.
 >> However, in this case 'ppc' (could be 32 or 64 as it is not specified)
 >> is more generic than 'ppc64'.
 >
 > But it's called '/proc/ppc64' right now on lots of machines, so you
 > can't go changing it.

Ok. No problem


 >>
 >> Indeed, however I can only test on CHRP. I'll remove the check in the
 >> upcomming patch.
 >
 > That should be fine AFAICT, you should probably just check that each of
 > the proc routines checks for errors - ie. just because you have an
 > "/rtas" node doesn't mean you necessarily have "/rtas/set-indicator" or
 > whatever.

ok

 >
 >> The patch also include a small code to create the /proc/ppc/rtas entry.
 >> Should this be done here, or somewhere in arch/powerpc/chrp/setup.c ?
 >
 > That code is almost entirely the same as proc_ppc64_create(), so I think
 > you should try and merge them - we want to minimise the number of
 > foo_ppc64() and foo_ppc32() routines we have.
 >

Ok.
proc_ppc64_create() is now locate in arch/powerpc/kernel/proc_ppc64.c. 
Maybe a new file could be created (arch/powerpc/kernel/proc_ppc.c) ?

Any other suggestions?

Just indicate me the direction, and I would provide others patches.

Regards

[-- Attachment #2: nd.vcf --]
[-- Type: text/x-vcard, Size: 249 bytes --]

begin:vcard
fn:Nicolas DET ( bplan GmbH )
n:DET;Nicolas
org:bplan GmbH
adr:;;;;;;Germany
email;internet:nd@bplan-gmbh.de
title:Software Entwicklung
tel;work:+49 6171 9187 - 31
x-mozilla-html:FALSE
url:http://www.bplan-gmbh.de
version:2.1
end:vcard

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Olaf Hering]

On Wed, Oct 18, Michael Ellerman wrote:

> But it's called '/proc/ppc64' right now on lots of machines, so you
> can't go changing it.

if test -d /proc/ppc64 .. is a quick way to check wether the system is
64bit or not. Similar to if test -d /proc/iSeries ..
You better leave that simple interface.

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Benjamin Herrenschmidt]

On Wed, 2006-10-18 at 09:38 +0200, Olaf Hering wrote:
> On Wed, Oct 18, Michael Ellerman wrote:
> 
> > But it's called '/proc/ppc64' right now on lots of machines, so you
> > can't go changing it.
> 
> if test -d /proc/ppc64 .. is a quick way to check wether the system is
> 64bit or not. Similar to if test -d /proc/iSeries ..

That's really ugly ! You really do that ? Gack...

> You better leave that simple interface.

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Olaf Hering]

On Thu, Oct 19, Benjamin Herrenschmidt wrote:

> On Wed, 2006-10-18 at 09:38 +0200, Olaf Hering wrote:
> > On Wed, Oct 18, Michael Ellerman wrote:
> > 
> > > But it's called '/proc/ppc64' right now on lots of machines, so you
> > > can't go changing it.
> > 
> > if test -d /proc/ppc64 .. is a quick way to check wether the system is
> > 64bit or not. Similar to if test -d /proc/iSeries ..
> 
> That's really ugly ! You really do that ? Gack...

What way should I use?

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Benjamin Herrenschmidt]

On Thu, 2006-10-19 at 09:03 +0200, Olaf Hering wrote:
> On Thu, Oct 19, Benjamin Herrenschmidt wrote:
> 
> > On Wed, 2006-10-18 at 09:38 +0200, Olaf Hering wrote:
> > > On Wed, Oct 18, Michael Ellerman wrote:
> > > 
> > > > But it's called '/proc/ppc64' right now on lots of machines, so you
> > > > can't go changing it.
> > > 
> > > if test -d /proc/ppc64 .. is a quick way to check wether the system is
> > > 64bit or not. Similar to if test -d /proc/iSeries ..
> > 
> > That's really ugly ! You really do that ? Gack...
> 
> What way should I use?

uname ? /proc/cpuinfo ? Whatever but not that.

Ben.

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Olaf Hering]

On Fri, Oct 20, Benjamin Herrenschmidt wrote:

> On Thu, 2006-10-19 at 09:03 +0200, Olaf Hering wrote:
> > On Thu, Oct 19, Benjamin Herrenschmidt wrote:
> > 
> > > On Wed, 2006-10-18 at 09:38 +0200, Olaf Hering wrote:
> > > > On Wed, Oct 18, Michael Ellerman wrote:
> > > > 
> > > > > But it's called '/proc/ppc64' right now on lots of machines, so you
> > > > > can't go changing it.
> > > > 
> > > > if test -d /proc/ppc64 .. is a quick way to check wether the system is
> > > > 64bit or not. Similar to if test -d /proc/iSeries ..
> > > 
> > > That's really ugly ! You really do that ? Gack...
> > 
> > What way should I use?
> 
> uname ? /proc/cpuinfo ? Whatever but not that.

uname can be faked with powerpc32 or setarch. grep POWER /proc/cpuinfo
might be an option. I think its only used in rpm preinstall scripts to
reject 64bit kernel on 32bit systems or the other way around.

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Benjamin Herrenschmidt]

On Fri, 2006-10-20 at 07:44 +0200, Olaf Hering wrote:
> On Fri, Oct 20, Benjamin Herrenschmidt wrote:
> 
> > On Thu, 2006-10-19 at 09:03 +0200, Olaf Hering wrote:
> > > On Thu, Oct 19, Benjamin Herrenschmidt wrote:
> > > 
> > > > On Wed, 2006-10-18 at 09:38 +0200, Olaf Hering wrote:
> > > > > On Wed, Oct 18, Michael Ellerman wrote:
> > > > > 
> > > > > > But it's called '/proc/ppc64' right now on lots of machines, so you
> > > > > > can't go changing it.
> > > > > 
> > > > > if test -d /proc/ppc64 .. is a quick way to check wether the system is
> > > > > 64bit or not. Similar to if test -d /proc/iSeries ..
> > > > 
> > > > That's really ugly ! You really do that ? Gack...
> > > 
> > > What way should I use?
> > 
> > uname ? /proc/cpuinfo ? Whatever but not that.
> 
> uname can be faked with powerpc32 or setarch. grep POWER /proc/cpuinfo
> might be an option. I think its only used in rpm preinstall scripts to
> reject 64bit kernel on 32bit systems or the other way around.

grep POWER won't do good on 970 among others.

uname can be faked but why do you care ?

In fact, is somebody is faking it, maybe it's because they -want- your
scripts to think they are running on a 32 bits box....

I still think uname is the way to go.
 
Ben.

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Sven Luther]

On Fri, Oct 20, 2006 at 03:56:37PM +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2006-10-20 at 07:44 +0200, Olaf Hering wrote:
> > On Fri, Oct 20, Benjamin Herrenschmidt wrote:
> > 
> > > On Thu, 2006-10-19 at 09:03 +0200, Olaf Hering wrote:
> > > > On Thu, Oct 19, Benjamin Herrenschmidt wrote:
> > > > 
> > > > > On Wed, 2006-10-18 at 09:38 +0200, Olaf Hering wrote:
> > > > > > On Wed, Oct 18, Michael Ellerman wrote:
> > > > > > 
> > > > > > > But it's called '/proc/ppc64' right now on lots of machines, so you
> > > > > > > can't go changing it.
> > > > > > 
> > > > > > if test -d /proc/ppc64 .. is a quick way to check wether the system is
> > > > > > 64bit or not. Similar to if test -d /proc/iSeries ..
> > > > > 
> > > > > That's really ugly ! You really do that ? Gack...
> > > > 
> > > > What way should I use?
> > > 
> > > uname ? /proc/cpuinfo ? Whatever but not that.
> > 
> > uname can be faked with powerpc32 or setarch. grep POWER /proc/cpuinfo
> > might be an option. I think its only used in rpm preinstall scripts to
> > reject 64bit kernel on 32bit systems or the other way around.
> 
> grep POWER won't do good on 970 among others.
> 
> uname can be faked but why do you care ?
> 
> In fact, is somebody is faking it, maybe it's because they -want- your
> scripts to think they are running on a 32 bits box....
> 
> I still think uname is the way to go.
>  

Ben, would it make sense to add two informations to /proc/cpuinfo ?
It would be great to have a field there which will allow to check for the
kernel flavour to run, namely if the kernel/processor/whatever is running in
32bit or 64bit mode. The second field would be a generalized machine field,
or plateform or whatever, which would easily allow to differentiate between an
apple machine, a ibm chrp, a pegasos or other genesi product, etc. Without
necessarily having to maintain a huge amount of userlanf mappings from all
those different machine: fields, which all have CHRP, true, but no finer
grained informations, while we do have it in the kernel.

Friendly,

Sven Luther

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Olaf Hering]

On Fri, Oct 20, Sven Luther wrote:

> Ben, would it make sense to add two informations to /proc/cpuinfo ?
> It would be great to have a field there which will allow to check for the
> kernel flavour to run, namely if the kernel/processor/whatever is running in
> 32bit or 64bit mode. The second field would be a generalized machine field,
> or plateform or whatever, which would easily allow to differentiate between an
> apple machine, a ibm chrp, a pegasos or other genesi product, etc. Without
> necessarily having to maintain a huge amount of userlanf mappings from all
> those different machine: fields, which all have CHRP, true, but no finer
> grained informations, while we do have it in the kernel.

/proc/device-tree/{model,device_type,compatible} is enough to map
everything reliable in the installer. Dont pollute /proc/cpuinfo
further. There has already enough stuff that doesnt belong there, like
pmac-generation, 'pmac flags', 'detected as', motherboard and machine.
Oh, and bogomips...

About 32bit/64bit, maybe VmallocTotal from /proc/meminfo can be used.
incredible large numer == must be a 64bit kernel
No idea how reliable it is. There are those 36bit systems, but I bet
they dont run a distro.

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Sven Luther]

On Fri, Oct 20, 2006 at 08:44:56AM +0200, Olaf Hering wrote:
> On Fri, Oct 20, Sven Luther wrote:
> 
> > Ben, would it make sense to add two informations to /proc/cpuinfo ?
> > It would be great to have a field there which will allow to check for the
> > kernel flavour to run, namely if the kernel/processor/whatever is running in
> > 32bit or 64bit mode. The second field would be a generalized machine field,
> > or plateform or whatever, which would easily allow to differentiate between an
> > apple machine, a ibm chrp, a pegasos or other genesi product, etc. Without
> > necessarily having to maintain a huge amount of userlanf mappings from all
> > those different machine: fields, which all have CHRP, true, but no finer
> > grained informations, while we do have it in the kernel.
> 
> /proc/device-tree/{model,device_type,compatible} is enough to map

Yeah, well.

> everything reliable in the installer. Dont pollute /proc/cpuinfo
> further. There has already enough stuff that doesnt belong there, like
> pmac-generation, 'pmac flags', 'detected as', motherboard and machine.
> Oh, and bogomips...

Exact, it is not about poluting, but about rationalizing. The fact is that
these are the two infos that are the most necessary to userland, then why
disseminate the info in thousand different places, instead of making it easily
available in a canonical place in a standard format for everyone ? 

> About 32bit/64bit, maybe VmallocTotal from /proc/meminfo can be used.
> incredible large numer == must be a 64bit kernel
> No idea how reliable it is. There are those 36bit systems, but I bet
> they dont run a distro.

This sounds like the ugliest hack i have seen around, and is prone to break. A
proper /proc/cpuinfo flag would be most welcome to solve this cleanly.

Friendly,

Sven Luther

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Benjamin Herrenschmidt]

> Exact, it is not about poluting, but about rationalizing. The fact is that
> these are the two infos that are the most necessary to userland, then why
> disseminate the info in thousand different places, instead of making it easily
> available in a canonical place in a standard format for everyone ? 

Why would that be absolutely necessary to userland ? We've lived very
well without that so far.
 
> > About 32bit/64bit, maybe VmallocTotal from /proc/meminfo can be used.
> > incredible large numer == must be a 64bit kernel
> > No idea how reliable it is. There are those 36bit systems, but I bet
> > they dont run a distro.
> 
> This sounds like the ugliest hack i have seen around, and is prone to break. A
> proper /proc/cpuinfo flag would be most welcome to solve this cleanly.

uname is your friend... damn, and if you want to be real sure, then just
try to run a 64 bits binary and see what happens :)

Ben.

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Sven Luther]

On Fri, Oct 20, 2006 at 05:12:52PM +1000, Benjamin Herrenschmidt wrote:
> 
> > Exact, it is not about poluting, but about rationalizing. The fact is that
> > these are the two infos that are the most necessary to userland, then why
> > disseminate the info in thousand different places, instead of making it easily
> > available in a canonical place in a standard format for everyone ? 
> 
> Why would that be absolutely necessary to userland ? We've lived very
> well without that so far.

Because i already coded at least 4 times in 4 different places, code which
does the differentiation for userland, and it is only in debian. All other
distributions have this kind of code in their own individual way.

And if there is a new hardware coming out you didn't think about, it is not
enough to add support in the kernel, but you have to change all those userland
tools all over the place, with load of chances to forget something and break
them.

The kernel knows about this, and it would be easy enough to pass that
information clearly to userland. /proc/device-tree is indeed a solution for
this part.

> > > About 32bit/64bit, maybe VmallocTotal from /proc/meminfo can be used.
> > > incredible large numer == must be a 64bit kernel
> > > No idea how reliable it is. There are those 36bit systems, but I bet
> > > they dont run a distro.
> > 
> > This sounds like the ugliest hack i have seen around, and is prone to break. A
> > proper /proc/cpuinfo flag would be most welcome to solve this cleanly.
> 
> uname is your friend... damn, and if you want to be real sure, then just
> try to run a 64 bits binary and see what happens :)

Why not say it directly in a standard way ? So we avoid all distros and/or
different tools who need to know, to implement their own slightly different
way ? 

Friendly,

Sven Luther

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Benjamin Herrenschmidt]

> Why not say it directly in a standard way ? So we avoid all distros and/or
> different tools who need to know, to implement their own slightly different
> way ? 

There is a standard way... uname

Ben.

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Olaf Hering]

On Fri, Oct 20, Sven Luther wrote:

> Exact, it is not about poluting, but about rationalizing. The fact is that
> these are the two infos that are the most necessary to userland, then why
> disseminate the info in thousand different places, instead of making it easily
> available in a canonical place in a standard format for everyone ? 

cpuinfo is about the cpu, not the board layout nor the firmware
expectations.

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Sven Luther]

On Fri, Oct 20, 2006 at 09:20:37AM +0200, Olaf Hering wrote:
> On Fri, Oct 20, Sven Luther wrote:
> 
> > Exact, it is not about poluting, but about rationalizing. The fact is that
> > these are the two infos that are the most necessary to userland, then why
> > disseminate the info in thousand different places, instead of making it easily
> > available in a canonical place in a standard format for everyone ? 
> 
> cpuinfo is about the cpu, not the board layout nor the firmware
> expectations.

What is the machine: field doing there then ? Would it make sense to provide a
machineinfo then ? 

Friendly,

Sven Luther

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Olaf Hering]

On Fri, Oct 20, Sven Luther wrote:

> On Fri, Oct 20, 2006 at 09:20:37AM +0200, Olaf Hering wrote:
> > On Fri, Oct 20, Sven Luther wrote:
> > 
> > > Exact, it is not about poluting, but about rationalizing. The fact is that
> > > these are the two infos that are the most necessary to userland, then why
> > > disseminate the info in thousand different places, instead of making it easily
> > > available in a canonical place in a standard format for everyone ? 
> > 
> > cpuinfo is about the cpu, not the board layout nor the firmware
> > expectations.
> 
> What is the machine: field doing there then ? Would it make sense to provide a
> machineinfo then ? 

All historical bullshit. Use the device-tree.

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Segher Boessenkool]

>> cpuinfo is about the cpu, not the board layout nor the firmware
>> expectations.
>
> What is the machine: field doing there then ? Would it make sense  
> to provide a
> machineinfo then ?

Yes.  Or better, don't use /proc, get all info from sysfs
(with a nice userland tool to summarize the info, perhaps).


Segher

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Sven Luther]

On Fri, Oct 20, 2006 at 10:12:40AM +0200, Segher Boessenkool wrote:
> >>cpuinfo is about the cpu, not the board layout nor the firmware
> >>expectations.
> >
> >What is the machine: field doing there then ? Would it make sense  
> >to provide a
> >machineinfo then ?
> 
> Yes.  Or better, don't use /proc, get all info from sysfs
> (with a nice userland tool to summarize the info, perhaps).

Yeah, that makes sense. Will the device-tree move to /sys some day ? What
about things like the /rtas node ? 

But it will be years before all the distros move to it probably, see how most
of them still use parsing of the parted command line output for handling
partition table detection, and thus fail on anything not MBR.

Friendly,

Sven Luther

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Olaf Hering]

On Fri, Oct 20, Sven Luther wrote:

> Yeah, that makes sense. Will the device-tree move to /sys some day ? What

Moving /proc/device-tree would be an insane change.

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Segher Boessenkool]

>> Yeah, that makes sense. Will the device-tree move to /sys some  
>> day ? What
>
> Moving /proc/device-tree would be an insane change.

It should really be a separate filesystem, not part of procfs;
and then people can mount it wherever they want (in theory).


Segher

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Benjamin Herrenschmidt]

On Fri, 2006-10-20 at 08:44 +0200, Olaf Hering wrote:
> On Fri, Oct 20, Sven Luther wrote:
> 
> > Ben, would it make sense to add two informations to /proc/cpuinfo ?
> > It would be great to have a field there which will allow to check for the
> > kernel flavour to run, namely if the kernel/processor/whatever is running in
> > 32bit or 64bit mode. The second field would be a generalized machine field,
> > or plateform or whatever, which would easily allow to differentiate between an
> > apple machine, a ibm chrp, a pegasos or other genesi product, etc. Without
> > necessarily having to maintain a huge amount of userlanf mappings from all
> > those different machine: fields, which all have CHRP, true, but no finer
> > grained informations, while we do have it in the kernel.
> 
> /proc/device-tree/{model,device_type,compatible} is enough to map
> everything reliable in the installer. Dont pollute /proc/cpuinfo
> further. There has already enough stuff that doesnt belong there, like
> pmac-generation, 'pmac flags', 'detected as', motherboard and machine.
> Oh, and bogomips...

Some powermac detection bits for older models are not actually available
via the device-tree :)

> About 32bit/64bit, maybe VmallocTotal from /proc/meminfo can be used.
> incredible large numer == must be a 64bit kernel
> No idea how reliable it is. There are those 36bit systems, but I bet
> they dont run a distro.

But why are you absolutely trying to defeat uname ?

If somebody is trying to fake it, then use the faked version !

Ben.

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Olaf Hering]

On Fri, Oct 20, Benjamin Herrenschmidt wrote:

> > /proc/device-tree/{model,device_type,compatible} is enough to map
> > everything reliable in the installer. Dont pollute /proc/cpuinfo
> > further. There has already enough stuff that doesnt belong there, like
> > pmac-generation, 'pmac flags', 'detected as', motherboard and machine.
> > Oh, and bogomips...
> 
> Some powermac detection bits for older models are not actually available
> via the device-tree :)

What is missing?

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Benjamin Herrenschmidt]

On Fri, 2006-10-20 at 09:14 +0200, Olaf Hering wrote:
> On Fri, Oct 20, Benjamin Herrenschmidt wrote:
> 
> > > /proc/device-tree/{model,device_type,compatible} is enough to map
> > > everything reliable in the installer. Dont pollute /proc/cpuinfo
> > > further. There has already enough stuff that doesnt belong there, like
> > > pmac-generation, 'pmac flags', 'detected as', motherboard and machine.
> > > Oh, and bogomips...
> > 
> > Some powermac detection bits for older models are not actually available
> > via the device-tree :)
> 
> What is missing?

Well, PowerBook 2400 vs. 3400 typically. There is more that we don't
even implement in the kernel yet but maybe one day ... :)

Ben.

Re: [PATCH] enable RTAS /proc for PowerPC/CHRP platform

[Paul Mackerras]

Olaf Hering writes:

> About 32bit/64bit, maybe VmallocTotal from /proc/meminfo can be used.
> incredible large numer == must be a 64bit kernel
> No idea how reliable it is. There are those 36bit systems, but I bet
> they dont run a distro.

They only have 32-bit effective addresses and therefore a similar
VmallocTotal to any other 32-bit powerpc machine.

Paul.