* [PATCH v2] xen/mce: don't issue error message for failed /dev/mcelog registration
@ 2017-06-13 13:45 Juergen Gross
2017-06-13 15:20 ` Ingo Molnar
0 siblings, 1 reply; 7+ messages in thread
From: Juergen Gross @ 2017-06-13 13:45 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86
Cc: boris.ostrovsky, hpa, tglx, mingo, tony.luck, bp, Juergen Gross
When running under Xen as dom0 /dev/mcelog is being registered by Xen
instead of the normal mcelog driver. Avoid an error message being
issued by the mcelog driver in this case. Instead issue an informative
message that Xen has registered the device.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/kernel/cpu/mcheck/dev-mcelog.c | 11 +++++++++--
drivers/xen/mcelog.c | 2 ++
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/mcheck/dev-mcelog.c b/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
index 9c632cb88546..4eb5f7d1d593 100644
--- a/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
+++ b/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
@@ -388,9 +388,16 @@ static __init int dev_mcelog_init_device(void)
/* register character device /dev/mcelog */
err = misc_register(&mce_chrdev_device);
if (err) {
- pr_err("Unable to init device /dev/mcelog (rc: %d)\n", err);
- return err;
+ if (err == -EBUSY)
+ /* Xen dom0 might have registered the device already. */
+ pr_info("Unable to init device /dev/mcelog, already registered");
+ else {
+ pr_err("Unable to init device /dev/mcelog (rc: %d)\n",
+ err);
+ return err;
+ }
}
+
mce_register_decode_chain(&dev_mcelog_nb);
return 0;
}
diff --git a/drivers/xen/mcelog.c b/drivers/xen/mcelog.c
index a493c7315e94..6cc1c15bcd84 100644
--- a/drivers/xen/mcelog.c
+++ b/drivers/xen/mcelog.c
@@ -408,6 +408,8 @@ static int __init xen_late_init_mcelog(void)
if (ret)
goto deregister;
+ pr_info("/dev/mcelog registered by Xen\n");
+
return 0;
deregister:
--
2.12.3
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2] xen/mce: don't issue error message for failed /dev/mcelog registration
2017-06-13 13:45 [PATCH v2] xen/mce: don't issue error message for failed /dev/mcelog registration Juergen Gross
@ 2017-06-13 15:20 ` Ingo Molnar
2017-06-13 16:13 ` Juergen Gross
0 siblings, 1 reply; 7+ messages in thread
From: Ingo Molnar @ 2017-06-13 15:20 UTC (permalink / raw)
To: Juergen Gross
Cc: linux-kernel, xen-devel, x86, boris.ostrovsky, hpa, tglx, mingo,
tony.luck, bp
* Juergen Gross <jgross@suse.com> wrote:
> When running under Xen as dom0 /dev/mcelog is being registered by Xen
> instead of the normal mcelog driver. Avoid an error message being
> issued by the mcelog driver in this case. Instead issue an informative
> message that Xen has registered the device.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> arch/x86/kernel/cpu/mcheck/dev-mcelog.c | 11 +++++++++--
> drivers/xen/mcelog.c | 2 ++
> 2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mcheck/dev-mcelog.c b/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
> index 9c632cb88546..4eb5f7d1d593 100644
> --- a/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
> +++ b/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
> @@ -388,9 +388,16 @@ static __init int dev_mcelog_init_device(void)
> /* register character device /dev/mcelog */
> err = misc_register(&mce_chrdev_device);
> if (err) {
> - pr_err("Unable to init device /dev/mcelog (rc: %d)\n", err);
> - return err;
> + if (err == -EBUSY)
> + /* Xen dom0 might have registered the device already. */
> + pr_info("Unable to init device /dev/mcelog, already registered");
> + else {
> + pr_err("Unable to init device /dev/mcelog (rc: %d)\n",
> + err);
> + return err;
> + }
Please only use balanced curly braces in conditional statements.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] xen/mce: don't issue error message for failed /dev/mcelog registration
2017-06-13 15:20 ` Ingo Molnar
@ 2017-06-13 16:13 ` Juergen Gross
2017-06-13 16:31 ` Joe Perches
0 siblings, 1 reply; 7+ messages in thread
From: Juergen Gross @ 2017-06-13 16:13 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-kernel, xen-devel, x86, boris.ostrovsky, hpa, tglx, mingo,
tony.luck, bp
On 13/06/17 17:20, Ingo Molnar wrote:
>
> * Juergen Gross <jgross@suse.com> wrote:
>
>> When running under Xen as dom0 /dev/mcelog is being registered by Xen
>> instead of the normal mcelog driver. Avoid an error message being
>> issued by the mcelog driver in this case. Instead issue an informative
>> message that Xen has registered the device.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> arch/x86/kernel/cpu/mcheck/dev-mcelog.c | 11 +++++++++--
>> drivers/xen/mcelog.c | 2 ++
>> 2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/mcheck/dev-mcelog.c b/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
>> index 9c632cb88546..4eb5f7d1d593 100644
>> --- a/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
>> +++ b/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
>> @@ -388,9 +388,16 @@ static __init int dev_mcelog_init_device(void)
>> /* register character device /dev/mcelog */
>> err = misc_register(&mce_chrdev_device);
>> if (err) {
>> - pr_err("Unable to init device /dev/mcelog (rc: %d)\n", err);
>> - return err;
>> + if (err == -EBUSY)
>> + /* Xen dom0 might have registered the device already. */
>> + pr_info("Unable to init device /dev/mcelog, already registered");
>> + else {
>> + pr_err("Unable to init device /dev/mcelog (rc: %d)\n",
>> + err);
>> + return err;
>> + }
>
> Please only use balanced curly braces in conditional statements.
Okay, will change.
Juergen
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] xen/mce: don't issue error message for failed /dev/mcelog registration
2017-06-13 16:13 ` Juergen Gross
@ 2017-06-13 16:31 ` Joe Perches
2017-06-13 19:27 ` Juergen Gross
0 siblings, 1 reply; 7+ messages in thread
From: Joe Perches @ 2017-06-13 16:31 UTC (permalink / raw)
To: Juergen Gross, Ingo Molnar
Cc: linux-kernel, xen-devel, x86, boris.ostrovsky, hpa, tglx, mingo,
tony.luck, bp
On Tue, 2017-06-13 at 18:13 +0200, Juergen Gross wrote:
> On 13/06/17 17:20, Ingo Molnar wrote:
> > * Juergen Gross <jgross@suse.com> wrote:
> >
> > > When running under Xen as dom0 /dev/mcelog is being registered by Xen
> > > instead of the normal mcelog driver. Avoid an error message being
> > > issued by the mcelog driver in this case. Instead issue an informative
> > > message that Xen has registered the device.
[]
> > > diff --git a/arch/x86/kernel/cpu/mcheck/dev-mcelog.c b/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
[]
> > > @@ -388,9 +388,16 @@ static __init int dev_mcelog_init_device(void)
> > > /* register character device /dev/mcelog */
> > > err = misc_register(&mce_chrdev_device);
> > > if (err) {
> > > - pr_err("Unable to init device /dev/mcelog (rc: %d)\n", err);
> > > - return err;
> > > + if (err == -EBUSY)
> > > + /* Xen dom0 might have registered the device already. */
> > > + pr_info("Unable to init device /dev/mcelog, already registered");
> > > + else {
> > > + pr_err("Unable to init device /dev/mcelog (rc: %d)\n",
> > > + err);
> > > + return err;
> > > + }
> >
> > Please only use balanced curly braces in conditional statements.
>
> Okay, will change.
Perhaps better is to reverse the test
if (err != -EBUSY) {
pr_err("Unable to ....", err);
return err;
}
pr_info("etc...");
[ rest of code at this indentation ]
but it looks like you added a logic defect too and
this code should be:
if (err) {
if (err == -EBUSY)
pr_info(...)
else
pr_err(...)
return err;
}
or less indented using
err = misc_register(&mce_chrdev_device);
if (err == -EBUSY) {
pr_info(...);
return err;
} else if (err) {
pr_err(...);
return err;
}
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] xen/mce: don't issue error message for failed /dev/mcelog registration
2017-06-13 16:31 ` Joe Perches
@ 2017-06-13 19:27 ` Juergen Gross
2017-06-13 19:30 ` Joe Perches
0 siblings, 1 reply; 7+ messages in thread
From: Juergen Gross @ 2017-06-13 19:27 UTC (permalink / raw)
To: Joe Perches, Ingo Molnar
Cc: linux-kernel, xen-devel, x86, boris.ostrovsky, hpa, tglx, mingo,
tony.luck, bp
On 13/06/17 18:31, Joe Perches wrote:
> On Tue, 2017-06-13 at 18:13 +0200, Juergen Gross wrote:
>> On 13/06/17 17:20, Ingo Molnar wrote:
>>> * Juergen Gross <jgross@suse.com> wrote:
>>>
>>>> When running under Xen as dom0 /dev/mcelog is being registered by Xen
>>>> instead of the normal mcelog driver. Avoid an error message being
>>>> issued by the mcelog driver in this case. Instead issue an informative
>>>> message that Xen has registered the device.
> []
>>>> diff --git a/arch/x86/kernel/cpu/mcheck/dev-mcelog.c b/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
> []
>>>> @@ -388,9 +388,16 @@ static __init int dev_mcelog_init_device(void)
>>>> /* register character device /dev/mcelog */
>>>> err = misc_register(&mce_chrdev_device);
>>>> if (err) {
>>>> - pr_err("Unable to init device /dev/mcelog (rc: %d)\n", err);
>>>> - return err;
>>>> + if (err == -EBUSY)
>>>> + /* Xen dom0 might have registered the device already. */
>>>> + pr_info("Unable to init device /dev/mcelog, already registered");
>>>> + else {
>>>> + pr_err("Unable to init device /dev/mcelog (rc: %d)\n",
>>>> + err);
>>>> + return err;
>>>> + }
>>>
>>> Please only use balanced curly braces in conditional statements.
>>
>> Okay, will change.
>
> Perhaps better is to reverse the test
>
> if (err != -EBUSY) {
> pr_err("Unable to ....", err);
> return err;
> }
> pr_info("etc...");
Okay.
>
> [ rest of code at this indentation ]
>
> but it looks like you added a logic defect too and
> this code should be:
>
> if (err) {
> if (err == -EBUSY)
> pr_info(...)
> else
> pr_err(...)
> return err;
> }
>
> or less indented using
>
> err = misc_register(&mce_chrdev_device);
> if (err == -EBUSY) {
> pr_info(...);
> return err;
> } else if (err) {
> pr_err(...);
> return err;
> }
I didn't want to omit the call to mce_register_decode_chain() in the Xen
case.
Juergen
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] xen/mce: don't issue error message for failed /dev/mcelog registration
2017-06-13 19:27 ` Juergen Gross
@ 2017-06-13 19:30 ` Joe Perches
2017-06-14 7:49 ` Juergen Gross
0 siblings, 1 reply; 7+ messages in thread
From: Joe Perches @ 2017-06-13 19:30 UTC (permalink / raw)
To: Juergen Gross, Ingo Molnar
Cc: linux-kernel, xen-devel, x86, boris.ostrovsky, hpa, tglx, mingo,
tony.luck, bp
On Tue, 2017-06-13 at 21:27 +0200, Juergen Gross wrote:
> On 13/06/17 18:31, Joe Perches wrote:
> > On Tue, 2017-06-13 at 18:13 +0200, Juergen Gross wrote:
> > > On 13/06/17 17:20, Ingo Molnar wrote:
> > > > * Juergen Gross <jgross@suse.com> wrote:
> > > >
> > > > > When running under Xen as dom0 /dev/mcelog is being registered by Xen
> > > > > instead of the normal mcelog driver. Avoid an error message being
> > > > > issued by the mcelog driver in this case. Instead issue an informative
> > > > > message that Xen has registered the device.
> >
> > []
> > > > > diff --git a/arch/x86/kernel/cpu/mcheck/dev-mcelog.c b/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
> >
> > []
> > > > > @@ -388,9 +388,16 @@ static __init int dev_mcelog_init_device(void)
> > > > > /* register character device /dev/mcelog */
> > > > > err = misc_register(&mce_chrdev_device);
> > > > > if (err) {
> > > > > - pr_err("Unable to init device /dev/mcelog (rc: %d)\n", err);
> > > > > - return err;
> > > > > + if (err == -EBUSY)
> > > > > + /* Xen dom0 might have registered the device already. */
> > > > > + pr_info("Unable to init device /dev/mcelog, already registered");
> > > > > + else {
> > > > > + pr_err("Unable to init device /dev/mcelog (rc: %d)\n",
> > > > > + err);
> > > > > + return err;
> > > > > + }
> > > >
> > > > Please only use balanced curly braces in conditional statements.
> > >
> > > Okay, will change.
> >
> > Perhaps better is to reverse the test
> >
> > if (err != -EBUSY) {
> > pr_err("Unable to ....", err);
> > return err;
> > }
> > pr_info("etc...");
>
> Okay.
>
> >
> > [ rest of code at this indentation ]
> >
> > but it looks like you added a logic defect too and
> > this code should be:
> >
> > if (err) {
> > if (err == -EBUSY)
> > pr_info(...)
> > else
> > pr_err(...)
> > return err;
> > }
> >
> > or less indented using
> >
> > err = misc_register(&mce_chrdev_device);
> > if (err == -EBUSY) {
> > pr_info(...);
> > return err;
> > } else if (err) {
> > pr_err(...);
> > return err;
> > }
>
> I didn't want to omit the call to mce_register_decode_chain() in the Xen
> case.
You should definitely mention that behavior change
in the changelog.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] xen/mce: don't issue error message for failed /dev/mcelog registration
2017-06-13 19:30 ` Joe Perches
@ 2017-06-14 7:49 ` Juergen Gross
0 siblings, 0 replies; 7+ messages in thread
From: Juergen Gross @ 2017-06-14 7:49 UTC (permalink / raw)
To: Joe Perches, Ingo Molnar
Cc: linux-kernel, xen-devel, x86, boris.ostrovsky, hpa, tglx, mingo,
tony.luck, bp
On 13/06/17 21:30, Joe Perches wrote:
> On Tue, 2017-06-13 at 21:27 +0200, Juergen Gross wrote:
>> On 13/06/17 18:31, Joe Perches wrote:
>>> On Tue, 2017-06-13 at 18:13 +0200, Juergen Gross wrote:
>>>> On 13/06/17 17:20, Ingo Molnar wrote:
>>>>> * Juergen Gross <jgross@suse.com> wrote:
>>>>>
>>>>>> When running under Xen as dom0 /dev/mcelog is being registered by Xen
>>>>>> instead of the normal mcelog driver. Avoid an error message being
>>>>>> issued by the mcelog driver in this case. Instead issue an informative
>>>>>> message that Xen has registered the device.
>>>
>>> []
>>>>>> diff --git a/arch/x86/kernel/cpu/mcheck/dev-mcelog.c b/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
>>>
>>> []
>>>>>> @@ -388,9 +388,16 @@ static __init int dev_mcelog_init_device(void)
>>>>>> /* register character device /dev/mcelog */
>>>>>> err = misc_register(&mce_chrdev_device);
>>>>>> if (err) {
>>>>>> - pr_err("Unable to init device /dev/mcelog (rc: %d)\n", err);
>>>>>> - return err;
>>>>>> + if (err == -EBUSY)
>>>>>> + /* Xen dom0 might have registered the device already. */
>>>>>> + pr_info("Unable to init device /dev/mcelog, already registered");
>>>>>> + else {
>>>>>> + pr_err("Unable to init device /dev/mcelog (rc: %d)\n",
>>>>>> + err);
>>>>>> + return err;
>>>>>> + }
>>>>>
>>>>> Please only use balanced curly braces in conditional statements.
>>>>
>>>> Okay, will change.
>>>
>>> Perhaps better is to reverse the test
>>>
>>> if (err != -EBUSY) {
>>> pr_err("Unable to ....", err);
>>> return err;
>>> }
>>> pr_info("etc...");
>>
>> Okay.
>>
>>>
>>> [ rest of code at this indentation ]
>>>
>>> but it looks like you added a logic defect too and
>>> this code should be:
>>>
>>> if (err) {
>>> if (err == -EBUSY)
>>> pr_info(...)
>>> else
>>> pr_err(...)
>>> return err;
>>> }
>>>
>>> or less indented using
>>>
>>> err = misc_register(&mce_chrdev_device);
>>> if (err == -EBUSY) {
>>> pr_info(...);
>>> return err;
>>> } else if (err) {
>>> pr_err(...);
>>> return err;
>>> }
>>
>> I didn't want to omit the call to mce_register_decode_chain() in the Xen
>> case.
>
> You should definitely mention that behavior change
> in the changelog.
Looking more thoroughly it seems I really don't need the call for Xen.
So thanks for bringing it up. :-)
Juergen
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-06-14 7:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-13 13:45 [PATCH v2] xen/mce: don't issue error message for failed /dev/mcelog registration Juergen Gross
2017-06-13 15:20 ` Ingo Molnar
2017-06-13 16:13 ` Juergen Gross
2017-06-13 16:31 ` Joe Perches
2017-06-13 19:27 ` Juergen Gross
2017-06-13 19:30 ` Joe Perches
2017-06-14 7:49 ` Juergen Gross
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox