[v2,12/14] HID: logitech-hidpp: Fix connect event race

Commit Message

Hans de Goede Oct. 8, 2023, 9:54 a.m. UTC

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 <[e-mail]>
---
 drivers/hid/hid-logitech-hidpp.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

Patch

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