From b96e4c1308dedeb5c4882b5cb139399a96a354dd Mon Sep 17 00:00:00 2001 From: Liav A Date: Sat, 27 Mar 2021 09:53:59 +0300 Subject: [PATCH] Kernel/Storage: Use more locking in the IDE code This change should make it less possible for race conditions to happen and cause fatal errors when accessing the hardware. --- Kernel/Storage/BMIDEChannel.cpp | 42 +++++++++++++------------ Kernel/Storage/IDEChannel.cpp | 55 +++++++++++++++++++-------------- Kernel/Storage/IDEChannel.h | 5 +-- 3 files changed, 58 insertions(+), 44 deletions(-) diff --git a/Kernel/Storage/BMIDEChannel.cpp b/Kernel/Storage/BMIDEChannel.cpp index 0a74041e98..42f6d68c4e 100644 --- a/Kernel/Storage/BMIDEChannel.cpp +++ b/Kernel/Storage/BMIDEChannel.cpp @@ -119,14 +119,14 @@ void BMIDEChannel::complete_current_request(AsyncDeviceRequest::RequestResult re dbgln_if(PATA_DEBUG, "BMIDEChannel::complete_current_request result: {}", (int)result); ScopedSpinLock lock(m_request_lock); VERIFY(m_current_request); - auto& request = *m_current_request; - m_current_request = nullptr; + auto current_request = m_current_request; + m_current_request.clear(); if (result == AsyncDeviceRequest::Success) { - if (request.request_type() == AsyncBlockDeviceRequest::Read) { - if (!request.write_to_buffer(request.buffer(), m_dma_buffer_region->vaddr().as_ptr(), 512 * request.block_count())) { + if (current_request->request_type() == AsyncBlockDeviceRequest::Read) { + if (!current_request->write_to_buffer(current_request->buffer(), m_dma_buffer_region->vaddr().as_ptr(), 512 * current_request->block_count())) { lock.unlock(); - request.complete(AsyncDeviceRequest::MemoryFault); + current_request->complete(AsyncDeviceRequest::MemoryFault); return; } } @@ -137,21 +137,23 @@ void BMIDEChannel::complete_current_request(AsyncDeviceRequest::RequestResult re } lock.unlock(); - request.complete(result); + current_request->complete(result); }); } void BMIDEChannel::ata_write_sectors(bool slave_request, u16 capabilities) { - VERIFY(m_request_lock.is_locked()); - auto& request = *m_current_request; - auto lba = request.block_index(); - dbgln_if(PATA_DEBUG, "BMIDEChannel::ata_write_sectors_with_dma ({} x {})", lba, request.block_count()); + VERIFY(m_lock.is_locked()); + VERIFY(!m_current_request.is_null()); + VERIFY(m_current_request->block_count() <= 256); + + ScopedSpinLock m_lock(m_request_lock); + dbgln_if(PATA_DEBUG, "BMIDEChannel::ata_write_sectors_with_dma ({} x {})", m_current_request->block_index(), m_current_request->block_count()); prdt().offset = m_dma_buffer_page->paddr().get(); - prdt().size = 512 * request.block_count(); + prdt().size = 512 * m_current_request->block_count(); - if (!request.read_from_buffer(request.buffer(), m_dma_buffer_region->vaddr().as_ptr(), 512 * request.block_count())) { + if (!m_current_request->read_from_buffer(m_current_request->buffer(), m_dma_buffer_region->vaddr().as_ptr(), 512 * m_current_request->block_count())) { complete_current_request(AsyncDeviceRequest::MemoryFault); return; } @@ -167,7 +169,7 @@ void BMIDEChannel::ata_write_sectors(bool slave_request, u16 capabilities) // Turn on "Interrupt" and "Error" flag. The error flag should be cleared by hardware. m_io_group.bus_master_base().value().offset(2).out(m_io_group.bus_master_base().value().offset(2).in() | 0x6); - ata_access(Direction::Write, slave_request, lba, request.block_count(), capabilities); + ata_access(Direction::Write, slave_request, m_current_request->block_index(), m_current_request->block_count(), capabilities); // Start bus master m_io_group.bus_master_base().value().out(0x1); @@ -184,13 +186,15 @@ void BMIDEChannel::send_ata_io_command(LBAMode lba_mode, Direction direction) co void BMIDEChannel::ata_read_sectors(bool slave_request, u16 capabilities) { - VERIFY(m_request_lock.is_locked()); - auto& request = *m_current_request; - auto lba = request.block_index(); - dbgln_if(PATA_DEBUG, "BMIDEChannel::ata_read_sectors_with_dma ({} x {})", lba, request.block_count()); + VERIFY(m_lock.is_locked()); + VERIFY(!m_current_request.is_null()); + VERIFY(m_current_request->block_count() <= 256); + + ScopedSpinLock m_lock(m_request_lock); + dbgln_if(PATA_DEBUG, "BMIDEChannel::ata_read_sectors_with_dma ({} x {})", m_current_request->block_index(), m_current_request->block_count()); prdt().offset = m_dma_buffer_page->paddr().get(); - prdt().size = 512 * request.block_count(); + prdt().size = 512 * m_current_request->block_count(); VERIFY(prdt().size <= PAGE_SIZE); @@ -207,7 +211,7 @@ void BMIDEChannel::ata_read_sectors(bool slave_request, u16 capabilities) // Set transfer direction m_io_group.bus_master_base().value().out(0x8); - ata_access(Direction::Read, slave_request, lba, request.block_count(), capabilities); + ata_access(Direction::Read, slave_request, m_current_request->block_index(), m_current_request->block_count(), capabilities); // Start bus master m_io_group.bus_master_base().value().out(0x9); diff --git a/Kernel/Storage/IDEChannel.cpp b/Kernel/Storage/IDEChannel.cpp index 0ddbb787ea..52e324a8d3 100644 --- a/Kernel/Storage/IDEChannel.cpp +++ b/Kernel/Storage/IDEChannel.cpp @@ -94,11 +94,12 @@ UNMAP_AFTER_INIT IDEChannel::~IDEChannel() void IDEChannel::start_request(AsyncBlockDeviceRequest& request, bool is_slave, u16 capabilities) { - ScopedSpinLock lock(m_request_lock); + LOCKER(m_lock); + VERIFY(m_current_request.is_null()); dbgln_if(PATA_DEBUG, "IDEChannel::start_request"); - m_current_request = &request; + m_current_request = request; m_current_request_block_index = 0; m_current_request_flushing_cache = false; @@ -120,13 +121,11 @@ void IDEChannel::complete_current_request(AsyncDeviceRequest::RequestResult resu // before Processor::deferred_call_queue returns! g_io_work->queue([this, result]() { dbgln_if(PATA_DEBUG, "IDEChannel::complete_current_request result: {}", (int)result); - ScopedSpinLock lock(m_request_lock); + LOCKER(m_lock); VERIFY(m_current_request); - auto& request = *m_current_request; - m_current_request = nullptr; - - lock.unlock(); - request.complete(result); + auto current_request = m_current_request; + m_current_request.clear(); + current_request->complete(result); }); } @@ -145,6 +144,7 @@ static void print_ide_status(u8 status) void IDEChannel::try_disambiguate_error() { + VERIFY(m_lock.is_locked()); dbgln("IDEChannel: Error cause:"); switch (m_device_error) { @@ -208,10 +208,12 @@ void IDEChannel::handle_irq(const RegisterState&) // Now schedule reading/writing the buffer as soon as we leave the irq handler. // This is important so that we can safely access the buffers, which could // trigger page faults - Processor::deferred_call_queue([this]() { + g_io_work->queue([this]() { + LOCKER(m_lock); ScopedSpinLock lock(m_request_lock); if (m_current_request->request_type() == AsyncBlockDeviceRequest::Read) { dbgln_if(PATA_DEBUG, "IDEChannel: Read block {}/{}", m_current_request_block_index, m_current_request->block_count()); + if (ata_do_read_sector()) { if (++m_current_request_block_index >= m_current_request->block_count()) { complete_current_request(AsyncDeviceRequest::Success); @@ -355,6 +357,8 @@ UNMAP_AFTER_INIT void IDEChannel::detect_disks() void IDEChannel::ata_access(Direction direction, bool slave_request, u64 lba, u8 block_count, u16 capabilities) { + VERIFY(m_lock.is_locked()); + VERIFY(m_request_lock.is_locked()); LBAMode lba_mode; u8 head = 0; @@ -403,6 +407,9 @@ void IDEChannel::send_ata_io_command(LBAMode lba_mode, Direction direction) cons bool IDEChannel::ata_do_read_sector() { + VERIFY(m_lock.is_locked()); + VERIFY(m_request_lock.is_locked()); + VERIFY(!m_current_request.is_null()); dbgln_if(PATA_DEBUG, "IDEChannel::ata_do_read_sector"); auto& request = *m_current_request; auto out_buffer = request.buffer().offset(m_current_request_block_index * 512); @@ -422,18 +429,21 @@ bool IDEChannel::ata_do_read_sector() // FIXME: This doesn't quite work and locks up reading LBA 3. void IDEChannel::ata_read_sectors(bool slave_request, u16 capabilities) { - auto& request = *m_current_request; - VERIFY(request.block_count() <= 256); + VERIFY(m_lock.is_locked()); + VERIFY(!m_current_request.is_null()); + VERIFY(m_current_request->block_count() <= 256); + + ScopedSpinLock m_lock(m_request_lock); dbgln_if(PATA_DEBUG, "IDEChannel::ata_read_sectors"); - - auto lba = request.block_index(); - dbgln_if(PATA_DEBUG, "IDEChannel: Reading {} sector(s) @ LBA {}", request.block_count(), lba); - - ata_access(Direction::Read, slave_request, lba, request.block_count(), capabilities); + dbgln_if(PATA_DEBUG, "IDEChannel: Reading {} sector(s) @ LBA {}", m_current_request->block_count(), m_current_request->block_index()); + ata_access(Direction::Read, slave_request, m_current_request->block_index(), m_current_request->block_count(), capabilities); } void IDEChannel::ata_do_write_sector() { + VERIFY(m_lock.is_locked()); + VERIFY(m_request_lock.is_locked()); + VERIFY(!m_current_request.is_null()); auto& request = *m_current_request; io_delay(); @@ -457,14 +467,13 @@ void IDEChannel::ata_do_write_sector() // FIXME: I'm assuming this doesn't work based on the fact PIO read doesn't work. void IDEChannel::ata_write_sectors(bool slave_request, u16 capabilities) { - auto& request = *m_current_request; + VERIFY(m_lock.is_locked()); + VERIFY(!m_current_request.is_null()); + VERIFY(m_current_request->block_count() <= 256); - VERIFY(request.block_count() <= 256); - auto start_sector = request.block_index(); - u32 count = request.block_count(); - dbgln_if(PATA_DEBUG, "IDEChannel: Writing {} sector(s) @ LBA {}", count, start_sector); - - ata_access(Direction::Write, slave_request, start_sector, request.block_count(), capabilities); + ScopedSpinLock m_lock(m_request_lock); + dbgln_if(PATA_DEBUG, "IDEChannel: Writing {} sector(s) @ LBA {}", m_current_request->block_count(), m_current_request->block_index()); + ata_access(Direction::Write, slave_request, m_current_request->block_index(), m_current_request->block_count(), capabilities); ata_do_write_sector(); } } diff --git a/Kernel/Storage/IDEChannel.h b/Kernel/Storage/IDEChannel.h index 7651df4bba..c6b3598297 100644 --- a/Kernel/Storage/IDEChannel.h +++ b/Kernel/Storage/IDEChannel.h @@ -163,10 +163,11 @@ protected: RefPtr m_master; RefPtr m_slave; - AsyncBlockDeviceRequest* m_current_request { nullptr }; - u32 m_current_request_block_index { 0 }; + RefPtr m_current_request; + size_t m_current_request_block_index { 0 }; bool m_current_request_flushing_cache { false }; SpinLock m_request_lock; + Lock m_lock { "IDEChannel" }; IOAddressGroup m_io_group; NonnullRefPtr m_parent_controller;