[PATCH] Fix infinite loop in grub_pit_wait()

[PATCH] Fix infinite loop in grub_pit_wait()

[Christian Franke]

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

grub2 from current SVN hangs if run in VirtualPC, the problem was 
introduced with 'svn diff -r 1779:1780'.

Here is a proposed fix.

Christian

2008-08-07  Christian Franke  <franke@computer.org>

	* kern/i386/pit.c (TIMER2_SPEAKER): New define.
	(TIMER2_GATE): Likewise.
	(grub_pit_wait): Add enable/disable of the timer2 gate
	bit of port 0x61. This fixes a possible infinite loop.



[-- Attachment #2: grub2-pit-gate.patch --]
[-- Type: text/x-diff, Size: 1058 bytes --]

diff --git a/kern/i386/pit.c b/kern/i386/pit.c
index d0a6eda..a3fab26 100644
--- a/kern/i386/pit.c
+++ b/kern/i386/pit.c
@@ -28,13 +28,26 @@
 #define TIMER_ENABLE_LSB	0x20
 #define TIMER_ENABLE_MSB	0x10
 #define TIMER2_LATCH		0x20
+#define TIMER2_SPEAKER		0x02
+#define TIMER2_GATE		0x01
 
 void
 grub_pit_wait (grub_uint16_t tics)
 {
+  /* Disable timer2 gate and speaker.  */
+  grub_outb (grub_inb (TIMER2_REG_LATCH) & ~ (TIMER2_SPEAKER | TIMER2_GATE), TIMER2_REG_LATCH);
+
+  /* Set tics.  */
   grub_outb (TIMER2_SELECT | TIMER_ENABLE_LSB | TIMER_ENABLE_MSB, TIMER_REG_COMMAND);
   grub_outb (tics & 0xff, TIMER2_REG_CONTROL);
   grub_outb (tics >> 8, TIMER2_REG_CONTROL);
 
+  /* Enable timer2 gate, keep speaker disabled.  */
+  grub_outb ((grub_inb (TIMER2_REG_LATCH) & ~ TIMER2_SPEAKER) | TIMER2_GATE, TIMER2_REG_LATCH);
+
+  /* Wait.  */
   while ((grub_inb (TIMER2_REG_LATCH) & TIMER2_LATCH) == 0x00);
+
+  /* Disable timer2 gate and speaker.  */
+  grub_outb (grub_inb (TIMER2_REG_LATCH) & ~ (TIMER2_SPEAKER | TIMER2_GATE), TIMER2_REG_LATCH);
 }

Re: [PATCH] Fix infinite loop in grub_pit_wait()

[Robert Millan]

On Thu, Aug 07, 2008 at 05:32:42PM +0200, Christian Franke wrote:
> grub2 from current SVN hangs if run in VirtualPC, the problem was 
> introduced with 'svn diff -r 1779:1780'.

I'm not familiar with this part of the PIT interface;  why does it hang
only on VirtualPC?

>  void
>  grub_pit_wait (grub_uint16_t tics)
>  {
> +  /* Disable timer2 gate and speaker.  */
> +  grub_outb (grub_inb (TIMER2_REG_LATCH) & ~ (TIMER2_SPEAKER | TIMER2_GATE), TIMER2_REG_LATCH);
> +
> +  /* Set tics.  */
>    grub_outb (TIMER2_SELECT | TIMER_ENABLE_LSB | TIMER_ENABLE_MSB, TIMER_REG_COMMAND);
>    grub_outb (tics & 0xff, TIMER2_REG_CONTROL);
>    grub_outb (tics >> 8, TIMER2_REG_CONTROL);
>  
> +  /* Enable timer2 gate, keep speaker disabled.  */
> +  grub_outb ((grub_inb (TIMER2_REG_LATCH) & ~ TIMER2_SPEAKER) | TIMER2_GATE, TIMER2_REG_LATCH);
> +
> +  /* Wait.  */
>    while ((grub_inb (TIMER2_REG_LATCH) & TIMER2_LATCH) == 0x00);
> +
> +  /* Disable timer2 gate and speaker.  */
> +  grub_outb (grub_inb (TIMER2_REG_LATCH) & ~ (TIMER2_SPEAKER | TIMER2_GATE), TIMER2_REG_LATCH);
>  }

This doesn't AFAICT preserve the existing value of the timer2 gate and speaker.
I assume that is ok?

Thanks!

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Fix infinite loop in grub_pit_wait()

[Marco Gerards]

Christian Franke <Christian.Franke@t-online.de> writes:

> grub2 from current SVN hangs if run in VirtualPC, the problem was
> introduced with 'svn diff -r 1779:1780'.
>
> Here is a proposed fix.
>
> Christian
>
> 2008-08-07  Christian Franke  <franke@computer.org>
>
> 	* kern/i386/pit.c (TIMER2_SPEAKER): New define.
> 	(TIMER2_GATE): Likewise.
> 	(grub_pit_wait): Add enable/disable of the timer2 gate
> 	bit of port 0x61. This fixes a possible infinite loop.

Small nitpick: use a double space

> diff --git a/kern/i386/pit.c b/kern/i386/pit.c
> index d0a6eda..a3fab26 100644
> --- a/kern/i386/pit.c
> +++ b/kern/i386/pit.c
> @@ -28,13 +28,26 @@
>  #define TIMER_ENABLE_LSB	0x20
>  #define TIMER_ENABLE_MSB	0x10
>  #define TIMER2_LATCH		0x20
> +#define TIMER2_SPEAKER		0x02
> +#define TIMER2_GATE		0x01
>  
>  void
>  grub_pit_wait (grub_uint16_t tics)
>  {
> +  /* Disable timer2 gate and speaker.  */
> +  grub_outb (grub_inb (TIMER2_REG_LATCH) & ~ (TIMER2_SPEAKER | TIMER2_GATE), TIMER2_REG_LATCH);
> +
> +  /* Set tics.  */
>    grub_outb (TIMER2_SELECT | TIMER_ENABLE_LSB | TIMER_ENABLE_MSB, TIMER_REG_COMMAND);
>    grub_outb (tics & 0xff, TIMER2_REG_CONTROL);
>    grub_outb (tics >> 8, TIMER2_REG_CONTROL);
>  
> +  /* Enable timer2 gate, keep speaker disabled.  */
> +  grub_outb ((grub_inb (TIMER2_REG_LATCH) & ~ TIMER2_SPEAKER) | TIMER2_GATE, TIMER2_REG_LATCH);
> +
> +  /* Wait.  */
>    while ((grub_inb (TIMER2_REG_LATCH) & TIMER2_LATCH) == 0x00);
> +
> +  /* Disable timer2 gate and speaker.  */
> +  grub_outb (grub_inb (TIMER2_REG_LATCH) & ~ (TIMER2_SPEAKER | TIMER2_GATE), TIMER2_REG_LATCH);
These lines are too long, please break them properly.



--
Marco

Re: [PATCH] Fix infinite loop in grub_pit_wait()

[Christian Franke]

Robert Millan wrote:
> On Thu, Aug 07, 2008 at 05:32:42PM +0200, Christian Franke wrote:
>   
>> grub2 from current SVN hangs if run in VirtualPC, the problem was 
>> introduced with 'svn diff -r 1779:1780'.
>>     
>
> I'm not familiar with this part of the PIT interface;  why does it hang
> only on VirtualPC?
>
>   

Possibly different BIOS init of 0x61? Or different implementation of the 
legacy hardware, which IIRC looks like this:

.                 +-------+     /--------->0x61[5]
.    ...--Clock-->| Timer |     |
.0x61[0]---Gate-->|   2   |-Out-+->+---+
.                 +-------+        | & |-->Speaker
.0x61[1]-------------------------->+---+


Resulting values of "tsc_ticks_per_ms" (AMD 3.2@2Ghz):

.           orig    with patch
VMware:     ~1500    ~2000000
VirtualBox: ~2000000 ~2000000
VirtualPC:  hang     ~2000000


>> ...
>> +  /* Disable timer2 gate and speaker.  */
>> +  grub_outb (grub_inb (TIMER2_REG_LATCH) & ~ (TIMER2_SPEAKER | TIMER2_GATE), TIMER2_REG_LATCH);
>>  }
>>     
>
> This doesn't AFAICT preserve the existing value of the timer2 gate and speaker.
> I assume that is ok?
>
>   

IMO yes. The timer state itself cannot be preserved, so it should be 
safe to leave it disabled. To test, try If BIOS speaker output (echo -e 
"\a") still works.


> Thanks!
>
>   

You're welcome.

Patch committed.

Christian

Re: [PATCH] Fix infinite loop in grub_pit_wait()

[Christian Franke]

Marco Gerards wrote:
> Christian Franke <Christian.Franke@t-online.de> writes:
>
> ...
>> 	(grub_pit_wait): Add enable/disable of the timer2 gate
>> 	bit of port 0x61. This fixes a possible infinite loop.
>>     
>
> Small nitpick: use a double space
>
>   
>> ...
>> +
>> +  /* Disable timer2 gate and speaker.  */
>> +  grub_outb (grub_inb (TIMER2_REG_LATCH) & ~ (TIMER2_SPEAKER | TIMER2_GATE), TIMER2_REG_LATCH);
>>     
> These lines are too long, please break them properly.
>
>   

Included in commit. Thanks.

Christian