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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxxx>
> ---
> 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@xxxxxxxxxx>