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

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

[Hans de Goede]

Hi Benjamin,

Here is a v2 of my series to fix issues with hidpp_connect_event() running
while restarting IO, which now also fixes the issues you mentioned with
potentially missing connect events.

This series is best explained by a brief sketch of how probe()
looks at the end of the series (1):

Prep work:

1. All code depending on a device being in connected state is moved to
   the hidpp_connect_event() workqueue item

2. hidpp_connect_event() now checks the connected state itself by
   checking that hidpp_root_get_protocol_version() succeeds, instead
   of relying on possibly stale (racy) data in struct hidpp_device

With this in place the new probe() sequence looks like this:

1. enable_connect_event flag starts at 0, this filters out / ignores any
   connect-events in hidpp_raw_hidpp_event() avoiding
   hidpp_connect_event() getting queued before the IO restart

2. IO is started with connect-mask = 0
   this avoids hid-input and hidraw connecting while probe() is setting
   hdev->name and hdev->uniq

3. Name and serialnr are retrieved and stored in hdev

4. IO is fully restarted (including hw_open + io_start, not just hw_start)
   with the actual connect-mask.

5. enable_connect_event atomic_t is set to 1 to enable processing of
   connect events.

6. hidpp_connect_event() is queued + flushed to query the connected state
   and do the connect work if the device is connected.

7. probe() now ends with:

        /*
         * This relies on logi_dj_ll_close() being a no-op so that
         * DJ connection events will still be received.
         */
        hid_hw_close(hdev);

   Since on restarting IO it now is fully restarted so the hid_hw_open()
   there needs to be balanced. 

This series now obviously is no longer 6.6 material, instead I hope we
can get this rework (and IMHO nice cleanup) into 6.7 .

Regards,

Hans


1) For reviewing you may also want to apply the entire series and look
at the end result to help you understand why various intermediate steps
are taken.


Hans de Goede (14):
  HID: logitech-hidpp: Revert "Don't restart communication if not
    necessary"
  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: Move get_wireless_feature_index() check to
    hidpp_connect_event()
  HID: logitech-hidpp: Remove wtp_get_config() call from probe()
  HID: logitech-hidpp: Remove connected check for g920_get_config() call
  HID: logitech-hidpp: Add a hidpp_connect_and_start() helper
  HID: logitech-hidpp: Move the connected check to after restarting IO
  HID: logitech-hidpp: Move g920_get_config() to just before
    hidpp_ff_init()
  HID: logitech-hidpp: Remove unused connected param from *_connect()
  HID: logitech-hidpp: Fix connect event race
  HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe()
    restarts IO
  HID: logitech-hidpp: Drop delayed_work_cb()

 drivers/hid/hid-logitech-hidpp.c | 211 +++++++++++++------------------
 1 file changed, 91 insertions(+), 120 deletions(-)

-- 
2.41.0

[PATCH v2 01/14] 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 is 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.

Revert the change to limit the restart behavior to unifying devices to
fix hdev->name changing after userspace facing devices have already been
registered.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index a209d51bd247..09f7723eaabe 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);
@@ -4463,7 +4458,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	 * Plain USB connections need to actually call start and open
 	 * on the transport driver to allow incoming data.
 	 */
-	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;
@@ -4502,7 +4497,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)) {
@@ -4518,21 +4512,19 @@ 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) {
-		/* Reset the HID node state */
-		hid_device_io_stop(hdev);
-		hid_hw_close(hdev);
-		hid_hw_stop(hdev);
+	/* 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;
+	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);
-		if (ret) {
-			hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
-			goto hid_hw_start_fail;
-		}
+	/* Now export the actual inputs and hidraw nodes to the world */
+	ret = hid_hw_start(hdev, connect_mask);
+	if (ret) {
+		hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
+		goto hid_hw_start_fail;
 	}
 
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
-- 
2.41.0

[PATCH v2 02/14] 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 the connect check to after
restarting IO, in case we miss a connect packet coming in while IO
is disabled during the restart.

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.

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 09f7723eaabe..b1965b91c5bb 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4136,19 +4136,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)
@@ -4476,8 +4469,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);
@@ -4487,8 +4484,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);
 	}
 
 	if (connected && hidpp->protocol_major >= 2) {
-- 
2.41.0

[PATCH v2 03/14] 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.

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 b1965b91c5bb..e4dbbdf46b97 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4131,11 +4131,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);
@@ -4467,14 +4472,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 v2 04/14] 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 querying if the device is connected
to after restarting IO, in case we miss a connect packet coming in while
IO is disabled during the restart.

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 e4dbbdf46b97..82f9723154f1 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4480,13 +4480,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;
-		}
-	}
 
 	if (connected && hidpp->protocol_major >= 2) {
 		ret = hidpp_set_wireless_feature_index(hidpp);
-- 
2.41.0

[PATCH v2 05/14] 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.

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 82f9723154f1..fe4c4871ea5c 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;
 }
@@ -4247,6 +4246,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) {
@@ -4481,14 +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 (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 v2 06/14] 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 the hidpp_connect_event() for those at probe time.

Drop the unnecessary wtp_get_config() call from probe().

This is a preparation patch for moving the connect check to after
restarting IO, in case we miss a connect packet coming in while IO
is disabled during the restart.

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 fe4c4871ea5c..d7375885b0c7 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4487,11 +4487,7 @@ 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 (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 v2 07/14] HID: logitech-hidpp: Remove connected check for g920_get_config() call

[Hans de Goede]

The G920 is a wired USB device, so it is always connected, remove
the unnecessary connected check.

This is a preparation patch for moving the connect check to after
restarting IO, in case we miss a connect packet coming in while IO
is disabled during the restart.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index d7375885b0c7..bbb1c6d8ccc9 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4487,7 +4487,7 @@ 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 (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) {
+	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
 		ret = g920_get_config(hidpp, &data);
 		if (ret)
 			goto hid_hw_init_fail;
-- 
2.41.0

[PATCH v2 08/14] HID: logitech-hidpp: Add a hidpp_connect_and_start() helper

[Hans de Goede]

Add a helper to take the 3 hid_hw_start() + hid_hw_open() +
hid_device_io_start() steps which are necessary to be able to
talk to the hw.

This is a preparation patch for moving the connect check to after
restarting IO, in case we miss a connect packet coming in while IO
is disabled during the restart.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index bbb1c6d8ccc9..45b371e7b9ee 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4391,6 +4391,29 @@ static bool hidpp_application_equals(struct hid_device *hdev,
 	return report && report->application == application;
 }
 
+static int hidpp_connect_and_start(struct hidpp_device *hidpp, unsigned int connect_mask)
+{
+	struct hid_device *hdev = hidpp->hid_dev;
+	int ret;
+
+	ret = hid_hw_start(hdev, connect_mask);
+	if (ret) {
+		hid_err(hdev, "hw start failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = hid_hw_open(hdev);
+	if (ret) {
+		hid_err(hdev, "hw open failed: %d\n", ret);
+		hid_hw_stop(hdev);
+		return ret;
+	}
+
+	/* Allow incoming packets */
+	hid_device_io_start(hdev);
+	return 0;
+}
+
 static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	struct hidpp_device *hidpp;
@@ -4462,21 +4485,9 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	 * Plain USB connections need to actually call start and open
 	 * on the transport driver to allow incoming data.
 	 */
-	ret = hid_hw_start(hdev, 0);
-	if (ret) {
-		hid_err(hdev, "hw start failed\n");
+	ret = hidpp_connect_and_start(hidpp, 0);
+	if (ret)
 		goto hid_hw_start_fail;
-	}
-
-	ret = hid_hw_open(hdev);
-	if (ret < 0) {
-		dev_err(&hdev->dev, "%s:hid_hw_open returned error:%d\n",
-			__func__, ret);
-		goto hid_hw_open_fail;
-	}
-
-	/* Allow incoming packets */
-	hid_device_io_start(hdev);
 
 	/* Get name + serial, store in hdev->name + hdev->uniq */
 	if (hidpp->quirks & HIDPP_QUIRK_UNIFYING)
@@ -4523,7 +4534,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 hid_hw_init_fail:
 	hid_hw_close(hdev);
-hid_hw_open_fail:
 	hid_hw_stop(hdev);
 hid_hw_start_fail:
 	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
-- 
2.41.0

[PATCH v2 09/14] HID: logitech-hidpp: Move the connected check to after restarting IO

[Hans de Goede]

While restarting IO incoming packets get disabled by calling
hid_device_io_stop() and they do not get re-enabled again until
the hid-core enables the when probe() is done.

This leaves a window where connect events may get lost, causing
hidpp to not be aware that a device has (dis)connected.

To fix this fully restart IO using the new hidpp_connect_and_start()
helper and move the connected check to after restarting IO.

This requires calling hid_hw_close() at the end of probe() to balance
the open() now done on restart.

Reported-by: Benjamin Tissoires <bentiss@kernel.org>
Closes: https://lore.kernel.org/linux-input/zjiang3fdy4o7r3daupwpnx6zesmeeerldpx5fno2adzialpre@cdp7tq4araww/
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 33 ++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 45b371e7b9ee..ff834f905eda 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4482,8 +4482,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 = hidpp_connect_and_start(hidpp, 0);
 	if (ret)
@@ -4495,18 +4497,12 @@ 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);
-
 	if (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);
-
 	/* Reset the HID node state */
 	hid_device_io_stop(hdev);
 	hid_hw_close(hdev);
@@ -4516,11 +4512,19 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		connect_mask &= ~HID_CONNECT_HIDINPUT;
 
 	/* Now export the actual inputs and hidraw nodes to the world */
-	ret = hid_hw_start(hdev, connect_mask);
-	if (ret) {
-		hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
+	ret = hidpp_connect_and_start(hidpp, connect_mask);
+	if (ret)
 		goto hid_hw_start_fail;
-	}
+
+	/*
+	 * Now that incoming packets are enabled and will not be disabled
+	 * again (which may cause missing packets) check the connected state
+	 * of the device.
+	 */
+	connected = hidpp_root_get_protocol_version(hidpp) == 0;
+	atomic_set(&hidpp->connected, connected);
+	schedule_work(&hidpp->work);
+	flush_work(&hidpp->work);
 
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
 		ret = hidpp_ff_init(hidpp, &data);
@@ -4530,6 +4534,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 v2 10/14] HID: logitech-hidpp: Move g920_get_config() to just before hidpp_ff_init()

[Hans de Goede]

The data retrieved by g920_get_config() is onyl 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.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index ff834f905eda..b278e2b8924a 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4420,7 +4420,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);
@@ -4497,12 +4496,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	else
 		hidpp_non_unifying_init(hidpp);
 
-	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
-		ret = g920_get_config(hidpp, &data);
-		if (ret)
-			goto hid_hw_init_fail;
-	}
-
 	/* Reset the HID node state */
 	hid_device_io_stop(hdev);
 	hid_hw_close(hdev);
@@ -4527,7 +4520,12 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	flush_work(&hidpp->work);
 
 	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",
@@ -4541,9 +4539,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	hid_hw_close(hdev);
 	return ret;
 
-hid_hw_init_fail:
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
 hid_hw_start_fail:
 	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
 	cancel_work_sync(&hidpp->work);
-- 
2.41.0

[PATCH v2 11/14] 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().

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 b278e2b8924a..2b0a2ea5da22 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 v2 12/14] 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.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 2b0a2ea5da22..37213dcc9d9c 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;
@@ -4235,17 +4234,6 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 		if (ret)
 			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;
 
@@ -4418,7 +4406,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 */
@@ -4511,11 +4498,9 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	/*
 	 * Now that incoming packets are enabled and will not be disabled
-	 * again (which may cause missing packets) check the connected state
-	 * of the device.
+	 * again (which may cause missing packets) queue hidpp_connect_event()
+	 * to check the connected state of the device.
 	 */
-	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 v2 13/14] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO

[Hans de Goede]

hidpp_probe() restarts IO after setting things up, if we get a connect
event just before hidpp_probe() stops all IO then hidpp_connect_event()
will be running from the workqueue and it will see IO errors.

To avoid this add an enable_connect_event flag and only set that
after the IO restart is done.

Since an hidpp_connect_event() run is queued after restarting IO
anyways, connect events before that run can safely be ignored.

Note this also means that hidpp_connect_event() is now guaranteed to
not get queued on hidpp_connect_and_start() failures so the work
for this no longer needs to be cancelled on failure.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Use an enable_connect_event flag instead of a mutex
---
 drivers/hid/hid-logitech-hidpp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 37213dcc9d9c..9e34a29619a0 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -192,6 +192,7 @@ struct hidpp_device {
 
 	void *private_data;
 
+	atomic_t enable_connect_event;
 	struct work_struct work;
 	struct kfifo delayed_work_fifo;
 	struct input_dev *delayed_input;
@@ -3892,6 +3893,9 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
 	}
 
 	if (unlikely(hidpp_report_is_connect_event(hidpp, report))) {
+		if (!atomic_read(&hidpp->enable_connect_event))
+			return 1;
+
 		if (schedule_work(&hidpp->work) == 0)
 			dbg_hid("%s: connect event already queued\n", __func__);
 		return 1;
@@ -4501,6 +4505,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	 * again (which may cause missing packets) queue hidpp_connect_event()
 	 * to check the connected state of the device.
 	 */
+	atomic_set(&hidpp->enable_connect_event, 1);
 	schedule_work(&hidpp->work);
 	flush_work(&hidpp->work);
 
@@ -4526,7 +4531,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 hid_hw_start_fail:
 	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
-	cancel_work_sync(&hidpp->work);
 	mutex_destroy(&hidpp->send_mutex);
 	return ret;
 }
-- 
2.41.0

[PATCH v2 14/14] 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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Link: https://lore.kernel.org/r/20231006081858.17677-3-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 9e34a29619a0..547544a5649b 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -235,8 +235,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)
 {
@@ -450,13 +448,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)
 {
@@ -4187,8 +4178,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;
@@ -4461,7 +4453,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

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

[Hans de Goede]

Hi,

On 10/8/23 11:54, Hans de Goede wrote:
> Hi Benjamin,
> 
> Here is a v2 of my series to fix issues with hidpp_connect_event() running
> while restarting IO, which now also fixes the issues you mentioned with
> potentially missing connect events.
> 
> This series is best explained by a brief sketch of how probe()
> looks at the end of the series (1):
> 
> Prep work:
> 
> 1. All code depending on a device being in connected state is moved to
>    the hidpp_connect_event() workqueue item
> 
> 2. hidpp_connect_event() now checks the connected state itself by
>    checking that hidpp_root_get_protocol_version() succeeds, instead
>    of relying on possibly stale (racy) data in struct hidpp_device
> 
> With this in place the new probe() sequence looks like this:
> 
> 1. enable_connect_event flag starts at 0, this filters out / ignores any
>    connect-events in hidpp_raw_hidpp_event() avoiding
>    hidpp_connect_event() getting queued before the IO restart
> 
> 2. IO is started with connect-mask = 0
>    this avoids hid-input and hidraw connecting while probe() is setting
>    hdev->name and hdev->uniq
> 
> 3. Name and serialnr are retrieved and stored in hdev
> 
> 4. IO is fully restarted (including hw_open + io_start, not just hw_start)
>    with the actual connect-mask.
> 
> 5. enable_connect_event atomic_t is set to 1 to enable processing of
>    connect events.
> 
> 6. hidpp_connect_event() is queued + flushed to query the connected state
>    and do the connect work if the device is connected.
> 
> 7. probe() now ends with:
> 
>         /*
>          * This relies on logi_dj_ll_close() being a no-op so that
>          * DJ connection events will still be received.
>          */
>         hid_hw_close(hdev);
> 
>    Since on restarting IO it now is fully restarted so the hid_hw_open()
>    there needs to be balanced. 
> 
> This series now obviously is no longer 6.6 material, instead I hope we
> can get this rework (and IMHO nice cleanup) into 6.7 .
> 
> Regards,
> 
> Hans

I forgot to add info on the list of devices I tested this on:

Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0)
Logitech M720 Triathlon (bluetooth, 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 Keyboard LX501 (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0)
Logitech 27Mhz mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0)

Regards,

Hans



> 1) For reviewing you may also want to apply the entire series and look
> at the end result to help you understand why various intermediate steps
> are taken.
> 
> 
> Hans de Goede (14):
>   HID: logitech-hidpp: Revert "Don't restart communication if not
>     necessary"
>   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: Move get_wireless_feature_index() check to
>     hidpp_connect_event()
>   HID: logitech-hidpp: Remove wtp_get_config() call from probe()
>   HID: logitech-hidpp: Remove connected check for g920_get_config() call
>   HID: logitech-hidpp: Add a hidpp_connect_and_start() helper
>   HID: logitech-hidpp: Move the connected check to after restarting IO
>   HID: logitech-hidpp: Move g920_get_config() to just before
>     hidpp_ff_init()
>   HID: logitech-hidpp: Remove unused connected param from *_connect()
>   HID: logitech-hidpp: Fix connect event race
>   HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe()
>     restarts IO
>   HID: logitech-hidpp: Drop delayed_work_cb()
> 
>  drivers/hid/hid-logitech-hidpp.c | 211 +++++++++++++------------------
>  1 file changed, 91 insertions(+), 120 deletions(-)
> 

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

[Benjamin Tissoires]

Hi Hans,

On Oct 08 2023, Hans de Goede wrote:
> Hi,
> 
> On 10/8/23 11:54, Hans de Goede wrote:
> > Hi Benjamin,
> > 
> > Here is a v2 of my series to fix issues with hidpp_connect_event() running
> > while restarting IO, which now also fixes the issues you mentioned with
> > potentially missing connect events.

Great, thanks a lot for this hard work.

> > 
> > This series is best explained by a brief sketch of how probe()
> > looks at the end of the series (1):

TBH, I couldn't parse the following yesterday evening, but after looking
at all patches one by one I can now get it :)

> > 
> > Prep work:
> > 
> > 1. All code depending on a device being in connected state is moved to
> >    the hidpp_connect_event() workqueue item
> > 
> > 2. hidpp_connect_event() now checks the connected state itself by
> >    checking that hidpp_root_get_protocol_version() succeeds, instead
> >    of relying on possibly stale (racy) data in struct hidpp_device
> > 
> > With this in place the new probe() sequence looks like this:
> > 
> > 1. enable_connect_event flag starts at 0, this filters out / ignores any
> >    connect-events in hidpp_raw_hidpp_event() avoiding
> >    hidpp_connect_event() getting queued before the IO restart
> > 
> > 2. IO is started with connect-mask = 0
> >    this avoids hid-input and hidraw connecting while probe() is setting
> >    hdev->name and hdev->uniq
> > 
> > 3. Name and serialnr are retrieved and stored in hdev
> > 
> > 4. IO is fully restarted (including hw_open + io_start, not just hw_start)
> >    with the actual connect-mask.
> > 
> > 5. enable_connect_event atomic_t is set to 1 to enable processing of
> >    connect events.
> > 
> > 6. hidpp_connect_event() is queued + flushed to query the connected state
> >    and do the connect work if the device is connected.
> > 
> > 7. probe() now ends with:
> > 
> >         /*
> >          * This relies on logi_dj_ll_close() being a no-op so that
> >          * DJ connection events will still be received.
> >          */
> >         hid_hw_close(hdev);
> > 
> >    Since on restarting IO it now is fully restarted so the hid_hw_open()
> >    there needs to be balanced. 
> > 
> > This series now obviously is no longer 6.6 material, instead I hope we
> > can get this rework (and IMHO nice cleanup) into 6.7 .

Yeah, not 6.6 anymore, but should be doable for 6.7.

> > 
> > Regards,
> > 
> > Hans
> 
> I forgot to add info on the list of devices I tested this on:
> 
> Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0)
> Logitech M720 Triathlon (bluetooth, 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 Keyboard LX501 (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0)
> Logitech 27Mhz mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0)

We should probably add this list to the commit messages.
I'll need to test also myself on some problematic devices that have a
special case (WTP, USB wired, BLE).

Anyway, I'll have to test everything, but this looks like it's in a
better shape than previously.

However, the thing I am afraid is that commit 498ba2069035 ("HID:
logitech-hidpp: Don't restart communication if not necessary") was
fixing devices that did not like the hid_hw_stop/start. I can't find the
bug numbers however... So with your series, we might breaking those
once again.

How about we do the following (in pseudo code):
probe():
  hidpp_connect_and_start(connect_mask = 0)
  // retrieve name and serial
  hid_connect(connect_mask) // with connect_mask ensuring we don't
                            // create inputs if HIDPP_QUIRK_DELAYED_INIT
                            // is set, instead of stop/start
  hid_hw_close(hdev); // to balance hidpp_connect_and_start()

I think the above should even remove the need for the
enable_connect_event atomic_t given that now we are not restarting the
devices at all.

> 
> Regards,
> 
> Hans
> 
> 
> 
> > 1) For reviewing you may also want to apply the entire series and look
> > at the end result to help you understand why various intermediate steps
> > are taken.
> > 
> > 
> > Hans de Goede (14):
> >   HID: logitech-hidpp: Revert "Don't restart communication if not
> >     necessary"
> >   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: Move get_wireless_feature_index() check to
> >     hidpp_connect_event()
> >   HID: logitech-hidpp: Remove wtp_get_config() call from probe()
> >   HID: logitech-hidpp: Remove connected check for g920_get_config() call
> >   HID: logitech-hidpp: Add a hidpp_connect_and_start() helper
> >   HID: logitech-hidpp: Move the connected check to after restarting IO
> >   HID: logitech-hidpp: Move g920_get_config() to just before
> >     hidpp_ff_init()
> >   HID: logitech-hidpp: Remove unused connected param from *_connect()
> >   HID: logitech-hidpp: Fix connect event race
> >   HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe()
> >     restarts IO
> >   HID: logitech-hidpp: Drop delayed_work_cb()
> > 
> >  drivers/hid/hid-logitech-hidpp.c | 211 +++++++++++++------------------
> >  1 file changed, 91 insertions(+), 120 deletions(-)
> > 

I like when the total number of deletions is higher than the additions
:)

Cheers,
Benjamin

> 

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

[Hans de Goede]

Hi Benjamin,

On 10/9/23 10:13, Benjamin Tissoires wrote:
> 
> Hi Hans,
> 
> On Oct 08 2023, Hans de Goede wrote:
>> Hi,
>>
>> On 10/8/23 11:54, Hans de Goede wrote:
>>> Hi Benjamin,
>>>
>>> Here is a v2 of my series to fix issues with hidpp_connect_event() running
>>> while restarting IO, which now also fixes the issues you mentioned with
>>> potentially missing connect events.
> 
> Great, thanks a lot for this hard work.

You're welcome.

>>> This series is best explained by a brief sketch of how probe()
>>> looks at the end of the series (1):
> 
> TBH, I couldn't parse the following yesterday evening, but after looking
> at all patches one by one I can now get it :)

I'm glad you get it now :)

> 
>>>
>>> Prep work:
>>>
>>> 1. All code depending on a device being in connected state is moved to
>>>    the hidpp_connect_event() workqueue item
>>>
>>> 2. hidpp_connect_event() now checks the connected state itself by
>>>    checking that hidpp_root_get_protocol_version() succeeds, instead
>>>    of relying on possibly stale (racy) data in struct hidpp_device
>>>
>>> With this in place the new probe() sequence looks like this:
>>>
>>> 1. enable_connect_event flag starts at 0, this filters out / ignores any
>>>    connect-events in hidpp_raw_hidpp_event() avoiding
>>>    hidpp_connect_event() getting queued before the IO restart
>>>
>>> 2. IO is started with connect-mask = 0
>>>    this avoids hid-input and hidraw connecting while probe() is setting
>>>    hdev->name and hdev->uniq
>>>
>>> 3. Name and serialnr are retrieved and stored in hdev
>>>
>>> 4. IO is fully restarted (including hw_open + io_start, not just hw_start)
>>>    with the actual connect-mask.
>>>
>>> 5. enable_connect_event atomic_t is set to 1 to enable processing of
>>>    connect events.
>>>
>>> 6. hidpp_connect_event() is queued + flushed to query the connected state
>>>    and do the connect work if the device is connected.
>>>
>>> 7. probe() now ends with:
>>>
>>>         /*
>>>          * This relies on logi_dj_ll_close() being a no-op so that
>>>          * DJ connection events will still be received.
>>>          */
>>>         hid_hw_close(hdev);
>>>
>>>    Since on restarting IO it now is fully restarted so the hid_hw_open()
>>>    there needs to be balanced. 
>>>
>>> This series now obviously is no longer 6.6 material, instead I hope we
>>> can get this rework (and IMHO nice cleanup) into 6.7 .
> 
> Yeah, not 6.6 anymore, but should be doable for 6.7.
> 
>>>
>>> Regards,
>>>
>>> Hans
>>
>> I forgot to add info on the list of devices I tested this on:
>>
>> Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0)
>> Logitech M720 Triathlon (bluetooth, 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 Keyboard LX501 (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0)
>> Logitech 27Mhz mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0)
> 
> We should probably add this list to the commit messages.
> I'll need to test also myself on some problematic devices that have a
> special case (WTP, USB wired, BLE).

So you want to add this to all 14 commit messages ?

> Anyway, I'll have to test everything, but this looks like it's in a
> better shape than previously.
> 
> However, the thing I am afraid is that commit 498ba2069035 ("HID:
> logitech-hidpp: Don't restart communication if not necessary") was
> fixing devices that did not like the hid_hw_stop/start. I can't find the
> bug numbers however... So with your series, we might breaking those
> once again.
> 
> How about we do the following (in pseudo code):
> probe():
>   hidpp_connect_and_start(connect_mask = 0)
>   // retrieve name and serial
>   hid_connect(connect_mask) // with connect_mask ensuring we don't
>                             // create inputs if HIDPP_QUIRK_DELAYED_INIT
>                             // is set, instead of stop/start
>   hid_hw_close(hdev); // to balance hidpp_connect_and_start()
> 
> I think the above should even remove the need for the
> enable_connect_event atomic_t given that now we are not restarting the
> devices at all.

Interesting yes that looks good, any idea why this was not done
in commit 91cf9a98ae41 ("HID: logitech-hidpp: make .probe usbhid capable")
right away ?

Let me rework the series to use that tomorrow. This will probably also
allow dropping a bunch of the patches.

Regards,

Hans




> 
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>> 1) For reviewing you may also want to apply the entire series and look
>>> at the end result to help you understand why various intermediate steps
>>> are taken.
>>>
>>>
>>> Hans de Goede (14):
>>>   HID: logitech-hidpp: Revert "Don't restart communication if not
>>>     necessary"
>>>   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: Move get_wireless_feature_index() check to
>>>     hidpp_connect_event()
>>>   HID: logitech-hidpp: Remove wtp_get_config() call from probe()
>>>   HID: logitech-hidpp: Remove connected check for g920_get_config() call
>>>   HID: logitech-hidpp: Add a hidpp_connect_and_start() helper
>>>   HID: logitech-hidpp: Move the connected check to after restarting IO
>>>   HID: logitech-hidpp: Move g920_get_config() to just before
>>>     hidpp_ff_init()
>>>   HID: logitech-hidpp: Remove unused connected param from *_connect()
>>>   HID: logitech-hidpp: Fix connect event race
>>>   HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe()
>>>     restarts IO
>>>   HID: logitech-hidpp: Drop delayed_work_cb()
>>>
>>>  drivers/hid/hid-logitech-hidpp.c | 211 +++++++++++++------------------
>>>  1 file changed, 91 insertions(+), 120 deletions(-)
>>>
> 
> I like when the total number of deletions is higher than the additions
> :)
> 
> Cheers,
> Benjamin
> 
>>
> 

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

[Benjamin Tissoires]

On Oct 09 2023, Hans de Goede wrote:
> Hi Benjamin,
> 
> On 10/9/23 10:13, Benjamin Tissoires wrote:
> > 
> > Hi Hans,
> > 
> > On Oct 08 2023, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 10/8/23 11:54, Hans de Goede wrote:
> >>> Hi Benjamin,
> >>>
> >>> Here is a v2 of my series to fix issues with hidpp_connect_event() running
> >>> while restarting IO, which now also fixes the issues you mentioned with
> >>> potentially missing connect events.
> > 
> > Great, thanks a lot for this hard work.
> 
> You're welcome.
> 
> >>> This series is best explained by a brief sketch of how probe()
> >>> looks at the end of the series (1):
> > 
> > TBH, I couldn't parse the following yesterday evening, but after looking
> > at all patches one by one I can now get it :)
> 
> I'm glad you get it now :)
> 
> > 
> >>>
> >>> Prep work:
> >>>
> >>> 1. All code depending on a device being in connected state is moved to
> >>>    the hidpp_connect_event() workqueue item
> >>>
> >>> 2. hidpp_connect_event() now checks the connected state itself by
> >>>    checking that hidpp_root_get_protocol_version() succeeds, instead
> >>>    of relying on possibly stale (racy) data in struct hidpp_device
> >>>
> >>> With this in place the new probe() sequence looks like this:
> >>>
> >>> 1. enable_connect_event flag starts at 0, this filters out / ignores any
> >>>    connect-events in hidpp_raw_hidpp_event() avoiding
> >>>    hidpp_connect_event() getting queued before the IO restart
> >>>
> >>> 2. IO is started with connect-mask = 0
> >>>    this avoids hid-input and hidraw connecting while probe() is setting
> >>>    hdev->name and hdev->uniq
> >>>
> >>> 3. Name and serialnr are retrieved and stored in hdev
> >>>
> >>> 4. IO is fully restarted (including hw_open + io_start, not just hw_start)
> >>>    with the actual connect-mask.
> >>>
> >>> 5. enable_connect_event atomic_t is set to 1 to enable processing of
> >>>    connect events.
> >>>
> >>> 6. hidpp_connect_event() is queued + flushed to query the connected state
> >>>    and do the connect work if the device is connected.
> >>>
> >>> 7. probe() now ends with:
> >>>
> >>>         /*
> >>>          * This relies on logi_dj_ll_close() being a no-op so that
> >>>          * DJ connection events will still be received.
> >>>          */
> >>>         hid_hw_close(hdev);
> >>>
> >>>    Since on restarting IO it now is fully restarted so the hid_hw_open()
> >>>    there needs to be balanced. 
> >>>
> >>> This series now obviously is no longer 6.6 material, instead I hope we
> >>> can get this rework (and IMHO nice cleanup) into 6.7 .
> > 
> > Yeah, not 6.6 anymore, but should be doable for 6.7.
> > 
> >>>
> >>> Regards,
> >>>
> >>> Hans
> >>
> >> I forgot to add info on the list of devices I tested this on:
> >>
> >> Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0)
> >> Logitech M720 Triathlon (bluetooth, 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 Keyboard LX501 (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0)
> >> Logitech 27Mhz mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0)
> > 
> > We should probably add this list to the commit messages.
> > I'll need to test also myself on some problematic devices that have a
> > special case (WTP, USB wired, BLE).
> 
> So you want to add this to all 14 commit messages ?

I'd like to have this list recorded in the git tree. In case we have to
do forensic once again. Whether it's added in all messages is up to you
I think. But honestly, maybe add those only when you actually tested the
patch. No?

> 
> > Anyway, I'll have to test everything, but this looks like it's in a
> > better shape than previously.
> > 
> > However, the thing I am afraid is that commit 498ba2069035 ("HID:
> > logitech-hidpp: Don't restart communication if not necessary") was
> > fixing devices that did not like the hid_hw_stop/start. I can't find the
> > bug numbers however... So with your series, we might breaking those
> > once again.
> > 
> > How about we do the following (in pseudo code):
> > probe():
> >   hidpp_connect_and_start(connect_mask = 0)
> >   // retrieve name and serial
> >   hid_connect(connect_mask) // with connect_mask ensuring we don't
> >                             // create inputs if HIDPP_QUIRK_DELAYED_INIT
> >                             // is set, instead of stop/start
> >   hid_hw_close(hdev); // to balance hidpp_connect_and_start()
> > 
> > I think the above should even remove the need for the
> > enable_connect_event atomic_t given that now we are not restarting the
> > devices at all.
> 
> Interesting yes that looks good, any idea why this was not done
> in commit 91cf9a98ae41 ("HID: logitech-hidpp: make .probe usbhid capable")
> right away ?

I tried to do a little bit of digging. Initially I thought that's
because I was always doing that stop/start dance, and so I continued
with that.

But looking slightly more carefully, I think I understand why:
- calling twice hid_connect should fail, because it'll create twice the
  sysfs nodes for the HID device itself
- so we need to call hid_disconnect() first
- this was done through hid_hw_stop() and given that the dj driver has
  empty stubs for start/stop, this was semantically the same.

So using hid_hw_stop/start was a shortcut because DJ had empty stubs.
But when I enabled direct use of hidpp from other transport layers, I
did not realized that there was this glitch.

And then I completely forgot to clean up that because "at the end of a
HID driver we are supposed to call hid_hw_start()".

> 
> Let me rework the series to use that tomorrow. This will probably also
> allow dropping a bunch of the patches.

Yeah, I think we should be able to remove the sync mechanism.
And be careful to call hid_disconnect() before
hid_connect(connect_mask), or you'll probably have errors if not when
plugging the device, but when unplugging it for sure.

Cheers,
Benjamin

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> > 
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >>
> >>
> >>> 1) For reviewing you may also want to apply the entire series and look
> >>> at the end result to help you understand why various intermediate steps
> >>> are taken.
> >>>
> >>>
> >>> Hans de Goede (14):
> >>>   HID: logitech-hidpp: Revert "Don't restart communication if not
> >>>     necessary"
> >>>   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: Move get_wireless_feature_index() check to
> >>>     hidpp_connect_event()
> >>>   HID: logitech-hidpp: Remove wtp_get_config() call from probe()
> >>>   HID: logitech-hidpp: Remove connected check for g920_get_config() call
> >>>   HID: logitech-hidpp: Add a hidpp_connect_and_start() helper
> >>>   HID: logitech-hidpp: Move the connected check to after restarting IO
> >>>   HID: logitech-hidpp: Move g920_get_config() to just before
> >>>     hidpp_ff_init()
> >>>   HID: logitech-hidpp: Remove unused connected param from *_connect()
> >>>   HID: logitech-hidpp: Fix connect event race
> >>>   HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe()
> >>>     restarts IO
> >>>   HID: logitech-hidpp: Drop delayed_work_cb()
> >>>
> >>>  drivers/hid/hid-logitech-hidpp.c | 211 +++++++++++++------------------
> >>>  1 file changed, 91 insertions(+), 120 deletions(-)
> >>>
> > 
> > I like when the total number of deletions is higher than the additions
> > :)
> > 
> > Cheers,
> > Benjamin
> > 
> >>
> > 
> 

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

[Hans de Goede]

Hi,

On 10/10/23 11:36, Benjamin Tissoires wrote:
> On Oct 09 2023, Hans de Goede wrote:

<snip>

>>> However, the thing I am afraid is that commit 498ba2069035 ("HID:
>>> logitech-hidpp: Don't restart communication if not necessary") was
>>> fixing devices that did not like the hid_hw_stop/start. I can't find the
>>> bug numbers however... So with your series, we might breaking those
>>> once again.
>>>
>>> How about we do the following (in pseudo code):
>>> probe():
>>>   hidpp_connect_and_start(connect_mask = 0)
>>>   // retrieve name and serial
>>>   hid_connect(connect_mask) // with connect_mask ensuring we don't
>>>                             // create inputs if HIDPP_QUIRK_DELAYED_INIT
>>>                             // is set, instead of stop/start
>>>   hid_hw_close(hdev); // to balance hidpp_connect_and_start()
>>>
>>> I think the above should even remove the need for the
>>> enable_connect_event atomic_t given that now we are not restarting the
>>> devices at all.
>>
>> Interesting yes that looks good, any idea why this was not done
>> in commit 91cf9a98ae41 ("HID: logitech-hidpp: make .probe usbhid capable")
>> right away ?
> 
> I tried to do a little bit of digging. Initially I thought that's
> because I was always doing that stop/start dance, and so I continued
> with that.
> 
> But looking slightly more carefully, I think I understand why:
> - calling twice hid_connect should fail, because it'll create twice the
>   sysfs nodes for the HID device itself
> - so we need to call hid_disconnect() first
> - this was done through hid_hw_stop() and given that the dj driver has
>   empty stubs for start/stop, this was semantically the same.
> 
> So using hid_hw_stop/start was a shortcut because DJ had empty stubs.
> But when I enabled direct use of hidpp from other transport layers, I
> did not realized that there was this glitch.
> 
> And then I completely forgot to clean up that because "at the end of a
> HID driver we are supposed to call hid_hw_start()".
> 
>>
>> Let me rework the series to use that tomorrow. This will probably also
>> allow dropping a bunch of the patches.
> 
> Yeah, I think we should be able to remove the sync mechanism.
> And be careful to call hid_disconnect() before
> hid_connect(connect_mask), or you'll probably have errors if not when
> plugging the device, but when unplugging it for sure.

Note that hid_hw_start() never calls hid_connect() when called with
a connect_mask of 0, so just simply calling hid_connect() later should
be fine without needing to do a hid_disconnect() before:

int hid_hw_start(struct hid_device *hdev, unsigned int connect_mask)
{
        int error;

        error = hdev->ll_driver->start(hdev);
        if (error)
                return error;

        if (connect_mask) {
                error = hid_connect(hdev, connect_mask);
                if (error) {
                        hdev->ll_driver->stop(hdev);
                        return error;
                }
        }

        return 0;
}
EXPORT_SYMBOL_GPL(hid_hw_start);

Regards,

Hans