Re: [tyndur-devel] [PATCH] Mehrere ISRs pro IRQ

[Kevin Wolf <kevin@xxxxxxxxxx>]

On Sun, Aug 30, 2009 at 01:53:22PM +0200, max@xxxxxxxxxx wrote:
> From: Max Reitz <max@xxxxxxxxxx>
> 
> Signed-off-by: Max Reitz <max@xxxxxxxxxx>
> ---
>  src/kernel/src/intr.c |   76 +++++++++++++++++++++++++++++++------------------
>  1 files changed, 48 insertions(+), 28 deletions(-)
> 
> diff --git a/src/kernel/src/intr.c b/src/kernel/src/intr.c
> index 9fa3ff3..be90e8c 100644
> --- a/src/kernel/src/intr.c
> +++ b/src/kernel/src/intr.c
> @@ -38,6 +38,7 @@
>   */
>  
>  #include "cpu.h"
> +#include <collections.h>
>  #include "ports.h"
>  #include "types.h"
>  #include "kernel.h"
> @@ -65,8 +66,8 @@ typedef struct {
>  
>  gate_descriptor idt[IDT_SIZE];
>  
> -pfIrqHandler irq_handlers[16] = { 0 }; // f?r jeden IRQ genau ein Handler. Wenn es keinen Handler gibt, ist der entsprechende Wert 0.
> -struct task * intr_handling_task[256] = { NULL };
> +static list_t* irq_handlers[16] = { NULL };
> +list_t* intr_handling_task[256] = { NULL };

Der kann auch static sein, wenn wir schon dabei sind, oder?

>  
>  void handle_irq(int irq, dword* esp);
>  
> @@ -120,7 +121,7 @@ dword irqs_to_send[16];
>  extern dword kernel_pd_id;
>  
>  /*
> - * gibt einen neuen Wert f?r esp zurck
> + * gibt einen neuen Wert fuer esp zurck
>   */
>  dword handle_int(dword esp)
>  {
> @@ -146,7 +147,7 @@ dword handle_int(dword esp)
>      }
>      else
>      {
> -        // TODO: Abst?rzen, denn diese Interrupts d?rften nicht aufgerufen werden
> +        // TODO: Abstuerzen, denn diese Interrupts duerften nicht aufgerufen werden
>      }
>  
>      send_irqs(&esp);
> @@ -186,11 +187,11 @@ dword handle_int(dword esp)
>  
>  /**
>   * Legt eine IDT an und installiert die Exception Handler. Nicht genutzte
> - * Eintr?ge werden mit Verweisen auf den Null-Handler initalisiert.
> - * Anschlie?end wird die IDT geladen.
> + * Eintraege werden mit Verweisen auf den Null-Handler initalisiert.
> + * Anschliessend wird die IDT geladen.
>   */
>  void init_idt() {
> -  int i;
> +   int i;

Die Einrückung ist vorher und nachher nicht korrekt. ;-)

>  
>    // Tabelle initialisieren
>    for (i = 0; i < IDT_SIZE; i++)
> @@ -198,7 +199,7 @@ void init_idt() {
>      set_intr(i, 0x08, &null_handler, 0, IDT_INTERRUPT_GATE);
>    }
>  
> -  // Register befllen
> +  // Register befuellen
>    struct {
>      word size;
>      dword base;
> @@ -265,9 +266,9 @@ void init_idt() {
>    outb_wait(PIC1_DATA, 0x00);
>    outb_wait(PIC2_DATA, 0x00);
>  
> -  // Handler f?r den System Call installieren
> +  // Handler fuer den System Call installieren
>    // TODO: SOllte eigentlich ein IDT_TRAP_GATE sein, aber irgendwo ist da 
> -  // noch ein Problem, das dann f?r einen Page Fault sorgt, wenn ich zu
> +  // noch ein Problem, das dann fuer einen Page Fault sorgt, wenn ich zu
>    // schnell tippe...
>    set_intr(SYSCALL_INTERRUPT, 0x08, syscall_stub, 3, IDT_INTERRUPT_GATE);
>    //set_intr(SYSCALL_INTERRUPT, 0x08, syscall_stub, 3, IDT_TRAP_GATE);
> @@ -323,26 +324,29 @@ int request_irq(int irq, void * handler)
>          return 1;
>      }
>  
> -    if(irq_handlers[irq] != 0)
> -    {
> -        return 1;
> +    if (irq_handlers[irq] == NULL) {
> +        irq_handlers[irq] = list_create();
>      }
>  
> -    irq_handlers[irq] = handler;
> +    list_push(irq_handlers[irq], handler);
>  
>      return 0;
>  }
>  
>  int release_irq(int irq)
>  {
> -    if(irq < 0 || irq > 15)
> +    /*if(irq < 0 || irq > 15)
>      {
>          return 1;
>      }
>  
>      irq_handlers[irq] = 0;
>  
> -    return 0;
> +    return 0;*/
> +
> +    //FIXME Ich weiss ja nicht, wer das braucht. Aber wenn es noetig ist, dann
> +    //muesste man das in release_irq(int irq, void* handler) oder sowas aendern
> +    return 1;
>  }

Dann änder den Prototypen eben ab. FIXMEs für triviale Sachen mag ich
nicht so.

>  
>  
> @@ -361,7 +365,7 @@ void handle_irq(int irq, dword* esp)
>          }
>      }*/
>          
> -    // IRQs 7 und 15 k?nnen unabsichtlich aufgerufen werden
> +    // IRQs 7 und 15 koennen unabsichtlich aufgerufen werden
>      // In diesem Fall beim PIC nachschauen, ob wirklich was zu verarbeiten 
>      // ist, ansonsten gleich wieder abbrechen.
>      if ((irq == 7) || (irq == 15))
> @@ -386,9 +390,16 @@ void handle_irq(int irq, dword* esp)
>          //printf("disable IRQ %d\n", irq);
>          disable_irq(irq);
>      } 
> -    else if (irq_handlers[irq] != 0)
> +    else if (irq_handlers[irq] != NULL)

else if ist jetzt nicht mehr richtig. Es könnte sowohl für den Kernel
als auch für ein Modul ein Handler installiert sein.


>      {
> -        irq_handlers[irq](irq, esp);
> +        int i;
> +        pfIrqHandler handler;
> +
> +        for (i = 0; (handler = list_get_element_at(irq_handlers[irq], i))
> +            != NULL; i++)
> +        {
> +            handler(irq, esp);
> +        }
>      }
>  
>  send_eoi:
> @@ -402,7 +413,10 @@ send_eoi:
>  void set_intr_handling_task(byte intr, struct task * task)
>  {
>      //kprintf("Interrupt %d wird jetzt von Task %d behandelt\n", intr, task->pid);
> -    intr_handling_task[intr] = task;
> +    if (intr_handling_task[intr] == NULL) {
> +        intr_handling_task[intr] = list_create();
> +    }
> +    list_push(intr_handling_task[intr], task);
>  }
>  		
>  void handle_exception(dword* esp)
> @@ -426,7 +440,7 @@ void handle_exception(dword* esp)
>          case 14:
>              cr2 = read_cr2();
>              diff = (cr2 >> PAGE_SHIFT) - (isf->esp >> PAGE_SHIFT);
> -            // Ueberpr?fen ob der Pagefault durch einen Stackoverflow
> +            // Ueberpruefen, ob der Pagefault durch einen Stackoverflow
>              // hervorgerufen wurde
>              if ((diff >= -2) && (diff <= 2)
>                  && (current_task != NULL)
> @@ -440,7 +454,7 @@ void handle_exception(dword* esp)
>              break;
>      }
>      
> -    // Pr?fen, ob ein VM86-Task die Exception ausgel?st hat, und evtl reagieren
> +    // Pruefen, ob ein VM86-Task die Exception ausgeloest hat, und evtl reagieren
>      if (isf->eflags & 0x20000) {
>          if (vm86_exception(esp)) {
>              return;
> @@ -512,19 +526,25 @@ void handle_exception(dword* esp)
>  
>  void send_irqs(dword* esp) 
>  {
> +    int i;
> +    struct task* tsk;
>      dword irq;
>      for (irq = 0; irq < 16; irq++)
>      {
>          dword rpc_data = irq + IRQ_BASE;
>          while (irqs_to_send[irq] > 0) {
> -            schedule_to_task(intr_handling_task[irq + IRQ_BASE], esp);
> -            if (!fastrpc_irq(intr_handling_task[irq + IRQ_BASE], 0, 0, 4, 
> -                (char*) &rpc_data, irq)) 
> +            for (i = 0; (tsk = list_get_element_at(
> +                intr_handling_task[irq + IRQ_BASE], i)) != NULL; i++)
>              {
> -                //kprintf("SYSCALL_RPC = FALSE");
> -                break;
> +                schedule_to_task(tsk, esp);
> +                if (!fastrpc_irq(tsk, 0, 0, 4, 
> +                     (char*) &rpc_data, irq)) 

Der Zeilenumbruch kommt mir überflüssig vor

> +                {
> +                    //kprintf("SYSCALL_RPC = FALSE");
> +                    break;

Wenn der zweite IRQ-Handler beim ersten Versuch scheitert, wird der
erste Handler zweimal aufgerufen. Bin mir aber nicht sicher, ob es sich
lohnt, das zu fixen - die Treiber müssen eh damit umgehen können, wenn
zu viele Interrupts kommen.