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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@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.

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

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

Jan