diff --git a/Kernel/Arch/i386/CPU.cpp b/Kernel/Arch/i386/CPU.cpp index 03cbdc7e33..16fc4c6b13 100644 --- a/Kernel/Arch/i386/CPU.cpp +++ b/Kernel/Arch/i386/CPU.cpp @@ -384,64 +384,77 @@ static void unimp_trap() GenericInterruptHandler& get_interrupt_handler(u8 interrupt_number) { - VERIFY(s_interrupt_handler[interrupt_number] != nullptr); - return *s_interrupt_handler[interrupt_number]; + auto*& handler_slot = s_interrupt_handler[interrupt_number]; + VERIFY(handler_slot != nullptr); + return *handler_slot; } static void revert_to_unused_handler(u8 interrupt_number) { - new UnhandledInterruptHandler(interrupt_number); + auto handler = new UnhandledInterruptHandler(interrupt_number); + handler->register_interrupt_handler(); } void register_generic_interrupt_handler(u8 interrupt_number, GenericInterruptHandler& handler) { VERIFY(interrupt_number < GENERIC_INTERRUPT_HANDLERS_COUNT); - if (s_interrupt_handler[interrupt_number] != nullptr) { - if (s_interrupt_handler[interrupt_number]->type() == HandlerType::UnhandledInterruptHandler) { - s_interrupt_handler[interrupt_number] = &handler; + auto*& handler_slot = s_interrupt_handler[interrupt_number]; + if (handler_slot != nullptr) { + if (handler_slot->type() == HandlerType::UnhandledInterruptHandler) { + if (handler_slot) { + auto* unhandled_handler = static_cast(handler_slot); + unhandled_handler->unregister_interrupt_handler(); + delete unhandled_handler; + } + handler_slot = &handler; return; } - if (s_interrupt_handler[interrupt_number]->is_shared_handler() && !s_interrupt_handler[interrupt_number]->is_sharing_with_others()) { - VERIFY(s_interrupt_handler[interrupt_number]->type() == HandlerType::SharedIRQHandler); - static_cast(s_interrupt_handler[interrupt_number])->register_handler(handler); + if (handler_slot->is_shared_handler() && !handler_slot->is_sharing_with_others()) { + VERIFY(handler_slot->type() == HandlerType::SharedIRQHandler); + static_cast(handler_slot)->register_handler(handler); return; } - if (!s_interrupt_handler[interrupt_number]->is_shared_handler()) { - if (s_interrupt_handler[interrupt_number]->type() == HandlerType::SpuriousInterruptHandler) { - static_cast(s_interrupt_handler[interrupt_number])->register_handler(handler); + if (!handler_slot->is_shared_handler()) { + if (handler_slot->type() == HandlerType::SpuriousInterruptHandler) { + static_cast(handler_slot)->register_handler(handler); return; } - VERIFY(s_interrupt_handler[interrupt_number]->type() == HandlerType::IRQHandler); - auto& previous_handler = *s_interrupt_handler[interrupt_number]; - s_interrupt_handler[interrupt_number] = nullptr; + VERIFY(handler_slot->type() == HandlerType::IRQHandler); + auto& previous_handler = *handler_slot; + handler_slot = nullptr; SharedIRQHandler::initialize(interrupt_number); - static_cast(s_interrupt_handler[interrupt_number])->register_handler(previous_handler); - static_cast(s_interrupt_handler[interrupt_number])->register_handler(handler); + VERIFY(handler_slot); + static_cast(handler_slot)->register_handler(previous_handler); + static_cast(handler_slot)->register_handler(handler); return; } VERIFY_NOT_REACHED(); } else { - s_interrupt_handler[interrupt_number] = &handler; + handler_slot = &handler; } } void unregister_generic_interrupt_handler(u8 interrupt_number, GenericInterruptHandler& handler) { - VERIFY(s_interrupt_handler[interrupt_number] != nullptr); - if (s_interrupt_handler[interrupt_number]->type() == HandlerType::UnhandledInterruptHandler) { + auto*& handler_slot = s_interrupt_handler[interrupt_number]; + VERIFY(handler_slot != nullptr); + if (handler_slot->type() == HandlerType::UnhandledInterruptHandler) { dbgln("Trying to unregister unused handler (?)"); return; } - if (s_interrupt_handler[interrupt_number]->is_shared_handler() && !s_interrupt_handler[interrupt_number]->is_sharing_with_others()) { - VERIFY(s_interrupt_handler[interrupt_number]->type() == HandlerType::SharedIRQHandler); - static_cast(s_interrupt_handler[interrupt_number])->unregister_handler(handler); - if (!static_cast(s_interrupt_handler[interrupt_number])->sharing_devices_count()) { + if (handler_slot->is_shared_handler() && !handler_slot->is_sharing_with_others()) { + VERIFY(handler_slot->type() == HandlerType::SharedIRQHandler); + auto* shared_handler = static_cast(handler_slot); + shared_handler->unregister_handler(handler); + if (!shared_handler->sharing_devices_count()) { + handler_slot = nullptr; revert_to_unused_handler(interrupt_number); } return; } - if (!s_interrupt_handler[interrupt_number]->is_shared_handler()) { - VERIFY(s_interrupt_handler[interrupt_number]->type() == HandlerType::IRQHandler); + if (!handler_slot->is_shared_handler()) { + VERIFY(handler_slot->type() == HandlerType::IRQHandler); + handler_slot = nullptr; revert_to_unused_handler(interrupt_number); return; } @@ -671,7 +684,8 @@ UNMAP_AFTER_INIT static void idt_init() dbgln("Installing Unhandled Handlers"); for (u8 i = 0; i < GENERIC_INTERRUPT_HANDLERS_COUNT; ++i) { - new UnhandledInterruptHandler(i); + auto* handler = new UnhandledInterruptHandler(i); + handler->register_interrupt_handler(); } flush_idt(); diff --git a/Kernel/Interrupts/APIC.cpp b/Kernel/Interrupts/APIC.cpp index d892ab5fba..82ee0915e4 100644 --- a/Kernel/Interrupts/APIC.cpp +++ b/Kernel/Interrupts/APIC.cpp @@ -87,7 +87,8 @@ public: static void initialize(u8 interrupt_number) { - new APICIPIInterruptHandler(interrupt_number); + auto* handler = new APICIPIInterruptHandler(interrupt_number); + handler->register_interrupt_handler(); } virtual void handle_interrupt(const RegisterState&) override; @@ -117,7 +118,8 @@ public: static void initialize(u8 interrupt_number) { - new APICErrInterruptHandler(interrupt_number); + auto* handler = new APICErrInterruptHandler(interrupt_number); + handler->register_interrupt_handler(); } virtual void handle_interrupt(const RegisterState&) override; diff --git a/Kernel/Interrupts/GenericInterruptHandler.cpp b/Kernel/Interrupts/GenericInterruptHandler.cpp index 97f57ed3ce..878e3757e3 100644 --- a/Kernel/Interrupts/GenericInterruptHandler.cpp +++ b/Kernel/Interrupts/GenericInterruptHandler.cpp @@ -40,25 +40,50 @@ GenericInterruptHandler::GenericInterruptHandler(u8 interrupt_number, bool disab : m_interrupt_number(interrupt_number) , m_disable_remap(disable_remap) { + // NOTE: We cannot register or unregister the handler while the object + // is being constructed or deconstructed! +} + +void GenericInterruptHandler::will_be_destroyed() +{ + // This will be called for RefCounted interrupt handlers before the + // object is being destroyed. As soon as the destructor is invoked + // it is no longer advisable to unregister the handler (which causes + // calls to virtual functions), so let's do this right before + // invoking it + unregister_interrupt_handler(); +} + +void GenericInterruptHandler::register_interrupt_handler() +{ + if (m_registered) + return; if (m_disable_remap) register_generic_interrupt_handler(m_interrupt_number, *this); else register_generic_interrupt_handler(InterruptManagement::acquire_mapped_interrupt_number(m_interrupt_number), *this); + m_registered = true; } -GenericInterruptHandler::~GenericInterruptHandler() +void GenericInterruptHandler::unregister_interrupt_handler() { + if (!m_registered) + return; if (m_disable_remap) unregister_generic_interrupt_handler(m_interrupt_number, *this); else unregister_generic_interrupt_handler(InterruptManagement::acquire_mapped_interrupt_number(m_interrupt_number), *this); + m_registered = false; } void GenericInterruptHandler::change_interrupt_number(u8 number) { VERIFY_INTERRUPTS_DISABLED(); VERIFY(!m_disable_remap); - unregister_generic_interrupt_handler(InterruptManagement::acquire_mapped_interrupt_number(interrupt_number()), *this); + if (m_registered) { + unregister_generic_interrupt_handler(InterruptManagement::acquire_mapped_interrupt_number(interrupt_number()), *this); + m_registered = false; + } m_interrupt_number = number; register_generic_interrupt_handler(InterruptManagement::acquire_mapped_interrupt_number(interrupt_number()), *this); } diff --git a/Kernel/Interrupts/GenericInterruptHandler.h b/Kernel/Interrupts/GenericInterruptHandler.h index e6eed7dd6d..73385e9276 100644 --- a/Kernel/Interrupts/GenericInterruptHandler.h +++ b/Kernel/Interrupts/GenericInterruptHandler.h @@ -43,9 +43,17 @@ enum class HandlerType : u8 { class GenericInterruptHandler { public: static GenericInterruptHandler& from(u8 interrupt_number); - virtual ~GenericInterruptHandler(); + virtual ~GenericInterruptHandler() + { + VERIFY(!m_registered); + } virtual void handle_interrupt(const RegisterState& regs) = 0; + void will_be_destroyed(); + bool is_registered() const { return m_registered; } + void register_interrupt_handler(); + void unregister_interrupt_handler(); + u8 interrupt_number() const { return m_interrupt_number; } size_t get_invoking_count() const { return m_invoking_count; } @@ -74,5 +82,6 @@ private: Atomic m_invoking_count { 0 }; u8 m_interrupt_number { 0 }; bool m_disable_remap { false }; + bool m_registered { false }; }; } diff --git a/Kernel/Interrupts/IRQHandler.cpp b/Kernel/Interrupts/IRQHandler.cpp index 8fd0a5edc8..308eafb11d 100644 --- a/Kernel/Interrupts/IRQHandler.cpp +++ b/Kernel/Interrupts/IRQHandler.cpp @@ -56,6 +56,8 @@ bool IRQHandler::eoi() void IRQHandler::enable_irq() { dbgln_if(IRQ_DEBUG, "Enable IRQ {}", interrupt_number()); + if (!is_registered()) + register_interrupt_handler(); m_enabled = true; if (!m_shared_with_others) m_responsible_irq_controller->enable(*this); diff --git a/Kernel/Interrupts/SharedIRQHandler.cpp b/Kernel/Interrupts/SharedIRQHandler.cpp index 95c1c2eb39..8fc5989335 100644 --- a/Kernel/Interrupts/SharedIRQHandler.cpp +++ b/Kernel/Interrupts/SharedIRQHandler.cpp @@ -36,7 +36,9 @@ namespace Kernel { UNMAP_AFTER_INIT void SharedIRQHandler::initialize(u8 interrupt_number) { - new SharedIRQHandler(interrupt_number); + auto* handler = new SharedIRQHandler(interrupt_number); + handler->register_interrupt_handler(); + handler->disable_interrupt_vector(); } void SharedIRQHandler::register_handler(GenericInterruptHandler& handler) @@ -71,7 +73,6 @@ SharedIRQHandler::SharedIRQHandler(u8 irq) #if INTERRUPT_DEBUG klog() << "Shared Interrupt Handler registered @ " << interrupt_number(); #endif - disable_interrupt_vector(); } SharedIRQHandler::~SharedIRQHandler() diff --git a/Kernel/Interrupts/SpuriousInterruptHandler.cpp b/Kernel/Interrupts/SpuriousInterruptHandler.cpp index 116823c142..9e3f4783cc 100644 --- a/Kernel/Interrupts/SpuriousInterruptHandler.cpp +++ b/Kernel/Interrupts/SpuriousInterruptHandler.cpp @@ -31,7 +31,8 @@ namespace Kernel { UNMAP_AFTER_INIT void SpuriousInterruptHandler::initialize(u8 interrupt_number) { - new SpuriousInterruptHandler(interrupt_number); + auto* handler = new SpuriousInterruptHandler(interrupt_number); + handler->register_interrupt_handler(); } void SpuriousInterruptHandler::register_handler(GenericInterruptHandler& handler) diff --git a/Kernel/Time/APICTimer.cpp b/Kernel/Time/APICTimer.cpp index fb81ba0435..6b68563d5c 100644 --- a/Kernel/Time/APICTimer.cpp +++ b/Kernel/Time/APICTimer.cpp @@ -38,12 +38,12 @@ namespace Kernel { UNMAP_AFTER_INIT APICTimer* APICTimer::initialize(u8 interrupt_number, HardwareTimerBase& calibration_source) { - auto* timer = new APICTimer(interrupt_number, nullptr); + auto timer = adopt(*new APICTimer(interrupt_number, nullptr)); + timer->register_interrupt_handler(); if (!timer->calibrate(calibration_source)) { - delete timer; return nullptr; } - return timer; + return &timer.leak_ref(); } UNMAP_AFTER_INIT APICTimer::APICTimer(u8 interrupt_number, Function callback) diff --git a/Kernel/Time/APICTimer.h b/Kernel/Time/APICTimer.h index 7e9b24e630..8ed4f147e3 100644 --- a/Kernel/Time/APICTimer.h +++ b/Kernel/Time/APICTimer.h @@ -51,6 +51,7 @@ public: virtual bool is_capable_of_frequency(size_t frequency) const override; virtual size_t calculate_nearest_possible_frequency(size_t frequency) const override; + void will_be_destroyed() { HardwareTimer::will_be_destroyed(); } void enable_local_timer(); void disable_local_timer(); diff --git a/Kernel/Time/HPETComparator.cpp b/Kernel/Time/HPETComparator.cpp index 78d438bb19..f47f4dca0d 100644 --- a/Kernel/Time/HPETComparator.cpp +++ b/Kernel/Time/HPETComparator.cpp @@ -33,7 +33,9 @@ namespace Kernel { UNMAP_AFTER_INIT NonnullRefPtr HPETComparator::create(u8 number, u8 irq, bool periodic_capable) { - return adopt(*new HPETComparator(number, irq, periodic_capable)); + auto timer = adopt(*new HPETComparator(number, irq, periodic_capable)); + timer->register_interrupt_handler(); + return timer; } UNMAP_AFTER_INIT HPETComparator::HPETComparator(u8 number, u8 irq, bool periodic_capable) diff --git a/Kernel/Time/HardwareTimer.h b/Kernel/Time/HardwareTimer.h index c5998e1cb9..5790cf97ab 100644 --- a/Kernel/Time/HardwareTimer.h +++ b/Kernel/Time/HardwareTimer.h @@ -49,6 +49,12 @@ class HardwareTimerBase public: virtual ~HardwareTimerBase() { } + // We need to create a virtual will_be_destroyed here because we derive + // from RefCounted here, which means that RefCounted<> + // will only call will_be_destroyed if we define it here. The derived + // classes then should forward this to e.g. GenericInterruptHandler. + virtual void will_be_destroyed() = 0; + virtual const char* model() const = 0; virtual HardwareTimerType timer_type() const = 0; virtual Function set_callback(Function) = 0; @@ -73,6 +79,11 @@ class HardwareTimer : public HardwareTimerBase , public IRQHandler { public: + virtual void will_be_destroyed() override + { + IRQHandler::will_be_destroyed(); + } + virtual const char* purpose() const override { if (TimeManagement::the().is_system_timer(*this)) @@ -115,6 +126,11 @@ class HardwareTimer : public HardwareTimerBase , public GenericInterruptHandler { public: + virtual void will_be_destroyed() override + { + GenericInterruptHandler::will_be_destroyed(); + } + virtual const char* purpose() const override { return model();