From 11ca0322a41920df2b462d2e45b0731e47ff475b Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 10 Oct 2023 12:20:18 +0200 Subject: HID: logitech-hidpp: Don't restart IO, instead defer hid_connect() only Restarting IO causes 2 problems: 1. Some devices do not like IO being restarted this was addressed in commit 498ba2069035 ("HID: logitech-hidpp: Don't restart communication if not necessary"), but that change has issues of its own and needs to be reverted. 2. Restarting IO and specifically calling hid_device_io_stop() causes received packets to be missed, which may cause connect-events to get missed. Restarting IO was introduced in commit 91cf9a98ae41 ("HID: logitech-hidpp: make .probe usbhid capable") to allow to retrieve the device's name and serial number and store these in hdev->name and hdev->uniq before connecting any hid subdrivers (hid-input, hidraw) exporting this info to userspace. But this does not require restarting IO, this merely requires deferring calling hid_connect(). Calling hid_hw_start() with a connect-mask of 0 makes it skip calling hid_connect(), so hidpp_probe() can simply call hid_connect() later without needing to restart IO. Remove the stop + restart of IO and instead just call hid_connect() later to avoid the issues caused by restarting IO. Now that IO is no longer stopped, hid_hw_close() must be called at the end of probe() to balance the hid_hw_open() done at the beginning probe(). This series has been tested on the following devices: Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0) Logitech M720 Triathlon (bluetooth, HID++ 4.5) Logitech M720 Triathlon (unifying, HID++ 4.5) Logitech K400 Pro (unifying, HID++ 4.1) Logitech K270 (eQUAD nano Lite, HID++ 2.0) Logitech M185 (eQUAD nano Lite, HID++ 4.5) Logitech LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0) Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0) And by bentiss: Logitech Touchpad T650 (unifying) Logitech Touchpad T651 (bluetooth) Logitech MX Master 3B (BLE) Logitech G403 (plain USB / Gaming receiver) Fixes: 498ba2069035 ("HID: logitech-hidpp: Don't restart communication if not necessary") Suggested-by: Benjamin Tissoires Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20231010102029.111003-2-hdegoede@redhat.com Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-logitech-hidpp.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) (limited to 'drivers/hid') diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index a209d51bd247..aa4f232c4518 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -4460,8 +4460,10 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) hdev->name); /* - * Plain USB connections need to actually call start and open - * on the transport driver to allow incoming data. + * First call hid_hw_start(hdev, 0) to allow IO without connecting any + * hid subdrivers (hid-input, hidraw). This allows retrieving the dev's + * name and serial number and store these in hdev->name and hdev->uniq, + * before the hid-input and hidraw drivers expose these to userspace. */ ret = hid_hw_start(hdev, will_restart ? 0 : connect_mask); if (ret) { @@ -4519,19 +4521,14 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) flush_work(&hidpp->work); if (will_restart) { - /* Reset the HID node state */ - hid_device_io_stop(hdev); - hid_hw_close(hdev); - hid_hw_stop(hdev); - if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT) connect_mask &= ~HID_CONNECT_HIDINPUT; /* Now export the actual inputs and hidraw nodes to the world */ - ret = hid_hw_start(hdev, connect_mask); + ret = hid_connect(hdev, connect_mask); if (ret) { - hid_err(hdev, "%s:hid_hw_start returned error\n", __func__); - goto hid_hw_start_fail; + hid_err(hdev, "%s:hid_connect returned error %d\n", __func__, ret); + goto hid_hw_init_fail; } } @@ -4543,6 +4540,11 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) ret); } + /* + * This relies on logi_dj_ll_close() being a no-op so that DJ connection + * events will still be received. + */ + hid_hw_close(hdev); return ret; hid_hw_init_fail: -- cgit v1.2.3 From 55bf70362ffc4ddd7c8745e2fe880edac00e4aff Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 10 Oct 2023 12:20:19 +0200 Subject: HID: logitech-hidpp: Revert "Don't restart communication if not necessary" Commit 91cf9a98ae41 ("HID: logitech-hidpp: make .probe usbhid capable") makes hidpp_probe() first call hid_hw_start(hdev, 0) to allow IO without connecting any hid subdrivers (hid-input, hidraw). This is done to allow to retrieve the device's name and serial number and store these in hdev->name and hdev->uniq. Then later on IO was stopped and started again with hid_hw_start(hdev, HID_CONNECT_DEFAULT) connecting hid-input and hidraw after the name and serial number have been setup. Commit 498ba2069035 ("HID: logitech-hidpp: Don't restart communication if not necessary") changed the probe() code to only do the start with a 0 connect-mask + restart later for unifying devices. But for non unifying devices hdev->name and hdev->uniq are updated too. So this change re-introduces the problem for which the start with a 0 connect-mask + restart later behavior was introduced. The previous patch in this series changes the unifying path to instead of restarting IO only call hid_connect() later. This avoids possible issues with restarting IO seen on non unifying devices. Revert the change to limit the restart behavior to unifying devices to fix hdev->name changing after userspace facing devices have already been registered. This series has been tested on the following devices: Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0) Logitech M720 Triathlon (bluetooth, HID++ 4.5) Logitech M720 Triathlon (unifying, HID++ 4.5) Logitech K400 Pro (unifying, HID++ 4.1) Logitech K270 (eQUAD nano Lite, HID++ 2.0) Logitech M185 (eQUAD nano Lite, HID++ 4.5) Logitech LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0) Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0) And by bentiss: Logitech Touchpad T650 (unifying) Logitech Touchpad T651 (bluetooth) Logitech MX Master 3B (BLE) Logitech G403 (plain USB / Gaming receiver) Fixes: 498ba2069035 ("HID: logitech-hidpp: Don't restart communication if not necessary") Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20231010102029.111003-3-hdegoede@redhat.com Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-logitech-hidpp.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) (limited to 'drivers/hid') diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index aa4f232c4518..c582d19b11d6 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -4394,7 +4394,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) bool connected; unsigned int connect_mask = HID_CONNECT_DEFAULT; struct hidpp_ff_private_data data; - bool will_restart = false; /* report_fixup needs drvdata to be set before we call hid_parse */ hidpp = devm_kzalloc(&hdev->dev, sizeof(*hidpp), GFP_KERNEL); @@ -4445,10 +4444,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) return ret; } - if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT || - hidpp->quirks & HIDPP_QUIRK_UNIFYING) - will_restart = true; - INIT_WORK(&hidpp->work, delayed_work_cb); mutex_init(&hidpp->send_mutex); init_waitqueue_head(&hidpp->wait); @@ -4465,7 +4460,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) * name and serial number and store these in hdev->name and hdev->uniq, * before the hid-input and hidraw drivers expose these to userspace. */ - ret = hid_hw_start(hdev, will_restart ? 0 : connect_mask); + ret = hid_hw_start(hdev, 0); if (ret) { hid_err(hdev, "hw start failed\n"); goto hid_hw_start_fail; @@ -4504,7 +4499,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) hidpp->wireless_feature_index = 0; else if (ret) goto hid_hw_init_fail; - ret = 0; } if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) { @@ -4520,16 +4514,14 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) schedule_work(&hidpp->work); flush_work(&hidpp->work); - if (will_restart) { - if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT) - connect_mask &= ~HID_CONNECT_HIDINPUT; + if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT) + connect_mask &= ~HID_CONNECT_HIDINPUT; - /* Now export the actual inputs and hidraw nodes to the world */ - ret = hid_connect(hdev, connect_mask); - if (ret) { - hid_err(hdev, "%s:hid_connect returned error %d\n", __func__, ret); - goto hid_hw_init_fail; - } + /* Now export the actual inputs and hidraw nodes to the world */ + ret = hid_connect(hdev, connect_mask); + if (ret) { + hid_err(hdev, "%s:hid_connect returned error %d\n", __func__, ret); + goto hid_hw_init_fail; } if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) { -- cgit v1.2.3 From ba9de350509504fb748837b71e23d7e84c83d93c Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 10 Oct 2023 12:20:20 +0200 Subject: HID: logitech-hidpp: Move get_wireless_feature_index() check to hidpp_connect_event() Calling get_wireless_feature_index() from probe() causes the wireless_feature_index to only get set for unifying devices which are already connected at probe() time. It does not get set for devices which connect later. Fix this by moving get_wireless_feature_index() to hidpp_connect_event(), this does not make a difference for devices connected at probe() since probe() will queue the hidpp_connect_event() for those at probe time. This series has been tested on the following devices: Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0) Logitech M720 Triathlon (bluetooth, HID++ 4.5) Logitech M720 Triathlon (unifying, HID++ 4.5) Logitech K400 Pro (unifying, HID++ 4.1) Logitech K270 (eQUAD nano Lite, HID++ 2.0) Logitech M185 (eQUAD nano Lite, HID++ 4.5) Logitech LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0) Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0) And by bentiss: Logitech Touchpad T650 (unifying) Logitech Touchpad T651 (bluetooth) Logitech MX Master 3B (BLE) Logitech G403 (plain USB / Gaming receiver) Fixes: 0da0a63b7cba ("HID: logitech-hidpp: Support WirelessDeviceStatus connect events") Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20231010102029.111003-4-hdegoede@redhat.com Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-logitech-hidpp.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) (limited to 'drivers/hid') diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index c582d19b11d6..7bf12ca0eb4a 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -1835,15 +1835,14 @@ static int hidpp_battery_get_property(struct power_supply *psy, /* -------------------------------------------------------------------------- */ #define HIDPP_PAGE_WIRELESS_DEVICE_STATUS 0x1d4b -static int hidpp_set_wireless_feature_index(struct hidpp_device *hidpp) +static int hidpp_get_wireless_feature_index(struct hidpp_device *hidpp, u8 *feature_index) { u8 feature_type; int ret; ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_WIRELESS_DEVICE_STATUS, - &hidpp->wireless_feature_index, - &feature_type); + feature_index, &feature_type); return ret; } @@ -4249,6 +4248,13 @@ static void hidpp_connect_event(struct hidpp_device *hidpp) } } + if (hidpp->protocol_major >= 2) { + u8 feature_index; + + if (!hidpp_get_wireless_feature_index(hidpp, &feature_index)) + hidpp->wireless_feature_index = feature_index; + } + if (hidpp->name == hdev->name && hidpp->protocol_major >= 2) { name = hidpp_get_device_name(hidpp); if (name) { @@ -4493,14 +4499,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) hidpp_overwrite_name(hdev); } - if (connected && hidpp->protocol_major >= 2) { - ret = hidpp_set_wireless_feature_index(hidpp); - if (ret == -ENOENT) - hidpp->wireless_feature_index = 0; - else if (ret) - goto hid_hw_init_fail; - } - if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) { ret = wtp_get_config(hidpp); if (ret) -- cgit v1.2.3 From a3643036d7a8c9c81f574b235afcc0582cce76ab Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 10 Oct 2023 12:20:21 +0200 Subject: HID: logitech-hidpp: Remove wtp_get_config() call from probe() For WTP devices which start disconnected (paired with the unifying receiver, but not connected atm) hidpp_connect_event() takes care of calling wtp_get_config() when the device later connects. There is no need to have a separate code path for WTP devices which are connected at probe() time, these can use the same code-path since probe() will queue hidpp_connect_event() for those at probe time. Drop the unnecessary wtp_get_config() call from probe(). This series has been tested on the following devices: Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0) Logitech M720 Triathlon (bluetooth, HID++ 4.5) Logitech M720 Triathlon (unifying, HID++ 4.5) Logitech K400 Pro (unifying, HID++ 4.1) Logitech K270 (eQUAD nano Lite, HID++ 2.0) Logitech M185 (eQUAD nano Lite, HID++ 4.5) Logitech LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0) Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0) And by bentiss: Logitech Touchpad T650 (unifying) Logitech Touchpad T651 (bluetooth) Logitech MX Master 3B (BLE) Logitech G403 (plain USB / Gaming receiver) Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20231010102029.111003-5-hdegoede@redhat.com Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-logitech-hidpp.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) (limited to 'drivers/hid') diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index 7bf12ca0eb4a..707fb9616fc5 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -4499,11 +4499,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) hidpp_overwrite_name(hdev); } - if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) { - ret = wtp_get_config(hidpp); - if (ret) - goto hid_hw_init_fail; - } else if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) { + if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) { ret = g920_get_config(hidpp, &data); if (ret) goto hid_hw_init_fail; -- cgit v1.2.3 From 219ccfb60003a4b3874ddc0205588c8e925fd946 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 10 Oct 2023 12:20:22 +0200 Subject: HID: logitech-hidpp: Move g920_get_config() to just before hidpp_ff_init() The data retrieved by g920_get_config() is only used by hidpp_ff_init(). Now that the hw is kept open till the end of probe() the g920_get_config() call can be moved to just before hidpp_ff_init() to have all the HIDPP_QUIRK_CLASS_G920 together in a single place. This series has been tested on the following devices: Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0) Logitech M720 Triathlon (bluetooth, HID++ 4.5) Logitech M720 Triathlon (unifying, HID++ 4.5) Logitech K400 Pro (unifying, HID++ 4.1) Logitech K270 (eQUAD nano Lite, HID++ 2.0) Logitech M185 (eQUAD nano Lite, HID++ 4.5) Logitech LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0) Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0) And by bentiss: Logitech Touchpad T650 (unifying) Logitech Touchpad T651 (bluetooth) Logitech MX Master 3B (BLE) Logitech G403 (plain USB / Gaming receiver) Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20231010102029.111003-6-hdegoede@redhat.com Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-logitech-hidpp.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) (limited to 'drivers/hid') diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index 707fb9616fc5..dd67749301a8 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -4399,7 +4399,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) int ret; bool connected; unsigned int connect_mask = HID_CONNECT_DEFAULT; - struct hidpp_ff_private_data data; /* report_fixup needs drvdata to be set before we call hid_parse */ hidpp = devm_kzalloc(&hdev->dev, sizeof(*hidpp), GFP_KERNEL); @@ -4499,12 +4498,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) hidpp_overwrite_name(hdev); } - if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) { - ret = g920_get_config(hidpp, &data); - if (ret) - goto hid_hw_init_fail; - } - schedule_work(&hidpp->work); flush_work(&hidpp->work); @@ -4519,7 +4512,12 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) } if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) { - ret = hidpp_ff_init(hidpp, &data); + struct hidpp_ff_private_data data; + + ret = g920_get_config(hidpp, &data); + if (!ret) + ret = hidpp_ff_init(hidpp, &data); + if (ret) hid_warn(hidpp->hid_dev, "Unable to initialize force feedback support, errno %d\n", -- cgit v1.2.3 From 8954dac18c681fe59855e294b133d967873e5325 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 10 Oct 2023 12:20:23 +0200 Subject: HID: logitech-hidpp: Move hidpp_overwrite_name() to before connect check Move the hidpp_overwrite_name() call to before the connect check, this puts it at the same place in the probe() order as hidpp_serial_init() which seems more logical. This should not make a difference since this is in the non-unifying path and only unifying devices can be probed in non-connected state. This is a preparation patch for moving all connected state handling to hidpp_connect_event(). Doing this before the connect check requires dropping the protocol version check since protocol_major is not set yet now. Instead this relies on hidpp_root_get_feature(HIDPP_PAGE_GET_DEVICE_NAME_TYPE) failing on older devices, just like how hidpp_get_serial() relies on hidpp_root_get_feature(HIDPP_PAGE_DEVICE_INFORMATION) failing on older devices. So this again makes the code more consistent. Also stop printing an error on failure now, since with the proto version check gone failures are expected to happen on older devices. This series has been tested on the following devices: Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0) Logitech M720 Triathlon (bluetooth, HID++ 4.5) Logitech M720 Triathlon (unifying, HID++ 4.5) Logitech K400 Pro (unifying, HID++ 4.1) Logitech K270 (eQUAD nano Lite, HID++ 2.0) Logitech M185 (eQUAD nano Lite, HID++ 4.5) Logitech LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0) Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0) And by bentiss: Logitech Touchpad T650 (unifying) Logitech Touchpad T651 (bluetooth) Logitech MX Master 3B (BLE) Logitech G403 (plain USB / Gaming receiver) Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20231010102029.111003-7-hdegoede@redhat.com Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-logitech-hidpp.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) (limited to 'drivers/hid') diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index dd67749301a8..799d1a429d81 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -4135,19 +4135,12 @@ static void hidpp_overwrite_name(struct hid_device *hdev) struct hidpp_device *hidpp = hid_get_drvdata(hdev); char *name; - if (hidpp->protocol_major < 2) - return; - name = hidpp_get_device_name(hidpp); - - if (!name) { - hid_err(hdev, "unable to retrieve the name of the device"); - } else { + if (name) { dbg_hid("HID++: Got name: %s\n", name); snprintf(hdev->name, sizeof(hdev->name), "%s", name); + kfree(name); } - - kfree(name); } static int hidpp_input_open(struct input_dev *dev) @@ -4483,8 +4476,12 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) if (hidpp->quirks & HIDPP_QUIRK_UNIFYING) hidpp_unifying_init(hidpp); - else if (hid_is_usb(hidpp->hid_dev)) - hidpp_serial_init(hidpp); + else { + if (hid_is_usb(hidpp->hid_dev)) + hidpp_serial_init(hidpp); + + hidpp_overwrite_name(hdev); + } connected = hidpp_root_get_protocol_version(hidpp) == 0; atomic_set(&hidpp->connected, connected); @@ -4494,8 +4491,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) hid_err(hdev, "Device not connected"); goto hid_hw_init_fail; } - - hidpp_overwrite_name(hdev); } schedule_work(&hidpp->work); -- cgit v1.2.3 From c14f1485c60507c0db00d1ba187bafb563fae318 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 10 Oct 2023 12:20:24 +0200 Subject: HID: logitech-hidpp: Add hidpp_non_unifying_init() helper Turn hidpp_overwrite_name() into a hidpp_non_unifying_init() helper which takes care of setting both the name and the serial for non unifying devices. This mirrors the hidpp_unifying_init() helper for unifying devices. This series has been tested on the following devices: Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0) Logitech M720 Triathlon (bluetooth, HID++ 4.5) Logitech M720 Triathlon (unifying, HID++ 4.5) Logitech K400 Pro (unifying, HID++ 4.1) Logitech K270 (eQUAD nano Lite, HID++ 2.0) Logitech M185 (eQUAD nano Lite, HID++ 4.5) Logitech LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0) Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0) And by bentiss: Logitech Touchpad T650 (unifying) Logitech Touchpad T651 (bluetooth) Logitech MX Master 3B (BLE) Logitech G403 (plain USB / Gaming receiver) Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20231010102029.111003-8-hdegoede@redhat.com Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-logitech-hidpp.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) (limited to 'drivers/hid') diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index 799d1a429d81..55fc80f3be05 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -4130,11 +4130,16 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp) return ret; } -static void hidpp_overwrite_name(struct hid_device *hdev) +/* Get name + serial for USB and Bluetooth HID++ devices */ +static void hidpp_non_unifying_init(struct hidpp_device *hidpp) { - struct hidpp_device *hidpp = hid_get_drvdata(hdev); + struct hid_device *hdev = hidpp->hid_dev; char *name; + /* Bluetooth devices already have their serialnr set */ + if (hid_is_usb(hdev)) + hidpp_serial_init(hidpp); + name = hidpp_get_device_name(hidpp); if (name) { dbg_hid("HID++: Got name: %s\n", name); @@ -4474,14 +4479,11 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) /* Allow incoming packets */ hid_device_io_start(hdev); + /* Get name + serial, store in hdev->name + hdev->uniq */ if (hidpp->quirks & HIDPP_QUIRK_UNIFYING) hidpp_unifying_init(hidpp); - else { - if (hid_is_usb(hidpp->hid_dev)) - hidpp_serial_init(hidpp); - - hidpp_overwrite_name(hdev); - } + else + hidpp_non_unifying_init(hidpp); connected = hidpp_root_get_protocol_version(hidpp) == 0; atomic_set(&hidpp->connected, connected); -- cgit v1.2.3 From 6f335b47adc396ed40655ab37aa9b2284c8f3783 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 10 Oct 2023 12:20:25 +0200 Subject: HID: logitech-hidpp: Remove connected check for non-unifying devices Non-unifying devices (USB, Bluetooth) are always connected during probe(), remove the unnecessary connected check. This is a preparation patch for moving all connected state handling to hidpp_connect_event(). This series has been tested on the following devices: Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0) Logitech M720 Triathlon (bluetooth, HID++ 4.5) Logitech M720 Triathlon (unifying, HID++ 4.5) Logitech K400 Pro (unifying, HID++ 4.1) Logitech K270 (eQUAD nano Lite, HID++ 2.0) Logitech M185 (eQUAD nano Lite, HID++ 4.5) Logitech LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0) Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0) And by bentiss: Logitech Touchpad T650 (unifying) Logitech Touchpad T651 (bluetooth) Logitech MX Master 3B (BLE) Logitech G403 (plain USB / Gaming receiver) Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20231010102029.111003-9-hdegoede@redhat.com Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-logitech-hidpp.c | 7 ------- 1 file changed, 7 deletions(-) (limited to 'drivers/hid') diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index 55fc80f3be05..21f41c147f9b 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -4487,13 +4487,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) connected = hidpp_root_get_protocol_version(hidpp) == 0; atomic_set(&hidpp->connected, connected); - if (!(hidpp->quirks & HIDPP_QUIRK_UNIFYING)) { - if (!connected) { - ret = -ENODEV; - hid_err(hdev, "Device not connected"); - goto hid_hw_init_fail; - } - } schedule_work(&hidpp->work); flush_work(&hidpp->work); -- cgit v1.2.3 From bb17b2c6dd876d2bf6ef518d440b90f2095c5ea1 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 10 Oct 2023 12:20:26 +0200 Subject: HID: logitech-hidpp: Remove unused connected param from *_connect() Remove the unused connected function parameter from wtp_connect(), m560_send_config_command() and k400_connect(). This is a preparation patch for moving all connected state handling to hidpp_connect_event(). This series has been tested on the following devices: Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0) Logitech M720 Triathlon (bluetooth, HID++ 4.5) Logitech M720 Triathlon (unifying, HID++ 4.5) Logitech K400 Pro (unifying, HID++ 4.1) Logitech K270 (eQUAD nano Lite, HID++ 2.0) Logitech M185 (eQUAD nano Lite, HID++ 4.5) Logitech LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0) Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0) And by bentiss: Logitech Touchpad T650 (unifying) Logitech Touchpad T651 (bluetooth) Logitech MX Master 3B (BLE) Logitech G403 (plain USB / Gaming receiver) Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20231010102029.111003-10-hdegoede@redhat.com Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-logitech-hidpp.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'drivers/hid') diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index 21f41c147f9b..e945e4e8180f 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -3141,7 +3141,7 @@ static int wtp_allocate(struct hid_device *hdev, const struct hid_device_id *id) return 0; }; -static int wtp_connect(struct hid_device *hdev, bool connected) +static int wtp_connect(struct hid_device *hdev) { struct hidpp_device *hidpp = hid_get_drvdata(hdev); struct wtp_data *wd = hidpp->private_data; @@ -3203,7 +3203,7 @@ static const u8 m560_config_parameter[] = {0x00, 0xaf, 0x03}; #define M560_SUB_ID 0x0a #define M560_BUTTON_MODE_REGISTER 0x35 -static int m560_send_config_command(struct hid_device *hdev, bool connected) +static int m560_send_config_command(struct hid_device *hdev) { struct hidpp_report response; struct hidpp_device *hidpp_dev; @@ -3398,7 +3398,7 @@ static int k400_allocate(struct hid_device *hdev) return 0; }; -static int k400_connect(struct hid_device *hdev, bool connected) +static int k400_connect(struct hid_device *hdev) { struct hidpp_device *hidpp = hid_get_drvdata(hdev); @@ -4205,15 +4205,15 @@ static void hidpp_connect_event(struct hidpp_device *hidpp) } if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) { - ret = wtp_connect(hdev, connected); + ret = wtp_connect(hdev); if (ret) return; } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560) { - ret = m560_send_config_command(hdev, connected); + ret = m560_send_config_command(hdev); if (ret) return; } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_K400) { - ret = k400_connect(hdev, connected); + ret = k400_connect(hdev); if (ret) return; } -- cgit v1.2.3 From 680ee411a98e875f0f3baba735205ced61693e67 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 10 Oct 2023 12:20:27 +0200 Subject: HID: logitech-hidpp: Fix connect event race There is a connect event race in hidpp_probe() in these 2 lines: connected = hidpp_root_get_protocol_version(hidpp) == 0; atomic_set(&hidpp->connected, connected); Specifically the following can happen: 1. This line from hidpp_probe() is executed: connected = hidpp_root_get_protocol_version(hidpp) == 0; and sets connected to false; 2. A connect-event packet is received and does: atomic_set(&hidpp->connected, true); 3. The next line from hidpp_probe() is executed: atomic_set(&hidpp->connected, connected); and sets the atomic_t back to 0 again. 4. hidpp_connect_event() runs and sees the connected device as disconnected because of this. To fix this make hidpp_connect_event() query the connection status of the device itself instead of having it rely on possibly stale data cached in struct hidpp_device. This series has been tested on the following devices: Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0) Logitech M720 Triathlon (bluetooth, HID++ 4.5) Logitech M720 Triathlon (unifying, HID++ 4.5) Logitech K400 Pro (unifying, HID++ 4.1) Logitech K270 (eQUAD nano Lite, HID++ 2.0) Logitech M185 (eQUAD nano Lite, HID++ 4.5) Logitech LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0) Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0) And by bentiss: Logitech Touchpad T650 (unifying) Logitech Touchpad T651 (bluetooth) Logitech MX Master 3B (BLE) Logitech G403 (plain USB / Gaming receiver) Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20231010102029.111003-11-hdegoede@redhat.com Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-logitech-hidpp.c | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-) (limited to 'drivers/hid') diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index e945e4e8180f..4793a3236f17 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -194,7 +194,6 @@ struct hidpp_device { struct work_struct work; struct kfifo delayed_work_fifo; - atomic_t connected; struct input_dev *delayed_input; unsigned long quirks; @@ -3893,8 +3892,6 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data, } if (unlikely(hidpp_report_is_connect_event(hidpp, report))) { - atomic_set(&hidpp->connected, - !(report->rap.params[0] & (1 << 6))); if (schedule_work(&hidpp->work) == 0) dbg_hid("%s: connect event already queued\n", __func__); return 1; @@ -4189,12 +4186,14 @@ static struct input_dev *hidpp_allocate_input(struct hid_device *hdev) static void hidpp_connect_event(struct hidpp_device *hidpp) { struct hid_device *hdev = hidpp->hid_dev; - int ret = 0; - bool connected = atomic_read(&hidpp->connected); struct input_dev *input; char *name, *devm_name; + int ret; - if (!connected) { + /* Get device version to check if it is connected */ + ret = hidpp_root_get_protocol_version(hidpp); + if (ret) { + hid_info(hidpp->hid_dev, "Disconnected\n"); if (hidpp->battery.ps) { hidpp->battery.online = false; hidpp->battery.status = POWER_SUPPLY_STATUS_UNKNOWN; @@ -4236,16 +4235,6 @@ static void hidpp_connect_event(struct hidpp_device *hidpp) return; } - /* the device is already connected, we can ask for its name and - * protocol */ - if (!hidpp->protocol_major) { - ret = hidpp_root_get_protocol_version(hidpp); - if (ret) { - hid_err(hdev, "Can not get the protocol version.\n"); - return; - } - } - if (hidpp->protocol_major >= 2) { u8 feature_index; @@ -4395,7 +4384,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) { struct hidpp_device *hidpp; int ret; - bool connected; unsigned int connect_mask = HID_CONNECT_DEFAULT; /* report_fixup needs drvdata to be set before we call hid_parse */ @@ -4485,9 +4473,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) else hidpp_non_unifying_init(hidpp); - connected = hidpp_root_get_protocol_version(hidpp) == 0; - atomic_set(&hidpp->connected, connected); - schedule_work(&hidpp->work); flush_work(&hidpp->work); -- cgit v1.2.3 From f3c4ee7166f2048b88dfe3a4a9cc68a58f0abeb4 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 10 Oct 2023 12:20:28 +0200 Subject: HID: logitech-hidpp: Drop delayed_work_cb() Drop delayed_work_cb() instead make hidpp_connect_event() the workqueue function itself. Besides resulting in a small cleanup this will hopefully also make it clearer that going forward hidpp_connect_event() should only be run from a workqueue and not be directly invoked. This series has been tested on the following devices: Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0) Logitech M720 Triathlon (bluetooth, HID++ 4.5) Logitech M720 Triathlon (unifying, HID++ 4.5) Logitech K400 Pro (unifying, HID++ 4.1) Logitech K270 (eQUAD nano Lite, HID++ 2.0) Logitech M185 (eQUAD nano Lite, HID++ 4.5) Logitech LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0) Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0) And by bentiss: Logitech Touchpad T650 (unifying) Logitech Touchpad T651 (bluetooth) Logitech MX Master 3B (BLE) Logitech G403 (plain USB / Gaming receiver) Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20231010102029.111003-12-hdegoede@redhat.com Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-logitech-hidpp.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) (limited to 'drivers/hid') diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index 4793a3236f17..e75cf4300c59 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -234,8 +234,6 @@ struct hidpp_device { #define HIDPP20_ERROR_UNSUPPORTED 0x09 #define HIDPP20_ERROR 0xff -static void hidpp_connect_event(struct hidpp_device *hidpp_dev); - static int __hidpp_send_report(struct hid_device *hdev, struct hidpp_report *hidpp_report) { @@ -449,13 +447,6 @@ static int hidpp_send_rap_command_sync(struct hidpp_device *hidpp_dev, return ret; } -static void delayed_work_cb(struct work_struct *work) -{ - struct hidpp_device *hidpp = container_of(work, struct hidpp_device, - work); - hidpp_connect_event(hidpp); -} - static inline bool hidpp_match_answer(struct hidpp_report *question, struct hidpp_report *answer) { @@ -4183,8 +4174,9 @@ static struct input_dev *hidpp_allocate_input(struct hid_device *hdev) return input_dev; } -static void hidpp_connect_event(struct hidpp_device *hidpp) +static void hidpp_connect_event(struct work_struct *work) { + struct hidpp_device *hidpp = container_of(work, struct hidpp_device, work); struct hid_device *hdev = hidpp->hid_dev; struct input_dev *input; char *name, *devm_name; @@ -4435,7 +4427,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) return ret; } - INIT_WORK(&hidpp->work, delayed_work_cb); + INIT_WORK(&hidpp->work, hidpp_connect_event); mutex_init(&hidpp->send_mutex); init_waitqueue_head(&hidpp->wait); -- cgit v1.2.3 From 9ce363aa009c9ec1b921e3c316cbed7639c024e0 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 10 Oct 2023 12:20:29 +0200 Subject: HID: logitech-hidpp: Drop HIDPP_QUIRK_UNIFYING HIDPP unifying is only checked once in probe() and it is also set by probe() itself. Drop the quirk and replace its one check by the condition which is used to set the quirk in the first place. This series has been tested on the following devices: Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0) Logitech M720 Triathlon (bluetooth, HID++ 4.5) Logitech M720 Triathlon (unifying, HID++ 4.5) Logitech K400 Pro (unifying, HID++ 4.1) Logitech K270 (eQUAD nano Lite, HID++ 2.0) Logitech M185 (eQUAD nano Lite, HID++ 4.5) Logitech LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0) Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0) And by bentiss: Logitech Touchpad T650 (unifying) Logitech Touchpad T651 (bluetooth) Logitech MX Master 3B (BLE) Logitech G403 (plain USB / Gaming receiver) Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20231010102029.111003-13-hdegoede@redhat.com Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-logitech-hidpp.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) (limited to 'drivers/hid') diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index e75cf4300c59..c1bc89560612 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -69,12 +69,11 @@ MODULE_PARM_DESC(disable_tap_to_click, #define HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS BIT(22) #define HIDPP_QUIRK_DELAYED_INIT BIT(23) #define HIDPP_QUIRK_FORCE_OUTPUT_REPORTS BIT(24) -#define HIDPP_QUIRK_UNIFYING BIT(25) -#define HIDPP_QUIRK_HIDPP_WHEELS BIT(26) -#define HIDPP_QUIRK_HIDPP_EXTRA_MOUSE_BTNS BIT(27) -#define HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS BIT(28) -#define HIDPP_QUIRK_HI_RES_SCROLL_1P0 BIT(29) -#define HIDPP_QUIRK_WIRELESS_STATUS BIT(30) +#define HIDPP_QUIRK_HIDPP_WHEELS BIT(25) +#define HIDPP_QUIRK_HIDPP_EXTRA_MOUSE_BTNS BIT(26) +#define HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS BIT(27) +#define HIDPP_QUIRK_HI_RES_SCROLL_1P0 BIT(28) +#define HIDPP_QUIRK_WIRELESS_STATUS BIT(29) /* These are just aliases for now */ #define HIDPP_QUIRK_KBD_SCROLL_WHEEL HIDPP_QUIRK_HIDPP_WHEELS @@ -4405,9 +4404,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) return hid_hw_start(hdev, HID_CONNECT_DEFAULT); } - if (id->group == HID_GROUP_LOGITECH_DJ_DEVICE) - hidpp->quirks |= HIDPP_QUIRK_UNIFYING; - if (id->group == HID_GROUP_LOGITECH_27MHZ_DEVICE && hidpp_application_equals(hdev, HID_GD_MOUSE)) hidpp->quirks |= HIDPP_QUIRK_HIDPP_WHEELS | @@ -4460,7 +4456,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) hid_device_io_start(hdev); /* Get name + serial, store in hdev->name + hdev->uniq */ - if (hidpp->quirks & HIDPP_QUIRK_UNIFYING) + if (id->group == HID_GROUP_LOGITECH_DJ_DEVICE) hidpp_unifying_init(hidpp); else hidpp_non_unifying_init(hidpp); -- cgit v1.2.3 From 3e6b0bb22a803532a7ddf8ba255c46213c24a478 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Wed, 25 Oct 2023 21:01:51 +0200 Subject: HID: logitech-hidpp: Stop IO before calling hid_connect() hid_connect() will call hid_pidff_init() which does hid_device_io_start() leading to an "io already started" warning. To fix this call hid_device_io_stop() before calling hid_connect(), stopping IO means that connect events may be lost while hid_connect() runs, re-enable IO and move the hidpp_connect_event() work queuing after the hid_connect(). Note re-enabling IO is also necessary for the g920_get_config() call later during hidpp_probe(). Signed-off-by: Hans de Goede Link: https://lore.kernel.org/r/20231025190151.302376-1-hdegoede@redhat.com Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-logitech-hidpp.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'drivers/hid') diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index c1bc89560612..fd6d8f1d9b8f 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -4461,19 +4461,22 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) else hidpp_non_unifying_init(hidpp); - schedule_work(&hidpp->work); - flush_work(&hidpp->work); - if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT) connect_mask &= ~HID_CONNECT_HIDINPUT; /* Now export the actual inputs and hidraw nodes to the world */ + hid_device_io_stop(hdev); ret = hid_connect(hdev, connect_mask); if (ret) { hid_err(hdev, "%s:hid_connect returned error %d\n", __func__, ret); goto hid_hw_init_fail; } + /* Check for connected devices now that incoming packets will not be disabled again */ + hid_device_io_start(hdev); + schedule_work(&hidpp->work); + flush_work(&hidpp->work); + if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) { struct hidpp_ff_private_data data; -- cgit v1.2.3