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

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