Re: [PATCH 1/3] powerpc/vphn: Check for error from hcall_vphn

[Nathan Lynch]

Hi Srikar,

Srikar Dronamraju  writes:
> There is no point in unpacking associativity, if
> H_HOME_NODE_ASSOCIATIVITY hcall has returned an error.
>
> Also added error messages for H_PARAMETER and default case in
> vphn_get_associativity.

These are two logical changes and should be separated IMO.


> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index 50d68d2..88b5157 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -1191,6 +1191,10 @@ static long vphn_get_associativity(unsigned long cpu,
>   VPHN_FLAG_VCPU, associativity);
>  
>   switch (rc) {
> + case H_SUCCESS:
> + dbg("VPHN hcall succeeded. Reset polling...\n");
> + timed_topology_update(0);
> + break;
>   case H_FUNCTION:
>   printk_once(KERN_INFO
>   "VPHN is not supported. Disabling polling...\n");
> @@ -1202,9 +1206,15 @@ static long vphn_get_associativity(unsigned long cpu,
>   "preventing VPHN. Disabling polling...\n");
>   stop_topology_update();
>   break;
> - case H_SUCCESS:
> - dbg("VPHN hcall succeeded. Reset polling...\n");
> - timed_topology_update(0);
> + case H_PARAMETER:
> + printk(KERN_ERR
> + "hcall_vphn() was passed an invalid parameter."
> + "Disabling polling...\n");

This will come out as:

hcall_vphn() was passed an invalid parameter.Disabling polling...
 ^

And it's misleading to say VPHN polling is being disabled when this case
does not invoke stop_topology_update().

> + break;
> + default:
> + printk(KERN_ERR
> + "hcall_vphn() returned %ld. Disabling polling \n", rc);
> + stop_topology_update();
>   break;

Any added prints in this routine must be _once or _ratelimited to avoid
log floods. Also use the pr_ APIs instead of printk please.

[PATCH 1/3] powerpc/vphn: Check for error from hcall_vphn

[Srikar Dronamraju]

There is no point in unpacking associativity, if
H_HOME_NODE_ASSOCIATIVITY hcall has returned an error.

Also added error messages for H_PARAMETER and default case in
vphn_get_associativity.

Signed-off-by: Srikar Dronamraju 
Cc: Michael Ellerman 
Cc: Nicholas Piggin 
Cc: Nathan Lynch 
Cc: linuxppc-dev@lists.ozlabs.org
Reported-by: Satheesh Rajendran 
Reported-by: Abdul Haleem 
---
 arch/powerpc/mm/numa.c| 16 +---
 arch/powerpc/platforms/pseries/vphn.c |  3 ++-
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index 50d68d2..88b5157 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -1191,6 +1191,10 @@ static long vphn_get_associativity(unsigned long cpu,
VPHN_FLAG_VCPU, associativity);
 
switch (rc) {
+   case H_SUCCESS:
+   dbg("VPHN hcall succeeded. Reset polling...\n");
+   timed_topology_update(0);
+   break;
case H_FUNCTION:
printk_once(KERN_INFO
"VPHN is not supported. Disabling polling...\n");
@@ -1202,9 +1206,15 @@ static long vphn_get_associativity(unsigned long cpu,
"preventing VPHN. Disabling polling...\n");
stop_topology_update();
break;
-   case H_SUCCESS:
-   dbg("VPHN hcall succeeded. Reset polling...\n");
-   timed_topology_update(0);
+   case H_PARAMETER:
+   printk(KERN_ERR
+   "hcall_vphn() was passed an invalid parameter."
+   "Disabling polling...\n");
+   break;
+   default:
+   printk(KERN_ERR
+   "hcall_vphn() returned %ld. Disabling polling \n", rc);
+   stop_topology_update();
break;
}
 
diff --git a/arch/powerpc/platforms/pseries/vphn.c 
b/arch/powerpc/platforms/pseries/vphn.c
index 3f07bf6..cca474a 100644
--- a/arch/powerpc/platforms/pseries/vphn.c
+++ b/arch/powerpc/platforms/pseries/vphn.c
@@ -82,7 +82,8 @@ long hcall_vphn(unsigned long cpu, u64 flags, __be32 
*associativity)
long retbuf[PLPAR_HCALL9_BUFSIZE] = {0};
 
rc = plpar_hcall9(H_HOME_NODE_ASSOCIATIVITY, retbuf, flags, cpu);
-   vphn_unpack_associativity(retbuf, associativity);
+   if (rc == H_SUCCESS)
+   vphn_unpack_associativity(retbuf, associativity);
 
return rc;
 }
-- 
1.8.3.1