[trivial PATCH] fix typo in nmi.c of apic

[trivial PATCH] fix typo in nmi.c of apic

[Luming Yu]

[-- Attachment #1: Type: text/plain, Size: 2421 bytes --]

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 <luming.yu@intel.com>

 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)

[-- Attachment #2: 9.patch --]
[-- Type: application/octet-stream, Size: 2012 bytes --]

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)

Re: [trivial PATCH] fix typo in nmi.c of apic

[Maciej W. Rozycki]

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

Re: [trivial PATCH] fix typo in nmi.c of apic

[Jiri Kosina]

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 <luming.yu@intel.com>
> 
>  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.

Re: [trivial PATCH] fix typo in nmi.c of apic

[Ingo Molnar]

* Jiri Kosina <jkosina@suse.cz> 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 <luming.yu@intel.com>
> > 
> >  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

Re: [trivial PATCH] fix typo in nmi.c of apic

[Jiri Kosina]

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 <luming.yu@intel.com>
> > > 
> > >  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.

Re: [trivial PATCH] fix typo in nmi.c of apic

[Ingo Molnar]

* Jiri Kosina <jkosina@suse.cz> 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 <luming.yu@intel.com>
> > > > 
> > > >  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

Re: [trivial PATCH] fix typo in nmi.c of apic

[Luming Yu]

On Tue, Oct 13, 2009 at 3:18 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Jiri Kosina <jkosina@suse.cz> 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 <luming.yu@intel.com>
>> > > >
>> > > >  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
>