From d3ce97e8b21d77cedd22e70fb593f9d55f4689a4 Mon Sep 17 00:00:00 2001 From: Lorenz Steinert Date: Wed, 23 Feb 2022 22:09:39 +0100 Subject: [PATCH] Kernel/Devices/HID: Propagate errors of HIDDevices properly Some error indication was done by returning bool. This was changed to propagate the error by ErrorOr from the underlying functions. The returntype of the underlying functions was also changed to propagate the error. --- Kernel/Devices/HID/I8042Controller.cpp | 21 +++-- Kernel/Devices/HID/I8042Controller.h | 32 +++---- Kernel/Devices/HID/PS2KeyboardDevice.cpp | 21 ++--- Kernel/Devices/HID/PS2KeyboardDevice.h | 4 +- Kernel/Devices/HID/PS2MouseDevice.cpp | 103 +++++++++++------------ Kernel/Devices/HID/PS2MouseDevice.h | 14 +-- Kernel/Devices/HID/VMWareMouseDevice.cpp | 16 ++-- Kernel/Devices/HID/VMWareMouseDevice.h | 2 +- 8 files changed, 103 insertions(+), 110 deletions(-) diff --git a/Kernel/Devices/HID/I8042Controller.cpp b/Kernel/Devices/HID/I8042Controller.cpp index ad69b8dc7ec..f05f5aa6cc9 100644 --- a/Kernel/Devices/HID/I8042Controller.cpp +++ b/Kernel/Devices/HID/I8042Controller.cpp @@ -164,29 +164,38 @@ UNMAP_AFTER_INIT ErrorOr I8042Controller::detect_devices() // Try to detect and initialize the devices if (m_first_port_available) { - m_keyboard_device = PS2KeyboardDevice::try_to_initialize(*this); - if (!m_keyboard_device) { + auto error_or_device = PS2KeyboardDevice::try_to_initialize(*this); + if (error_or_device.is_error()) { dbgln("I8042: Keyboard device failed to initialize, disable"); m_first_port_available = false; configuration &= ~I8042ConfigurationFlag::FirstPS2PortInterrupt; configuration |= I8042ConfigurationFlag::FirstPS2PortClock; + m_keyboard_device = nullptr; SpinlockLocker lock(m_lock); TRY(do_wait_then_write(I8042Port::Command, I8042Command::WriteConfiguration)); TRY(do_wait_then_write(I8042Port::Buffer, configuration)); + } else { + m_keyboard_device = error_or_device.release_value(); } } if (m_second_port_available) { - m_mouse_device = VMWareMouseDevice::try_to_initialize(*this); - if (!m_mouse_device) { - m_mouse_device = PS2MouseDevice::try_to_initialize(*this); - if (!m_mouse_device) { + auto vmmouse_device_or_error = VMWareMouseDevice::try_to_initialize(*this); + if (vmmouse_device_or_error.is_error()) { + // FIXME: is there something to do with the VMWare errors? + auto mouse_device_or_error = PS2MouseDevice::try_to_initialize(*this); + if (mouse_device_or_error.is_error()) { dbgln("I8042: Mouse device failed to initialize, disable"); m_second_port_available = false; configuration |= I8042ConfigurationFlag::SecondPS2PortClock; + m_mouse_device = nullptr; SpinlockLocker lock(m_lock); TRY(do_wait_then_write(I8042Port::Command, I8042Command::WriteConfiguration)); TRY(do_wait_then_write(I8042Port::Buffer, configuration)); + } else { + m_mouse_device = mouse_device_or_error.release_value(); } + } else { + m_mouse_device = vmmouse_device_or_error.release_value(); } } diff --git a/Kernel/Devices/HID/I8042Controller.h b/Kernel/Devices/HID/I8042Controller.h index f6d250b7b1b..923ab83dd3f 100644 --- a/Kernel/Devices/HID/I8042Controller.h +++ b/Kernel/Devices/HID/I8042Controller.h @@ -91,47 +91,39 @@ public: ErrorOr detect_devices(); - bool reset_device(HIDDevice::Type device) + ErrorOr reset_device(HIDDevice::Type device) { SpinlockLocker lock(m_lock); - // FIXME: Propagate errors properly - if (auto result = do_reset_device(device); result.is_error()) - return false; - return true; + return do_reset_device(device); } - u8 send_command(HIDDevice::Type device, u8 command) + ErrorOr send_command(HIDDevice::Type device, u8 command) { SpinlockLocker lock(m_lock); - // FIXME: Propagate errors properly - return MUST(do_send_command(device, command)); + return do_send_command(device, command); } - u8 send_command(HIDDevice::Type device, u8 command, u8 data) + ErrorOr send_command(HIDDevice::Type device, u8 command, u8 data) { SpinlockLocker lock(m_lock); - // FIXME: Propagate errors properly - return MUST(do_send_command(device, command, data)); + return do_send_command(device, command, data); } - u8 read_from_device(HIDDevice::Type device) + ErrorOr read_from_device(HIDDevice::Type device) { SpinlockLocker lock(m_lock); - // FIXME: Propagate errors properly - return MUST(do_read_from_device(device)); + return do_read_from_device(device); } - void wait_then_write(u8 port, u8 data) + ErrorOr wait_then_write(u8 port, u8 data) { SpinlockLocker lock(m_lock); - // FIXME: Propagate errors properly - MUST(do_wait_then_write(port, data)); + return do_wait_then_write(port, data); } - u8 wait_then_read(u8 port) + ErrorOr wait_then_read(u8 port) { SpinlockLocker lock(m_lock); - // FIXME: Propagate errors properly - return MUST(do_wait_then_read(port)); + return do_wait_then_read(port); } ErrorOr prepare_for_output(); diff --git a/Kernel/Devices/HID/PS2KeyboardDevice.cpp b/Kernel/Devices/HID/PS2KeyboardDevice.cpp index 3eeb1d37b4f..23f696fa307 100644 --- a/Kernel/Devices/HID/PS2KeyboardDevice.cpp +++ b/Kernel/Devices/HID/PS2KeyboardDevice.cpp @@ -86,23 +86,18 @@ bool PS2KeyboardDevice::handle_irq(const RegisterState&) return m_i8042_controller->irq_process_input_buffer(HIDDevice::Type::Keyboard); } -UNMAP_AFTER_INIT RefPtr PS2KeyboardDevice::try_to_initialize(const I8042Controller& ps2_controller) +UNMAP_AFTER_INIT ErrorOr> PS2KeyboardDevice::try_to_initialize(const I8042Controller& ps2_controller) { - auto keyboard_device_or_error = DeviceManagement::try_create_device(ps2_controller); - // FIXME: Find a way to propagate errors - VERIFY(!keyboard_device_or_error.is_error()); - if (keyboard_device_or_error.value()->initialize()) - return keyboard_device_or_error.release_value(); - return nullptr; + auto keyboard_device = TRY(DeviceManagement::try_create_device(ps2_controller)); + + TRY(keyboard_device->initialize()); + + return keyboard_device; } -UNMAP_AFTER_INIT bool PS2KeyboardDevice::initialize() +UNMAP_AFTER_INIT ErrorOr PS2KeyboardDevice::initialize() { - if (!m_i8042_controller->reset_device(HIDDevice::Type::Keyboard)) { - dbgln("KeyboardDevice: I8042 controller failed to reset device"); - return false; - } - return true; + return m_i8042_controller->reset_device(HIDDevice::Type::Keyboard); } // FIXME: UNMAP_AFTER_INIT might not be correct, because in practice PS/2 devices diff --git a/Kernel/Devices/HID/PS2KeyboardDevice.h b/Kernel/Devices/HID/PS2KeyboardDevice.h index 348fac563ac..737b870bcd8 100644 --- a/Kernel/Devices/HID/PS2KeyboardDevice.h +++ b/Kernel/Devices/HID/PS2KeyboardDevice.h @@ -23,9 +23,9 @@ class PS2KeyboardDevice final : public IRQHandler friend class DeviceManagement; public: - static RefPtr try_to_initialize(const I8042Controller&); + static ErrorOr> try_to_initialize(const I8042Controller&); virtual ~PS2KeyboardDevice() override; - bool initialize(); + ErrorOr initialize(); virtual StringView purpose() const override { return class_name(); } diff --git a/Kernel/Devices/HID/PS2MouseDevice.cpp b/Kernel/Devices/HID/PS2MouseDevice.cpp index ed45d780348..5f6bcce8c0c 100644 --- a/Kernel/Devices/HID/PS2MouseDevice.cpp +++ b/Kernel/Devices/HID/PS2MouseDevice.cpp @@ -137,70 +137,69 @@ MousePacket PS2MouseDevice::parse_data_packet(const RawPacket& raw_packet) return packet; } -u8 PS2MouseDevice::get_device_id() +ErrorOr PS2MouseDevice::get_device_id() { - if (send_command(I8042Command::GetDeviceID) != I8042Response::Acknowledge) - return 0; + TRY(send_command(I8042Command::GetDeviceID)); return read_from_device(); } -u8 PS2MouseDevice::read_from_device() +ErrorOr PS2MouseDevice::read_from_device() { return m_i8042_controller->read_from_device(instrument_type()); } -u8 PS2MouseDevice::send_command(u8 command) +ErrorOr PS2MouseDevice::send_command(u8 command) { - u8 response = m_i8042_controller->send_command(instrument_type(), command); - if (response != I8042Response::Acknowledge) + u8 response = TRY(m_i8042_controller->send_command(instrument_type(), command)); + + if (response != I8042Response::Acknowledge) { dbgln("PS2MouseDevice: Command {} got {} but expected ack: {}", command, response, static_cast(I8042Response::Acknowledge)); - return response; -} - -u8 PS2MouseDevice::send_command(u8 command, u8 data) -{ - u8 response = m_i8042_controller->send_command(instrument_type(), command, data); - if (response != I8042Response::Acknowledge) - dbgln("PS2MouseDevice: Command {} got {} but expected ack: {}", command, response, static_cast(I8042Response::Acknowledge)); - return response; -} - -void PS2MouseDevice::set_sample_rate(u8 rate) -{ - send_command(I8042Command::SetSampleRate, rate); -} - -UNMAP_AFTER_INIT RefPtr PS2MouseDevice::try_to_initialize(const I8042Controller& ps2_controller) -{ - auto mouse_device_or_error = DeviceManagement::try_create_device(ps2_controller); - // FIXME: Find a way to propagate errors - VERIFY(!mouse_device_or_error.is_error()); - if (mouse_device_or_error.value()->initialize()) - return mouse_device_or_error.release_value(); - return nullptr; -} - -UNMAP_AFTER_INIT bool PS2MouseDevice::initialize() -{ - if (!m_i8042_controller->reset_device(instrument_type())) { - dbgln("PS2MouseDevice: I8042 controller failed to reset device"); - return false; + // FIXME: Is this the correct errno value for this? + return Error::from_errno(EIO); } + return response; +} - u8 device_id = read_from_device(); +ErrorOr PS2MouseDevice::send_command(u8 command, u8 data) +{ + u8 response = TRY(m_i8042_controller->send_command(instrument_type(), command, data)); + if (response != I8042Response::Acknowledge) { + dbgln("PS2MouseDevice: Command {} got {} but expected ack: {}", command, response, static_cast(I8042Response::Acknowledge)); + // FIXME: Is this the correct errno value for this? + return Error::from_errno(EIO); + } + return response; +} - if (send_command(I8042Command::SetDefaults) != I8042Response::Acknowledge) - return false; +ErrorOr PS2MouseDevice::set_sample_rate(u8 rate) +{ + TRY(send_command(I8042Command::SetSampleRate, rate)); + return {}; +} - if (send_command(I8042Command::EnablePacketStreaming) != I8042Response::Acknowledge) - return false; +UNMAP_AFTER_INIT ErrorOr> PS2MouseDevice::try_to_initialize(const I8042Controller& ps2_controller) +{ + auto mouse_device = TRY(DeviceManagement::try_create_device(ps2_controller)); + TRY(mouse_device->initialize()); + return mouse_device; +} + +UNMAP_AFTER_INIT ErrorOr PS2MouseDevice::initialize() +{ + TRY(m_i8042_controller->reset_device(instrument_type())); + + u8 device_id = TRY(read_from_device()); + + TRY(send_command(I8042Command::SetDefaults)); + + TRY(send_command(I8042Command::EnablePacketStreaming)); if (device_id != PS2MOUSE_INTELLIMOUSE_ID) { // Send magical wheel initiation sequence. - set_sample_rate(200); - set_sample_rate(100); - set_sample_rate(80); - device_id = get_device_id(); + TRY(set_sample_rate(200)); + TRY(set_sample_rate(100)); + TRY(set_sample_rate(80)); + device_id = TRY(get_device_id()); } if (device_id == PS2MOUSE_INTELLIMOUSE_ID) { m_has_wheel = true; @@ -211,17 +210,17 @@ UNMAP_AFTER_INIT bool PS2MouseDevice::initialize() if (device_id == PS2MOUSE_INTELLIMOUSE_ID) { // Try to enable 5 buttons as well! - set_sample_rate(200); - set_sample_rate(200); - set_sample_rate(80); - device_id = get_device_id(); + TRY(set_sample_rate(200)); + TRY(set_sample_rate(200)); + TRY(set_sample_rate(80)); + device_id = TRY(get_device_id()); } if (device_id == PS2MOUSE_INTELLIMOUSE_EXPLORER_ID) { m_has_five_buttons = true; dmesgln("PS2MouseDevice: 5 buttons enabled!"); } - return true; + return {}; } } diff --git a/Kernel/Devices/HID/PS2MouseDevice.h b/Kernel/Devices/HID/PS2MouseDevice.h index b232bf2d1f7..6822b7ef9d9 100644 --- a/Kernel/Devices/HID/PS2MouseDevice.h +++ b/Kernel/Devices/HID/PS2MouseDevice.h @@ -20,8 +20,8 @@ class PS2MouseDevice : public IRQHandler friend class DeviceManagement; public: - static RefPtr try_to_initialize(const I8042Controller&); - bool initialize(); + static ErrorOr> try_to_initialize(const I8042Controller&); + ErrorOr initialize(); virtual ~PS2MouseDevice() override; @@ -47,12 +47,12 @@ protected: }; }; - u8 read_from_device(); - u8 send_command(u8 command); - u8 send_command(u8 command, u8 data); + ErrorOr read_from_device(); + ErrorOr send_command(u8 command); + ErrorOr send_command(u8 command, u8 data); MousePacket parse_data_packet(const RawPacket&); - void set_sample_rate(u8); - u8 get_device_id(); + ErrorOr set_sample_rate(u8); + ErrorOr get_device_id(); u8 m_data_state { 0 }; RawPacket m_data; diff --git a/Kernel/Devices/HID/VMWareMouseDevice.cpp b/Kernel/Devices/HID/VMWareMouseDevice.cpp index 2d143f983eb..aaeeb4f9c60 100644 --- a/Kernel/Devices/HID/VMWareMouseDevice.cpp +++ b/Kernel/Devices/HID/VMWareMouseDevice.cpp @@ -11,18 +11,16 @@ namespace Kernel { -UNMAP_AFTER_INIT RefPtr VMWareMouseDevice::try_to_initialize(const I8042Controller& ps2_controller) +UNMAP_AFTER_INIT ErrorOr> VMWareMouseDevice::try_to_initialize(const I8042Controller& ps2_controller) { + // FIXME: return the correct error if (!VMWareBackdoor::the()) - return {}; + return Error::from_errno(EIO); if (!VMWareBackdoor::the()->vmmouse_is_absolute()) - return {}; - auto mouse_device_or_error = DeviceManagement::try_create_device(ps2_controller); - // FIXME: Find a way to propagate errors - VERIFY(!mouse_device_or_error.is_error()); - if (mouse_device_or_error.value()->initialize()) - return mouse_device_or_error.release_value(); - return {}; + return Error::from_errno(EIO); + auto mouse_device = TRY(DeviceManagement::try_create_device(ps2_controller)); + TRY(mouse_device->initialize()); + return mouse_device; } void VMWareMouseDevice::irq_handle_byte_read(u8) diff --git a/Kernel/Devices/HID/VMWareMouseDevice.h b/Kernel/Devices/HID/VMWareMouseDevice.h index 39680999900..64680d54ff6 100644 --- a/Kernel/Devices/HID/VMWareMouseDevice.h +++ b/Kernel/Devices/HID/VMWareMouseDevice.h @@ -18,7 +18,7 @@ namespace Kernel { class VMWareMouseDevice final : public PS2MouseDevice { public: friend class DeviceManagement; - static RefPtr try_to_initialize(const I8042Controller&); + static ErrorOr> try_to_initialize(const I8042Controller&); virtual ~VMWareMouseDevice() override; // ^I8042Device