From c5555ad3c04e295fccf9600cb0aa6eb45eadfbf5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stephan=20A=C3=9Fmus?= Date: Fri, 24 Oct 2008 21:46:36 +0000 Subject: [PATCH] * Ignore B_ENTRY_REMOVED node monitor messages. * When the device watcher/control thread encounteres an error when read()ing or ioctl()ing the device, don't just quit the thread and leave a stale device add-on hanging there, but instead trigger _RemoveDevice() to exit this cleanly. This also takes care of calling _RemoveDevice() only from one thread. However, it adds a race condition should a mouse or keyboard be unplugged and plugged at the same time. I need to think about how to fix that cleanly, although the situation may be theoretical only... This fix seems to fix another problem with hot-plugging USB mice, before this change, the first mouse entry in /dev/input/mouse/usb/ was never gone and I got two entries after unplugging and replugging. * When using BObjectList configured to own the entries - don't delete the entries! Also don't call RemoveItem() before still using the item. Took me all day to find this one, because the code looked so... correct. :-} * In _AddDevice() call _RemoveDevice() just for the sake of it. It is really important that no device with the same name is published twice. The PS/2 driver behaves strange in that it publishes device more than once, if I understand correctly, until it decides that there is no device. * Only StartMonitoringDevice() /after/ having performed the initial device scan! Or else we may get ourselves confused. I don't know if this was an actual problem, but the code was like that before and it seems saner to me. Seeing there is no locking in the device add-on itself, we may already enter the code from the node monitor thread. This should fix #2894. git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@28321 a95241bf-73f2-0310-859d-f6bbb57e9c96 --- .../devices/keyboard/KeyboardInputDevice.cpp | 61 +++++++++++------ .../devices/keyboard/KeyboardInputDevice.h | 5 ++ .../devices/mouse/MouseInputDevice.cpp | 66 ++++++++++++------- .../devices/mouse/MouseInputDevice.h | 4 ++ 4 files changed, 92 insertions(+), 44 deletions(-) diff --git a/src/add-ons/input_server/devices/keyboard/KeyboardInputDevice.cpp b/src/add-ons/input_server/devices/keyboard/KeyboardInputDevice.cpp index 2982421c46..4811d4a1b9 100644 --- a/src/add-ons/input_server/devices/keyboard/KeyboardInputDevice.cpp +++ b/src/add-ons/input_server/devices/keyboard/KeyboardInputDevice.cpp @@ -379,8 +379,8 @@ KeyboardInputDevice::KeyboardInputDevice() { CALLED(); - StartMonitoringDevice(kKeyboardDevicesDirectory); _RecursiveScan(kKeyboardDevicesDirectory); + StartMonitoringDevice(kKeyboardDevicesDirectory); } @@ -525,14 +525,32 @@ KeyboardInputDevice::_HandleMonitor(BMessage* message) const char* path; int32 opcode; if (message->FindInt32("opcode", &opcode) != B_OK - || opcode != B_ENTRY_CREATED && opcode != B_ENTRY_REMOVED + || (opcode != B_ENTRY_CREATED && opcode != B_ENTRY_REMOVED) || message->FindString("path", &path) != B_OK) return B_BAD_VALUE; if (opcode == B_ENTRY_CREATED) return _AddDevice(path); +#if 0 return _RemoveDevice(path); +#else + // Don't handle B_ENTRY_REMOVED, let the control thread take care of it. + return B_OK; +#endif +} + + +keyboard_device* +KeyboardInputDevice::_FindDevice(const char* path) const +{ + for (int i = fDevices.CountItems() - 1; i >= 0; i--) { + keyboard_device* device = fDevices.ItemAt(i); + if (strcmp(device->path, path) == 0) + return device; + } + + return NULL; } @@ -540,6 +558,9 @@ status_t KeyboardInputDevice::_AddDevice(const char* path) { CALLED(); + + _RemoveDevice(path); + keyboard_device* device = new(std::nothrow) keyboard_device(path); if (device == NULL) return B_NO_MEMORY; @@ -560,22 +581,20 @@ status_t KeyboardInputDevice::_RemoveDevice(const char* path) { CALLED(); - keyboard_device* device; - for (int i = 0; (device = fDevices.ItemAt(i)) != NULL; i++) { - if (!strcmp(device->path, path)) { - fDevices.RemoveItemAt(i); - input_device_ref* devices[2]; - devices[0] = &device->device_ref; - devices[1] = NULL; - UnregisterDevices(devices); + keyboard_device* device = _FindDevice(path); + if (device == NULL) + return B_ENTRY_NOT_FOUND; - delete device; - return B_OK; - } - } + input_device_ref* devices[2]; + devices[0] = &device->device_ref; + devices[1] = NULL; - return B_ENTRY_NOT_FOUND; + UnregisterDevices(devices); + + fDevices.RemoveItem(device); + + return B_OK; } @@ -598,8 +617,12 @@ KeyboardInputDevice::_DeviceWatcher(void* arg) LOG("%s\n", __PRETTY_FUNCTION__); while (device->active) { - if (ioctl(device->fd, KB_READ, &buffer) != B_OK) + if (ioctl(device->fd, KB_READ, &buffer) != B_OK) { + device->device_watcher = -1; + device->owner->_RemoveDevice(device->path); + // TOAST! return 0; + } uint32 keycode = 0; bool isKeyDown = false; @@ -697,7 +720,7 @@ KeyboardInputDevice::_DeviceWatcher(void* arg) msg->AddInt32("modifiers", device->modifiers); msg->AddData("states", B_UINT8_TYPE, states, 16); - if (owner->EnqueueMessage(msg)!=B_OK) + if (owner->EnqueueMessage(msg) != B_OK) delete msg; if (modifiers & (B_CAPS_LOCK | B_NUM_LOCK | B_SCROLL_LOCK)) @@ -717,8 +740,8 @@ KeyboardInputDevice::_DeviceWatcher(void* arg) BMessage* msg = new BMessage; if (msg == NULL) { - free(string); - free(rawString); + delete[] string; + delete[] rawString; continue; } diff --git a/src/add-ons/input_server/devices/keyboard/KeyboardInputDevice.h b/src/add-ons/input_server/devices/keyboard/KeyboardInputDevice.h index 4a78411304..e05da6d43c 100644 --- a/src/add-ons/input_server/devices/keyboard/KeyboardInputDevice.h +++ b/src/add-ons/input_server/devices/keyboard/KeyboardInputDevice.h @@ -60,10 +60,15 @@ public: virtual status_t SystemShuttingDown(); private: + friend struct keyboard_device; + // TODO: needed by the control thread to remove a dead device + // find a better way... + status_t _HandleMonitor(BMessage* message); status_t _InitFromSettings(void* cookie, uint32 opcode = 0); void _RecursiveScan(const char* directory); + keyboard_device* _FindDevice(const char* path) const; status_t _AddDevice(const char* path); status_t _RemoveDevice(const char* path); diff --git a/src/add-ons/input_server/devices/mouse/MouseInputDevice.cpp b/src/add-ons/input_server/devices/mouse/MouseInputDevice.cpp index a1f7fd3f70..8d2ac4d645 100644 --- a/src/add-ons/input_server/devices/mouse/MouseInputDevice.cpp +++ b/src/add-ons/input_server/devices/mouse/MouseInputDevice.cpp @@ -38,16 +38,20 @@ # define LOG_ERR(text...) debug_printf(text) #endif +//#define LOG_DEVICES(text...) debug_printf(text) +#define LOG_DEVICES(text...) LOG(text) + #define CALLED() LOG("%s\n", __PRETTY_FUNCTION__) + const static uint32 kMouseThreadPriority = B_FIRST_REAL_TIME_PRIORITY + 4; const static char* kMouseDevicesDirectory = "/dev/input/mouse"; class MouseDevice { public: - MouseDevice(BInputServerDevice& target, const char* path); + MouseDevice(MouseInputDevice& target, const char* path); ~MouseDevice(); status_t Start(); @@ -71,16 +75,16 @@ class MouseDevice { char* _BuildShortName() const; private: - BInputServerDevice& fTarget; - BString fPath; - int fDevice; + MouseInputDevice& fTarget; + BString fPath; + int fDevice; - input_device_ref fDeviceRef; - mouse_settings fSettings; - bool fDeviceRemapsButtons; + input_device_ref fDeviceRef; + mouse_settings fSettings; + bool fDeviceRemapsButtons; - thread_id fThread; - volatile bool fActive; + thread_id fThread; + volatile bool fActive; }; @@ -94,15 +98,14 @@ instantiate_input_device() // #pragma mark - -MouseDevice::MouseDevice(BInputServerDevice& target, const char* driverPath) +MouseDevice::MouseDevice(MouseInputDevice& target, const char* driverPath) : fTarget(target), + fPath(driverPath), fDevice(-1), fThread(-1), fActive(false) { - fPath = driverPath; - fDeviceRef.name = _BuildShortName(); fDeviceRef.type = B_POINTING_DEVICE; fDeviceRef.cookie = this; @@ -230,8 +233,12 @@ MouseDevice::_Run() mouse_movement movements; memset(&movements, 0, sizeof(movements)); - if (ioctl(fDevice, MS_READ, &movements) != B_OK) + if (ioctl(fDevice, MS_READ, &movements) != B_OK) { + fThread = -1; + fTarget._RemoveDevice(fPath.String()); + // TOAST! return; + } uint32 buttons = lastButtons ^ movements.buttons; @@ -244,8 +251,6 @@ MouseDevice::_Run() movements.clicks, movements.wheel_xdelta, movements.wheel_ydelta); LOG("%s: x: %ld, y: %ld\n", fDeviceRef.name, deltaX, deltaY); - BMessage* message = NULL; - // Send single messages for each event if (buttons != 0) { @@ -273,7 +278,7 @@ MouseDevice::_Run() } if ((movements.wheel_ydelta != 0) || (movements.wheel_xdelta != 0)) { - message = new BMessage(B_MOUSE_WHEEL_CHANGED); + BMessage* message = new BMessage(B_MOUSE_WHEEL_CHANGED); if (message == NULL) continue; @@ -405,8 +410,8 @@ MouseInputDevice::MouseInputDevice() { CALLED(); - StartMonitoringDevice(kMouseDevicesDirectory); _RecursiveScan(kMouseDevicesDirectory); + StartMonitoringDevice(kMouseDevicesDirectory); } @@ -464,7 +469,6 @@ MouseInputDevice::Control(const char* name, void* cookie, } -// TODO: Test this. USB doesn't work on my machine status_t MouseInputDevice::_HandleMonitor(BMessage* message) { @@ -473,14 +477,19 @@ MouseInputDevice::_HandleMonitor(BMessage* message) const char* path; int32 opcode; if (message->FindInt32("opcode", &opcode) != B_OK - || opcode != B_ENTRY_CREATED && opcode != B_ENTRY_REMOVED + || (opcode != B_ENTRY_CREATED && opcode != B_ENTRY_REMOVED) || message->FindString("path", &path) != B_OK) return B_BAD_VALUE; if (opcode == B_ENTRY_CREATED) return _AddDevice(path); +#if 0 return _RemoveDevice(path); +#else + // Don't handle B_ENTRY_REMOVED, let the control thread take care of it. + return B_OK; +#endif } @@ -489,9 +498,9 @@ MouseInputDevice::_FindDevice(const char* path) { CALLED(); - for (int32 i = fDevices.CountItems(); i-- > 0;) { + for (int32 i = fDevices.CountItems() - 1; i >= 0; i--) { MouseDevice* device = fDevices.ItemAt(i); - if (!strcmp(device->Path(), path)) + if (strcmp(device->Path(), path) == 0) return device; } @@ -504,9 +513,11 @@ MouseInputDevice::_AddDevice(const char* path) { CALLED(); + _RemoveDevice(path); + MouseDevice* device = new(std::nothrow) MouseDevice(*this, path); if (!device) { - LOG("No memory\n"); + LOG("MouseInputDevice::_AddDevice() - No memory\n"); return B_NO_MEMORY; } @@ -519,6 +530,9 @@ MouseInputDevice::_AddDevice(const char* path) devices[0] = device->DeviceRef(); devices[1] = NULL; + LOG_DEVICES("MouseInputDevice::_AddDevice(%s), name: %s\n", path, + devices[0]->name); + return RegisterDevices(devices); } @@ -532,15 +546,17 @@ MouseInputDevice::_RemoveDevice(const char* path) if (device == NULL) return B_ENTRY_NOT_FOUND; - fDevices.RemoveItem(device); - input_device_ref* devices[2]; devices[0] = device->DeviceRef(); devices[1] = NULL; + LOG_DEVICES("MouseInputDevice::_RemoveDevice(%s), name: %s\n", path, + devices[0]->name); + UnregisterDevices(devices); - delete device; + fDevices.RemoveItem(device); + return B_OK; } diff --git a/src/add-ons/input_server/devices/mouse/MouseInputDevice.h b/src/add-ons/input_server/devices/mouse/MouseInputDevice.h index 61a5354897..546f30888c 100644 --- a/src/add-ons/input_server/devices/mouse/MouseInputDevice.h +++ b/src/add-ons/input_server/devices/mouse/MouseInputDevice.h @@ -31,6 +31,10 @@ public: uint32 command, BMessage* message); private: + friend class MouseDevice; + // TODO: needed by the control thread to remove a dead device + // find a better way... + status_t _HandleMonitor(BMessage* message); void _RecursiveScan(const char* directory);