From 30d3f2789e3478da415ccdc6766872d3d58918e2 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sun, 11 Dec 2022 18:48:20 +0100 Subject: [PATCH] Kernel: Propagate errors during network adapter detection/initialization When scanning for network adapters, we give each driver a chance to claim the PCI device and whoever claims it first gets to keep it. Before this patch, the driver API returned a LockRefPtr, which made it impossible to propagate errors that occurred during detection and/or initialization. This patch changes the API so that errors can bubble all the way out the PCI enumeration in NetworkingManagement::initialize() where we perform all the network adapter auto-detection on boot. When we eventually start to support hot-plugging network adapter in the future, it will be even more important to propagate errors instead of swallowing them. Importantly, before this patch, some errors were "handled" by panicking the kernel. This is no longer the case. 7 FIXMEs were killed in the making of this commit. :^) --- Kernel/Net/Intel/E1000ENetworkAdapter.cpp | 23 ++++++-------- Kernel/Net/Intel/E1000ENetworkAdapter.h | 2 +- Kernel/Net/Intel/E1000NetworkAdapter.cpp | 23 ++++++-------- Kernel/Net/Intel/E1000NetworkAdapter.h | 2 +- Kernel/Net/NE2000/NetworkAdapter.cpp | 13 +++----- Kernel/Net/NE2000/NetworkAdapter.h | 2 +- Kernel/Net/NetworkingManagement.cpp | 32 +++++++++++--------- Kernel/Net/NetworkingManagement.h | 2 +- Kernel/Net/Realtek/RTL8139NetworkAdapter.cpp | 13 +++----- Kernel/Net/Realtek/RTL8139NetworkAdapter.h | 2 +- Kernel/Net/Realtek/RTL8168NetworkAdapter.cpp | 15 ++++----- Kernel/Net/Realtek/RTL8168NetworkAdapter.h | 2 +- 12 files changed, 58 insertions(+), 73 deletions(-) diff --git a/Kernel/Net/Intel/E1000ENetworkAdapter.cpp b/Kernel/Net/Intel/E1000ENetworkAdapter.cpp index cdbf1fab46..395416d1a7 100644 --- a/Kernel/Net/Intel/E1000ENetworkAdapter.cpp +++ b/Kernel/Net/Intel/E1000ENetworkAdapter.cpp @@ -181,24 +181,19 @@ static bool is_valid_device_id(u16 device_id) } } -UNMAP_AFTER_INIT LockRefPtr E1000ENetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier) +UNMAP_AFTER_INIT ErrorOr> E1000ENetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier) { if (pci_device_identifier.hardware_id().vendor_id != PCI::VendorID::Intel) - return {}; + return nullptr; if (!is_valid_device_id(pci_device_identifier.hardware_id().device_id)) - return {}; + return nullptr; u8 irq = pci_device_identifier.interrupt_line().value(); - // FIXME: Better propagate errors here - auto interface_name_or_error = NetworkingManagement::generate_interface_name_from_pci_address(pci_device_identifier); - if (interface_name_or_error.is_error()) - return {}; - auto registers_io_window = IOWindow::create_for_pci_device_bar(pci_device_identifier, PCI::HeaderType0BaseRegister::BAR0).release_value_but_fixme_should_propagate_errors(); - auto adapter = adopt_lock_ref_if_nonnull(new (nothrow) E1000ENetworkAdapter(pci_device_identifier.address(), irq, move(registers_io_window), interface_name_or_error.release_value())); - if (!adapter) - return {}; - if (adapter->initialize()) - return adapter; - return {}; + auto interface_name = TRY(NetworkingManagement::generate_interface_name_from_pci_address(pci_device_identifier)); + auto registers_io_window = TRY(IOWindow::create_for_pci_device_bar(pci_device_identifier, PCI::HeaderType0BaseRegister::BAR0)); + auto adapter = TRY(adopt_nonnull_lock_ref_or_enomem(new (nothrow) E1000ENetworkAdapter(pci_device_identifier.address(), irq, move(registers_io_window), move(interface_name)))); + if (!adapter->initialize()) + return Error::from_string_literal("E1000ENetworkAdapter: Unable to initialize adapter"); + return adapter; } UNMAP_AFTER_INIT bool E1000ENetworkAdapter::initialize() diff --git a/Kernel/Net/Intel/E1000ENetworkAdapter.h b/Kernel/Net/Intel/E1000ENetworkAdapter.h index 64ef266401..ea62860d1f 100644 --- a/Kernel/Net/Intel/E1000ENetworkAdapter.h +++ b/Kernel/Net/Intel/E1000ENetworkAdapter.h @@ -21,7 +21,7 @@ namespace Kernel { class E1000ENetworkAdapter final : public E1000NetworkAdapter { public: - static LockRefPtr try_to_initialize(PCI::DeviceIdentifier const&); + static ErrorOr> try_to_initialize(PCI::DeviceIdentifier const&); virtual bool initialize() override; diff --git a/Kernel/Net/Intel/E1000NetworkAdapter.cpp b/Kernel/Net/Intel/E1000NetworkAdapter.cpp index d7e5239b9b..9a755569e1 100644 --- a/Kernel/Net/Intel/E1000NetworkAdapter.cpp +++ b/Kernel/Net/Intel/E1000NetworkAdapter.cpp @@ -159,24 +159,19 @@ UNMAP_AFTER_INIT static bool is_valid_device_id(u16 device_id) } } -UNMAP_AFTER_INIT LockRefPtr E1000NetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier) +UNMAP_AFTER_INIT ErrorOr> E1000NetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier) { if (pci_device_identifier.hardware_id().vendor_id != PCI::VendorID::Intel) - return {}; + return nullptr; if (!is_valid_device_id(pci_device_identifier.hardware_id().device_id)) - return {}; + return nullptr; u8 irq = pci_device_identifier.interrupt_line().value(); - // FIXME: Better propagate errors here - auto interface_name_or_error = NetworkingManagement::generate_interface_name_from_pci_address(pci_device_identifier); - if (interface_name_or_error.is_error()) - return {}; - auto registers_io_window = IOWindow::create_for_pci_device_bar(pci_device_identifier, PCI::HeaderType0BaseRegister::BAR0).release_value_but_fixme_should_propagate_errors(); - auto adapter = adopt_lock_ref_if_nonnull(new (nothrow) E1000NetworkAdapter(pci_device_identifier.address(), irq, move(registers_io_window), interface_name_or_error.release_value())); - if (!adapter) - return {}; - if (adapter->initialize()) - return adapter; - return {}; + auto interface_name = TRY(NetworkingManagement::generate_interface_name_from_pci_address(pci_device_identifier)); + auto registers_io_window = TRY(IOWindow::create_for_pci_device_bar(pci_device_identifier, PCI::HeaderType0BaseRegister::BAR0)); + auto adapter = TRY(adopt_nonnull_lock_ref_or_enomem(new (nothrow) E1000NetworkAdapter(pci_device_identifier.address(), irq, move(registers_io_window), move(interface_name)))); + if (!adapter->initialize()) + return Error::from_string_literal("E1000NetworkAdapter: Unable to initialize adapter"); + return adapter; } UNMAP_AFTER_INIT void E1000NetworkAdapter::setup_link() diff --git a/Kernel/Net/Intel/E1000NetworkAdapter.h b/Kernel/Net/Intel/E1000NetworkAdapter.h index 3f40733fbf..d14920dda5 100644 --- a/Kernel/Net/Intel/E1000NetworkAdapter.h +++ b/Kernel/Net/Intel/E1000NetworkAdapter.h @@ -20,7 +20,7 @@ class E1000NetworkAdapter : public NetworkAdapter , public PCI::Device , public IRQHandler { public: - static LockRefPtr try_to_initialize(PCI::DeviceIdentifier const&); + static ErrorOr> try_to_initialize(PCI::DeviceIdentifier const&); virtual bool initialize(); diff --git a/Kernel/Net/NE2000/NetworkAdapter.cpp b/Kernel/Net/NE2000/NetworkAdapter.cpp index 30dd731c3d..65fba19d1f 100644 --- a/Kernel/Net/NE2000/NetworkAdapter.cpp +++ b/Kernel/Net/NE2000/NetworkAdapter.cpp @@ -138,7 +138,7 @@ struct [[gnu::packed]] received_packet_header { u16 length; }; -UNMAP_AFTER_INIT LockRefPtr NE2000NetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier) +UNMAP_AFTER_INIT ErrorOr> NE2000NetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier) { constexpr auto ne2k_ids = Array { PCI::HardwareID { 0x10EC, 0x8029 }, // RealTek RTL-8029(AS) @@ -156,14 +156,11 @@ UNMAP_AFTER_INIT LockRefPtr NE2000NetworkAdapter::try_to_i PCI::HardwareID { 0x8c4a, 0x1980 }, // Winbond W89C940 (misprogrammed) }; if (!ne2k_ids.span().contains_slow(pci_device_identifier.hardware_id())) - return {}; + return nullptr; u8 irq = pci_device_identifier.interrupt_line().value(); - // FIXME: Better propagate errors here - auto interface_name_or_error = NetworkingManagement::generate_interface_name_from_pci_address(pci_device_identifier); - if (interface_name_or_error.is_error()) - return {}; - auto registers_io_window = MUST(IOWindow::create_for_pci_device_bar(pci_device_identifier, PCI::HeaderType0BaseRegister::BAR0)); - return adopt_lock_ref_if_nonnull(new (nothrow) NE2000NetworkAdapter(pci_device_identifier.address(), irq, move(registers_io_window), interface_name_or_error.release_value())); + auto interface_name = TRY(NetworkingManagement::generate_interface_name_from_pci_address(pci_device_identifier)); + auto registers_io_window = TRY(IOWindow::create_for_pci_device_bar(pci_device_identifier, PCI::HeaderType0BaseRegister::BAR0)); + return TRY(adopt_nonnull_lock_ref_or_enomem(new (nothrow) NE2000NetworkAdapter(pci_device_identifier.address(), irq, move(registers_io_window), move(interface_name)))); } UNMAP_AFTER_INIT NE2000NetworkAdapter::NE2000NetworkAdapter(PCI::Address address, u8 irq, NonnullOwnPtr registers_io_window, NonnullOwnPtr interface_name) diff --git a/Kernel/Net/NE2000/NetworkAdapter.h b/Kernel/Net/NE2000/NetworkAdapter.h index 1460da1614..ee37fdd6a2 100644 --- a/Kernel/Net/NE2000/NetworkAdapter.h +++ b/Kernel/Net/NE2000/NetworkAdapter.h @@ -20,7 +20,7 @@ class NE2000NetworkAdapter final : public NetworkAdapter , public PCI::Device , public IRQHandler { public: - static LockRefPtr try_to_initialize(PCI::DeviceIdentifier const&); + static ErrorOr> try_to_initialize(PCI::DeviceIdentifier const&); virtual ~NE2000NetworkAdapter() override; diff --git a/Kernel/Net/NetworkingManagement.cpp b/Kernel/Net/NetworkingManagement.cpp index e3fe772bf6..2da3aef1e9 100644 --- a/Kernel/Net/NetworkingManagement.cpp +++ b/Kernel/Net/NetworkingManagement.cpp @@ -93,19 +93,19 @@ ErrorOr> NetworkingManagement::generate_interface_name_fr return name; } -UNMAP_AFTER_INIT LockRefPtr NetworkingManagement::determine_network_device(PCI::DeviceIdentifier const& device_identifier) const +UNMAP_AFTER_INIT ErrorOr> NetworkingManagement::determine_network_device(PCI::DeviceIdentifier const& device_identifier) const { - if (auto candidate = E1000NetworkAdapter::try_to_initialize(device_identifier); !candidate.is_null()) - return candidate; - if (auto candidate = E1000ENetworkAdapter::try_to_initialize(device_identifier); !candidate.is_null()) - return candidate; - if (auto candidate = RTL8139NetworkAdapter::try_to_initialize(device_identifier); !candidate.is_null()) - return candidate; - if (auto candidate = RTL8168NetworkAdapter::try_to_initialize(device_identifier); !candidate.is_null()) - return candidate; - if (auto candidate = NE2000NetworkAdapter::try_to_initialize(device_identifier); !candidate.is_null()) - return candidate; - return {}; + if (auto candidate = TRY(E1000NetworkAdapter::try_to_initialize(device_identifier))) + return candidate.release_nonnull(); + if (auto candidate = TRY(E1000ENetworkAdapter::try_to_initialize(device_identifier))) + return candidate.release_nonnull(); + if (auto candidate = TRY(RTL8139NetworkAdapter::try_to_initialize(device_identifier))) + return candidate.release_nonnull(); + if (auto candidate = TRY(RTL8168NetworkAdapter::try_to_initialize(device_identifier))) + return candidate.release_nonnull(); + if (auto candidate = TRY(NE2000NetworkAdapter::try_to_initialize(device_identifier))) + return candidate.release_nonnull(); + return Error::from_string_literal("Unsupported network adapter"); } bool NetworkingManagement::initialize() @@ -115,8 +115,12 @@ bool NetworkingManagement::initialize() // Note: PCI class 2 is the class of Network devices if (device_identifier.class_code().value() != 0x02) return; - if (auto adapter = determine_network_device(device_identifier); !adapter.is_null()) - m_adapters.with([&](auto& adapters) { adapters.append(adapter.release_nonnull()); }); + auto result = determine_network_device(device_identifier); + if (result.is_error()) { + dmesgln("Failed to initialize network adapter ({} {}): {}", device_identifier.address(), device_identifier.hardware_id(), result.error()); + return; + } + m_adapters.with([&](auto& adapters) { adapters.append(result.release_value()); }); })); } auto loopback = LoopbackAdapter::try_create(); diff --git a/Kernel/Net/NetworkingManagement.h b/Kernel/Net/NetworkingManagement.h index 2876993dfd..8b8955e919 100644 --- a/Kernel/Net/NetworkingManagement.h +++ b/Kernel/Net/NetworkingManagement.h @@ -40,7 +40,7 @@ public: NonnullLockRefPtr loopback_adapter() const; private: - LockRefPtr determine_network_device(PCI::DeviceIdentifier const&) const; + ErrorOr> determine_network_device(PCI::DeviceIdentifier const&) const; SpinlockProtected> m_adapters { LockRank::None }; LockRefPtr m_loopback_adapter; diff --git a/Kernel/Net/Realtek/RTL8139NetworkAdapter.cpp b/Kernel/Net/Realtek/RTL8139NetworkAdapter.cpp index da87dfa8ba..3d2fecdb89 100644 --- a/Kernel/Net/Realtek/RTL8139NetworkAdapter.cpp +++ b/Kernel/Net/Realtek/RTL8139NetworkAdapter.cpp @@ -112,18 +112,15 @@ namespace Kernel { #define RX_BUFFER_SIZE 32768 #define TX_BUFFER_SIZE PACKET_SIZE_MAX -UNMAP_AFTER_INIT LockRefPtr RTL8139NetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier) +UNMAP_AFTER_INIT ErrorOr> RTL8139NetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier) { constexpr PCI::HardwareID rtl8139_id = { 0x10EC, 0x8139 }; if (pci_device_identifier.hardware_id() != rtl8139_id) - return {}; + return nullptr; u8 irq = pci_device_identifier.interrupt_line().value(); - // FIXME: Better propagate errors here - auto interface_name_or_error = NetworkingManagement::generate_interface_name_from_pci_address(pci_device_identifier); - if (interface_name_or_error.is_error()) - return {}; - auto registers_io_window = MUST(IOWindow::create_for_pci_device_bar(pci_device_identifier, PCI::HeaderType0BaseRegister::BAR0)); - return adopt_lock_ref_if_nonnull(new (nothrow) RTL8139NetworkAdapter(pci_device_identifier.address(), irq, move(registers_io_window), interface_name_or_error.release_value())); + auto interface_name = TRY(NetworkingManagement::generate_interface_name_from_pci_address(pci_device_identifier)); + auto registers_io_window = TRY(IOWindow::create_for_pci_device_bar(pci_device_identifier, PCI::HeaderType0BaseRegister::BAR0)); + return TRY(adopt_nonnull_lock_ref_or_enomem(new (nothrow) RTL8139NetworkAdapter(pci_device_identifier.address(), irq, move(registers_io_window), move(interface_name)))); } UNMAP_AFTER_INIT RTL8139NetworkAdapter::RTL8139NetworkAdapter(PCI::Address address, u8 irq, NonnullOwnPtr registers_io_window, NonnullOwnPtr interface_name) diff --git a/Kernel/Net/Realtek/RTL8139NetworkAdapter.h b/Kernel/Net/Realtek/RTL8139NetworkAdapter.h index 72d82f08af..6d95c85785 100644 --- a/Kernel/Net/Realtek/RTL8139NetworkAdapter.h +++ b/Kernel/Net/Realtek/RTL8139NetworkAdapter.h @@ -22,7 +22,7 @@ class RTL8139NetworkAdapter final : public NetworkAdapter , public PCI::Device , public IRQHandler { public: - static LockRefPtr try_to_initialize(PCI::DeviceIdentifier const&); + static ErrorOr> try_to_initialize(PCI::DeviceIdentifier const&); virtual ~RTL8139NetworkAdapter() override; diff --git a/Kernel/Net/Realtek/RTL8168NetworkAdapter.cpp b/Kernel/Net/Realtek/RTL8168NetworkAdapter.cpp index 85ce6b437b..d61d0f629d 100644 --- a/Kernel/Net/Realtek/RTL8168NetworkAdapter.cpp +++ b/Kernel/Net/Realtek/RTL8168NetworkAdapter.cpp @@ -182,19 +182,16 @@ namespace Kernel { #define TX_BUFFER_SIZE 0x1FF8 #define RX_BUFFER_SIZE 0x1FF8 // FIXME: this should be increased (0x3FFF) -UNMAP_AFTER_INIT LockRefPtr RTL8168NetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier) +UNMAP_AFTER_INIT ErrorOr> RTL8168NetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier) { if (pci_device_identifier.hardware_id().vendor_id != PCI::VendorID::Realtek) - return {}; + return nullptr; if (pci_device_identifier.hardware_id().device_id != 0x8168) - return {}; + return nullptr; u8 irq = pci_device_identifier.interrupt_line().value(); - // FIXME: Better propagate errors here - auto interface_name_or_error = NetworkingManagement::generate_interface_name_from_pci_address(pci_device_identifier); - if (interface_name_or_error.is_error()) - return {}; - auto registers_io_window = MUST(IOWindow::create_for_pci_device_bar(pci_device_identifier, PCI::HeaderType0BaseRegister::BAR0)); - return adopt_lock_ref_if_nonnull(new (nothrow) RTL8168NetworkAdapter(pci_device_identifier.address(), irq, move(registers_io_window), interface_name_or_error.release_value())); + auto interface_name = TRY(NetworkingManagement::generate_interface_name_from_pci_address(pci_device_identifier)); + auto registers_io_window = TRY(IOWindow::create_for_pci_device_bar(pci_device_identifier, PCI::HeaderType0BaseRegister::BAR0)); + return TRY(adopt_nonnull_lock_ref_or_enomem(new (nothrow) RTL8168NetworkAdapter(pci_device_identifier.address(), irq, move(registers_io_window), move(interface_name)))); } bool RTL8168NetworkAdapter::determine_supported_version() const diff --git a/Kernel/Net/Realtek/RTL8168NetworkAdapter.h b/Kernel/Net/Realtek/RTL8168NetworkAdapter.h index a0ca643c2b..b075c25aee 100644 --- a/Kernel/Net/Realtek/RTL8168NetworkAdapter.h +++ b/Kernel/Net/Realtek/RTL8168NetworkAdapter.h @@ -22,7 +22,7 @@ class RTL8168NetworkAdapter final : public NetworkAdapter , public PCI::Device , public IRQHandler { public: - static LockRefPtr try_to_initialize(PCI::DeviceIdentifier const&); + static ErrorOr> try_to_initialize(PCI::DeviceIdentifier const&); virtual ~RTL8168NetworkAdapter() override;