Browse Source

Kernel: Decouple Intel HDA interrupt handling from controller

The driver would crash if it was unable to find an output route, and
subsequently the destruction of controller did not invoke
`GenericInterruptHandler::will_be_destroyed()` because on the level of
`AudioController`, that method is unavailable.

By decoupling the interrupt handling from the controller, we get a new
refcounted class that correctly cleans up after itself :^)
Jelle Raaijmakers 2 years ago
parent
commit
859ac200b7

+ 1 - 0
Kernel/CMakeLists.txt

@@ -47,6 +47,7 @@ set(KERNEL_SOURCES
     Devices/Audio/IntelHDA/Codec.cpp
     Devices/Audio/IntelHDA/Controller.cpp
     Devices/Audio/IntelHDA/Format.cpp
+    Devices/Audio/IntelHDA/InterruptHandler.cpp
     Devices/Audio/IntelHDA/Stream.cpp
     Devices/Audio/Management.cpp
     Devices/BlockDevice.cpp

+ 5 - 8
Kernel/Devices/Audio/IntelHDA/Controller.cpp

@@ -10,6 +10,7 @@
 #include <Kernel/Arch/Delay.h>
 #include <Kernel/Bus/PCI/API.h>
 #include <Kernel/Devices/Audio/IntelHDA/Codec.h>
+#include <Kernel/Devices/Audio/IntelHDA/InterruptHandler.h>
 #include <Kernel/Devices/Audio/IntelHDA/Stream.h>
 #include <Kernel/Devices/Audio/IntelHDA/Timing.h>
 #include <Kernel/Time/TimeManagement.h>
@@ -30,7 +31,6 @@ UNMAP_AFTER_INIT ErrorOr<NonnullRefPtr<AudioController>> Controller::create(PCI:
 
 UNMAP_AFTER_INIT Controller::Controller(PCI::DeviceIdentifier const& pci_device_identifier, NonnullOwnPtr<IOWindow> controller_io_window)
     : PCI::Device(const_cast<PCI::DeviceIdentifier&>(pci_device_identifier))
-    , PCIIRQHandler(*this, device_identifier().interrupt_line().value())
     , m_controller_io_window(move(controller_io_window))
 {
 }
@@ -39,7 +39,7 @@ UNMAP_AFTER_INIT ErrorOr<void> Controller::initialize(Badge<AudioManagement>)
 {
     // Enable DMA and interrupts
     PCI::enable_bus_mastering(device_identifier());
-    enable_irq();
+    m_interrupt_handler = TRY(InterruptHandler::create(*this));
 
     // 3.3.3, 3.3.4: Controller version
     auto version_minor = m_controller_io_window->read8(ControllerRegister::VersionMinor);
@@ -299,7 +299,7 @@ ErrorOr<void> Controller::reset()
     return {};
 }
 
-bool Controller::handle_irq(Kernel::RegisterState const&)
+ErrorOr<bool> Controller::handle_interrupt(Badge<InterruptHandler>)
 {
     // Check if any interrupt status bit is set
     auto interrupt_status = m_controller_io_window->read32(ControllerRegister::InterruptStatus);
@@ -308,11 +308,8 @@ bool Controller::handle_irq(Kernel::RegisterState const&)
 
     // FIXME: Actually look at interrupt_status and iterate over streams as soon as
     //        we support multiple streams.
-    if (m_output_path) {
-        auto maybe_error = m_output_path->output_stream().handle_interrupt({});
-        if (maybe_error.is_error())
-            dbgln("IntelHDA: Error during interrupt handling: {}", maybe_error.error());
-    }
+    if (m_output_path)
+        TRY(m_output_path->output_stream().handle_interrupt({}));
 
     return true;
 }

+ 4 - 9
Kernel/Devices/Audio/IntelHDA/Controller.h

@@ -14,9 +14,9 @@
 #include <Kernel/Bus/PCI/Device.h>
 #include <Kernel/Devices/Audio/Channel.h>
 #include <Kernel/Devices/Audio/Controller.h>
+#include <Kernel/Devices/Audio/IntelHDA/InterruptHandler.h>
 #include <Kernel/Devices/Audio/IntelHDA/OutputPath.h>
 #include <Kernel/Devices/Audio/IntelHDA/RingBuffer.h>
-#include <Kernel/Interrupts/PCIIRQHandler.h>
 #include <Kernel/Library/IOWindow.h>
 
 namespace Kernel::Audio::IntelHDA {
@@ -27,8 +27,7 @@ class Codec;
 
 class Controller final
     : public AudioController
-    , public PCI::Device
-    , public PCIIRQHandler {
+    , public PCI::Device {
 public:
     static ErrorOr<bool> probe(PCI::DeviceIdentifier const&);
     static ErrorOr<NonnullRefPtr<AudioController>> create(PCI::DeviceIdentifier const&);
@@ -37,9 +36,7 @@ public:
     // ^PCI::Device
     virtual StringView device_name() const override { return "IntelHDA"sv; }
 
-    // ^PCIIRQHandler
-    virtual StringView purpose() const override { return "IntelHDA IRQ Handler"sv; }
-
+    ErrorOr<bool> handle_interrupt(Badge<InterruptHandler>);
     ErrorOr<u32> send_command(u8 codec_address, u8 node_id, CodecControlVerb verb, u16 payload);
 
 private:
@@ -81,9 +78,6 @@ private:
     ErrorOr<void> configure_output_route();
     ErrorOr<void> reset();
 
-    // ^PCIIRQHandler
-    virtual bool handle_irq(RegisterState const&) override;
-
     // ^AudioController
     virtual RefPtr<AudioChannel> audio_channel(u32 index) const override;
     virtual ErrorOr<size_t> write(size_t channel_index, UserOrKernelBuffer const& data, size_t length) override;
@@ -97,6 +91,7 @@ private:
     u8 m_number_of_bidirectional_streams;
     OwnPtr<CommandOutboundRingBuffer> m_command_buffer;
     OwnPtr<ResponseInboundRingBuffer> m_response_buffer;
+    RefPtr<InterruptHandler> m_interrupt_handler;
     Vector<NonnullRefPtr<Codec>> m_codecs {};
     OwnPtr<OutputPath> m_output_path;
     RefPtr<AudioChannel> m_audio_channel;

+ 29 - 0
Kernel/Devices/Audio/IntelHDA/InterruptHandler.cpp

@@ -0,0 +1,29 @@
+/*
+ * Copyright (c) 2023, Jelle Raaijmakers <jelle@gmta.nl>
+ *
+ * SPDX-License-Identifier: BSD-2-Clause
+ */
+
+#include <Kernel/Devices/Audio/IntelHDA/Controller.h>
+#include <Kernel/Devices/Audio/IntelHDA/InterruptHandler.h>
+
+namespace Kernel::Audio::IntelHDA {
+
+InterruptHandler::InterruptHandler(Controller& controller)
+    : PCIIRQHandler(controller, controller.device_identifier().interrupt_line().value())
+    , m_controller(controller)
+{
+    enable_irq();
+}
+
+bool InterruptHandler::handle_irq(RegisterState const&)
+{
+    auto result_or_error = m_controller.handle_interrupt({});
+    if (result_or_error.is_error()) {
+        dmesgln("IntelHDA: Error during interrupt handling: {}", result_or_error.release_error());
+        return false;
+    }
+    return result_or_error.release_value();
+}
+
+}

+ 37 - 0
Kernel/Devices/Audio/IntelHDA/InterruptHandler.h

@@ -0,0 +1,37 @@
+/*
+ * Copyright (c) 2023, Jelle Raaijmakers <jelle@gmta.nl>
+ *
+ * SPDX-License-Identifier: BSD-2-Clause
+ */
+
+#pragma once
+
+#include <AK/NonnullRefPtr.h>
+#include <Kernel/Interrupts/PCIIRQHandler.h>
+
+namespace Kernel::Audio::IntelHDA {
+
+class Controller;
+
+class InterruptHandler
+    : public PCIIRQHandler
+    , public RefCounted<InterruptHandler> {
+public:
+    static ErrorOr<NonnullRefPtr<InterruptHandler>> create(Controller& controller)
+    {
+        return adopt_nonnull_ref_or_enomem(new (nothrow) InterruptHandler(controller));
+    }
+
+    // ^PCIIRQHandler
+    virtual StringView purpose() const override { return "IntelHDA IRQ Handler"sv; }
+
+private:
+    InterruptHandler(Controller& controller);
+
+    // ^PCIIRQHandler
+    virtual bool handle_irq(RegisterState const&) override;
+
+    Controller& m_controller;
+};
+
+}