Kernel: Fix ACPI Embedded Controllers

I was polling the wrong bit in read/write which lead to dead locks on
machines that used embedded controllers :D

Also EC GPEs are now only initialized AFTER the ACPI post initialization
is done. This fixes some issues where _GPE body was not fully available
yet

kernel/include/kernel/ACPI/ACPI.h

@@ -54,6 +54,7 @@ namespace Kernel::ACPI
 
 		BAN::ErrorOr<void> initialize_embedded_controller(const AML::Scope& embedded_controller);
 		BAN::ErrorOr<void> initialize_embedded_controllers();
+		void initialize_embedded_controller_gpes();
 
 		BAN::Optional<GAS> find_gpe_block(size_t index);
 		bool enable_gpe(uint8_t gpe);

kernel/include/kernel/ACPI/EmbeddedController.h

@@ -14,7 +14,7 @@ namespace Kernel::ACPI
 	class EmbeddedController
 	{
 	public:
-		static BAN::ErrorOr<BAN::UniqPtr<EmbeddedController>> create(AML::Scope&& scope, uint16_t command_port, uint16_t data_port, BAN::Optional<uint8_t> gpe);
+		static BAN::ErrorOr<BAN::UniqPtr<EmbeddedController>> create(AML::Scope&& scope, uint16_t command_port, uint16_t data_port);
 		~EmbeddedController();
 
 		BAN::ErrorOr<uint8_t> read_byte(uint8_t offset);
@@ -22,21 +22,21 @@ namespace Kernel::ACPI
 
 		const AML::Scope& scope() const { return m_scope; }
 
+		static void handle_gpe_trampoline(void*);
+
 	private:
-		EmbeddedController(AML::Scope&& scope, uint16_t command_port, uint16_t data_port, bool has_gpe)
+		EmbeddedController(AML::Scope&& scope, uint16_t command_port, uint16_t data_port)
 			: m_scope(BAN::move(scope))
 			, m_command_port(command_port)
 			, m_data_port(data_port)
-			, m_has_gpe(has_gpe)
 		{ }
 
 	private:
-		void wait_status_bit(uint8_t bit, uint8_t value);
+		void wait_status_bit(uint8_t mask, bool set);
 
 		uint8_t read_one(uint16_t port);
 		void write_one(uint16_t port, uint8_t value);
 
-		static void handle_gpe_wrapper(void*);
 		void handle_gpe();
 
 		BAN::ErrorOr<void> call_query_method(uint8_t notification);
@@ -57,7 +57,6 @@ namespace Kernel::ACPI
 		const AML::Scope m_scope;
 		const uint16_t m_command_port;
 		const uint16_t m_data_port;
-		const bool m_has_gpe;
 
 		Mutex m_mutex;
 		ThreadBlocker m_thread_blocker;

kernel/kernel/ACPI/ACPI.cpp

@@ -784,26 +784,6 @@ acpi_release_global_lock:
 
 	BAN::ErrorOr<void> ACPI::initialize_embedded_controller(const AML::Scope& embedded_controller)
 	{
-		BAN::Optional<uint8_t> gpe_int;
-
-		do {
-			auto [gpe_path, gpe_obj] = TRY(m_namespace->find_named_object(embedded_controller, TRY(AML::NameString::from_string("_GPE"_sv)), true));
-			if (gpe_obj == nullptr)
-			{
-				dwarnln("EC {} does not have _GPE", embedded_controller);
-				break;
-			}
-
-			auto gpe = TRY(AML::evaluate_node(gpe_path, gpe_obj->node));
-			if (gpe.type == AML::Node::Type::Package)
-			{
-				dwarnln("TODO: EC {} has package _GPE");
-				break;
-			}
-
-			gpe_int = TRY(AML::convert_node(BAN::move(gpe), AML::ConvInteger, -1)).as.integer.value;
-		} while (false);
-
 		auto [crs_path, crs_obj] = TRY(m_namespace->find_named_object(embedded_controller, TRY(AML::NameString::from_string("_CRS"_sv)), true));
 		if (crs_obj == nullptr)
 		{
@@ -857,7 +837,7 @@ acpi_release_global_lock:
 		const auto data_port = TRY(extract_io_port(crs_buffer));
 		const auto command_port = TRY(extract_io_port(crs_buffer));
 
-		TRY(m_embedded_controllers.push_back(TRY(EmbeddedController::create(TRY(embedded_controller.copy()), command_port, data_port, gpe_int))));
+		TRY(m_embedded_controllers.push_back(TRY(EmbeddedController::create(TRY(embedded_controller.copy()), command_port, data_port))));
 		return {};
 	}
 
@@ -877,6 +857,33 @@ acpi_release_global_lock:
 		return {};
 	}
 
+	void ACPI::initialize_embedded_controller_gpes()
+	{
+		const auto initialize_gpe = [this](EmbeddedController& embedded_controller) -> BAN::ErrorOr<void> {
+			auto [gpe_path, gpe_obj] = TRY(m_namespace->find_named_object(embedded_controller.scope(), TRY(AML::NameString::from_string("_GPE"_sv)), true));
+			if (gpe_obj == nullptr)
+			{
+				dprintln("EC {} does not have _GPE", embedded_controller.scope());
+				return {};
+			}
+
+			auto gpe = TRY(AML::evaluate_node(gpe_path, gpe_obj->node));
+			if (gpe.type == AML::Node::Type::Package)
+			{
+				dwarnln("TODO: EC {} has package _GPE");
+				return {};
+			}
+
+			const auto gpe_int = TRY(AML::convert_node(BAN::move(gpe), AML::ConvInteger, -1)).as.integer.value;
+			TRY(register_gpe_handler(gpe_int, &EmbeddedController::handle_gpe_trampoline, &embedded_controller));
+
+			return {};
+		};
+
+		for (auto& embedded_controller : m_embedded_controllers)
+			(void)initialize_gpe(*embedded_controller);
+	}
+
 	BAN::ErrorOr<void> ACPI::register_gpe_handler(uint8_t gpe, void (*callback)(void*), void* argument)
 	{
 		if (m_gpe_methods[gpe].method)
@@ -996,42 +1003,12 @@ acpi_release_global_lock:
 			// FIXME: add support for GPE blocks inside the ACPI namespace
 		}
 
-		if (auto ret = initialize_embedded_controllers(); ret.is_error())
-			dwarnln("Failed to initialize Embedded Controllers: {}", ret.error());
-
-		if (auto ret = m_namespace->post_load_initialize(); ret.is_error())
-			dwarnln("Failed to initialize ACPI namespace: {}", ret.error());
-
-		auto [pic_path, pic_obj] = TRY(m_namespace->find_named_object({}, TRY(AML::NameString::from_string("\\_PIC"_sv))));
-		if (pic_obj && pic_obj->node.type == AML::Node::Type::Method)
-		{
-			auto& pic_node = pic_obj->node;
-			if (pic_node.as.method.arg_count != 1)
-			{
-				dwarnln("Method \\_PIC has {} arguments, expected 1", pic_node.as.method.arg_count);
-				return BAN::Error::from_errno(EINVAL);
-			}
-
-			AML::Reference arg_ref;
-			arg_ref.node.type = AML::Node::Type::Integer;
-			arg_ref.node.as.integer.value = InterruptController::get().is_using_apic() ? 1 : 0;
-			arg_ref.ref_count = 2;
-
-			BAN::Array<AML::Reference*, 7> arguments(nullptr);
-			arguments[0] = &arg_ref; // method call should not delete argument
-			TRY(AML::method_call(pic_path, pic_node, BAN::move(arguments)));
-		}
-
-		dprintln("Evaluated \\_PIC({})", InterruptController::get().is_using_apic() ? 1 : 0);
-
 		uint8_t irq = fadt().sci_int;
 		if (auto ret = InterruptController::get().reserve_irq(irq); ret.is_error())
 			dwarnln("Could not enable ACPI interrupt: {}", ret.error());
 		else
 		{
-			auto hex_sv_to_int =
+			const auto hex_sv_to_int = [](BAN::StringView sv) -> BAN::Optional<uint32_t> {
-				[](BAN::StringView sv) -> BAN::Optional<uint32_t>
-				{
 				uint32_t ret = 0;
 				for (char c : sv)
 				{
@@ -1094,9 +1071,40 @@ acpi_release_global_lock:
 				dwarnln("Failed to create ACPI thread, power button will not work: {}", thread_or_error.error());
 			else if (auto ret = Processor::scheduler().add_thread(thread_or_error.value()); ret.is_error())
 				dwarnln("Failed to create ACPI thread, power button will not work: {}", ret.error());
+			else
+				dprintln("Initialized ACPI interrupts");
 		}
 
-		dprintln("Initialized ACPI interrupts");
+		if (auto ret = initialize_embedded_controllers(); ret.is_error())
+			dwarnln("Failed to initialize Embedded Controllers: {}", ret.error());
+
+		if (auto ret = m_namespace->post_load_initialize(); ret.is_error())
+			dwarnln("Failed to initialize ACPI namespace: {}", ret.error());
+
+		// NOTE: We cannot initialize EC GPEs before the post init is done, but post init does need ECs initialized
+		initialize_embedded_controller_gpes();
+
+		auto [pic_path, pic_obj] = TRY(m_namespace->find_named_object({}, TRY(AML::NameString::from_string("\\_PIC"_sv))));
+		if (pic_obj && pic_obj->node.type == AML::Node::Type::Method)
+		{
+			auto& pic_node = pic_obj->node;
+			if (pic_node.as.method.arg_count != 1)
+			{
+				dwarnln("Method \\_PIC has {} arguments, expected 1", pic_node.as.method.arg_count);
+				return BAN::Error::from_errno(EINVAL);
+			}
+
+			AML::Reference arg_ref;
+			arg_ref.node.type = AML::Node::Type::Integer;
+			arg_ref.node.as.integer.value = InterruptController::get().is_using_apic() ? 1 : 0;
+			arg_ref.ref_count = 2;
+
+			BAN::Array<AML::Reference*, 7> arguments(nullptr);
+			arguments[0] = &arg_ref; // method call should not delete argument
+			TRY(AML::method_call(pic_path, pic_node, BAN::move(arguments)));
+
+			dprintln("Evaluated \\_PIC({})", InterruptController::get().is_using_apic() ? 1 : 0);
+		}
 
 		if (InterruptController::get().is_using_apic())
 		{

kernel/kernel/ACPI/EmbeddedController.cpp

@@ -30,9 +30,9 @@ namespace Kernel::ACPI
 		CMD_QUERY     = 0x84,
 	};
 
-	BAN::ErrorOr<BAN::UniqPtr<EmbeddedController>> EmbeddedController::create(AML::Scope&& scope, uint16_t command_port, uint16_t data_port, BAN::Optional<uint8_t> gpe)
+	BAN::ErrorOr<BAN::UniqPtr<EmbeddedController>> EmbeddedController::create(AML::Scope&& scope, uint16_t command_port, uint16_t data_port)
 	{
-		auto* embedded_controller_ptr = new EmbeddedController(BAN::move(scope), command_port, data_port, gpe.has_value());
+		auto* embedded_controller_ptr = new EmbeddedController(BAN::move(scope), command_port, data_port);
 		if (embedded_controller_ptr == nullptr)
 			return BAN::Error::from_errno(ENOMEM);
 
@@ -42,16 +42,6 @@ namespace Kernel::ACPI
 		auto embedded_controller = BAN::UniqPtr<EmbeddedController>::adopt(embedded_controller_ptr);
 		embedded_controller->m_thread = thread;
 
-		if (gpe.has_value())
-			TRY(ACPI::get().register_gpe_handler(gpe.value(), &handle_gpe_wrapper, embedded_controller.ptr()));
-		else
-		{
-			// FIXME: Restructure EC such that SCI_EVT can be polled.
-			//        Simple solution would be spawning another thread,
-			//        but that feels too hacky.
-			dwarnln("TODO: SCI_EVT polling without GPE");
-		}
-
 		return embedded_controller;
 	}
 
@@ -95,26 +85,25 @@ namespace Kernel::ACPI
 
 	uint8_t EmbeddedController::read_one(uint16_t port)
 	{
-		wait_status_bit(STS_OBF, 1);
+		wait_status_bit(STS_OBF, true);
 		return IO::inb(port);
 	}
 
 	void EmbeddedController::write_one(uint16_t port, uint8_t value)
 	{
-		wait_status_bit(STS_IBF, 0);
+		wait_status_bit(STS_IBF, false);
 		IO::outb(port, value);
 	}
 
-	void EmbeddedController::wait_status_bit(uint8_t bit, uint8_t value)
+	void EmbeddedController::wait_status_bit(uint8_t mask, bool set)
 	{
 		// FIXME: timeouts
-		const uint8_t mask = 1 << bit;
+		const uint8_t comp = set ? mask : 0;
-		const uint8_t comp = value ? mask : 0;
 		while ((IO::inb(m_command_port) & mask) != comp)
 			continue;
 	}
 
-	void EmbeddedController::handle_gpe_wrapper(void* embedded_controller)
+	void EmbeddedController::handle_gpe_trampoline(void* embedded_controller)
 	{
 		static_cast<EmbeddedController*>(embedded_controller)->handle_gpe();
 	}
@@ -217,7 +206,7 @@ namespace Kernel::ACPI
 
 		for (;;)
 		{
-			Command* const command = m_queued_command.has_value() ? m_queued_command.value() : nullptr;
+			auto* const command = m_queued_command.value_or(nullptr);
 			m_queued_command.clear();
 
 			if (command == nullptr)
@@ -226,12 +215,10 @@ namespace Kernel::ACPI
 				continue;
 			}
 
-			m_mutex.unlock();
-
-			if (command)
-			{
 			// TODO: use burst mode
 
+			m_mutex.unlock();
+
 			write_one(m_command_port, command->command);
 			if (command->data1.has_value())
 				write_one(m_data_port, command->data1.value());
@@ -241,12 +228,9 @@ namespace Kernel::ACPI
 				*command->response = read_one(m_data_port);
 
 			m_mutex.lock();
+
 			command->done = true;
 			m_thread_blocker.unblock();
-				m_mutex.unlock();
-			}
-
-			m_mutex.lock();
 		}
 	}