[PATCH] Typo in x86 apic.c with DIVISOR setup

[PATCH] Typo in x86 apic.c with DIVISOR setup

[Venki Pallipadi]

Fix a typo in x86 apic.c CONFG_X86_64 -> CONFIG_X86_64.
The intention looks like was to have divisor of 1 on x86_64 kernel.

Also, for DIVISOR value of 1 to work, APIC_TDR_DIV should also change
depending on divide by 1 or divide by 16 is used.

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>

---
 arch/x86/kernel/apic.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: tip/arch/x86/kernel/apic.c
===================================================================
--- tip.orig/arch/x86/kernel/apic.c	2008-10-09 17:12:39.000000000 -0700
+++ tip/arch/x86/kernel/apic.c	2008-10-09 17:13:29.000000000 -0700
@@ -332,10 +332,12 @@ int lapic_get_maxlvt(void)
  */
 
 /* Clock divisor */
-#ifdef CONFG_X86_64
+#ifdef CONFIG_X86_64
 #define APIC_DIVISOR 1
+#define APIC_TDR_DIV APIC_TDR_DIV_1
 #else
 #define APIC_DIVISOR 16
+#define APIC_TDR_DIV APIC_TDR_DIV_16
 #endif
 
 /*
@@ -369,7 +371,7 @@ static void __setup_APIC_LVTT(unsigned i
 	tmp_value = apic_read(APIC_TDCR);
 	apic_write(APIC_TDCR,
 		(tmp_value & ~(APIC_TDR_DIV_1 | APIC_TDR_DIV_TMBASE)) |
-		APIC_TDR_DIV_16);
+		APIC_TDR_DIV);
 
 	if (!oneshot)
 		apic_write(APIC_TMICT, clocks / APIC_DIVISOR);

Re: [PATCH] Typo in x86 apic.c with DIVISOR setup

[Cyrill Gorcunov]

On Fri, Oct 10, 2008 at 4:22 AM, Venki Pallipadi
<venkatesh.pallipadi@intel.com> wrote:
>
> Fix a typo in x86 apic.c CONFG_X86_64 -> CONFIG_X86_64.
> The intention looks like was to have divisor of 1 on x86_64 kernel.
>
> Also, for DIVISOR value of 1 to work, APIC_TDR_DIV should also change
> depending on divide by 1 or divide by 16 is used.
>
> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
>
> ---
>  arch/x86/kernel/apic.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> Index: tip/arch/x86/kernel/apic.c
> ===================================================================
> --- tip.orig/arch/x86/kernel/apic.c     2008-10-09 17:12:39.000000000 -0700
> +++ tip/arch/x86/kernel/apic.c  2008-10-09 17:13:29.000000000 -0700
> @@ -332,10 +332,12 @@ int lapic_get_maxlvt(void)
>  */
>
>  /* Clock divisor */
> -#ifdef CONFG_X86_64
> +#ifdef CONFIG_X86_64
>  #define APIC_DIVISOR 1
> +#define APIC_TDR_DIV APIC_TDR_DIV_1
>  #else
>  #define APIC_DIVISOR 16
> +#define APIC_TDR_DIV APIC_TDR_DIV_16
>  #endif
>
>  /*
> @@ -369,7 +371,7 @@ static void __setup_APIC_LVTT(unsigned i
>        tmp_value = apic_read(APIC_TDCR);
>        apic_write(APIC_TDCR,
>                (tmp_value & ~(APIC_TDR_DIV_1 | APIC_TDR_DIV_TMBASE)) |
> -               APIC_TDR_DIV_16);
> +               APIC_TDR_DIV);
>
>        if (!oneshot)
>                apic_write(APIC_TMICT, clocks / APIC_DIVISOR);
>

Hi Venki,

you know I don't have code under my hands now to take a look on
(i'm in office now) -- but if I recall correctly it's not a typo :) By
writting (1) instead of (16) you make counter decrease faster by
16 times (if to compare with what we have now). So we could get
counter underflow before we've finish calibration. That is was the
original intension of this 16 divisor being used. Did you test this
patch?

RE: [PATCH] Typo in x86 apic.c with DIVISOR setup

[Pallipadi, Venkatesh]

>-----Original Message-----
>From: Cyrill Gorcunov [mailto:gorcunov@gmail.com]
>Sent: Thursday, October 09, 2008 9:38 PM
>To: Pallipadi, Venkatesh
>Cc: Ingo Molnar; Thomas Gleixner; H. Peter Anvin; linux-kernel
>Subject: Re: [PATCH] Typo in x86 apic.c with DIVISOR setup
>
>On Fri, Oct 10, 2008 at 4:22 AM, Venki Pallipadi
><venkatesh.pallipadi@intel.com> wrote:
>>
>> Fix a typo in x86 apic.c CONFG_X86_64 -> CONFIG_X86_64.
>> The intention looks like was to have divisor of 1 on x86_64 kernel.
>>
>> Also, for DIVISOR value of 1 to work, APIC_TDR_DIV should also change
>> depending on divide by 1 or divide by 16 is used.
>>
>> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
>>
>> ---
>>  arch/x86/kernel/apic.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> Index: tip/arch/x86/kernel/apic.c
>> ===================================================================
>> --- tip.orig/arch/x86/kernel/apic.c     2008-10-09
>17:12:39.000000000 -0700
>> +++ tip/arch/x86/kernel/apic.c  2008-10-09 17:13:29.000000000 -0700
>> @@ -332,10 +332,12 @@ int lapic_get_maxlvt(void)
>>  */
>>
>>  /* Clock divisor */
>> -#ifdef CONFG_X86_64
>> +#ifdef CONFIG_X86_64
>>  #define APIC_DIVISOR 1
>> +#define APIC_TDR_DIV APIC_TDR_DIV_1
>>  #else
>>  #define APIC_DIVISOR 16
>> +#define APIC_TDR_DIV APIC_TDR_DIV_16
>>  #endif
>>
>>  /*
>> @@ -369,7 +371,7 @@ static void __setup_APIC_LVTT(unsigned i
>>        tmp_value = apic_read(APIC_TDCR);
>>        apic_write(APIC_TDCR,
>>                (tmp_value & ~(APIC_TDR_DIV_1 |
>APIC_TDR_DIV_TMBASE)) |
>> -               APIC_TDR_DIV_16);
>> +               APIC_TDR_DIV);
>>
>>        if (!oneshot)
>>                apic_write(APIC_TMICT, clocks / APIC_DIVISOR);
>>
>
>Hi Venki,
>
>you know I don't have code under my hands now to take a look on
>(i'm in office now) -- but if I recall correctly it's not a typo :) By
>writting (1) instead of (16) you make counter decrease faster by
>16 times (if to compare with what we have now). So we could get
>counter underflow before we've finish calibration. That is was the
>original intension of this 16 divisor being used. Did you test this
>patch?
>

Typo part I mentioned was this

>> -#ifdef CONFG_X86_64
>> +#ifdef CONFIG_X86_64

Looks like intention was to use DIVISOR of 1 for x86_64 and divisor of
16 for 32 bit kernel. Once I changed that, there was still one place in
64 bit code that was assuming divisor of 16. The rest of the patch
changes that part.

Yes. Tested this patch on couple of systems here and worked fine.

Thanks,
Venki

Re: [PATCH] Typo in x86 apic.c with DIVISOR setup

[Cyrill Gorcunov]

On Fri, Oct 10, 2008 at 9:10 AM, Pallipadi, Venkatesh
<venkatesh.pallipadi@intel.com> wrote:
>
>
>>-----Original Message-----
>>From: Cyrill Gorcunov [mailto:gorcunov@gmail.com]
>>Sent: Thursday, October 09, 2008 9:38 PM
>>To: Pallipadi, Venkatesh
>>Cc: Ingo Molnar; Thomas Gleixner; H. Peter Anvin; linux-kernel
>>Subject: Re: [PATCH] Typo in x86 apic.c with DIVISOR setup
>>
>>On Fri, Oct 10, 2008 at 4:22 AM, Venki Pallipadi
>><venkatesh.pallipadi@intel.com> wrote:
>>>
>>> Fix a typo in x86 apic.c CONFG_X86_64 -> CONFIG_X86_64.
>>> The intention looks like was to have divisor of 1 on x86_64 kernel.
>>>
>>> Also, for DIVISOR value of 1 to work, APIC_TDR_DIV should also change
>>> depending on divide by 1 or divide by 16 is used.
>>>
>>> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
>>>
>>> ---
>>>  arch/x86/kernel/apic.c |    6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> Index: tip/arch/x86/kernel/apic.c
>>> ===================================================================
>>> --- tip.orig/arch/x86/kernel/apic.c     2008-10-09
>>17:12:39.000000000 -0700
>>> +++ tip/arch/x86/kernel/apic.c  2008-10-09 17:13:29.000000000 -0700
>>> @@ -332,10 +332,12 @@ int lapic_get_maxlvt(void)
>>>  */
>>>
>>>  /* Clock divisor */
>>> -#ifdef CONFG_X86_64
>>> +#ifdef CONFIG_X86_64
>>>  #define APIC_DIVISOR 1
>>> +#define APIC_TDR_DIV APIC_TDR_DIV_1
>>>  #else
>>>  #define APIC_DIVISOR 16
>>> +#define APIC_TDR_DIV APIC_TDR_DIV_16
>>>  #endif
>>>
>>>  /*
>>> @@ -369,7 +371,7 @@ static void __setup_APIC_LVTT(unsigned i
>>>        tmp_value = apic_read(APIC_TDCR);
>>>        apic_write(APIC_TDCR,
>>>                (tmp_value & ~(APIC_TDR_DIV_1 |
>>APIC_TDR_DIV_TMBASE)) |
>>> -               APIC_TDR_DIV_16);
>>> +               APIC_TDR_DIV);
>>>
>>>        if (!oneshot)
>>>                apic_write(APIC_TMICT, clocks / APIC_DIVISOR);
>>>
>>
>>Hi Venki,
>>
>>you know I don't have code under my hands now to take a look on
>>(i'm in office now) -- but if I recall correctly it's not a typo :) By
>>writting (1) instead of (16) you make counter decrease faster by
>>16 times (if to compare with what we have now). So we could get
>>counter underflow before we've finish calibration. That is was the
>>original intension of this 16 divisor being used. Did you test this
>>patch?
>>
>
> Typo part I mentioned was this
>
>>> -#ifdef CONFG_X86_64
>>> +#ifdef CONFIG_X86_64
>
> Looks like intention was to use DIVISOR of 1 for x86_64 and divisor of
> 16 for 32 bit kernel. Once I changed that, there was still one place in
> 64 bit code that was assuming divisor of 16. The rest of the patch
> changes that part.
>
> Yes. Tested this patch on couple of systems here and worked fine.
>
> Thanks,
> Venki

Ah, it was CONFG who is guilty :) This is true indeed, thanks!

But here is not the same

       apic_write(APIC_TDCR,
               (tmp_value & ~(APIC_TDR_DIV_1 | APIC_TDR_DIV_TMBASE)) |
-               APIC_TDR_DIV_16);
+               APIC_TDR_DIV);

On x86_64 it will be 1 now but we've used 16 for a long time in purpose
to slowdown processor's bus CLKs from APIC point of view. So I don't think
it's good idea to change it now. If we start using divisor 1 today -- it would
work for some time 'till processor bus raised to the some speed when
we'll get counter underflow before calibration finished.

So could you please split the patch into two:
1) Plain CONFG typo fix (which is completely Ack'ed)
2) APIC divisor patch (which I'm not sure if we've to touch)

Thanks,
Cyrill

Re: [PATCH] Typo in x86 apic.c with DIVISOR setup

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> Ah, it was CONFG who is guilty :) This is true indeed, thanks!

yeah.

> But here is not the same
> 
>        apic_write(APIC_TDCR,
>                (tmp_value & ~(APIC_TDR_DIV_1 | APIC_TDR_DIV_TMBASE)) |
> -               APIC_TDR_DIV_16);
> +               APIC_TDR_DIV);
> 
> On x86_64 it will be 1 now but we've used 16 for a long time in 
> purpose to slowdown processor's bus CLKs from APIC point of view. So I 
> don't think it's good idea to change it now. If we start using divisor 
> 1 today -- it would work for some time 'till processor bus raised to 
> the some speed when we'll get counter underflow before calibration 
> finished.
> 
> So could you please split the patch into two:
> 1) Plain CONFG typo fix (which is completely Ack'ed)
> 2) APIC divisor patch (which I'm not sure if we've to touch)

okay, since we typoed that CONFG thing (sidenote: we really need a 
.config flag that will start a grep that fails the build if there's a 
non-existent CONFIG option anywhere in the tree), we basically tested 
the divisor of 16 on a wide range of boxes.

So .. how about just standardizing on the divisor of 16 on both 32-bit 
and 64-bit?

	Ingo

Re: [PATCH] Typo in x86 apic.c with DIVISOR setup

[Cyrill Gorcunov]

On Fri, Oct 10, 2008 at 1:01 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
>> Ah, it was CONFG who is guilty :) This is true indeed, thanks!
>
> yeah.
>
>> But here is not the same
>>
>>        apic_write(APIC_TDCR,
>>                (tmp_value & ~(APIC_TDR_DIV_1 | APIC_TDR_DIV_TMBASE)) |
>> -               APIC_TDR_DIV_16);
>> +               APIC_TDR_DIV);
>>
>> On x86_64 it will be 1 now but we've used 16 for a long time in
>> purpose to slowdown processor's bus CLKs from APIC point of view. So I
>> don't think it's good idea to change it now. If we start using divisor
>> 1 today -- it would work for some time 'till processor bus raised to
>> the some speed when we'll get counter underflow before calibration
>> finished.
>>
>> So could you please split the patch into two:
>> 1) Plain CONFG typo fix (which is completely Ack'ed)
>> 2) APIC divisor patch (which I'm not sure if we've to touch)
>
> okay, since we typoed that CONFG thing (sidenote: we really need a
> .config flag that will start a grep that fails the build if there's a
> non-existent CONFIG option anywhere in the tree), we basically tested
> the divisor of 16 on a wide range of boxes.
>
> So .. how about just standardizing on the divisor of 16 on both 32-bit
> and 64-bit?
>
>        Ingo
>

I think the best approach would be to use divisor 16 and initial counter value
set to 0xFFFFFFFF while doing APIC timer calibration cycle (iirc there was a
SMI related patch at August had been doing such a thing -- can't remember
the LKML ref to it). Or you meant something different?

Re: [PATCH] Typo in x86 apic.c with DIVISOR setup

[Maciej W. Rozycki]

On Fri, 10 Oct 2008, Ingo Molnar wrote:

> So .. how about just standardizing on the divisor of 16 on both 32-bit 
> and 64-bit?

 I'd second that -- the less code divergence the better.

  Maciej

Re: [PATCH] Typo in x86 apic.c with DIVISOR setup

[Cyrill Gorcunov]

On Fri, Oct 10, 2008 at 1:01 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
>> Ah, it was CONFG who is guilty :) This is true indeed, thanks!
>
> yeah.
>
>> But here is not the same
>>
>>        apic_write(APIC_TDCR,
>>                (tmp_value & ~(APIC_TDR_DIV_1 | APIC_TDR_DIV_TMBASE)) |
>> -               APIC_TDR_DIV_16);
>> +               APIC_TDR_DIV);
>>
>> On x86_64 it will be 1 now but we've used 16 for a long time in
>> purpose to slowdown processor's bus CLKs from APIC point of view. So I
>> don't think it's good idea to change it now. If we start using divisor
>> 1 today -- it would work for some time 'till processor bus raised to
>> the some speed when we'll get counter underflow before calibration
>> finished.
>>
>> So could you please split the patch into two:
>> 1) Plain CONFG typo fix (which is completely Ack'ed)
>> 2) APIC divisor patch (which I'm not sure if we've to touch)
>
> okay, since we typoed that CONFG thing (sidenote: we really need a
> .config flag that will start a grep that fails the build if there's a
> non-existent CONFIG option anywhere in the tree), we basically tested
> the divisor of 16 on a wide range of boxes.
>
> So .. how about just standardizing on the divisor of 16 on both 32-bit
> and 64-bit?
>
>        Ingo
>

Btw I don't remember but hope it was not me who made this typo --
I've been trying to bring into kernel CONFIX variable one day (which
was typo too) :-)

Re: [PATCH] Typo in x86 apic.c with DIVISOR setup

[Maciej W. Rozycki]

On Fri, 10 Oct 2008, Cyrill Gorcunov wrote:

> Btw I don't remember but hope it was not me who made this typo --

 What would be the difference so that you hope it wasn't you?  All the 
people make mistakes, this one has been caught and that's what really 
matters.

  Maciej

Re: [PATCH] Typo in x86 apic.c with DIVISOR setup

[Ingo Molnar]

* Maciej W. Rozycki <macro@linux-mips.org> wrote:

> On Fri, 10 Oct 2008, Cyrill Gorcunov wrote:
> 
> > Btw I don't remember but hope it was not me who made this typo --
> 
>  What would be the difference so that you hope it wasn't you?  All the 
> people make mistakes, this one has been caught and that's what really 
> matters.

yeah, like most kernel newbies Cyrill is worrying way too much about 
mistakes. In 10 years time he too will have his healthy collection of 
embarrassing brown paperbag incidents and will be a lot more relaxed 
about it, just like the rest of us ;-)

what matters is to fix them quickly and to learn from those mistakes 
that are avoidable.

	Ingo

Re: [PATCH] Typo in x86 apic.c with DIVISOR setup

[Cyrill Gorcunov]

[Ingo Molnar - Fri, Oct 10, 2008 at 01:56:55PM +0200]
| 
| * Maciej W. Rozycki <macro@linux-mips.org> wrote:
| 
| > On Fri, 10 Oct 2008, Cyrill Gorcunov wrote:
| > 
| > > Btw I don't remember but hope it was not me who made this typo --
| > 
| >  What would be the difference so that you hope it wasn't you?  All the 
| > people make mistakes, this one has been caught and that's what really 
| > matters.
| 
| yeah, like most kernel newbies Cyrill is worrying way too much about 
| mistakes. In 10 years time he too will have his healthy collection of 
| embarrassing brown paperbag incidents and will be a lot more relaxed 
| about it, just like the rest of us ;-)
| 
| what matters is to fix them quickly and to learn from those mistakes 
| that are avoidable.
| 
| 	Ingo
| 

Just brought first paperbag -- usefull thing I've to say :)
...

ok, what the conclusion will be on divisor side? I think
something like that maybe. Maciej?

		- Cyrill -

---

Index: linux-2.6.git/arch/x86/kernel/apic.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic.c	2008-09-26 20:43:47.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/apic.c	2008-10-10 16:37:26.000000000 +0400
@@ -332,11 +332,7 @@ int lapic_get_maxlvt(void)
  */
 
 /* Clock divisor */
-#ifdef CONFG_X86_64
-#define APIC_DIVISOR 1
-#else
 #define APIC_DIVISOR 16
-#endif
 
 /*
  * This function sets up the local APIC timer, with a timeout of
@@ -592,10 +588,10 @@ static int __init calibrate_APIC_clock(v
 	global_clock_event->event_handler = lapic_cal_handler;
 
 	/*
-	 * Setup the APIC counter to 1e9. There is no way the lapic
+	 * Setup the APIC counter to maximum. There is no way the lapic
 	 * can underflow in the 100ms detection time frame
 	 */
-	__setup_APIC_LVTT(1000000000, 0, 0);
+	__setup_APIC_LVTT(0xffffffff, 0, 0);
 
 	/* Let the interrupts run */
 	local_irq_enable();

Re: [PATCH] Typo in x86 apic.c with DIVISOR setup

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> ok, what the conclusion will be on divisor side? I think
> something like that maybe. Maciej?

looks good to me - could you please send it with a proper subject line 
and changelog, etc?

	Ingo

Re: [PATCH] Typo in x86 apic.c with DIVISOR setup

[Cyrill Gorcunov]

[Maciej W. Rozycki - Fri, Oct 10, 2008 at 12:24:35PM +0100]
| On Fri, 10 Oct 2008, Cyrill Gorcunov wrote:
| 
| > Btw I don't remember but hope it was not me who made this typo --
| 
|  What would be the difference so that you hope it wasn't you?  All the 
| people make mistakes, this one has been caught and that's what really 
| matters.
| 
|   Maciej
| 

Well this CONFG typo just remind me one I brought once :)

		- Cyrill -

RE: [PATCH] Typo in x86 apic.c with DIVISOR setup

[Pallipadi, Venkatesh]

>-----Original Message-----
>From: Cyrill Gorcunov [mailto:gorcunov@gmail.com]
>Sent: Thursday, October 09, 2008 10:39 PM
>To: Pallipadi, Venkatesh
>Cc: Ingo Molnar; Thomas Gleixner; H. Peter Anvin; linux-kernel
>Subject: Re: [PATCH] Typo in x86 apic.c with DIVISOR setup
>
>
>But here is not the same
>
>       apic_write(APIC_TDCR,
>               (tmp_value & ~(APIC_TDR_DIV_1 | APIC_TDR_DIV_TMBASE)) |
>-               APIC_TDR_DIV_16);
>+               APIC_TDR_DIV);
>
>On x86_64 it will be 1 now but we've used 16 for a long time in purpose
>to slowdown processor's bus CLKs from APIC point of view. So I
>don't think
>it's good idea to change it now. If we start using divisor 1
>today -- it would
>work for some time 'till processor bus raised to the some speed when
>we'll get counter underflow before calibration finished.
>

I understand that using 1 instead of 16 makes APIC timer count 16 times faster.
I did that change as I thought that was the intention of making APIC_DIVISOR as 1.
Otherwise, there is no real impact of APIC_DIVISOR #define from being there. It
Is just changing the value of calibration_result across 32 bit and 64 bit kernels.

For example, "host bus clock speed" message will print different values across
32 bit and 64 bit kernels on the same system, if we just make APIC_DIVISOR
16 for 32 bit and 1 for 64 bit kernel.


Thanks,
Venki

RE: [PATCH] Typo in x86 apic.c with DIVISOR setup

[Maciej W. Rozycki]

On Fri, 10 Oct 2008, Pallipadi, Venkatesh wrote:

> I understand that using 1 instead of 16 makes APIC timer count 16 times faster.
> I did that change as I thought that was the intention of making APIC_DIVISOR as 1.
> Otherwise, there is no real impact of APIC_DIVISOR #define from being there. It
> Is just changing the value of calibration_result across 32 bit and 64 bit kernels.

 To use the value of 1 for the predivisor, special arrangements would have 
to be made for the discrete 82489DX APIC.  And we want to keep code 
variations as little as possible, especially when uncommon configurations 
are considered.  The value of 16 is supported universally with no special 
cases.

> For example, "host bus clock speed" message will print different values across
> 32 bit and 64 bit kernels on the same system, if we just make APIC_DIVISOR
> 16 for 32 bit and 1 for 64 bit kernel.

 All places have to agree on the value of the predivisor of course -- if 
you can see a problem somewhere, then please do send a patch.

  Maciej