[Solved] IRQ Firing Bug

[Octacone]

Hello World. I decided to start fresh and recode my entire OS, since the previous version was really bad.
Now, I got GDT IDT set up and ready, they all work just fine. I made some proper IRQ/ISR handlers inside assembly and I know they work because I created a simple BSOD that displays isr exception number and error info, they all work fine (tested with division by zero). My ISR and IRQ are all set and installed.

Now here is one really annoying bug. I tried to code a simple keyboard driver, I always get a message saying that my keyboard handler is getting installed. That is all fine, I can install IRQ handlers. But I never get an expected message from my irq handler saying that some specific interrupt occurred. Now the thing is when I enable my timer or I use while(1) I get like 34907534098 messages saying that interrupt occurred, and while seeing that messages if I press any key I also see that my keyboard interrupt happened but I can see that only once, while timer interrupt keep occurring.

[SpyderTL]

Any chance you can post some code?

[Octacone]

Any chance you can post some code?

Posted.

[Octacone]

kernel.c ->firing sequence:

Code: Select all

                InstallGDT();
		PIC_Remap(0x20,0x28);
		for(int i = 15; i < 15; i++)
		{
			IRQ_Set_Mask(i);
		}
		InstallKeyboard();
		InstallIDT(); //all isrs and irqs are getting set in there and ofc calling flush idt function from asm
		for(int i = 15; i < 15; i++)
		{
			IRQ_Clear_Mask(i);
		}

how I handle my irqs: inside irq.c file

Code: Select all

void IRQ_Handler(struct regs *r)
{
	Disable_Interrupts();
	void (*handler)(struct regs *r);
	handler = irq_routines[r->int_no - 32];
	if (handler)
	{
		handler(r);
		Resume_Interrupts();
	}
	if (r->int_no >= 40)
	{
		outportb(0xA0, 0x20);
	}
	outportb(0x20, 0x20);
	PIC_SendEOI(r->int_no);
	PrintString("\nIRQ_Handler Called from asm with interupt number ");
	PrintString(IntToString(r->int_no));
}

how I install handlers: inside irq.c

Code: Select all

void IRQ_Install_Handler(int irq, void (*handler)(struct regs *r))
{
	SYNC_CLI();
	irq_routines[irq] = handler;
	SYNC_STI();
	PrintString("IRQ install handler call for this irq");
	PrintString(IntToString(irq));
}

how I set my handlers from drivers for e.g keyboard

Code: Select all

void InstallKeyboard()
{
	IRQ_Install_Handler(33, Keyboard_Handler);
	PIC_SendEOI(1);
	KeyboardSucceeded();
}

assembly irq stub:

Code: Select all

extern IRQ_Handler
IRQ_Common_Stub:
        pusha
        push ds
        push es
        push fs
        push gs
        mov ax, 0x10
        mov ds, ax
        mov es, ax
        mov fs, ax
        mov gs, ax
        mov eax, esp
        push eax
        mov eax, IRQ_Handler
        call eax
        pop eax
        pop gs
        pop fs
        pop es
        pop ds
        popa
        add esp, 8
        iret

[Ch4ozz]

Is "Resume_Interrupts();" calling "sti" ?
You should never call STI inside of your IRQ handler because when you call IRET it will restore the flags (also interrupt flag)
I already had problems with nested interrupts with my syscalls.

[Octacone]

Is "Resume_Interrupts();" calling "sti" ?
You should never call STI inside of your IRQ handler because when you call IRET it will restore the flags (also interrupt flag)
I already had problems with nested interrupts with my syscalls.

Here is what "Resume_Interrupts" does:

Code: Select all

void Resume_Interrupts(void)
{
	if (sync_depth == 0 || sync_depth == 1) 
	{
		SYNC_STI(); //aka asm volatile("sti");
	}
	 else 
	 {
		sync_depth--;
	}
}

[BrightLight]

Like already mentioned, CLI/STI inside an interrupt handler is absolutely useless.
If your timer interrupt gets called a lot, and your keyboard interrupt only once, you're either not sending EOI or reading the scancode. You actually need to read the scancode from the keyboard if you want it to send more IRQs.

Code: Select all

scancode = inb(0x60);

P.S.: The same goes for the mouse, when you want a mouse driver.

[Octacone]

Like already mentioned, CLI/STI inside an interrupt handler is absolutely useless.
If your timer interrupt gets called a lot, and your keyboard interrupt only once, you're either not sending EOI or reading the scancode. You actually need to read the scancode from the keyboard if you want it to send more IRQs.

Code: Select all

scancode = inb(0x60);

P.S.: The same goes for the mouse, when you want a mouse driver.

Hmm... Interesting, because I've seen many people using sti/cli inside their interrupt handlers. (on github)
Btw I already have that scancode mechanism:

Code: Select all

void Keyboard_Handler(struct regs *r)
{
	unsigned char scancode;
	scancode = inportb(0x60);
	if (scancode & 0x80)
    	{
        		//use this to see if user released any control keys aka shift ctrl alt altgr
    	}
    	else
    	{
		PrintString(kbdus[scancode]);
		WriteOutputToQemu(kbdus[scancode]);
    	}
    	PrintString("keyboard handler called from irq");
}

[BrightLight]

Hmm... Interesting, because I've seen many people using sti/cli inside their interrupt handlers. (on github)

Because people do it doesn't mean it's right.
Can we see your keyboard setup code? You should reset the keyboard, set repeat rate, and enable make/break codes before unmasking the IRQ. You never know how the BIOS was using the keyboard, and thus should never assume anything.

[Octacone]

Hmm... Interesting, because I've seen many people using sti/cli inside their interrupt handlers. (on github)

Because people do it doesn't mean it's right.
Can we see your keyboard setup code? You should reset the keyboard, set repeat rate, and enable make/break codes before unmasking the IRQ. You never know how the BIOS was using the keyboard, and thus should never assume anything.

Lol yeah, people just copy paste and they make the same mistake over and over.

here is my keyboard.c:

Code: Select all

#include "../Include/keyboard.h"

struct regs
{
	unsigned int gs, fs, es, ds;      
	unsigned int edi, esi, ebp, esp, ebx, edx, ecx, eax;  
	unsigned int int_no, err_code;   
	unsigned int eip, cs, eflags, useresp, ss;   
};

unsigned char kbdus[128] =
{
    0,  27, '1', '2', '3', '4', '5', '6', '7', '8',	/* 9 */
  '9', '0', '-', '=', '\b',	/* Backspace */
  '\t',			/* Tab */
  'q', 'w', 'e', 'r',	/* 19 */
  't', 'y', 'u', 'i', 'o', 'p', '[', ']', '\n',	/* Enter key */
    0,			/* 29   - Control */
  'a', 's', 'd', 'f', 'g', 'h', 'j', 'k', 'l', ';',	/* 39 */
 '\'', '`',   0,		/* Left shift */
 '\\', 'z', 'x', 'c', 'v', 'b', 'n',			/* 49 */
  'm', ',', '.', '/',   0,				/* Right shift */
  '*',
    0,	/* Alt */
  ' ',	/* Space bar */
    0,	/* Caps lock */
    0,	/* 59 - F1 key ... > */
    0,   0,   0,   0,   0,   0,   0,   0,
    0,	/* < ... F10 */
    0,	/* 69 - Num lock*/
    0,	/* Scroll Lock */
    0,	/* Home key */
    0,	/* Up Arrow */
    0,	/* Page Up */
  '-',
    0,	/* Left Arrow */
    0,
    0,	/* Right Arrow */
  '+',
    0,	/* 79 - End key*/
    0,	/* Down Arrow */
    0,	/* Page Down */
    0,	/* Insert Key */
    0,	/* Delete Key */
    0,   0,   0,
    0,	/* F11 Key */
    0,	/* F12 Key */
    0,	/* All other keys are undefined */
};		

void KeyboardSucceeded()
{
             PrintString("\nKeyboard Loaded!     ");
             PrintColoredString("[ Passed ]", 11, 0);
             ResetColors();
}

void Keyboard_Handler(struct regs *r)
{
	unsigned char scancode;
	scancode = inportb(0x60);
	if (scancode & 0x80)
    	{
        		//use this to see if user released any control keys aka shift ctrl alt altgr
    	}
    	else
    	{
		PrintString(kbdus[scancode]);
		WriteOutputToQemu(kbdus[scancode]);
    	}
    	PrintString("keyboard handler cllaed from irq");
}

void InstallKeyboard()
{
	IRQ_Install_Handler(33, Keyboard_Handler);
	PIC_SendEOI(1);
	KeyboardSucceeded();
}

[BrightLight]

I don't see your keyboard reset, or even enabling IRQ 1. You assume the BIOS was using IRQ 1, and you should never assume what the firmware does. You should probably do something like this in your initialization sequence:

Code: Select all

void ps2_wait_write()
{
	while(inb(0x64) & 2 == 0);
}

void ps2_wait_read()
{
	while(inb(0x64) & 1 == 0);
}

unsigned char ps2_send(unsigned char data)
{
	ps2_wait_write();
	outb(0x60, data);
	ps2_wait_read();
	return inb(0x60);
}

void ps2_kbd_init()
{
	dprint("[ps2] initializing PS/2 keyboard...\n");

	ps2_send(0xFF);		// reset

	ps2_send(0xF3);		// set autorepeat rate
	ps2_send(0x20);		// 500 milliseconds

	ps2_send(0xF0);		// set scancode
	ps2_send(0x02);		// scancode set 2

	ps2_send(0xFA);		// autorepeat, make/break codes

	ps2_send(0xF4);		// enable keyboard

	ps2_set_kbd_leds(0);

	install_isr(0x31, &irq01_handler);
	pic_unmask(1);
}

[Octacone]

I don't see your keyboard reset, or even enabling IRQ 1. You assume the BIOS was using IRQ 1, and you should never assume what the firmware does. You should probably do something like this in your initialization sequence:

Code: Select all

void ps2_wait_write()
{
	while(inb(0x64) & 2 == 0);
}

void ps2_wait_read()
{
	while(inb(0x64) & 1 == 0);
}

unsigned char ps2_send(unsigned char data)
{
	ps2_wait_write();
	outb(0x60, data);
	ps2_wait_read();
	return inb(0x60);
}

void ps2_kbd_init()
{
	dprint("[ps2] initializing PS/2 keyboard...\n");

	ps2_send(0xFF);		// reset

	ps2_send(0xF3);		// set autorepeat rate
	ps2_send(0x20);		// 500 milliseconds

	ps2_send(0xF0);		// set scancode
	ps2_send(0x02);		// scancode set 2

	ps2_send(0xFA);		// autorepeat, make/break codes

	ps2_send(0xF4);		// enable keyboard

	ps2_set_kbd_leds(0);

	install_isr(0x31, &irq01_handler);
	pic_unmask(1);
}

I am going to port and adapt that code and see if it makes any difference.

[SpyderTL]

how I set my handlers from drivers for e.g keyboard

Code: Select all

void InstallKeyboard()
{
	IRQ_Install_Handler(33, Keyboard_Handler);
	PIC_SendEOI(1);
	KeyboardSucceeded();
}

That does not look right to me...

You should send an EOI to the PIC after every IRQ interrupt. You should not call it when "installing" your keyboard handler.

Try moving PIC_SendEOI(1) inside of your Keyboard_Handler function.

[Octacone]

how I set my handlers from drivers for e.g keyboard

Code: Select all

void InstallKeyboard()
{
	IRQ_Install_Handler(33, Keyboard_Handler);
	PIC_SendEOI(1);
	KeyboardSucceeded();
}

That does not look right to me...

You should send an EOI to the PIC after every IRQ interrupt. You should not call it when "installing" your keyboard handler.

Try moving PIC_SendEOI(1) inside of your Keyboard_Handler function.

Updated my code. Good to know.

[Octacone]

I don't see your keyboard reset, or even enabling IRQ 1. You assume the BIOS was using IRQ 1, and you should never assume what the firmware does. You should probably do something like this in your initialization sequence:

Getting restart loop. --> if I put installKeyboard(); before InstallIDT();
Getting notinhg. --> if I put installKeyboard(); after InstallIDT();

Messages I get: pit timer handler installed and keyboard handler install
only one keyboard interrupt happened
infinite pit interrupts happened (if I enabled my timer_wait(interval);)