1. Patchew
  2. Xen
  3. View series

[PATCH] x86/platform: tidy do_platform_op() a little

Jan Beulich posted 1 patch 4 months, 2 weeks ago
Failed in applying to current master (apply log)
There is a newer version of this series
[PATCH] x86/platform: tidy do_platform_op() a little
Posted by Jan Beulich 4 months, 2 weeks ago 
The function has a few stray scopes and inconsistent use (indentation)
of break statements. Drop the stray braces and bring all the break-s in
line with one another. This in particular means dropping a redundant
break from XENPF_cpu_offline handling, pleasing Misra C:2012 rule 2.1.

No functional change intended.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
This is an alternative proposal to
https://lists.xen.org/archives/html/xen-devel/2023-12/msg01540.html.

A few more scopes could be eliminated if the various "cpu" variables
were consolidated to switch() of function scope.

--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -258,7 +258,6 @@ ret_t do_platform_op(
         break;
 
     case XENPF_add_memtype:
-    {
         ret = mtrr_add_page(
             op->u.add_memtype.mfn,
             op->u.add_memtype.nr_mfns,
@@ -273,11 +272,9 @@ ret_t do_platform_op(
             if ( ret != 0 )
                 mtrr_del_page(ret, 0, 0);
         }
-    }
-    break;
+        break;
 
     case XENPF_del_memtype:
-    {
         if (op->u.del_memtype.handle == 0
             /* mtrr/main.c otherwise does a lookup */
             && (int)op->u.del_memtype.reg >= 0)
@@ -288,8 +285,7 @@ ret_t do_platform_op(
         }
         else
             ret = -EINVAL;
-    }
-    break;
+        break;
 
     case XENPF_read_memtype:
     {
@@ -306,8 +302,8 @@ ret_t do_platform_op(
             ret = __copy_field_to_guest(u_xenpf_op, op, u.read_memtype)
                   ? -EFAULT : 0;
         }
+        break;
     }
-    break;
 
     case XENPF_microcode_update:
     {
@@ -316,8 +312,8 @@ ret_t do_platform_op(
         guest_from_compat_handle(data, op->u.microcode.data);
 
         ret = microcode_update(data, op->u.microcode.length);
+        break;
     }
-    break;
 
     case XENPF_platform_quirk:
     {
@@ -340,8 +336,8 @@ ret_t do_platform_op(
             ret = -EINVAL;
             break;
         }
+        break;
     }
-    break;
 
     case XENPF_firmware_info:
         switch ( op->u.firmware_info.type )
@@ -521,8 +517,8 @@ ret_t do_platform_op(
 
         if ( ret == 0 && __copy_field_to_guest(u_xenpf_op, op, u.getidletime) )
             ret = -EFAULT;
+        break;
     }
-    break;
 
     case XENPF_set_processor_pminfo:
         switch ( op->u.set_pminfo.type )
@@ -601,8 +597,8 @@ ret_t do_platform_op(
         put_cpu_maps();
 
         ret = __copy_field_to_guest(u_xenpf_op, op, u.pcpu_info) ? -EFAULT : 0;
+        break;
     }
-    break;
 
     case XENPF_get_cpu_version:
     {
@@ -637,8 +633,8 @@ ret_t do_platform_op(
 
         if ( __copy_field_to_guest(u_xenpf_op, op, u.pcpu_version) )
             ret = -EFAULT;
+        break;
     }
-    break;
 
     case XENPF_get_ucode_revision:
     {
@@ -666,8 +662,8 @@ ret_t do_platform_op(
 
         if ( __copy_field_to_guest(u_xenpf_op, op, u.ucode_revision) )
             ret = -EFAULT;
+        break;
     }
-    break;
 
     case XENPF_cpu_online:
     {
@@ -725,7 +721,6 @@ ret_t do_platform_op(
             0, cpu_down_helper, (void *)(unsigned long)cpu);
         break;
     }
-    break;
 
     case XENPF_cpu_hotadd:
         ret = xsm_resource_plug_core(XSM_HOOK);
@@ -735,7 +730,7 @@ ret_t do_platform_op(
         ret = cpu_add(op->u.cpu_add.apic_id,
                       op->u.cpu_add.acpi_id,
                       op->u.cpu_add.pxm);
-    break;
+        break;
 
     case XENPF_mem_hotadd:
         ret = xsm_resource_plug_core(XSM_HOOK);
@@ -775,8 +770,8 @@ ret_t do_platform_op(
             ret = -EINVAL;
             break;
         }
+        break;
     }
-    break;
 
     case XENPF_resource_op:
     {
@@ -842,8 +837,8 @@ ret_t do_platform_op(
             ret = ra.nr_done;
 
         xfree(ra.entries);
+        break;
     }
-    break;
 
     case XENPF_get_symbol:
     {
@@ -870,8 +865,8 @@ ret_t do_platform_op(
             ret = -EFAULT;
         if ( !ret && __copy_field_to_guest(u_xenpf_op, op, u.symdata) )
             ret = -EFAULT;
+        break;
     }
-    break;
 
 #ifdef CONFIG_VIDEO
     case XENPF_get_dom0_console:
Re: [PATCH] x86/platform: tidy do_platform_op() a little
Posted by Stefano Stabellini 4 months, 2 weeks ago 
On Tue, 19 Dec 2023, Jan Beulich wrote:
> The function has a few stray scopes and inconsistent use (indentation)
> of break statements. Drop the stray braces and bring all the break-s in
> line with one another. This in particular means dropping a redundant
> break from XENPF_cpu_offline handling, pleasing Misra C:2012 rule 2.1.
> 
> No functional change intended.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> This is an alternative proposal to
> https://lists.xen.org/archives/html/xen-devel/2023-12/msg01540.html.
> 
> A few more scopes could be eliminated if the various "cpu" variables
> were consolidated to switch() of function scope.
> 
> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -258,7 +258,6 @@ ret_t do_platform_op(
>          break;
>  
>      case XENPF_add_memtype:
> -    {
>          ret = mtrr_add_page(
>              op->u.add_memtype.mfn,
>              op->u.add_memtype.nr_mfns,
> @@ -273,11 +272,9 @@ ret_t do_platform_op(
>              if ( ret != 0 )
>                  mtrr_del_page(ret, 0, 0);
>          }
> -    }
> -    break;
> +        break;
>  
>      case XENPF_del_memtype:
> -    {
>          if (op->u.del_memtype.handle == 0
>              /* mtrr/main.c otherwise does a lookup */
>              && (int)op->u.del_memtype.reg >= 0)
> @@ -288,8 +285,7 @@ ret_t do_platform_op(
>          }
>          else
>              ret = -EINVAL;
> -    }
> -    break;
> +        break;
>  
>      case XENPF_read_memtype:
>      {
> @@ -306,8 +302,8 @@ ret_t do_platform_op(
>              ret = __copy_field_to_guest(u_xenpf_op, op, u.read_memtype)
>                    ? -EFAULT : 0;
>          }
> +        break;
>      }
> -    break;
>  
>      case XENPF_microcode_update:
>      {
> @@ -316,8 +312,8 @@ ret_t do_platform_op(
>          guest_from_compat_handle(data, op->u.microcode.data);
>  
>          ret = microcode_update(data, op->u.microcode.length);
> +        break;
>      }
> -    break;
>  
>      case XENPF_platform_quirk:
>      {
> @@ -340,8 +336,8 @@ ret_t do_platform_op(
>              ret = -EINVAL;
>              break;
>          }
> +        break;
>      }
> -    break;

I think you should be able to remove this break completely?
And also in XENPF_set_processor_pminfo. That's because the is a switch
with a default label and a break.

Everything else checks out. Given that this is already a good
improvement as it is:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Re: [PATCH] x86/platform: tidy do_platform_op() a little
Posted by Jan Beulich 4 months, 2 weeks ago 
On 20.12.2023 01:30, Stefano Stabellini wrote:
> On Tue, 19 Dec 2023, Jan Beulich wrote:
>> The function has a few stray scopes and inconsistent use (indentation)
>> of break statements. Drop the stray braces and bring all the break-s in
>> line with one another. This in particular means dropping a redundant
>> break from XENPF_cpu_offline handling, pleasing Misra C:2012 rule 2.1.
>>
>> No functional change intended.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> This is an alternative proposal to
>> https://lists.xen.org/archives/html/xen-devel/2023-12/msg01540.html.
>>
>> A few more scopes could be eliminated if the various "cpu" variables
>> were consolidated to switch() of function scope.
>>
>> --- a/xen/arch/x86/platform_hypercall.c
>> +++ b/xen/arch/x86/platform_hypercall.c
>> @@ -258,7 +258,6 @@ ret_t do_platform_op(
>>          break;
>>  
>>      case XENPF_add_memtype:
>> -    {
>>          ret = mtrr_add_page(
>>              op->u.add_memtype.mfn,
>>              op->u.add_memtype.nr_mfns,
>> @@ -273,11 +272,9 @@ ret_t do_platform_op(
>>              if ( ret != 0 )
>>                  mtrr_del_page(ret, 0, 0);
>>          }
>> -    }
>> -    break;
>> +        break;
>>  
>>      case XENPF_del_memtype:
>> -    {
>>          if (op->u.del_memtype.handle == 0
>>              /* mtrr/main.c otherwise does a lookup */
>>              && (int)op->u.del_memtype.reg >= 0)
>> @@ -288,8 +285,7 @@ ret_t do_platform_op(
>>          }
>>          else
>>              ret = -EINVAL;
>> -    }
>> -    break;
>> +        break;
>>  
>>      case XENPF_read_memtype:
>>      {
>> @@ -306,8 +302,8 @@ ret_t do_platform_op(
>>              ret = __copy_field_to_guest(u_xenpf_op, op, u.read_memtype)
>>                    ? -EFAULT : 0;
>>          }
>> +        break;
>>      }
>> -    break;
>>  
>>      case XENPF_microcode_update:
>>      {
>> @@ -316,8 +312,8 @@ ret_t do_platform_op(
>>          guest_from_compat_handle(data, op->u.microcode.data);
>>  
>>          ret = microcode_update(data, op->u.microcode.length);
>> +        break;
>>      }
>> -    break;
>>  
>>      case XENPF_platform_quirk:
>>      {
>> @@ -340,8 +336,8 @@ ret_t do_platform_op(
>>              ret = -EINVAL;
>>              break;
>>          }
>> +        break;
>>      }
>> -    break;
> 
> I think you should be able to remove this break completely?
> And also in XENPF_set_processor_pminfo. That's because the is a switch
> with a default label and a break.

Not really, no - those would then end up falling through to
XENPF_firmware_info (here) or XENPF_get_cpuinfo (there), as the break-s
I'm moving are for the outer switch(), sitting past the inner switch()'s
body's final brace.

I notice though that I missed at least one inconsistent break in an
inner switch(), so I guess I'll make a v2.

> Everything else checks out. Given that this is already a good
> improvement as it is:
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Thanks, but as said, I think a v2 is warranted.

Jan

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.