| Message ID | e944cc4f-354c-4752-8794-03e6a7517372@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mm: use "break" in arch_memory_op() | expand |
On Tue, 19 Dec 2023, Jan Beulich wrote: > The final return statement is unreachable and hence disliked by Misra > C:2012 (rule 2.1). Convert those case-specific (main) return statements > which already use "rc", or in one case when it can be used without > further adding of code, to break. > > 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/msg01537.html. > Yet another option would be to simply pull the default case out of the > switch() statement. > > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -4724,7 +4724,7 @@ long arch_memory_op(unsigned long cmd, X > spin_unlock(&d->arch.e820_lock); > > rcu_unlock_domain(d); > - return rc; > + break; > } > > case XENMEM_memory_map: > @@ -4818,7 +4818,7 @@ long arch_memory_op(unsigned long cmd, X > if ( __copy_to_guest(arg, &ctxt.map, 1) ) > return -EFAULT; > > - return 0; > + break; > } There are also two other return 0; under case XENMEM_memory_map and XENMEM_machphys_mapping. I would be consistent and either leave this return 0 alone, or change all the return 0. Either way, this patch is correct, so: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > case XENMEM_machphys_mapping: > @@ -4880,7 +4880,7 @@ long arch_memory_op(unsigned long cmd, X > } > > rcu_unlock_domain(d); > - return rc; > + break; > } > #endif > > @@ -4888,7 +4888,7 @@ long arch_memory_op(unsigned long cmd, X > return subarch_memory_op(cmd, arg); > } > > - return 0; > + return rc; > } > > int cf_check mmio_ro_emulated_write( >
On 20.12.2023 01:22, Stefano Stabellini wrote: > On Tue, 19 Dec 2023, Jan Beulich wrote: >> The final return statement is unreachable and hence disliked by Misra >> C:2012 (rule 2.1). Convert those case-specific (main) return statements >> which already use "rc", or in one case when it can be used without >> further adding of code, to break. >> >> 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/msg01537.html. >> Yet another option would be to simply pull the default case out of the >> switch() statement. >> >> --- a/xen/arch/x86/mm.c >> +++ b/xen/arch/x86/mm.c >> @@ -4724,7 +4724,7 @@ long arch_memory_op(unsigned long cmd, X >> spin_unlock(&d->arch.e820_lock); >> >> rcu_unlock_domain(d); >> - return rc; >> + break; >> } >> >> case XENMEM_memory_map: >> @@ -4818,7 +4818,7 @@ long arch_memory_op(unsigned long cmd, X >> if ( __copy_to_guest(arg, &ctxt.map, 1) ) >> return -EFAULT; >> >> - return 0; >> + break; >> } > > There are also two other return 0; under case XENMEM_memory_map and > XENMEM_machphys_mapping. I would be consistent and either leave this > return 0 alone, or change all the return 0. Yes, that would have been another possible pattern to follow. Due to the multiple possible approaches I had specifically outlined (in the description) the pattern I decided to follow. > Either way, this patch is correct, so: > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> Thanks. Jan
--- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -4724,7 +4724,7 @@ long arch_memory_op(unsigned long cmd, X spin_unlock(&d->arch.e820_lock); rcu_unlock_domain(d); - return rc; + break; } case XENMEM_memory_map: @@ -4818,7 +4818,7 @@ long arch_memory_op(unsigned long cmd, X if ( __copy_to_guest(arg, &ctxt.map, 1) ) return -EFAULT; - return 0; + break; } case XENMEM_machphys_mapping: @@ -4880,7 +4880,7 @@ long arch_memory_op(unsigned long cmd, X } rcu_unlock_domain(d); - return rc; + break; } #endif @@ -4888,7 +4888,7 @@ long arch_memory_op(unsigned long cmd, X return subarch_memory_op(cmd, arg); } - return 0; + return rc; } int cf_check mmio_ro_emulated_write(
The final return statement is unreachable and hence disliked by Misra C:2012 (rule 2.1). Convert those case-specific (main) return statements which already use "rc", or in one case when it can be used without further adding of code, to break. 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/msg01537.html. Yet another option would be to simply pull the default case out of the switch() statement.