| Message ID | c12f20a3-407c-4914-b6a0-1690962fd72c@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/platform: tidy do_platform_op() a little | expand |
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>
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
--- 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:
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.