diff --git a/docs/architecture/devices.md b/docs/architecture/devices.md index ec89831..c496fc7 100644 --- a/docs/architecture/devices.md +++ b/docs/architecture/devices.md @@ -266,6 +266,57 @@ existing mechanisms for keeping that true mechanically rather than by intention. --- +## Interrupts + +A PCI driver connects its interrupt with one call, `pci_irq_attach()`, and +does not choose how. In order of preference: + +1. **MSI**: the device writes a vector to the local APIC. No pin, no routing, + nothing to share. +2. **MSI-X**, but only if the driver passes `PCI_IRQF_MSIX`. Some parts change + how they report interrupt causes in that mode; the 82574 needs its IVAR + programmed, which `e1000` does not do, so it does not ask. +3. **INTx**, on the line named by the device's `INTERRUPT_LINE` register. + +MSI comes first because INTx on real hardware is **the wrong pin**. +`INTERRUPT_LINE` holds the number the firmware gave the 8259. In APIC mode the +device is wired to an IOAPIC input that only ACPI's `_PRT` describes, and +reading `_PRT` takes an AML interpreter, which SchOS does not have. Under QEMU +the two numbers happen to agree, which is why this went unnoticed until the +drivers were read with a real board in mind. + +INTx is therefore backed by the poller in `kernel/dev/irq.c`: + +- A level-triggered line that has had a handler for two seconds and has never + fired gets its handlers called every tick, from a kernel thread with + interrupts off. +- A line that **storms** (an idle ISA input configured active-low reads as + asserted forever) is masked and polled instead of being reported and left + running. + +Every handler already has to cope with being called when its device is idle, +because lines are shared, so a spurious call costs only a register read. +`/proc/interrupts` marks such a line `[polled]`, and an MSI line `msi`. + +The vector layout: + +| Lines | What they are | +|---|---| +| 0–23 | IOAPIC pins | +| 24–29 | MSI vectors, allocated by `irq_alloc_msi()` | +| 30 | LAPIC error (it was 19, which is a PCI pin on Intel chipsets) | +| 31 | spurious | + +Nothing goes as high as vector 64, which is `T_SYSCALL`. + +Tested under QEMU q35 with the root filesystem on a USB stick, in three +configurations: + +- xHCI and e1000e on MSI. +- MSI disabled and the INTx line deliberately misrouted. xHCI took no + interrupts at all, the poller carried every transfer, and the root still + mounted. +- The old INTx path, unchanged on i440fx. --- diff --git a/include/irq.h b/include/irq.h index 47fd1be..5f64c2e 100644 --- a/include/irq.h +++ b/include/irq.h @@ -20,7 +20,16 @@ // whether its own device actually raised the interrupt and do nothing if it // did not. -#define NIRQ 24 // IOAPIC redirection entries on a PC +// Lines 0-23 are the IOAPIC's pins. 24-29 are MSI vectors, which are not pins +// at all -- a device writes the vector number to the local APIC directly -- +// and are dispatched through the same table so that a handler cannot tell +// which kind it is on. 30 and 31 are the LAPIC's error and spurious vectors. +// Everything stops below vector 64, which is T_SYSCALL: a line there would +// let a user-mode `int $0x40` dispatch an interrupt and EOI one that is not. +#define NIRQ_IOAPIC 24 +#define IRQ_MSI_FIRST 24 +#define IRQ_MSI_LAST 29 +#define NIRQ 31 #define NCHAIN 4 // handlers that may share one line typedef void (*irq_handler_t)(void *arg); @@ -41,4 +50,11 @@ int irq_handler_count(int irq); void irq_dispatch(int irq); void irq_init(void); + +// An MSI vector for pci_irq_attach(). -1 when all six are in use. +int irq_alloc_msi(void); +void irq_free_msi(int irq); + +// The thread that polls PCI lines which never interrupt. After sinit(). +void irq_poll_start(void); int irq_info(char *buf, int cap); // /proc/interrupts diff --git a/include/pci.h b/include/pci.h index acaea9e..af2ba31 100644 --- a/include/pci.h +++ b/include/pci.h @@ -74,8 +74,22 @@ struct pci_dev { // The driver itself, so it can be asked to release the device. The name // alone cannot be called. const struct pci_driver *drv; + // How pci_irq_attach() connected the interrupt: PCI_IRQ_INTX, _MSI or + // _MSIX, and on which line. See kernel/dev/pci.c. + uint8 irq_kind; + int irq_line; }; +#define PCI_IRQ_NONE 0 +#define PCI_IRQ_INTX 1 +#define PCI_IRQ_MSI 2 +#define PCI_IRQ_MSIX 3 + +// pci_irq_attach() flags. MSI is always tried first; MSI-X only when the +// driver says it can take it, because some parts change how they report +// interrupt causes once MSI-X is on (the 82574's IVAR, for one). +#define PCI_IRQF_MSIX 0x1 + // One row of a driver's match table. Terminate the table with a row whose // vendor is 0. struct pci_match { @@ -90,6 +104,16 @@ struct pci_match { #include "device.h" enum dev_class pci_class_of(const struct pci_dev *); + +// Connect a device's interrupt to a handler: an MSI vector if the device has +// one, MSI-X if allowed and it has that instead, and its INTx line otherwise. +// Returns the line, or -1 if there is no way to interrupt at all -- which is +// not fatal; a driver with a poll path should say so and carry on. +#include "irq.h" +int pci_irq_attach(struct pci_dev *, irq_handler_t, void *arg, + const char *name, int flags); +void pci_irq_detach(struct pci_dev *, irq_handler_t, void *arg); +const char *pci_irq_kind(const struct pci_dev *); void pci_describe(const struct pci_dev *, char *out, int cap); struct pci_driver { diff --git a/include/traps.h b/include/traps.h index b8f2756..942714a 100644 --- a/include/traps.h +++ b/include/traps.h @@ -30,7 +30,9 @@ #define IRQ_COM1 4 #define IRQ_MOUSE 12 #define IRQ_IDE 14 -#define IRQ_ERROR 19 +// 30, not xv6's 19. Vector T_IRQ0+19 is IOAPIC pin 19, which real chipsets +// use for PCI; the LAPIC error interrupt would have shared it. +#define IRQ_ERROR 30 #define IRQ_SPURIOUS 31 // System call interrupt number for legacy int 0x40 or 0x80 if used diff --git a/kernel/core/main.c b/kernel/core/main.c index edfbce9..74d1ef9 100644 --- a/kernel/core/main.c +++ b/kernel/core/main.c @@ -362,6 +362,9 @@ kmain(uint32 fb_addr) // until mpmain() below enters the scheduler. netinit(); if (boot_verbose) cprintf("verbose boot: netinit done\n"); + // For the same reason: a kernel thread. See irq_poll_thread() for the + // lines it is there for, which on real hardware is most PCI INTx. + irq_poll_start(); // The debugger's own thread, for the same reason netinit's comes after // sinit: kthread_create needs a usable process table. From here on Ctrl-G // on the console answers, with no filesystem mounted and nothing in diff --git a/kernel/dev/irq.c b/kernel/dev/irq.c index 44e9dfc..31e8955 100644 --- a/kernel/dev/irq.c +++ b/kernel/dev/irq.c @@ -20,9 +20,15 @@ static struct { int n; // handlers installed; only ever grows during boot int enabled; int level_low; + int msi; // an MSI vector, not an IOAPIC pin: nothing to route + int polled; // handlers run from irq_poll_thread as well + uint64 delivered; // real interrupts, not polls + uint64 since; // ticks when the first handler was attached uint64 unhandled; } line[NIRQ]; +static int msi_used[NIRQ]; + // Guards registration against itself. The *dispatch* path deliberately does // not take it: an interrupt handler that blocked on a lock held by whoever // was registering would deadlock the CPU it fired on. Instead registration @@ -44,10 +50,10 @@ irq_init(void) } // IRQ_ERROR and IRQ_SPURIOUS are not IOAPIC pins -- they are local APIC -// vectors that xv6 numbered as though they were, so T_IRQ0+19 and T_IRQ0+31 -// are already spoken for. A PCI device routed to pin 19 would deliver on the -// same vector as the LAPIC error interrupt. Refuse it here rather than let it -// be debugged later. +// vectors that xv6 numbered as though they were. The error vector used to be +// 19, which *is* a pin: one of the four an Intel chipset wires its PCI +// interrupts to, so a device there would have been refused. It is 30 now, +// above every pin; see include/traps.h. static int reserved(int irq) { @@ -91,10 +97,13 @@ irq_register(int irq, irq_handler_t fn, void *arg, const char *name, a->count = 0; __sync_synchronize(); // publish the entry before it is reachable line[irq].n++; + if(line[irq].n == 1) + line[irq].since = ticks; if(!line[irq].enabled){ line[irq].level_low = level_low; - ioapicroute(irq, 0, level_low); + if(!line[irq].msi) + ioapicroute(irq, 0, level_low); line[irq].enabled = 1; } @@ -117,7 +126,11 @@ irq_unregister(int irq, irq_handler_t fn, void *arg) // last handler. Compacting the array moves entries that a dispatcher on // another CPU may be walking, so the line has to be quiet for the // duration, and then brought back only if anything is left on it. - ioapicmask(irq); + // + // An MSI vector has no pin to mask. Its caller, pci_irq_detach(), has + // already switched the device's MSI off, which is the same thing. + if(!line[irq].msi) + ioapicmask(irq); line[irq].enabled = 0; for(j = i; j + 1 < line[irq].n; j++) @@ -126,8 +139,12 @@ irq_unregister(int irq, irq_handler_t fn, void *arg) __sync_synchronize(); if(line[irq].n > 0){ - ioapicroute(irq, 0, line[irq].level_low); + if(!line[irq].msi && !line[irq].polled) + ioapicroute(irq, 0, line[irq].level_low); line[irq].enabled = 1; + } else { + line[irq].polled = 0; + line[irq].delivered = 0; } break; } @@ -189,6 +206,17 @@ storm_check(int irq) cprintf("irq %d: handler '%s' is attached\n", irq, line[irq].act[i].name); if(line[irq].n == 0) cprintf("irq %d: no handler is attached at all\n", irq); + + // And stop it. Reporting a storm and carrying on is a machine that spends + // every cycle in this handler -- which is what a PCI line configured on the + // wrong pin does on real hardware, where an idle ISA input read active-low + // is asserted for ever. The handlers move to the poller, so a device that + // was really on this line still works, ten milliseconds at a time. + if(irq < NIRQ_IOAPIC && !line[irq].msi){ + ioapicmask(irq); + line[irq].polled = 1; + cprintf("irq %d: masked; its handlers are polled from now on\n", irq); + } } void @@ -206,6 +234,8 @@ irq_dispatch(int irq) return; } + line[irq].delivered++; + // Every handler on a shared line is offered the interrupt; each one is // responsible for recognising that its device is idle and returning. for(i = 0; i < n; i++){ @@ -267,7 +297,10 @@ irq_info(char *buf, int cap) off = emit(buf, cap, off, "(unhandled)"); if(line[irq].unhandled) off = emit(buf, cap, off, " [spurious]"); - off = emit(buf, cap, off, line[irq].level_low ? " level\n" : " edge\n"); + if(line[irq].polled) + off = emit(buf, cap, off, " [polled]"); + off = emit(buf, cap, off, line[irq].msi ? " msi\n" : + line[irq].level_low ? " level\n" : " edge\n"); } // How many timer interrupts were lost. @@ -314,3 +347,106 @@ irq_handler_count(int irq) return 0; return line[irq].n; } + +// --------------------------------------------------------------------------- +// MSI vectors +// --------------------------------------------------------------------------- + +// A line that is an MSI vector rather than a pin. They are handed out from +// IRQ_MSI_FIRST..IRQ_MSI_LAST, which are vectors the IOAPIC never uses; see +// pci_irq_attach() in kernel/dev/pci.c, which is the only caller. +int +irq_alloc_msi(void) +{ + acquire(&irqlock); + for(int irq = IRQ_MSI_FIRST; irq <= IRQ_MSI_LAST; irq++){ + if(!msi_used[irq] && !reserved(irq)){ + msi_used[irq] = 1; + line[irq].msi = 1; + release(&irqlock); + return irq; + } + } + release(&irqlock); + return -1; +} + +void +irq_free_msi(int irq) +{ + if(irq < IRQ_MSI_FIRST || irq > IRQ_MSI_LAST) + return; + acquire(&irqlock); + if(line[irq].n == 0){ + msi_used[irq] = 0; + line[irq].msi = 0; + } + release(&irqlock); +} + +// --------------------------------------------------------------------------- +// Polling the lines that do not interrupt +// --------------------------------------------------------------------------- +// +// A PCI driver that cannot have MSI learns its line from the device's +// INTERRUPT_LINE register, and on real hardware that is the wrong answer. +// The register holds the number firmware gave the *8259* -- 10, 11 -- and in +// APIC mode the device is wired to an IOAPIC input the firmware only +// describes in ACPI's _PRT, which takes an AML interpreter to read. So the +// pin gets unmasked, the device's real pin stays masked, and nothing arrives. +// Under QEMU the two numbers happen to agree, which is why it never showed. +// +// Until there is an AML interpreter, a level-triggered line that has had a +// handler for two seconds and has not once fired is presumed to be that, +// and its handlers are called from here every tick. Every handler is already +// written to be called when its device has nothing to say -- a shared line +// needs that -- so being called spuriously costs a register read. A storm +// does the same from the other side; see storm_check(). +// +// The moment a real interrupt arrives the line stops being polled, unless it +// was put here for storming. +#define POLL_AFTER (2 * HZ) + +static void +irq_poll_thread(void) +{ + static uchar said[NIRQ]; + + for(;;){ + acquire(&tickslock); + uint start = ticks; + while(ticks == start) + sleep(&ticks, &tickslock); + uint now = ticks; + release(&tickslock); + + for(int irq = 0; irq < NIRQ_IOAPIC; irq++){ + if(line[irq].n == 0 || line[irq].msi || reserved(irq)) + continue; + int poll = line[irq].polled || + (line[irq].level_low && line[irq].delivered == 0 && + now - line[irq].since > POLL_AFTER); + if(!poll) + continue; + if(!said[irq] && !line[irq].polled){ + said[irq] = 1; + kinfo("irq %d: nothing has arrived on it; polling its handlers\n", + irq); + } + // As an interrupt would: on this CPU, with interrupts off. n is read + // once, as irq_dispatch() reads it. + pushcli(); + int n = line[irq].n; + for(int i = 0; i < n; i++) + line[irq].act[i].fn(line[irq].act[i].arg); + popcli(); + } + } +} + +void +irq_poll_start(void) +{ + if(kthread_create(irq_poll_thread, "irqpoll") < 0) + cprintf("irq: cannot start the poller; misrouted PCI lines stay silent\n"); +} diff --git a/kernel/dev/pci.c b/kernel/dev/pci.c index 55721b0..03af6b6 100644 --- a/kernel/dev/pci.c +++ b/kernel/dev/pci.c @@ -24,6 +24,10 @@ #include "sbuf.h" #include "dbg.h" #include "device.h" +#include "traps.h" +#include "irq.h" +#include "memlayout.h" +#include "proc.h" static struct pci_dev devs[NPCI]; static int ndevs; @@ -178,6 +182,135 @@ pci_msi_enable(struct pci_dev *d, int vector, int cpu) return 0; } +// The same for MSI-X, entry 0 only. +// +// MSI-X keeps its address/data pairs in a table inside one of the device's +// own BARs rather than in config space, which is the whole difference. Some +// controllers -- several AMD xHCIs among them -- offer MSI-X and no MSI, and +// without this they had only INTx, which on real hardware is the line that +// does not arrive. +static int +pci_msix_enable(struct pci_dev *d, int vector, int apicid) +{ + uint8 cap = pci_find_cap(d, PCI_CAP_MSIX); + if(cap == 0) + return -1; + + uint32 tbl = pci_read32(d->bus, d->slot, d->func, cap + 4); + int bir = tbl & 7; + uint64 off = tbl & ~7u; + if(bir > 5 || d->bar[bir].base == 0 || d->bar[bir].is_io) + return -1; + + volatile uint32 *t = ioremap(d->bar[bir].base + off, 16); + if(t == 0) + return -1; + + // Enable with the whole function masked, program entry 0, unmask it, then + // drop the function mask: at no point is an entry live with half an + // address in it. The other entries come out of reset masked and stay so. + uint32 dw = pci_read32(d->bus, d->slot, d->func, cap); + pci_write32(d->bus, d->slot, d->func, cap, dw | (3u << 30)); // enable + fmask + t[0] = 0xFEE00000u | ((uint32)apicid << 12); + t[1] = 0; + t[2] = (uint32)vector; + t[3] = 0; // unmask + dw = pci_read32(d->bus, d->slot, d->func, cap); + pci_write32(d->bus, d->slot, d->func, cap, (dw | (1u << 31)) & ~(1u << 30)); + + pci_write32(d->bus, d->slot, d->func, PCI_COMMAND, + pci_read32(d->bus, d->slot, d->func, PCI_COMMAND) | (1 << 10)); + return 0; +} + +// Turn off whichever of MSI or MSI-X is on, and give the device back its INTx +// -- the next driver to claim it may be one that only knows INTx. +static void +pci_msi_off(struct pci_dev *d) +{ + uint8 cap; + uint32 dw; + + if((cap = pci_find_cap(d, PCI_CAP_MSI)) != 0){ + dw = pci_read32(d->bus, d->slot, d->func, cap); + pci_write32(d->bus, d->slot, d->func, cap, dw & ~(1u << 16)); + } + if((cap = pci_find_cap(d, PCI_CAP_MSIX)) != 0){ + dw = pci_read32(d->bus, d->slot, d->func, cap); + pci_write32(d->bus, d->slot, d->func, cap, dw & ~(1u << 31)); + } + pci_write32(d->bus, d->slot, d->func, PCI_COMMAND, + pci_read32(d->bus, d->slot, d->func, PCI_COMMAND) & ~(1u << 10)); +} + +int +pci_irq_attach(struct pci_dev *d, irq_handler_t fn, void *arg, + const char *name, int flags) +{ + // The processor the interrupt goes to, by APIC ID: the boot processor's, + // which is the one IOAPIC lines are routed to as well. + int dest = cpus[0].apicid; + int irq = irq_alloc_msi(); + + if(irq >= 0){ + // Handler first, then the device. An MSI is an edge: one that arrives + // before anything is listening is gone, and an xHCI whose first event + // interrupt is lost will not raise another until it is acknowledged. + if(irq_register(irq, fn, arg, name, 0) == 0){ + int kind = PCI_IRQ_NONE; + if(pci_msi_enable(d, T_IRQ0 + irq, dest) == 0) + kind = PCI_IRQ_MSI; + else if((flags & PCI_IRQF_MSIX) && + pci_msix_enable(d, T_IRQ0 + irq, dest) == 0) + kind = PCI_IRQ_MSIX; + if(kind != PCI_IRQ_NONE){ + d->irq_kind = kind; + d->irq_line = irq; + return irq; + } + irq_unregister(irq, fn, arg); + } + irq_free_msi(irq); + } + + if(d->irq == 0xFF || d->irq >= NIRQ_IOAPIC) + return -1; + if(irq_register(d->irq, fn, arg, name, 1) < 0) + return -1; + d->irq_kind = PCI_IRQ_INTX; + d->irq_line = d->irq; + return d->irq; +} + +void +pci_irq_detach(struct pci_dev *d, irq_handler_t fn, void *arg) +{ + switch(d->irq_kind){ + case PCI_IRQ_MSI: + case PCI_IRQ_MSIX: + // The device first, so nothing is written to a vector being taken away. + pci_msi_off(d); + irq_unregister(d->irq_line, fn, arg); + irq_free_msi(d->irq_line); + break; + case PCI_IRQ_INTX: + irq_unregister(d->irq_line, fn, arg); + break; + } + d->irq_kind = PCI_IRQ_NONE; +} + +const char * +pci_irq_kind(const struct pci_dev *d) +{ + switch(d->irq_kind){ + case PCI_IRQ_MSI: return "msi"; + case PCI_IRQ_MSIX: return "msi-x"; + case PCI_IRQ_INTX: return "intx"; + default: return "none"; + } +} + // --------------------------------------------------------------------------- // Base address registers // --------------------------------------------------------------------------- diff --git a/kernel/drivers/bus/xhci/xhci.c b/kernel/drivers/bus/xhci/xhci.c index 24dfa58..d22ca63 100644 --- a/kernel/drivers/bus/xhci/xhci.c +++ b/kernel/drivers/bus/xhci/xhci.c @@ -1169,7 +1169,7 @@ xhci_dump(void) rd32(x->rt, XHCI_IR0 + IR_IMAN), rd32(x->rt, XHCI_IR0 + IR_IMOD), rd32(x->rt, XHCI_IR0 + IR_ERSTSZ), rd64(x->rt, XHCI_IR0 + IR_ERDP), x->ev.deq, x->ev.cycle); - cprintf(" pci irq %d\n", x->pci->irq); + cprintf(" pci irq %d (%s)\n", x->pci->irq_line, pci_irq_kind(x->pci)); for(p = 1; p <= x->maxports; p++){ uint32 sc = rd32(x->op, XHCI_PORTSC(p)); @@ -1328,17 +1328,26 @@ xhci_probe(struct pci_dev *d) if(x->ndevices == 0) cprintf("xhci: no devices attached\n"); - // Probing is over: from here a bulk transfer has a process behind it and - // can sleep rather than spin. - x->can_sleep = 1; // Now, and only now, turn interrupts on. The order is: claim the line // (which unmasks it and sets level/active-low), clear anything the // controller already considers pending so that it drives INTx low, and then // let it interrupt. The next event is a genuine low-to-high transition, // which is the only thing an interrupt controller can act on. - if(d->irq != 0xFF){ - irq_register(d->irq, xhci_irq, x, "xhci", 1); // PCI: level, active low + // + // MSI or MSI-X when the controller has either, which every xHCI does. On + // real hardware that is not a refinement: the INTx line the config space + // names is the 8259's number, the IOAPIC pin the controller is really on + // is a different one, and an xHCI waiting on INTx waited for ever -- the + // USB keyboard dead and a USB root never mounted. + // + // Probing is over: from here a bulk transfer has a process behind it and + // can sleep rather than spin -- but only if something will wake it. With + // no interrupt at all, transfers go on spinning as they did during probe. + if(pci_irq_attach(d, xhci_irq, x, "xhci", PCI_IRQF_MSIX) < 0){ + cprintf("xhci: no interrupt; transfers will be polled\n"); + } else { + x->can_sleep = 1; wr32(x->op, XHCI_USBSTS, STS_EINT); wr32(x->rt, XHCI_IR0 + IR_IMAN, IMAN_IP | IMAN_IE); diff --git a/kernel/drivers/net/e1000/e1000.c b/kernel/drivers/net/e1000/e1000.c index b0a0e49..2176d32 100644 --- a/kernel/drivers/net/e1000/e1000.c +++ b/kernel/drivers/net/e1000/e1000.c @@ -359,19 +359,20 @@ e1000_probe(struct pci_dev *d) wr(e, E1000_RCTL, E1000_RCTL_EN | E1000_RCTL_BAM | E1000_RCTL_SECRC | E1000_RCTL_SZ_2048 | E1000_RCTL_LBM_NONE); - if (d->irq != 0xFF) - // level_low = 1: this is a PCI interrupt, which is level-triggered and - // active low. Registering it as edge-triggered makes it fire once and then - // wedge, because the device holds the line asserted and no further edge - // ever arrives. See the comment on irq_register in include/irq.h. - irq_register(d->irq, e1000_intr, e, "e1000", 1); + // MSI where the part has it -- the 82574 and the rest of the PCIe + // generation do -- and INTx otherwise. MSI-X is not offered: the 82574 in + // MSI-X mode routes its causes through IVAR, which this driver does not + // program, and its interrupts would go nowhere. If neither works the + // network thread still collects frames on its tick. + pci_irq_attach(d, e1000_intr, e, "e1000", 0); wr(e, E1000_IMS, E1000_ICR_RXT0 | E1000_ICR_RXDMT0 | E1000_ICR_LSC); e->ready = 1; - kinfo("e1000: %x:%x:%x:%x:%x:%x, link %s, irq %d\n", + kinfo("e1000: %x:%x:%x:%x:%x:%x, link %s, irq %d (%s)\n", e->mac[0], e->mac[1], e->mac[2], e->mac[3], e->mac[4], e->mac[5], - (rd(e, E1000_STATUS) & E1000_STATUS_LU) ? "up" : "down", d->irq); + (rd(e, E1000_STATUS) & E1000_STATUS_LU) ? "up" : "down", + d->irq_line, pci_irq_kind(d)); return 0; } @@ -458,8 +459,7 @@ e1000_release(struct pci_dev *d) wr(e, E1000_TCTL, 0); // stop transmitting rd(e, E1000_STATUS); // post the writes - if (d->irq != 0xFF) - irq_unregister(d->irq, e1000_intr, e); + pci_irq_detach(d, e1000_intr, e); // And take the interface out of the stack. // diff --git a/kernel/drivers/net/r8169/r8169.c b/kernel/drivers/net/r8169/r8169.c index 7e11589..5f4c427 100644 --- a/kernel/drivers/net/r8169/r8169.c +++ b/kernel/drivers/net/r8169/r8169.c @@ -468,9 +468,9 @@ r8169_probe(struct pci_dev *d) wr16(r, R_IMR, ISR_ROK | ISR_RER | ISR_TOK | ISR_TER | ISR_RDU | ISR_LINKCHG); - if (d->irq != 0xFF && d->irq < 24) - irq_register(d->irq, r8169_intr, r, "r8169", 1); - else + // MSI first: every 8111 has it, and on the boards these are soldered to it + // is the only interrupt that is sure to arrive. See pci_irq_attach(). + if (pci_irq_attach(d, r8169_intr, r, "r8169", 0) < 0) cprintf("r8169: no usable interrupt line; the link will be polled\n"); r->ready = 1; @@ -487,10 +487,10 @@ r8169_probe(struct pci_dev *d) uint32 xid = (rd32(r, R_TCR) >> 20) & 0xFCF; int bmsr = mii_read(r, 1); - cprintf("r8169: xid %x, %x:%x:%x:%x:%x:%x, irq %d, %s (bmsr %x)\n", + cprintf("r8169: xid %x, %x:%x:%x:%x:%x:%x, irq %d (%s), %s (bmsr %x)\n", xid, r->mac[0], r->mac[1], r->mac[2], r->mac[3], r->mac[4], r->mac[5], - d->irq, + d->irq_line, pci_irq_kind(d), (rd8(r, R_PHYSTATUS) & PHYSTATUS_LINK) ? "link up" : "link down", bmsr); } diff --git a/kernel/drivers/net/rtl8139/rtl8139.c b/kernel/drivers/net/rtl8139/rtl8139.c index bdcecde..086113f 100644 --- a/kernel/drivers/net/rtl8139/rtl8139.c +++ b/kernel/drivers/net/rtl8139/rtl8139.c @@ -360,8 +360,7 @@ rtl8139_probe(struct pci_dev *d) wr16(r, R_IMR, ISR_ROK | ISR_RER | ISR_TOK | ISR_TER | ISR_RXOVW | ISR_FOVW | ISR_LINKCHG); - if (d->irq != 0xFF && d->irq < 24) - irq_register(d->irq, rtl8139_intr, r, "rtl8139", 1); + pci_irq_attach(d, rtl8139_intr, r, "rtl8139", 0); // no MSI on this part r->ready = 1; kinfo("rtl8139: %x:%x:%x:%x:%x:%x, %s, irq %d\n", @@ -405,8 +404,7 @@ rtl8139_release(struct pci_dev *d) wr8(r, R_CR, 0); // receiver and transmitter off release(&r->lock); - if (d->irq != 0xFF && d->irq < 24) - irq_unregister(d->irq, rtl8139_intr, r); + pci_irq_detach(d, rtl8139_intr, r); // Reset, and wait for it, so the chip has truly stopped before its buffers // are reused. The reset bit clears itself when the chip is done.