From 8de395694d39c4b12efbc3b3598be5c1cc80cdb0 Mon Sep 17 00:00:00 2001 From: Liav A Date: Thu, 14 Apr 2022 22:47:17 +0300 Subject: Kernel/Storage: Do proper locking & reset in the AHCIController code The initialize_hba method now calls the reset method to reset the HBA and initialize each AHCIPort. Also, after full HBA reset we need to turn on the AHCI functionality of the HBA and global interrupts since they are cleared to 0 according to the specification in the GHC register. --- Kernel/Storage/ATA/AHCIController.cpp | 78 +++++++++++++++++++---------------- Kernel/Storage/ATA/AHCIController.h | 5 +++ 2 files changed, 48 insertions(+), 35 deletions(-) (limited to 'Kernel/Storage/ATA') diff --git a/Kernel/Storage/ATA/AHCIController.cpp b/Kernel/Storage/ATA/AHCIController.cpp index 193118de75..7e353bcb9e 100644 --- a/Kernel/Storage/ATA/AHCIController.cpp +++ b/Kernel/Storage/ATA/AHCIController.cpp @@ -26,22 +26,46 @@ UNMAP_AFTER_INIT NonnullRefPtr AHCIController::initialize(PCI::D bool AHCIController::reset() { - hba().control_regs.ghc = 1; - - dbgln_if(AHCI_DEBUG, "{}: AHCI Controller reset", pci_address()); + dmesgln("{}: AHCI controller reset", pci_address()); + { + SpinlockLocker locker(m_hba_control_lock); + hba().control_regs.ghc = 1; + + dbgln_if(AHCI_DEBUG, "{}: AHCI Controller reset", pci_address()); + + full_memory_barrier(); + size_t retry = 0; + + // Note: The HBA is locked or hung if we waited more than 1 second! + while (true) { + if (retry > 1000) + return false; + if (!(hba().control_regs.ghc & 1)) + break; + IO::delay(1000); + retry++; + } + // Note: Turn on AHCI HBA and Global HBA Interrupts. + full_memory_barrier(); + hba().control_regs.ghc = (1 << 31) | (1 << 1); + full_memory_barrier(); + } - full_memory_barrier(); - size_t retry = 0; + // Note: According to the AHCI spec the PI register indicates which ports are exposed by the HBA. + // It is loaded by the BIOS. It indicates which ports that the HBA supports are available for software to use. + // For example, on an HBA that supports 6 ports as indicated in CAP.NP, only ports 1 and 3 could be available, + // with ports 0, 2, 4, and 5 being unavailable. + // Which means that even without clearing the AHCI ports array, we are never able to encounter + // a case that we would have stale left-over ports in there. We still clear the array + // for the sake of clarity and completeness, as it doesn't harm anything anyway. + m_ports.fill({}); - while (true) { - if (retry > 1000) - return false; - if (!(hba().control_regs.ghc & 1)) - break; - IO::delay(1000); - retry++; + auto implemented_ports = AHCI::MaskedBitField((u32 volatile&)(hba().control_regs.pi)); + for (auto index : implemented_ports.to_vector()) { + auto port = AHCIPort::create(*this, m_hba_capabilities, static_cast(hba().port_regs[index]), index).release_value_but_fixme_should_propagate_errors(); + m_ports[index] = port; + port->reset(); } - // The HBA is locked or hung if we waited more than 1 second! return true; } @@ -52,6 +76,7 @@ bool AHCIController::shutdown() size_t AHCIController::devices_count() const { + SpinlockLocker locker(m_hba_control_lock); size_t count = 0; for (auto port : m_ports) { if (port && port->connected_device()) @@ -137,15 +162,7 @@ AHCIController::~AHCIController() = default; UNMAP_AFTER_INIT void AHCIController::initialize_hba(PCI::DeviceIdentifier const& pci_device_identifier) { - if (!reset()) { - dmesgln("{}: AHCI controller reset failed", pci_address()); - return; - } - dmesgln("{}: AHCI controller reset", pci_address()); - dbgln("{}: AHCI command list entries count - {}", pci_address(), m_hba_capabilities.max_command_list_entries_count); - u32 version = hba().control_regs.version; - dbgln_if(AHCI_DEBUG, "{}: AHCI Controller Version = {:#08x}", pci_address(), version); hba().control_regs.ghc = 0x80000000; // Ensure that HBA knows we are AHCI aware. PCI::enable_interrupt_line(pci_address()); @@ -154,20 +171,9 @@ UNMAP_AFTER_INIT void AHCIController::initialize_hba(PCI::DeviceIdentifier const auto implemented_ports = AHCI::MaskedBitField((u32 volatile&)(hba().control_regs.pi)); m_irq_handler = AHCIInterruptHandler::create(*this, pci_device_identifier.interrupt_line().value(), implemented_ports).release_value_but_fixme_should_propagate_errors(); - - if (kernel_command_line().ahci_reset_mode() == AHCIResetMode::Aggressive) { - for (auto index : implemented_ports.to_vector()) { - auto port = AHCIPort::create(*this, m_hba_capabilities, static_cast(hba().port_regs[index]), index).release_value_but_fixme_should_propagate_errors(); - m_ports[index] = port; - port->reset(); - } - return; - } - for (auto index : implemented_ports.to_vector()) { - auto port = AHCIPort::create(*this, m_hba_capabilities, static_cast(hba().port_regs[index]), index).release_value_but_fixme_should_propagate_errors(); - m_ports[index] = port; - port->initialize_without_reset(); - } + reset(); + dbgln_if(AHCI_DEBUG, "{}: AHCI Controller Version = {:#08x}", pci_address(), version); + dbgln("{}: AHCI command list entries count - {}", pci_address(), m_hba_capabilities.max_command_list_entries_count); } void AHCIController::handle_interrupt_for_port(Badge, u32 port_index) const @@ -188,9 +194,11 @@ void AHCIController::enable_global_interrupts() const RefPtr AHCIController::device_by_port(u32 port_index) const { + SpinlockLocker locker(m_hba_control_lock); auto port = m_ports[port_index]; if (!port) return {}; + SpinlockLocker port_hard_locker(port->m_hard_lock); return port->connected_device(); } diff --git a/Kernel/Storage/ATA/AHCIController.h b/Kernel/Storage/ATA/AHCIController.h index 35e49188d6..79abc0d1f7 100644 --- a/Kernel/Storage/ATA/AHCIController.h +++ b/Kernel/Storage/ATA/AHCIController.h @@ -56,5 +56,10 @@ private: // FIXME: There could be multiple IRQ (MSI) handlers for AHCI. Find a way to use all of them. OwnPtr m_irq_handler; + + // Note: This lock is intended to be locked when doing changes to HBA registers + // that affect its core functionality in a manner that controls all attached storage devices + // to the HBA SATA ports. + mutable Spinlock m_hba_control_lock; }; } -- cgit v1.2.3