From 64aaf263a2adf562c20f0db371c01e8421b604f7 Mon Sep 17 00:00:00 2001 From: Liav A Date: Sun, 17 Oct 2021 16:05:14 +0300 Subject: [PATCH] Kernel/Net: Generate interface name near construction point This change allows the Kernel to actually construct other interfaces besides the E1000 type. This solves a breakage that was introduced recently because of move semantics. A couple of points on this patch: 1. In current situation, we can waste time to create a KString and throw it for nothing. This patch ensures we only create it near construction point so we know we actually need it. 2. It's very likely to assume that non-x86 machines will expose network device with a device tree (or with ACPI). The raspberry pi machine is a good example of that. Therefore, each driver should explicitly ask the correct interface name generation method, and this patch simplifies this pattern greatly, especially in a case where the same network device can appear as a PCI device or as device in another bus type on the same platform target. For example, the (in)famous ne2000 device can be used either as a PCI device or as an ISA device, depending on the model. 3. In my opinion, it seems much more readable to construct the name near calling point of the object constructor than to just pass it with move semantics. --- Kernel/Net/E1000ENetworkAdapter.cpp | 9 +++++++-- Kernel/Net/E1000ENetworkAdapter.h | 2 +- Kernel/Net/E1000NetworkAdapter.cpp | 9 +++++++-- Kernel/Net/E1000NetworkAdapter.h | 2 +- Kernel/Net/NE2000NetworkAdapter.cpp | 9 +++++++-- Kernel/Net/NE2000NetworkAdapter.h | 2 +- Kernel/Net/NetworkingManagement.cpp | 23 ++++++++++++----------- Kernel/Net/NetworkingManagement.h | 3 +++ Kernel/Net/RTL8139NetworkAdapter.cpp | 9 +++++++-- Kernel/Net/RTL8139NetworkAdapter.h | 2 +- Kernel/Net/RTL8168NetworkAdapter.cpp | 9 +++++++-- Kernel/Net/RTL8168NetworkAdapter.h | 2 +- 12 files changed, 55 insertions(+), 26 deletions(-) diff --git a/Kernel/Net/E1000ENetworkAdapter.cpp b/Kernel/Net/E1000ENetworkAdapter.cpp index 15a0699a131..8db7b9014a8 100644 --- a/Kernel/Net/E1000ENetworkAdapter.cpp +++ b/Kernel/Net/E1000ENetworkAdapter.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include namespace Kernel { @@ -180,14 +181,18 @@ static bool is_valid_device_id(u16 device_id) } } -UNMAP_AFTER_INIT RefPtr E1000ENetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier, NonnullOwnPtr interface_name) +UNMAP_AFTER_INIT RefPtr E1000ENetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier) { if (pci_device_identifier.hardware_id().vendor_id != PCI::VendorID::Intel) return {}; if (!is_valid_device_id(pci_device_identifier.hardware_id().device_id)) return {}; u8 irq = pci_device_identifier.interrupt_line().value(); - auto adapter = adopt_ref_if_nonnull(new (nothrow) E1000ENetworkAdapter(pci_device_identifier.address(), irq, move(interface_name))); + // 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 adapter = adopt_ref_if_nonnull(new (nothrow) E1000ENetworkAdapter(pci_device_identifier.address(), irq, interface_name_or_error.release_value())); if (!adapter) return {}; if (adapter->initialize()) diff --git a/Kernel/Net/E1000ENetworkAdapter.h b/Kernel/Net/E1000ENetworkAdapter.h index 22038206321..e79fafc0de4 100644 --- a/Kernel/Net/E1000ENetworkAdapter.h +++ b/Kernel/Net/E1000ENetworkAdapter.h @@ -21,7 +21,7 @@ namespace Kernel { class E1000ENetworkAdapter final : public E1000NetworkAdapter { public: - static RefPtr try_to_initialize(PCI::DeviceIdentifier const&, NonnullOwnPtr); + static RefPtr try_to_initialize(PCI::DeviceIdentifier const&); virtual bool initialize() override; diff --git a/Kernel/Net/E1000NetworkAdapter.cpp b/Kernel/Net/E1000NetworkAdapter.cpp index 46097bb9afd..e796c357a82 100644 --- a/Kernel/Net/E1000NetworkAdapter.cpp +++ b/Kernel/Net/E1000NetworkAdapter.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include namespace Kernel { @@ -158,14 +159,18 @@ UNMAP_AFTER_INIT static bool is_valid_device_id(u16 device_id) } } -UNMAP_AFTER_INIT RefPtr E1000NetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier, NonnullOwnPtr interface_name) +UNMAP_AFTER_INIT RefPtr E1000NetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier) { if (pci_device_identifier.hardware_id().vendor_id != PCI::VendorID::Intel) return {}; if (!is_valid_device_id(pci_device_identifier.hardware_id().device_id)) return {}; u8 irq = pci_device_identifier.interrupt_line().value(); - auto adapter = adopt_ref_if_nonnull(new (nothrow) E1000NetworkAdapter(pci_device_identifier.address(), irq, move(interface_name))); + // 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 adapter = adopt_ref_if_nonnull(new (nothrow) E1000NetworkAdapter(pci_device_identifier.address(), irq, interface_name_or_error.release_value())); if (!adapter) return {}; if (adapter->initialize()) diff --git a/Kernel/Net/E1000NetworkAdapter.h b/Kernel/Net/E1000NetworkAdapter.h index e695e4fd073..8ed931c5638 100644 --- a/Kernel/Net/E1000NetworkAdapter.h +++ b/Kernel/Net/E1000NetworkAdapter.h @@ -20,7 +20,7 @@ class E1000NetworkAdapter : public NetworkAdapter , public PCI::Device , public IRQHandler { public: - static RefPtr try_to_initialize(PCI::DeviceIdentifier const&, NonnullOwnPtr); + static RefPtr try_to_initialize(PCI::DeviceIdentifier const&); virtual bool initialize(); diff --git a/Kernel/Net/NE2000NetworkAdapter.cpp b/Kernel/Net/NE2000NetworkAdapter.cpp index 7319262b276..3062647aa2e 100644 --- a/Kernel/Net/NE2000NetworkAdapter.cpp +++ b/Kernel/Net/NE2000NetworkAdapter.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include namespace Kernel { @@ -137,7 +138,7 @@ struct [[gnu::packed]] received_packet_header { u16 length; }; -UNMAP_AFTER_INIT RefPtr NE2000NetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier, NonnullOwnPtr interface_name) +UNMAP_AFTER_INIT RefPtr NE2000NetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier) { constexpr auto ne2k_ids = Array { PCI::HardwareID { 0x10EC, 0x8029 }, // RealTek RTL-8029(AS) @@ -157,7 +158,11 @@ UNMAP_AFTER_INIT RefPtr NE2000NetworkAdapter::try_to_initi if (!ne2k_ids.span().contains_slow(pci_device_identifier.hardware_id())) return {}; u8 irq = pci_device_identifier.interrupt_line().value(); - return adopt_ref_if_nonnull(new (nothrow) NE2000NetworkAdapter(pci_device_identifier.address(), irq, move(interface_name))); + // 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 {}; + return adopt_ref_if_nonnull(new (nothrow) NE2000NetworkAdapter(pci_device_identifier.address(), irq, interface_name_or_error.release_value())); } UNMAP_AFTER_INIT NE2000NetworkAdapter::NE2000NetworkAdapter(PCI::Address address, u8 irq, NonnullOwnPtr interface_name) diff --git a/Kernel/Net/NE2000NetworkAdapter.h b/Kernel/Net/NE2000NetworkAdapter.h index 51e2c33769f..b29d3b514ac 100644 --- a/Kernel/Net/NE2000NetworkAdapter.h +++ b/Kernel/Net/NE2000NetworkAdapter.h @@ -20,7 +20,7 @@ class NE2000NetworkAdapter final : public NetworkAdapter , public PCI::Device , public IRQHandler { public: - static RefPtr try_to_initialize(PCI::DeviceIdentifier const&, NonnullOwnPtr); + static RefPtr try_to_initialize(PCI::DeviceIdentifier const&); virtual ~NE2000NetworkAdapter() override; diff --git a/Kernel/Net/NetworkingManagement.cpp b/Kernel/Net/NetworkingManagement.cpp index 0f2106193bc..3464695d7f1 100644 --- a/Kernel/Net/NetworkingManagement.cpp +++ b/Kernel/Net/NetworkingManagement.cpp @@ -74,26 +74,27 @@ RefPtr NetworkingManagement::lookup_by_name(const StringView& na return found_adapter; } -UNMAP_AFTER_INIT RefPtr NetworkingManagement::determine_network_device(PCI::DeviceIdentifier const& device_identifier) const +KResultOr> NetworkingManagement::generate_interface_name_from_pci_address(PCI::DeviceIdentifier const& device_identifier) { + VERIFY(device_identifier.class_code().value() == 0x2); // Note: This stands for e - "Ethernet", p - "Port" as for PCI bus, "s" for slot as for PCI slot auto name = String::formatted("ep{}s{}", device_identifier.address().bus(), device_identifier.address().device()); VERIFY(!NetworkingManagement::the().lookup_by_name(name)); - // TODO: We need some way to to format data into a `KString`. - auto interface_name_or_error = KString::try_create(name.view()); - if (interface_name_or_error.is_error()) - return {}; - auto interface_name = interface_name_or_error.release_value(); - if (auto candidate = E1000NetworkAdapter::try_to_initialize(device_identifier, move(interface_name)); !candidate.is_null()) + return KString::try_create(name.view()); +} + +UNMAP_AFTER_INIT RefPtr 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, move(interface_name)); !candidate.is_null()) + if (auto candidate = E1000ENetworkAdapter::try_to_initialize(device_identifier); !candidate.is_null()) return candidate; - if (auto candidate = RTL8139NetworkAdapter::try_to_initialize(device_identifier, move(interface_name)); !candidate.is_null()) + if (auto candidate = RTL8139NetworkAdapter::try_to_initialize(device_identifier); !candidate.is_null()) return candidate; - if (auto candidate = RTL8168NetworkAdapter::try_to_initialize(device_identifier, move(interface_name)); !candidate.is_null()) + if (auto candidate = RTL8168NetworkAdapter::try_to_initialize(device_identifier); !candidate.is_null()) return candidate; - if (auto candidate = NE2000NetworkAdapter::try_to_initialize(device_identifier, move(interface_name)); !candidate.is_null()) + if (auto candidate = NE2000NetworkAdapter::try_to_initialize(device_identifier); !candidate.is_null()) return candidate; return {}; } diff --git a/Kernel/Net/NetworkingManagement.h b/Kernel/Net/NetworkingManagement.h index cab54711caa..418b43c5268 100644 --- a/Kernel/Net/NetworkingManagement.h +++ b/Kernel/Net/NetworkingManagement.h @@ -12,6 +12,7 @@ #include #include #include +#include #include namespace Kernel { @@ -26,6 +27,8 @@ public: static bool is_initialized(); bool initialize(); + static KResultOr> generate_interface_name_from_pci_address(PCI::DeviceIdentifier const&); + NetworkingManagement(); void for_each(Function); diff --git a/Kernel/Net/RTL8139NetworkAdapter.cpp b/Kernel/Net/RTL8139NetworkAdapter.cpp index 91bb8a797fb..249adeedf9e 100644 --- a/Kernel/Net/RTL8139NetworkAdapter.cpp +++ b/Kernel/Net/RTL8139NetworkAdapter.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include @@ -112,13 +113,17 @@ namespace Kernel { #define RX_BUFFER_SIZE 32768 #define TX_BUFFER_SIZE PACKET_SIZE_MAX -UNMAP_AFTER_INIT RefPtr RTL8139NetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier, NonnullOwnPtr interface_name) +UNMAP_AFTER_INIT RefPtr 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 {}; u8 irq = pci_device_identifier.interrupt_line().value(); - return adopt_ref_if_nonnull(new (nothrow) RTL8139NetworkAdapter(pci_device_identifier.address(), irq, move(interface_name))); + // 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 {}; + return adopt_ref_if_nonnull(new (nothrow) RTL8139NetworkAdapter(pci_device_identifier.address(), irq, interface_name_or_error.release_value())); } UNMAP_AFTER_INIT RTL8139NetworkAdapter::RTL8139NetworkAdapter(PCI::Address address, u8 irq, NonnullOwnPtr interface_name) diff --git a/Kernel/Net/RTL8139NetworkAdapter.h b/Kernel/Net/RTL8139NetworkAdapter.h index b365fb45b9b..2046cf9c11c 100644 --- a/Kernel/Net/RTL8139NetworkAdapter.h +++ b/Kernel/Net/RTL8139NetworkAdapter.h @@ -22,7 +22,7 @@ class RTL8139NetworkAdapter final : public NetworkAdapter , public PCI::Device , public IRQHandler { public: - static RefPtr try_to_initialize(PCI::DeviceIdentifier const&, NonnullOwnPtr); + static RefPtr try_to_initialize(PCI::DeviceIdentifier const&); virtual ~RTL8139NetworkAdapter() override; diff --git a/Kernel/Net/RTL8168NetworkAdapter.cpp b/Kernel/Net/RTL8168NetworkAdapter.cpp index ea5fe8811ea..5ff7c90d89d 100644 --- a/Kernel/Net/RTL8168NetworkAdapter.cpp +++ b/Kernel/Net/RTL8168NetworkAdapter.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include @@ -181,14 +182,18 @@ namespace Kernel { #define TX_BUFFER_SIZE 0x1FF8 #define RX_BUFFER_SIZE 0x1FF8 // FIXME: this should be increased (0x3FFF) -UNMAP_AFTER_INIT RefPtr RTL8168NetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier, NonnullOwnPtr interface_name) +UNMAP_AFTER_INIT RefPtr RTL8168NetworkAdapter::try_to_initialize(PCI::DeviceIdentifier const& pci_device_identifier) { if (pci_device_identifier.hardware_id().vendor_id != PCI::VendorID::Realtek) return {}; if (pci_device_identifier.hardware_id().device_id != 0x8168) return {}; u8 irq = pci_device_identifier.interrupt_line().value(); - return adopt_ref_if_nonnull(new (nothrow) RTL8168NetworkAdapter(pci_device_identifier.address(), irq, move(interface_name))); + // 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 {}; + return adopt_ref_if_nonnull(new (nothrow) RTL8168NetworkAdapter(pci_device_identifier.address(), irq, interface_name_or_error.release_value())); } bool RTL8168NetworkAdapter::determine_supported_version() const diff --git a/Kernel/Net/RTL8168NetworkAdapter.h b/Kernel/Net/RTL8168NetworkAdapter.h index 3c7616ef264..03eca37ea71 100644 --- a/Kernel/Net/RTL8168NetworkAdapter.h +++ b/Kernel/Net/RTL8168NetworkAdapter.h @@ -22,7 +22,7 @@ class RTL8168NetworkAdapter final : public NetworkAdapter , public PCI::Device , public IRQHandler { public: - static RefPtr try_to_initialize(PCI::DeviceIdentifier const&, NonnullOwnPtr); + static RefPtr try_to_initialize(PCI::DeviceIdentifier const&); virtual ~RTL8168NetworkAdapter() override;