From a957907f4b449d140f8018b3170bdf0ae6be42b5 Mon Sep 17 00:00:00 2001 From: Idan Horowitz Date: Fri, 9 Feb 2024 17:06:02 +0200 Subject: [PATCH] Kernel: Merge NVME Queue complete_current_request implementations Most of the actual logic is identical, with the only real difference being that one wraps it with an async work item. Merge the implementations to reduce duplications (which will also require the fixes in the next commits to only be done once). --- .../Storage/NVMe/NVMeInterruptQueue.cpp | 35 +----------------- Kernel/Devices/Storage/NVMe/NVMePollQueue.cpp | 34 ----------------- Kernel/Devices/Storage/NVMe/NVMePollQueue.h | 3 -- Kernel/Devices/Storage/NVMe/NVMeQueue.cpp | 37 +++++++++++++++++++ Kernel/Devices/Storage/NVMe/NVMeQueue.h | 3 +- 5 files changed, 40 insertions(+), 72 deletions(-) diff --git a/Kernel/Devices/Storage/NVMe/NVMeInterruptQueue.cpp b/Kernel/Devices/Storage/NVMe/NVMeInterruptQueue.cpp index b669c6a918..47401278b5 100644 --- a/Kernel/Devices/Storage/NVMe/NVMeInterruptQueue.cpp +++ b/Kernel/Devices/Storage/NVMe/NVMeInterruptQueue.cpp @@ -43,40 +43,7 @@ void NVMeInterruptQueue::submit_sqe(NVMeSubmission& sub) void NVMeInterruptQueue::complete_current_request(u16 cmdid, u16 status) { auto work_item_creation_result = g_io_work->try_queue([this, cmdid, status]() { - SpinlockLocker lock(m_request_lock); - auto& request_pdu = m_requests.get(cmdid).release_value(); - auto current_request = request_pdu.request; - AsyncDeviceRequest::RequestResult req_result = AsyncDeviceRequest::Success; - - ScopeGuard guard = [req_result, status, &request_pdu, &lock] { - // FIXME: We should unlock at the end of this function to make sure no new requests is inserted - // before we complete the request and calling end_io_handler but that results in a deadlock - // For now this is avoided by asserting the `used` field while inserting. - lock.unlock(); - if (request_pdu.request) - request_pdu.request->complete(req_result); - if (request_pdu.end_io_handler) - request_pdu.end_io_handler(status); - request_pdu.clear(); - }; - - // There can be submission without any request associated with it such as with - // admin queue commands during init. If there is no request, we are done - if (!current_request) - return; - - if (status) { - req_result = AsyncBlockDeviceRequest::Failure; - return; - } - - if (current_request->request_type() == AsyncBlockDeviceRequest::RequestType::Read) { - if (auto result = current_request->write_to_buffer(current_request->buffer(), m_rw_dma_region->vaddr().as_ptr(), current_request->buffer_size()); result.is_error()) { - req_result = AsyncBlockDeviceRequest::MemoryFault; - return; - } - } - return; + NVMeQueue::complete_current_request(cmdid, status); }); if (work_item_creation_result.is_error()) { diff --git a/Kernel/Devices/Storage/NVMe/NVMePollQueue.cpp b/Kernel/Devices/Storage/NVMe/NVMePollQueue.cpp index 35b9b7ce41..109964719d 100644 --- a/Kernel/Devices/Storage/NVMe/NVMePollQueue.cpp +++ b/Kernel/Devices/Storage/NVMe/NVMePollQueue.cpp @@ -30,38 +30,4 @@ void NVMePollQueue::submit_sqe(NVMeSubmission& sub) } } -void NVMePollQueue::complete_current_request(u16 cmdid, u16 status) -{ - SpinlockLocker lock(m_request_lock); - auto& request_pdu = m_requests.get(cmdid).release_value(); - auto current_request = request_pdu.request; - AsyncDeviceRequest::RequestResult req_result = AsyncDeviceRequest::Success; - - ScopeGuard guard = [req_result, status, &request_pdu] { - if (request_pdu.request) - request_pdu.request->complete(req_result); - if (request_pdu.end_io_handler) - request_pdu.end_io_handler(status); - request_pdu.clear(); - }; - - // There can be submission without any request associated with it such as with - // admin queue commands during init. If there is no request, we are done - if (!current_request) - return; - - if (status) { - req_result = AsyncBlockDeviceRequest::Failure; - return; - } - - if (current_request->request_type() == AsyncBlockDeviceRequest::RequestType::Read) { - if (auto result = current_request->write_to_buffer(current_request->buffer(), m_rw_dma_region->vaddr().as_ptr(), current_request->buffer_size()); result.is_error()) { - req_result = AsyncBlockDeviceRequest::MemoryFault; - return; - } - } - - return; -} } diff --git a/Kernel/Devices/Storage/NVMe/NVMePollQueue.h b/Kernel/Devices/Storage/NVMe/NVMePollQueue.h index f30d614d79..e2b3182224 100644 --- a/Kernel/Devices/Storage/NVMe/NVMePollQueue.h +++ b/Kernel/Devices/Storage/NVMe/NVMePollQueue.h @@ -18,8 +18,5 @@ public: protected: NVMePollQueue(NonnullOwnPtr rw_dma_region, NonnullRefPtr rw_dma_page, u16 qid, u32 q_depth, OwnPtr cq_dma_region, OwnPtr sq_dma_region, Doorbell db_regs); - -private: - virtual void complete_current_request(u16 cmdid, u16 status) override; }; } diff --git a/Kernel/Devices/Storage/NVMe/NVMeQueue.cpp b/Kernel/Devices/Storage/NVMe/NVMeQueue.cpp index 8383cdebdc..bc8807625e 100644 --- a/Kernel/Devices/Storage/NVMe/NVMeQueue.cpp +++ b/Kernel/Devices/Storage/NVMe/NVMeQueue.cpp @@ -104,6 +104,43 @@ void NVMeQueue::submit_sqe(NVMeSubmission& sub) update_sq_doorbell(); } +void NVMeQueue::complete_current_request(u16 cmdid, u16 status) +{ + SpinlockLocker lock(m_request_lock); + auto& request_pdu = m_requests.get(cmdid).release_value(); + auto current_request = request_pdu.request; + AsyncDeviceRequest::RequestResult req_result = AsyncDeviceRequest::Success; + + ScopeGuard guard = [req_result, status, &request_pdu, &lock] { + // FIXME: We should unlock at the end of this function to make sure no new requests is inserted + // before we complete the request and calling end_io_handler but that results in a deadlock + // For now this is avoided by asserting the `used` field while inserting. + lock.unlock(); + if (request_pdu.request) + request_pdu.request->complete(req_result); + if (request_pdu.end_io_handler) + request_pdu.end_io_handler(status); + request_pdu.clear(); + }; + + // There can be submission without any request associated with it such as with + // admin queue commands during init. If there is no request, we are done + if (!current_request) + return; + + if (status) { + req_result = AsyncBlockDeviceRequest::Failure; + return; + } + + if (current_request->request_type() == AsyncBlockDeviceRequest::RequestType::Read) { + if (auto result = current_request->write_to_buffer(current_request->buffer(), m_rw_dma_region->vaddr().as_ptr(), current_request->buffer_size()); result.is_error()) { + req_result = AsyncBlockDeviceRequest::MemoryFault; + return; + } + } +} + u16 NVMeQueue::submit_sync_sqe(NVMeSubmission& sub) { // For now let's use sq tail as a unique command id. diff --git a/Kernel/Devices/Storage/NVMe/NVMeQueue.h b/Kernel/Devices/Storage/NVMe/NVMeQueue.h index a3c23a4072..9fab2a45f9 100644 --- a/Kernel/Devices/Storage/NVMe/NVMeQueue.h +++ b/Kernel/Devices/Storage/NVMe/NVMeQueue.h @@ -100,10 +100,11 @@ protected: } } + virtual void complete_current_request(u16 cmdid, u16 status); + private: bool cqe_available(); void update_cqe_head(); - virtual void complete_current_request(u16 cmdid, u16 status) = 0; void update_cq_doorbell() { full_memory_barrier();