[PATCH v3 00/12] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO

[PATCH v3 00/12] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO

[Hans de Goede]

Hi Benjamin,

As dicussed here is v3 of my series to rework / cleanup the hidpp
probing code.

Note the $subject of the cover-letter is not entirely accurate anymore,
but I kept it the same since this is the successor of series with
the same subject.

Changes in v3:
- Call hid_connect() to connect the hid-input and hidraw drivers
  after retrieving the name + serial instead of restarting IO
- Shuffle the series order to put patches with Fixes tags at the start
- Squash 2 changes to G920 handling into one
- Since probe() now no longer restarts IO the following patches have been dropped:
  "HID: logitech-hidpp: Add a hidpp_connect_and_start() helper"
  "HID: logitech-hidpp: Move the connected check to after restarting IO"
  "HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO"
- Add "HID: logitech-hidpp: Drop HIDPP_QUIRK_UNIFYING"

Regards,

Hans


Hans de Goede (12):
  HID: logitech-hidpp: Don't restart IO, instead defer hid_connect()
    only
  HID: logitech-hidpp: Revert "Don't restart communication if not
    necessary"
  HID: logitech-hidpp: Move get_wireless_feature_index() check to
    hidpp_connect_event()
  HID: logitech-hidpp: Remove wtp_get_config() call from probe()
  HID: logitech-hidpp: Move g920_get_config() to just before
    hidpp_ff_init()
  HID: logitech-hidpp: Move hidpp_overwrite_name() to before connect
    check
  HID: logitech-hidpp: Add hidpp_non_unifying_init() helper
  HID: logitech-hidpp: Remove connected check for non-unifying devices
  HID: logitech-hidpp: Remove unused connected param from *_connect()
  HID: logitech-hidpp: Fix connect event race
  HID: logitech-hidpp: Drop delayed_work_cb()
  HID: logitech-hidpp: Drop HIDPP_QUIRK_UNIFYING

 drivers/hid/hid-logitech-hidpp.c | 173 +++++++++++--------------------
 1 file changed, 61 insertions(+), 112 deletions(-)

-- 
2.41.0

[PATCH v3 01/12] HID: logitech-hidpp: Don't restart IO, instead defer hid_connect() only

[Hans de Goede]

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)

Fixes: 498ba2069035 ("HID: logitech-hidpp: Don't restart communication if not necessary")
Suggested-by: Benjamin Tissoires <bentiss@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

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:
-- 
2.41.0

[PATCH v3 02/12] HID: logitech-hidpp: Revert "Don't restart communication if not necessary"

[Hans de Goede]

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)

Fixes: 498ba2069035 ("HID: logitech-hidpp: Don't restart communication if not necessary")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

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) {
-- 
2.41.0

[PATCH v3 03/12] HID: logitech-hidpp: Move get_wireless_feature_index() check to hidpp_connect_event()

[Hans de Goede]

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)

Fixes: 0da0a63b7cba ("HID: logitech-hidpp: Support WirelessDeviceStatus connect events")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

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)
-- 
2.41.0

[PATCH v3 04/12] HID: logitech-hidpp: Remove wtp_get_config() call from probe()

[Hans de Goede]

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)

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

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;
-- 
2.41.0

[PATCH v3 05/12] HID: logitech-hidpp: Move g920_get_config() to just before hidpp_ff_init()

[Hans de Goede]

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)

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

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",
-- 
2.41.0

[PATCH v3 06/12] HID: logitech-hidpp: Move hidpp_overwrite_name() to before connect check

[Hans de Goede]

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)

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note: The weird unbalanced curly braces in the "else { ... }" go away
in the next patch in the series.
---
 drivers/hid/hid-logitech-hidpp.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

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);
-- 
2.41.0

[PATCH v3 07/12] HID: logitech-hidpp: Add hidpp_non_unifying_init() helper

[Hans de Goede]

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)

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

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);
-- 
2.41.0

[PATCH v3 08/12] HID: logitech-hidpp: Remove connected check for non-unifying devices

[Hans de Goede]

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)

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 7 -------
 1 file changed, 7 deletions(-)

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);
-- 
2.41.0

[PATCH v3 09/12] HID: logitech-hidpp: Remove unused connected param from *_connect()

[Hans de Goede]

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)

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

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;
 	}
-- 
2.41.0

[PATCH v3 10/12] HID: logitech-hidpp: Fix connect event race

[Hans de Goede]

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)

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)

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);
 
-- 
2.41.0

[PATCH v3 11/12] HID: logitech-hidpp: Drop delayed_work_cb()

[Hans de Goede]

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)

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

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);
 
-- 
2.41.0

[PATCH v3 12/12] HID: logitech-hidpp: Drop HIDPP_QUIRK_UNIFYING

[Hans de Goede]

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)

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

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);
-- 
2.41.0

Re: [PATCH v3 01/12] HID: logitech-hidpp: Don't restart IO, instead defer hid_connect() only

[Benjamin Tissoires]

Hi Hans,

FWIW, your series looks good, but I came accross a small nitpick here:

On Oct 10 2023, Hans de Goede wrote:
> 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)
> 
> Fixes: 498ba2069035 ("HID: logitech-hidpp: Don't restart communication if not necessary")
> Suggested-by: Benjamin Tissoires <bentiss@kernel.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/hid/hid-logitech-hidpp.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> 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);

On plain USB devices, we get a new warning here "io already started".

This is because hid_connect() will call hid_pidff_init() from
drivers/hid/usbhid/hid-pidff.c if connect_mask has the `HID_CONNECT_FF`
bit set.

And hid_pidff_init() blindly calls hid_device_io_start() then
hid_device_io_stop().

It's not a big issue, but still it's a new warning we have to tackly on.

I see 3 possible solutions:
- teach hid_pidff_init() to only start/stop the IOs if it's not already
  done so
- if a device is actually connected through USB, call
  hid_device_io_stop() before hid_connect()
- unconditionally call hid_device_io_stop() before hid_connect()

The latter has my current preference as we won't get biten in the future
if something else decides to change the io state.

However, do you thing it'll be an issue to disable IOs there?
And maybe we should re-enable them after?

If it's fine to simply disable the IOs, we can add an extra patch on top
of this series to fix that warning in the USB case.

As mentioned above, I have tested with the T650, T651 that were likely to
be a problem, and they are working just fine :)

So with that minor fix, we should be able to stage this series.

Thanks again for your hard work

Cheers,
Benjamin

>  		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:
> -- 
> 2.41.0
> 

Re: [PATCH v3 01/12] HID: logitech-hidpp: Don't restart IO, instead defer hid_connect() only

[Benjamin Tissoires]

On Oct 18 2023, Benjamin Tissoires wrote:
> Hi Hans,
> 
> FWIW, your series looks good, but I came accross a small nitpick here:
> 
> On Oct 10 2023, Hans de Goede wrote:
> > 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)
> > 
> > Fixes: 498ba2069035 ("HID: logitech-hidpp: Don't restart communication if not necessary")
> > Suggested-by: Benjamin Tissoires <bentiss@kernel.org>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  drivers/hid/hid-logitech-hidpp.c | 22 ++++++++++++----------
> >  1 file changed, 12 insertions(+), 10 deletions(-)
> > 
> > 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);
> 
> On plain USB devices, we get a new warning here "io already started".
> 
> This is because hid_connect() will call hid_pidff_init() from
> drivers/hid/usbhid/hid-pidff.c if connect_mask has the `HID_CONNECT_FF`
> bit set.
> 
> And hid_pidff_init() blindly calls hid_device_io_start() then
> hid_device_io_stop().
> 
> It's not a big issue, but still it's a new warning we have to tackly on.
> 
> I see 3 possible solutions:
> - teach hid_pidff_init() to only start/stop the IOs if it's not already
>   done so
> - if a device is actually connected through USB, call
>   hid_device_io_stop() before hid_connect()
> - unconditionally call hid_device_io_stop() before hid_connect()
> 
> The latter has my current preference as we won't get biten in the future
> if something else decides to change the io state.
> 
> However, do you thing it'll be an issue to disable IOs there?
> And maybe we should re-enable them after?
> 
> If it's fine to simply disable the IOs, we can add an extra patch on top
> of this series to fix that warning in the USB case.
> 
> As mentioned above, I have tested with the T650, T651 that were likely to
> be a problem, and they are working just fine :)
> 
> So with that minor fix, we should be able to stage this series.

The merge window is coming very soon. So I'm taking this series as it is
(I just added the few devices I made the tests), and we can work on an
extra patch to remove that warning.

Cheers,
Benjamin

> 
> Thanks again for your hard work
> 
> Cheers,
> Benjamin
> 
> >  		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:
> > -- 
> > 2.41.0
> > 

Re: [PATCH v3 01/12] HID: logitech-hidpp: Don't restart IO, instead defer hid_connect() only

[Hans de Goede]

Hi,

On 10/25/23 18:43, Benjamin Tissoires wrote:
> On Oct 18 2023, Benjamin Tissoires wrote:
>> Hi Hans,
>>
>> FWIW, your series looks good, but I came accross a small nitpick here:
>>
>> On Oct 10 2023, Hans de Goede wrote:
>>> 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)
>>>
>>> Fixes: 498ba2069035 ("HID: logitech-hidpp: Don't restart communication if not necessary")
>>> Suggested-by: Benjamin Tissoires <bentiss@kernel.org>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>  drivers/hid/hid-logitech-hidpp.c | 22 ++++++++++++----------
>>>  1 file changed, 12 insertions(+), 10 deletions(-)
>>>
>>> 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);
>>
>> On plain USB devices, we get a new warning here "io already started".
>>
>> This is because hid_connect() will call hid_pidff_init() from
>> drivers/hid/usbhid/hid-pidff.c if connect_mask has the `HID_CONNECT_FF`
>> bit set.
>>
>> And hid_pidff_init() blindly calls hid_device_io_start() then
>> hid_device_io_stop().
>>
>> It's not a big issue, but still it's a new warning we have to tackly on.
>>
>> I see 3 possible solutions:
>> - teach hid_pidff_init() to only start/stop the IOs if it's not already
>>   done so
>> - if a device is actually connected through USB, call
>>   hid_device_io_stop() before hid_connect()
>> - unconditionally call hid_device_io_stop() before hid_connect()
>>
>> The latter has my current preference as we won't get biten in the future
>> if something else decides to change the io state.
>>
>> However, do you thing it'll be an issue to disable IOs there?

Not really an issue, but if we disable IOs then we may loose
incoming packets with a connect event in there.

>> And maybe we should re-enable them after?

If we disable IOs before hid_connect() (or at any point
after enabling them) then connect events may be lost
so we must re-enable IOs then and move the hidpp_connect_event()
work queuing, which polls for already connected devices to
after the re-enabling.

Also IOs need to be re-enabled for the g920_get_config()
call later during hidpp_probe().

I have just written a patch for this and submitted it upstream :)

>> If it's fine to simply disable the IOs, we can add an extra patch on top
>> of this series to fix that warning in the USB case.
>>
>> As mentioned above, I have tested with the T650, T651 that were likely to
>> be a problem, and they are working just fine :)
>>
>> So with that minor fix, we should be able to stage this series.
> 
> The merge window is coming very soon. So I'm taking this series as it is
> (I just added the few devices I made the tests), and we can work on an
> extra patch to remove that warning.

Extra patch submitted :)

Regards,

Hans

Re: [PATCH v3 00/12] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO

[Benjamin Tissoires]

On Tue, 10 Oct 2023 12:20:17 +0200, Hans de Goede wrote:
> As dicussed here is v3 of my series to rework / cleanup the hidpp
> probing code.
> 
> Note the $subject of the cover-letter is not entirely accurate anymore,
> but I kept it the same since this is the successor of series with
> the same subject.
> 
> [...]

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git (for-6.7/logitech), thanks!

[01/12] HID: logitech-hidpp: Don't restart IO, instead defer hid_connect() only
        https://git.kernel.org/hid/hid/c/11ca0322a419
[02/12] HID: logitech-hidpp: Revert "Don't restart communication if not necessary"
        https://git.kernel.org/hid/hid/c/55bf70362ffc
[03/12] HID: logitech-hidpp: Move get_wireless_feature_index() check to hidpp_connect_event()
        https://git.kernel.org/hid/hid/c/ba9de3505095
[04/12] HID: logitech-hidpp: Remove wtp_get_config() call from probe()
        https://git.kernel.org/hid/hid/c/a3643036d7a8
[05/12] HID: logitech-hidpp: Move g920_get_config() to just before hidpp_ff_init()
        https://git.kernel.org/hid/hid/c/219ccfb60003
[06/12] HID: logitech-hidpp: Move hidpp_overwrite_name() to before connect check
        https://git.kernel.org/hid/hid/c/8954dac18c68
[07/12] HID: logitech-hidpp: Add hidpp_non_unifying_init() helper
        https://git.kernel.org/hid/hid/c/c14f1485c605
[08/12] HID: logitech-hidpp: Remove connected check for non-unifying devices
        https://git.kernel.org/hid/hid/c/6f335b47adc3
[09/12] HID: logitech-hidpp: Remove unused connected param from *_connect()
        https://git.kernel.org/hid/hid/c/bb17b2c6dd87
[10/12] HID: logitech-hidpp: Fix connect event race
        https://git.kernel.org/hid/hid/c/680ee411a98e
[11/12] HID: logitech-hidpp: Drop delayed_work_cb()
        https://git.kernel.org/hid/hid/c/f3c4ee7166f2
[12/12] HID: logitech-hidpp: Drop HIDPP_QUIRK_UNIFYING
        https://git.kernel.org/hid/hid/c/9ce363aa009c

Cheers,
-- 
Benjamin Tissoires <bentiss@kernel.org>