[PATCH] [POWERPC] fix up log_plpar_hcall_return

[PATCH] [POWERPC] fix up log_plpar_hcall_return

[Will Schmidt]

This is mostly cosmetic.  This updates log_plpar_hcall_return() to use a
case statement rather than an if-then-else jumble, and moves it to
rtas.c where it can be near the other rtas related functions. 

Signed-off-by: Will Schmidt <will_schmidt@vnet.ibm.com>
---
 arch/powerpc/kernel/lparcfg.c |   25 -------------------------
 arch/powerpc/kernel/rtas.c    |   29 +++++++++++++++++++++++++++++
 include/asm-powerpc/rtas.h    |    1 +
 3 files changed, 30 insertions(+), 25 deletions(-)

diff --git a/arch/powerpc/kernel/lparcfg.c b/arch/powerpc/kernel/lparcfg.c
index 89486b6..e89471f 100644
--- a/arch/powerpc/kernel/lparcfg.c
+++ b/arch/powerpc/kernel/lparcfg.c
@@ -130,31 +130,6 @@ #ifdef CONFIG_PPC_PSERIES
 /*
  * Methods used to fetch LPAR data when running on a pSeries platform.
  */
-/* find a better place for this function... */
-static void log_plpar_hcall_return(unsigned long rc, char *tag)
-{
-	if (rc == 0)		/* success, return */
-		return;
-/* check for null tag ? */
-	if (rc == H_HARDWARE)
-		printk(KERN_INFO
-		       "plpar-hcall (%s) failed with hardware fault\n", tag);
-	else if (rc == H_FUNCTION)
-		printk(KERN_INFO
-		       "plpar-hcall (%s) failed; function not allowed\n", tag);
-	else if (rc == H_AUTHORITY)
-		printk(KERN_INFO
-		       "plpar-hcall (%s) failed; not authorized to this"
-		       " function\n", tag);
-	else if (rc == H_PARAMETER)
-		printk(KERN_INFO "plpar-hcall (%s) failed; Bad parameter(s)\n",
-		       tag);
-	else
-		printk(KERN_INFO
-		       "plpar-hcall (%s) failed with unexpected rc(0x%lx)\n",
-		       tag, rc);
-
-}
 
 /*
  * H_GET_PPP hcall returns info in 4 parms.
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 9d0735a..b215b6b 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -506,6 +506,35 @@ int rtas_error_rc(int rtas_rc)
 	return rc;
 }
 
+/* provide a human-readible description of some hcall return codes. */
+void log_plpar_hcall_return(unsigned long rc, char *tag)
+{
+	switch(rc) {
+	case 0:
+		return;
+	case H_HARDWARE:
+		printk(KERN_INFO "plpar-hcall (%s) "
+					"Hardware fault\n", tag);
+		return;
+	case H_FUNCTION:
+		printk(KERN_INFO "plpar-hcall (%s) "
+					"Function not allowed\n", tag);
+		return;
+	case H_AUTHORITY:
+		printk(KERN_INFO "plpar-hcall (%s) "
+				"Not authorized to this function\n", tag);
+		return;
+	case H_PARAMETER:
+		printk(KERN_INFO "plpar-hcall (%s) "
+					"Bad parameter(s)\n",tag);
+		return;
+	default:
+		printk(KERN_INFO "plpar-hcall (%s) "
+					"Unexpected rc(0x%lx)\n", tag, rc);
+	}
+}
+EXPORT_SYMBOL(log_plpar_hcall_return);
+
 int rtas_get_power_level(int powerdomain, int *level)
 {
 	int token = rtas_token("get-power-level");
diff --git a/include/asm-powerpc/rtas.h b/include/asm-powerpc/rtas.h
index 8eaa7b2..f36aaba 100644
--- a/include/asm-powerpc/rtas.h
+++ b/include/asm-powerpc/rtas.h
@@ -185,6 +185,7 @@ extern int early_init_dt_scan_rtas(unsig
 		const char *uname, int depth, void *data);
 
 extern void pSeries_log_error(char *buf, unsigned int err_type, int fatal);
+extern void log_plpar_hcall_return(unsigned long rc, char *tag);
 
 /* Error types logged.  */
 #define ERR_FLAG_ALREADY_LOGGED	0x0

Re: [PATCH] [POWERPC] fix up log_plpar_hcall_return

[Stephen Rothwell]

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

[sfr done Blue Hair]

Hi Will,

On Tue, 06 Mar 2007 17:11:41 -0600 Will Schmidt <will_schmidt@vnet.ibm.com> wrote:
>
> This is mostly cosmetic.  This updates log_plpar_hcall_return() to use a
> case statement rather than an if-then-else jumble, and moves it to
> rtas.c where it can be near the other rtas related functions.

You are moving a static function from the only file that calls it.  You
are exporting it when the only callers are in a file that cannot be part
of a module and you are (maybe) optimizing something that is only used in
error paths.

Sorry, but unless you are doing to introduce others uses, that I don't
see that this change helps.

[sfr redons normal hair]

I don't mean to sound harsh  :-)

--
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] [POWERPC] fix up log_plpar_hcall_return

[Benjamin Herrenschmidt]

On Tue, 2007-03-06 at 17:11 -0600, Will Schmidt wrote:
> This is mostly cosmetic.  This updates log_plpar_hcall_return() to use a
> case statement rather than an if-then-else jumble, and moves it to
> rtas.c where it can be near the other rtas related functions. 

Except that this has nothing to do with RTAS or do I miss something ?

Ben.

Re: [PATCH] [POWERPC] fix up log_plpar_hcall_return

[Will Schmidt]

On Wed, 2007-07-03 at 15:36 +0100, Benjamin Herrenschmidt wrote:
> On Tue, 2007-03-06 at 17:11 -0600, Will Schmidt wrote:
> > This is mostly cosmetic.  This updates log_plpar_hcall_return() to use a
> > case statement rather than an if-then-else jumble, and moves it to
> > rtas.c where it can be near the other rtas related functions. 
> 
> Except that this has nothing to do with RTAS or do I miss something ?
> 
> Ben.

Hi Ben, hello to other folks too, 

Milton made a similar comment via IRC.

I am likely blending the rtas/hcall terminology in a bad way, but will
clarify what I was thinking so someone can correct me.  :-)

The lparcfg function is named log_plpar_hcall_return(), taking a hcall
return value and a string, and outputs a human-readable error message
that corresponds to that return value.  

There is no hcall.c that this function would belong in.  HvCall.S and
plpar_wrappers.h contain wrappers and the assembly code that does the
actual calls; but neither of those places look like good candidates for
this function.   
The file rtas.c, however, does make some plpar_hcall function calls, and
has a number of the 900# magic numbers that *look* like the H_* defines
to me. 

-Will

Re: [PATCH] [POWERPC] fix up log_plpar_hcall_return

[Will Schmidt]

On Wed, 2007-07-03 at 11:12 +1100, Stephen Rothwell wrote: 
> [sfr done Blue Hair]

Hi Stephen, 

/me looks for blue spraypaint..

> Hi Will,
> 
> On Tue, 06 Mar 2007 17:11:41 -0600 Will Schmidt <will_schmidt@vnet.ibm.com> wrote:
> >
> > This is mostly cosmetic.  This updates log_plpar_hcall_return() to use a
> > case statement rather than an if-then-else jumble, and moves it to
> > rtas.c where it can be near the other rtas related functions.
> 
> You are moving a static function from the only file that calls it.  You
> are exporting it when the only callers are in a file that cannot be part
> of a module and you are (maybe) optimizing something that is only used in
> error paths.

> Sorry, but unless you are doing to introduce others uses, that I don't
> see that this change helps.

Yes.  This moves the function to somewhere a bit more accessible so
others *can* call it.   The comment at the top of the function being
replaced even says something like "find a better place for me...".  

Lparcfg *did* begin it's life as a module, and I'm sure I've turned it
back into a module at least once.   I dont know when it was last
reverted, but suspect it was done by someone who didnt know about
EXPORT_PER_CPU_SYMBOL_GPL, but I digress.. 
I've got a patch to turn lparcfg back into a module, but wanted to start
with this simpler cleanup patch first.

All that, and I've wanted to get rid of that particular if-else mess for
a while.  You are right that it's an error path optimization, though
this change isn't meant as much for optimization as fixing up the horrid
coding style that was initially used.

> [sfr redons normal hair]
How about Purple?  :-)

> 
> I don't mean to sound harsh  :-)
Not a problem.    depending on what other comments I get back, I'll
reorganize a bit and submit a larger patchset with other changes too. 

-Will

> 
> --
> Cheers,
> Stephen Rothwell                    sfr@canb.auug.org.au
> http://www.canb.auug.org.au/~sfr/