| Message ID | 01aad263ad60f37ed74138b5abf2733361bb7d25.1715673586.git.Sergiy_Kibrik@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: make Intel/AMD vPMU & MCE support configurable | expand |
On 14.05.2024 10:26, Sergiy Kibrik wrote: > The default switch case block is likely wanted here, to handle situation > e.g. of unexpected c->x86_vendor value -- then no mcheck init is done, but > misleading message still gets logged anyway. With "likely" dropped I'm okay with this as far as the addition of the default label goes. However, ... > --- a/xen/arch/x86/cpu/mcheck/non-fatal.c > +++ b/xen/arch/x86/cpu/mcheck/non-fatal.c > @@ -29,9 +29,13 @@ static int __init cf_check init_nonfatal_mce_checker(void) > /* Assume we are on K8 or newer AMD or Hygon CPU here */ > amd_nonfatal_mcheck_init(c); > break; > + > case X86_VENDOR_INTEL: > intel_nonfatal_mcheck_init(c); > break; > + > + default: > + return -ENODEV; > } > printk(KERN_INFO "mcheck_poll: Machine check polling timer started.\n"); > return 0; ... converting this to an error return still isn't justified. On one hand it's benign because we still don't check return values from initcalls. Yet otoh it isn't really an error, is it? Jan
On 16.05.2024 11:44, Jan Beulich wrote: > On 14.05.2024 10:26, Sergiy Kibrik wrote: >> The default switch case block is likely wanted here, to handle situation >> e.g. of unexpected c->x86_vendor value -- then no mcheck init is done, but >> misleading message still gets logged anyway. > > With "likely" dropped I'm okay with this as far as the addition of the default > label goes. However, ... > >> --- a/xen/arch/x86/cpu/mcheck/non-fatal.c >> +++ b/xen/arch/x86/cpu/mcheck/non-fatal.c >> @@ -29,9 +29,13 @@ static int __init cf_check init_nonfatal_mce_checker(void) >> /* Assume we are on K8 or newer AMD or Hygon CPU here */ >> amd_nonfatal_mcheck_init(c); >> break; >> + >> case X86_VENDOR_INTEL: >> intel_nonfatal_mcheck_init(c); >> break; >> + >> + default: >> + return -ENODEV; >> } >> printk(KERN_INFO "mcheck_poll: Machine check polling timer started.\n"); >> return 0; > > ... converting this to an error return still isn't justified. On one hand it's > benign because we still don't check return values from initcalls. Yet otoh it > isn't really an error, is it? I realize earlier in the function there are error returns, too. I question at least the former of them as well. And the latter shouldn't be an error at least when the vendor isn't going to be handled in the switch(). Jan
16.05.24 12:46, Jan Beulich: >>> --- a/xen/arch/x86/cpu/mcheck/non-fatal.c >>> +++ b/xen/arch/x86/cpu/mcheck/non-fatal.c >>> @@ -29,9 +29,13 @@ static int __init cf_check init_nonfatal_mce_checker(void) >>> /* Assume we are on K8 or newer AMD or Hygon CPU here */ >>> amd_nonfatal_mcheck_init(c); >>> break; >>> + >>> case X86_VENDOR_INTEL: >>> intel_nonfatal_mcheck_init(c); >>> break; >>> + >>> + default: >>> + return -ENODEV; >>> } >>> printk(KERN_INFO "mcheck_poll: Machine check polling timer started.\n"); >>> return 0; >> ... converting this to an error return still isn't justified. On one hand it's >> benign because we still don't check return values from initcalls. Yet otoh it >> isn't really an error, is it? > I realize earlier in the function there are error returns, too. I question at > least the former of them as well. And the latter shouldn't be an error at least > when the vendor isn't going to be handled in the switch(). I'll just return 0 then, and maybe leave a comment, as this code will start to look a bit unnatural to me. -Sergiy
diff --git a/xen/arch/x86/cpu/mcheck/non-fatal.c b/xen/arch/x86/cpu/mcheck/non-fatal.c index 33cacd15c2..c5d8e3aea9 100644 --- a/xen/arch/x86/cpu/mcheck/non-fatal.c +++ b/xen/arch/x86/cpu/mcheck/non-fatal.c @@ -29,9 +29,13 @@ static int __init cf_check init_nonfatal_mce_checker(void) /* Assume we are on K8 or newer AMD or Hygon CPU here */ amd_nonfatal_mcheck_init(c); break; + case X86_VENDOR_INTEL: intel_nonfatal_mcheck_init(c); break; + + default: + return -ENODEV; } printk(KERN_INFO "mcheck_poll: Machine check polling timer started.\n"); return 0;
The default switch case block is likely wanted here, to handle situation e.g. of unexpected c->x86_vendor value -- then no mcheck init is done, but misleading message still gets logged anyway. Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> CC: Jan Beulich <jbeulich@suse.com> --- xen/arch/x86/cpu/mcheck/non-fatal.c | 4 ++++ 1 file changed, 4 insertions(+)