Hi there,
I came across x86/kernel/apic/nmi.c and found several typo.
It's trivial in terms of doing nothing on changing execution logic.
Please review. If make sense, please apply.
Ps. The patch is enclosed in attachment. The inline one
is c&p of it for reading.
Thanks,
Luming
Signed-off-by: Yu Luming <[email protected]>
nmi.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kernel/apic/nmi.c b/arch/x86/kernel/apic/nmi.c
index b3025b4..9ff1f6d 100644
--- a/arch/x86/kernel/apic/nmi.c
+++ b/arch/x86/kernel/apic/nmi.c
@@ -121,7 +121,7 @@ static void report_broken_nmi(int cpu, unsigned
int *prev_nmi_count)
atomic_dec(&nmi_active);
}
-static void __acpi_nmi_disable(void *__unused)
+static void __apic_nmi_disable(void *__unused)
{
apic_write(APIC_LVT0, APIC_DM_NMI | APIC_LVT_MASKED);
}
@@ -178,7 +178,7 @@ error:
if (nmi_watchdog == NMI_IO_APIC) {
if (!timer_through_8259)
disable_8259A_irq(0);
- on_each_cpu(__acpi_nmi_disable, NULL, 1);
+ on_each_cpu(__apic_nmi_disable, NULL, 1);
}
#ifdef CONFIG_X86_32
@@ -276,7 +276,7 @@ late_initcall(init_lapic_nmi_sysfs);
#endif /* CONFIG_PM */
-static void __acpi_nmi_enable(void *__unused)
+static void __apic_nmi_enable(void *__unused)
{
apic_write(APIC_LVT0, APIC_DM_NMI);
}
@@ -284,19 +284,19 @@ static void __acpi_nmi_enable(void *__unused)
/*
* Enable timer based NMIs on all CPUs:
*/
-void acpi_nmi_enable(void)
+void apic_nmi_enable(void)
{
if (atomic_read(&nmi_active) && nmi_watchdog == NMI_IO_APIC)
- on_each_cpu(__acpi_nmi_enable, NULL, 1);
+ on_each_cpu(__apic_nmi_enable, NULL, 1);
}
/*
* Disable timer based NMIs on all CPUs:
*/
-void acpi_nmi_disable(void)
+void apic_nmi_disable(void)
{
if (atomic_read(&nmi_active) && nmi_watchdog == NMI_IO_APIC)
- on_each_cpu(__acpi_nmi_disable, NULL, 1);
+ on_each_cpu(__apic_nmi_disable, NULL, 1);
}
/*
@@ -341,7 +341,7 @@ void stop_apic_nmi_watchdog(void *unused)
if (nmi_watchdog == NMI_LOCAL_APIC)
lapic_watchdog_stop();
else
- __acpi_nmi_disable(NULL);
+ __apic_nmi_disable(NULL);
__get_cpu_var(wd_enabled) = 0;
atomic_dec(&nmi_active);
}
@@ -472,7 +472,7 @@ static void enable_ioapic_nmi_watchdog_single(void *unused)
{
__get_cpu_var(wd_enabled) = 1;
atomic_inc(&nmi_active);
- __acpi_nmi_enable(NULL);
+ __apic_nmi_enable(NULL);
}
static void enable_ioapic_nmi_watchdog(void)
On Fri, 11 Sep 2009, Luming Yu wrote:
> I came across x86/kernel/apic/nmi.c and found several typo.
> It's trivial in terms of doing nothing on changing execution logic.
>
> Please review. If make sense, please apply.
Not a typo, but a historical leftover I believe. I think your change
makes sense these days now either way.
Maciej
On Fri, 11 Sep 2009, Luming Yu wrote:
> I came across x86/kernel/apic/nmi.c and found several typo.
> It's trivial in terms of doing nothing on changing execution logic.
>
> Please review. If make sense, please apply.
Hi,
I'd rather go this through x86 tree. Adding Ingo.
> Ps. The patch is enclosed in attachment. The inline one
> is c&p of it for reading.
>
>
> Thanks,
> Luming
>
> Signed-off-by: Yu Luming <[email protected]>
>
> nmi.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kernel/apic/nmi.c b/arch/x86/kernel/apic/nmi.c
> index b3025b4..9ff1f6d 100644
> --- a/arch/x86/kernel/apic/nmi.c
> +++ b/arch/x86/kernel/apic/nmi.c
> @@ -121,7 +121,7 @@ static void report_broken_nmi(int cpu, unsigned
> int *prev_nmi_count)
> atomic_dec(&nmi_active);
> }
>
> -static void __acpi_nmi_disable(void *__unused)
> +static void __apic_nmi_disable(void *__unused)
> {
> apic_write(APIC_LVT0, APIC_DM_NMI | APIC_LVT_MASKED);
> }
> @@ -178,7 +178,7 @@ error:
> if (nmi_watchdog == NMI_IO_APIC) {
> if (!timer_through_8259)
> disable_8259A_irq(0);
> - on_each_cpu(__acpi_nmi_disable, NULL, 1);
> + on_each_cpu(__apic_nmi_disable, NULL, 1);
> }
>
> #ifdef CONFIG_X86_32
> @@ -276,7 +276,7 @@ late_initcall(init_lapic_nmi_sysfs);
>
> #endif /* CONFIG_PM */
>
> -static void __acpi_nmi_enable(void *__unused)
> +static void __apic_nmi_enable(void *__unused)
> {
> apic_write(APIC_LVT0, APIC_DM_NMI);
> }
> @@ -284,19 +284,19 @@ static void __acpi_nmi_enable(void *__unused)
> /*
> * Enable timer based NMIs on all CPUs:
> */
> -void acpi_nmi_enable(void)
> +void apic_nmi_enable(void)
> {
> if (atomic_read(&nmi_active) && nmi_watchdog == NMI_IO_APIC)
> - on_each_cpu(__acpi_nmi_enable, NULL, 1);
> + on_each_cpu(__apic_nmi_enable, NULL, 1);
> }
>
> /*
> * Disable timer based NMIs on all CPUs:
> */
> -void acpi_nmi_disable(void)
> +void apic_nmi_disable(void)
> {
> if (atomic_read(&nmi_active) && nmi_watchdog == NMI_IO_APIC)
> - on_each_cpu(__acpi_nmi_disable, NULL, 1);
> + on_each_cpu(__apic_nmi_disable, NULL, 1);
> }
>
> /*
> @@ -341,7 +341,7 @@ void stop_apic_nmi_watchdog(void *unused)
> if (nmi_watchdog == NMI_LOCAL_APIC)
> lapic_watchdog_stop();
> else
> - __acpi_nmi_disable(NULL);
> + __apic_nmi_disable(NULL);
> __get_cpu_var(wd_enabled) = 0;
> atomic_dec(&nmi_active);
> }
> @@ -472,7 +472,7 @@ static void enable_ioapic_nmi_watchdog_single(void *unused)
> {
> __get_cpu_var(wd_enabled) = 1;
> atomic_inc(&nmi_active);
> - __acpi_nmi_enable(NULL);
> + __apic_nmi_enable(NULL);
> }
>
> static void enable_ioapic_nmi_watchdog(void)
>
--
Jiri Kosina
SUSE Labs, Novell Inc.
* Jiri Kosina <[email protected]> wrote:
> On Fri, 11 Sep 2009, Luming Yu wrote:
>
> > I came across x86/kernel/apic/nmi.c and found several typo.
> > It's trivial in terms of doing nothing on changing execution logic.
> >
> > Please review. If make sense, please apply.
>
> Hi,
>
> I'd rather go this through x86 tree. Adding Ingo.
>
> > Ps. The patch is enclosed in attachment. The inline one
> > is c&p of it for reading.
> >
> >
> > Thanks,
> > Luming
> >
> > Signed-off-by: Yu Luming <[email protected]>
> >
> > nmi.c | 18 +++++++++---------
> > 1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/x86/kernel/apic/nmi.c b/arch/x86/kernel/apic/nmi.c
> > index b3025b4..9ff1f6d 100644
> > --- a/arch/x86/kernel/apic/nmi.c
> > +++ b/arch/x86/kernel/apic/nmi.c
> > @@ -121,7 +121,7 @@ static void report_broken_nmi(int cpu, unsigned
> > int *prev_nmi_count)
> > atomic_dec(&nmi_active);
> > }
> >
> > -static void __acpi_nmi_disable(void *__unused)
> > +static void __apic_nmi_disable(void *__unused)
that's correctly named, as a 'git grep acpi_nmi_disable' should reveal.
Ingo
On Mon, 12 Oct 2009, Ingo Molnar wrote:
> > > I came across x86/kernel/apic/nmi.c and found several typo.
> > > It's trivial in terms of doing nothing on changing execution logic.
> > I'd rather go this through x86 tree. Adding Ingo.
> >
> > > Ps. The patch is enclosed in attachment. The inline one
> > > is c&p of it for reading.
> > >
> > >
> > > Thanks,
> > > Luming
> > >
> > > Signed-off-by: Yu Luming <[email protected]>
> > >
> > > nmi.c | 18 +++++++++---------
> > > 1 file changed, 9 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/arch/x86/kernel/apic/nmi.c b/arch/x86/kernel/apic/nmi.c
> > > index b3025b4..9ff1f6d 100644
> > > --- a/arch/x86/kernel/apic/nmi.c
> > > +++ b/arch/x86/kernel/apic/nmi.c
> > > @@ -121,7 +121,7 @@ static void report_broken_nmi(int cpu, unsigned
> > > int *prev_nmi_count)
> > > atomic_dec(&nmi_active);
> > > }
> > >
> > > -static void __acpi_nmi_disable(void *__unused)
> > > +static void __apic_nmi_disable(void *__unused)
>
> that's correctly named, as a 'git grep acpi_nmi_disable' should reveal.
I actually think that Luming Yu is right that the function is misnamed.
What does it have to do with ACPI?
All we do here is disable the NMI/LVT flags on the corresponding APIC. No
ACPI involved.
--
Jiri Kosina
SUSE Labs, Novell Inc.
* Jiri Kosina <[email protected]> wrote:
> On Mon, 12 Oct 2009, Ingo Molnar wrote:
>
> > > > I came across x86/kernel/apic/nmi.c and found several typo.
> > > > It's trivial in terms of doing nothing on changing execution logic.
> > > I'd rather go this through x86 tree. Adding Ingo.
> > >
> > > > Ps. The patch is enclosed in attachment. The inline one
> > > > is c&p of it for reading.
> > > >
> > > >
> > > > Thanks,
> > > > Luming
> > > >
> > > > Signed-off-by: Yu Luming <[email protected]>
> > > >
> > > > nmi.c | 18 +++++++++---------
> > > > 1 file changed, 9 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/arch/x86/kernel/apic/nmi.c b/arch/x86/kernel/apic/nmi.c
> > > > index b3025b4..9ff1f6d 100644
> > > > --- a/arch/x86/kernel/apic/nmi.c
> > > > +++ b/arch/x86/kernel/apic/nmi.c
> > > > @@ -121,7 +121,7 @@ static void report_broken_nmi(int cpu, unsigned
> > > > int *prev_nmi_count)
> > > > atomic_dec(&nmi_active);
> > > > }
> > > >
> > > > -static void __acpi_nmi_disable(void *__unused)
> > > > +static void __apic_nmi_disable(void *__unused)
> >
> > that's correctly named, as a 'git grep acpi_nmi_disable' should reveal.
>
> I actually think that Luming Yu is right that the function is misnamed.
> What does it have to do with ACPI?
It's not misnamed - it is a facility provided by architecture code to
the ACPI subsystem and hence named acpi_*(). See:
5d0e600: [PATCH] x86: fix laptop bootup hang in init_acpi()
Other architectures could opt to implement the same quirk - but it has
nothing to do with APIC.
Yes, on x86 we use the local APIC to disable NMIs, but that has no
effect on the naming of the facility ...
Ingo
On Tue, Oct 13, 2009 at 3:18 PM, Ingo Molnar <[email protected]> wrote:
>
> * Jiri Kosina <[email protected]> wrote:
>
>> On Mon, 12 Oct 2009, Ingo Molnar wrote:
>>
>> > > > I came across x86/kernel/apic/nmi.c and found several typo.
>> > > > It's trivial in terms of doing nothing on changing execution logic.
>> > > I'd rather go this through x86 tree. Adding Ingo.
>> > >
>> > > > Ps. The patch is enclosed in attachment. The inline one
>> > > > is c&p of it for reading.
>> > > >
>> > > >
>> > > > Thanks,
>> > > > Luming
>> > > >
>> > > > Signed-off-by: Yu Luming <[email protected]>
>> > > >
>> > > > nmi.c | 18 +++++++++---------
>> > > > 1 file changed, 9 insertions(+), 9 deletions(-)
>> > > >
>> > > > diff --git a/arch/x86/kernel/apic/nmi.c b/arch/x86/kernel/apic/nmi.c
>> > > > index b3025b4..9ff1f6d 100644
>> > > > --- a/arch/x86/kernel/apic/nmi.c
>> > > > +++ b/arch/x86/kernel/apic/nmi.c
>> > > > @@ -121,7 +121,7 @@ static void report_broken_nmi(int cpu, unsigned
>> > > > int *prev_nmi_count)
>> > > > atomic_dec(&nmi_active);
>> > > > }
>> > > >
>> > > > -static void __acpi_nmi_disable(void *__unused)
>> > > > +static void __apic_nmi_disable(void *__unused)
>> >
>> > that's correctly named, as a 'git grep acpi_nmi_disable' should reveal.
>>
>> I actually think that Luming Yu is right that the function is misnamed.
>> What does it have to do with ACPI?
>
> It's not misnamed - it is a facility provided by architecture code to
> the ACPI subsystem and hence named acpi_*(). See:
>
> 5d0e600: [PATCH] x86: fix laptop bootup hang in init_acpi()
Hmm..I would put some comments around those "__acpi_xxx",
so others can know it is intended name.
>
> Other architectures could opt to implement the same quirk - but it has
> nothing to do with APIC.
>
> Yes, on x86 we use the local APIC to disable NMIs, but that has no
> effect on the naming of the facility ...
>
> Ingo
>