Skip to contentSkip to conversation

Fix infinite loop in grub_pit_wait()

PATCH In review Christian Franke 3 participants 5 messages Aug 7, 2008

lore
Share & tools
lore

Christian Franke

author
#0 lore raw

PATCH 1/1 Fix infinite loop in grub_pit_wait()

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 <[email protected]>

* 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.

kern/i386/pit.c +13 -0
index d0a6eda..a3fab26 100644
@@ -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);
 }
#1 lore raw
Quoted 2 lines · Christian Franke

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?

Quoted 10 lines

void
grub_pit_wait (grub_uint16_t tics)
{

kern/i386/pit.c +4 -0
+  /* 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);

kern/i386/pit.c line 53

Quoted 10 lines
kern/i386/pit.c +7 -0
 
+  /* 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!

Signature · 5 lines
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."
author
#2 lore raw
Quoted 9 lines · Robert Millan
Quoted 2 lines · Christian Franke

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
Quoted 10 lines

kern/i386/pit.c line 53

Quoted 5 lines

...

kern/i386/pit.c +2 -0
+  /* 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.

Quoted 3 lines

Thanks!

You're welcome.

Patch committed.

Christian

#3 lore raw
In reply to Christian Franke
Quoted 5 lines · Christian Franke

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

Here is a proposed fix.

Quoted 8 lines

Christian

2008-08-07 Christian Franke <[email protected]>

* 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

Quoted 20 lines
kern/i386/pit.c +6 -0
index d0a6eda..a3fab26 100644
@@ -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);

kern/i386/pit.c line 52

Quoted 10 lines
kern/i386/pit.c +7 -0
   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

author
#4 lore raw
Quoted 10 lines · Marco Gerards

Christian Franke <[email protected]> writes:

...

Quoted 3 lines
	(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

Quoted 8 lines

kern/i386/pit.c line 52

Quoted 5 lines

...

kern/i386/pit.c +3 -0
+
+  /* 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