From 3af70cb0fcaddbf1d9888608496a474ab0f36c1b Mon Sep 17 00:00:00 2001 From: Liav A Date: Sat, 16 Jul 2022 10:39:57 +0300 Subject: [PATCH] Kernel/Devices: Abstract SysFS Device add/remove methods more properly It is starting to get a little messy with how each device can try to add or remove itself to either /sys/dev/block or /sys/dev/char directories. To better do this, we introduce 4 virtual methods to take care of that, so until we ensure all nodes in /sys/dev/block and /sys/dev/char are actual symlinks, we allow the Device base class to call virtual methods upon insertion or before being destroying, so it add itself elegantly to either of these directories or remove itself when needed. For special cases where we need to create symlinks, we have two virtual methods to be called otherwise to do almost the same thing mentioned before, but to use symlinks instead. --- Kernel/Devices/BlockDevice.cpp | 35 +++++++++++++++++++ Kernel/Devices/BlockDevice.h | 7 ++++ Kernel/Devices/CharacterDevice.cpp | 35 +++++++++++++++++++ Kernel/Devices/CharacterDevice.h | 7 ++++ Kernel/Devices/Device.cpp | 22 ++---------- Kernel/Devices/Device.h | 13 ++++++- .../DeviceIdentifiers/BlockDevicesDirectory.h | 7 ++-- .../CharacterDevicesDirectory.h | 6 ++-- Kernel/Graphics/DisplayConnector.cpp | 11 ++---- Kernel/Storage/StorageDevice.cpp | 10 ++---- 10 files changed, 107 insertions(+), 46 deletions(-) diff --git a/Kernel/Devices/BlockDevice.cpp b/Kernel/Devices/BlockDevice.cpp index 894e4a9462..1d4bd8fce6 100644 --- a/Kernel/Devices/BlockDevice.cpp +++ b/Kernel/Devices/BlockDevice.cpp @@ -5,6 +5,7 @@ */ #include +#include namespace Kernel { @@ -26,6 +27,40 @@ void AsyncBlockDeviceRequest::start() BlockDevice::~BlockDevice() = default; +void BlockDevice::after_inserting_add_symlink_to_device_identifier_directory() +{ + VERIFY(m_symlink_sysfs_component); + SysFSBlockDevicesDirectory::the().devices_list({}).with([&](auto& list) -> void { + list.append(*m_symlink_sysfs_component); + }); +} + +void BlockDevice::before_will_be_destroyed_remove_symlink_from_device_identifier_directory() +{ + VERIFY(m_symlink_sysfs_component); + SysFSBlockDevicesDirectory::the().devices_list({}).with([&](auto& list) -> void { + list.remove(*m_symlink_sysfs_component); + }); +} + +// FIXME: This method will be eventually removed after all nodes in /sys/dev/block/ are symlinks +void BlockDevice::after_inserting_add_to_device_identifier_directory() +{ + VERIFY(m_sysfs_component); + SysFSBlockDevicesDirectory::the().devices_list({}).with([&](auto& list) -> void { + list.append(*m_sysfs_component); + }); +} + +// FIXME: This method will be eventually removed after all nodes in /sys/dev/block/ are symlinks +void BlockDevice::before_will_be_destroyed_remove_from_device_identifier_directory() +{ + VERIFY(m_sysfs_component); + SysFSBlockDevicesDirectory::the().devices_list({}).with([&](auto& list) -> void { + list.remove(*m_sysfs_component); + }); +} + bool BlockDevice::read_block(u64 index, UserOrKernelBuffer& buffer) { auto read_request_or_error = try_make_request(AsyncBlockDeviceRequest::Read, index, 1, buffer, m_block_size); diff --git a/Kernel/Devices/BlockDevice.h b/Kernel/Devices/BlockDevice.h index db24588f7f..71f40bc7ab 100644 --- a/Kernel/Devices/BlockDevice.h +++ b/Kernel/Devices/BlockDevice.h @@ -41,7 +41,14 @@ protected: protected: virtual bool is_block_device() const final { return true; } + virtual void after_inserting_add_symlink_to_device_identifier_directory() override final; + virtual void before_will_be_destroyed_remove_symlink_from_device_identifier_directory() override final; + private: + // FIXME: These methods will be eventually removed after all nodes in /sys/dev/block/ are symlinks + virtual void after_inserting_add_to_device_identifier_directory() override final; + virtual void before_will_be_destroyed_remove_from_device_identifier_directory() override final; + size_t m_block_size { 0 }; u8 m_block_size_log { 0 }; }; diff --git a/Kernel/Devices/CharacterDevice.cpp b/Kernel/Devices/CharacterDevice.cpp index e44a1433ac..3379139dfa 100644 --- a/Kernel/Devices/CharacterDevice.cpp +++ b/Kernel/Devices/CharacterDevice.cpp @@ -5,9 +5,44 @@ */ #include +#include namespace Kernel { CharacterDevice::~CharacterDevice() = default; +void CharacterDevice::after_inserting_add_symlink_to_device_identifier_directory() +{ + VERIFY(m_symlink_sysfs_component); + SysFSCharacterDevicesDirectory::the().devices_list({}).with([&](auto& list) -> void { + list.append(*m_symlink_sysfs_component); + }); +} + +void CharacterDevice::before_will_be_destroyed_remove_symlink_from_device_identifier_directory() +{ + VERIFY(m_symlink_sysfs_component); + SysFSCharacterDevicesDirectory::the().devices_list({}).with([&](auto& list) -> void { + list.remove(*m_symlink_sysfs_component); + }); +} + +// FIXME: This method will be eventually removed after all nodes in /sys/dev/char/ are symlinks +void CharacterDevice::after_inserting_add_to_device_identifier_directory() +{ + VERIFY(m_sysfs_component); + SysFSCharacterDevicesDirectory::the().devices_list({}).with([&](auto& list) -> void { + list.append(*m_sysfs_component); + }); +} + +// FIXME: This method will be eventually removed after all nodes in /sys/dev/char/ are symlinks +void CharacterDevice::before_will_be_destroyed_remove_from_device_identifier_directory() +{ + VERIFY(m_sysfs_component); + SysFSCharacterDevicesDirectory::the().devices_list({}).with([&](auto& list) -> void { + list.remove(*m_sysfs_component); + }); +} + } diff --git a/Kernel/Devices/CharacterDevice.h b/Kernel/Devices/CharacterDevice.h index 01be6e246d..37bf085aec 100644 --- a/Kernel/Devices/CharacterDevice.h +++ b/Kernel/Devices/CharacterDevice.h @@ -20,8 +20,15 @@ protected: { } + virtual void after_inserting_add_symlink_to_device_identifier_directory() override final; + virtual void before_will_be_destroyed_remove_symlink_from_device_identifier_directory() override final; + private: virtual bool is_character_device() const final { return true; } + + // FIXME: These methods will be eventually removed after all nodes in /sys/dev/char/ are symlinks + virtual void after_inserting_add_to_device_identifier_directory() override final; + virtual void before_will_be_destroyed_remove_from_device_identifier_directory() override final; }; } diff --git a/Kernel/Devices/Device.cpp b/Kernel/Devices/Device.cpp index 25b727ce96..8d4c0b1877 100644 --- a/Kernel/Devices/Device.cpp +++ b/Kernel/Devices/Device.cpp @@ -38,31 +38,13 @@ void Device::after_inserting() VERIFY(!m_sysfs_component); auto sys_fs_component = SysFSDeviceComponent::must_create(*this); m_sysfs_component = sys_fs_component; - if (is_block_device()) { - SysFSBlockDevicesDirectory::the().devices_list({}).with([&](auto& list) -> void { - list.append(sys_fs_component); - }); - return; - } - VERIFY(is_character_device()); - SysFSCharacterDevicesDirectory::the().devices_list({}).with([&](auto& list) -> void { - list.append(sys_fs_component); - }); + after_inserting_add_to_device_identifier_directory(); } void Device::will_be_destroyed() { VERIFY(m_sysfs_component); - if (is_block_device()) { - SysFSBlockDevicesDirectory::the().devices_list({}).with([&](auto& list) -> void { - list.remove(*m_sysfs_component); - }); - } else { - VERIFY(is_character_device()); - SysFSCharacterDevicesDirectory::the().devices_list({}).with([&](auto& list) -> void { - list.remove(*m_sysfs_component); - }); - } + before_will_be_destroyed_remove_from_device_identifier_directory(); before_will_be_destroyed_remove_from_device_management(); } diff --git a/Kernel/Devices/Device.h b/Kernel/Devices/Device.h index 84f2086ac8..11e3a035a1 100644 --- a/Kernel/Devices/Device.h +++ b/Kernel/Devices/Device.h @@ -72,6 +72,14 @@ protected: void after_inserting_add_to_device_management(); void before_will_be_destroyed_remove_from_device_management(); + virtual void after_inserting_add_symlink_to_device_identifier_directory() = 0; + virtual void before_will_be_destroyed_remove_symlink_from_device_identifier_directory() = 0; + + // FIXME: These methods will be eventually removed after all nodes in /sys/dev/block/ and + // /sys/dev/char/ are symlinks. + virtual void after_inserting_add_to_device_identifier_directory() = 0; + virtual void before_will_be_destroyed_remove_from_device_identifier_directory() = 0; + private: MajorNumber const m_major { 0 }; MinorNumber const m_minor { 0 }; @@ -82,9 +90,12 @@ private: Spinlock m_requests_lock; DoublyLinkedList> m_requests; - RefPtr m_sysfs_component; protected: + // FIXME: This pointer will be eventually removed after all nodes in /sys/dev/block/ and + // /sys/dev/char/ are symlinks. + RefPtr m_sysfs_component; + RefPtr m_symlink_sysfs_component; RefPtr m_sysfs_device_directory; }; diff --git a/Kernel/FileSystem/SysFS/Subsystems/DeviceIdentifiers/BlockDevicesDirectory.h b/Kernel/FileSystem/SysFS/Subsystems/DeviceIdentifiers/BlockDevicesDirectory.h index a1d2069361..c90b5ede67 100644 --- a/Kernel/FileSystem/SysFS/Subsystems/DeviceIdentifiers/BlockDevicesDirectory.h +++ b/Kernel/FileSystem/SysFS/Subsystems/DeviceIdentifiers/BlockDevicesDirectory.h @@ -12,11 +12,8 @@ namespace Kernel { -class Device; -class StorageDevice; +class BlockDevice; class SysFSBlockDevicesDirectory final : public SysFSDirectory { - friend class Device; - friend class StorageDevice; public: virtual StringView name() const override { return "block"sv; } @@ -24,7 +21,7 @@ public: static SysFSBlockDevicesDirectory& the(); - ChildList& devices_list(Badge) { return m_child_components; } + ChildList& devices_list(Badge) { return m_child_components; } private: explicit SysFSBlockDevicesDirectory(SysFSDeviceIdentifiersDirectory const&); diff --git a/Kernel/FileSystem/SysFS/Subsystems/DeviceIdentifiers/CharacterDevicesDirectory.h b/Kernel/FileSystem/SysFS/Subsystems/DeviceIdentifiers/CharacterDevicesDirectory.h index 7fe13f7a52..ab22bc5e70 100644 --- a/Kernel/FileSystem/SysFS/Subsystems/DeviceIdentifiers/CharacterDevicesDirectory.h +++ b/Kernel/FileSystem/SysFS/Subsystems/DeviceIdentifiers/CharacterDevicesDirectory.h @@ -12,10 +12,8 @@ namespace Kernel { -class Device; -class DisplayConnector; +class CharacterDevice; class SysFSCharacterDevicesDirectory final : public SysFSDirectory { - friend class DisplayConnector; public: virtual StringView name() const override { return "char"sv; } @@ -23,7 +21,7 @@ public: static SysFSCharacterDevicesDirectory& the(); - ChildList& devices_list(Badge) { return m_child_components; } + ChildList& devices_list(Badge) { return m_child_components; } private: explicit SysFSCharacterDevicesDirectory(SysFSDeviceIdentifiersDirectory const&); diff --git a/Kernel/Graphics/DisplayConnector.cpp b/Kernel/Graphics/DisplayConnector.cpp index 9cf8f327b8..4f117b8490 100644 --- a/Kernel/Graphics/DisplayConnector.cpp +++ b/Kernel/Graphics/DisplayConnector.cpp @@ -62,10 +62,8 @@ void DisplayConnector::will_be_destroyed() GraphicsManagement::the().detach_display_connector({}, *this); VERIFY(m_symlink_sysfs_component); - VERIFY(!is_block_device()); - SysFSCharacterDevicesDirectory::the().m_child_components.with([&](auto& list) -> void { - list.remove(*m_symlink_sysfs_component); - }); + before_will_be_destroyed_remove_symlink_from_device_identifier_directory(); + m_symlink_sysfs_component.clear(); SysFSDisplayConnectorsDirectory::the().unplug({}, *m_sysfs_device_directory); before_will_be_destroyed_remove_from_device_management(); @@ -80,10 +78,7 @@ void DisplayConnector::after_inserting() VERIFY(!m_symlink_sysfs_component); auto sys_fs_component = MUST(SysFSSymbolicLinkDeviceComponent::try_create(SysFSDeviceIdentifiersDirectory::the(), *this, *m_sysfs_device_directory)); m_symlink_sysfs_component = sys_fs_component; - VERIFY(!is_block_device()); - SysFSCharacterDevicesDirectory::the().m_child_components.with([&](auto& list) -> void { - list.append(*m_symlink_sysfs_component); - }); + after_inserting_add_symlink_to_device_identifier_directory(); auto rounded_size = MUST(Memory::page_round_up(m_framebuffer_resource_size)); diff --git a/Kernel/Storage/StorageDevice.cpp b/Kernel/Storage/StorageDevice.cpp index 991962878c..d98a1af1c5 100644 --- a/Kernel/Storage/StorageDevice.cpp +++ b/Kernel/Storage/StorageDevice.cpp @@ -37,19 +37,13 @@ void StorageDevice::after_inserting() VERIFY(!m_symlink_sysfs_component); auto sys_fs_component = MUST(SysFSSymbolicLinkDeviceComponent::try_create(SysFSDeviceIdentifiersDirectory::the(), *this, *m_sysfs_device_directory)); m_symlink_sysfs_component = sys_fs_component; - VERIFY(is_block_device()); - SysFSBlockDevicesDirectory::the().m_child_components.with([&](auto& list) -> void { - list.append(sys_fs_component); - }); + after_inserting_add_symlink_to_device_identifier_directory(); } void StorageDevice::will_be_destroyed() { VERIFY(m_symlink_sysfs_component); - VERIFY(is_block_device()); - SysFSBlockDevicesDirectory::the().m_child_components.with([&](auto& list) -> void { - list.remove(*m_symlink_sysfs_component); - }); + before_will_be_destroyed_remove_symlink_from_device_identifier_directory(); m_symlink_sysfs_component.clear(); SysFSStorageDirectory::the().unplug({}, *m_sysfs_device_directory); before_will_be_destroyed_remove_from_device_management();