[PATCH v2 0/3] Logitech G920 fixes

[PATCH v2 0/3] Logitech G920 fixes

[Andrey Smirnov]

Everyone:

This series contains patches to fix a couple of regressions in G920
wheel support by hid-logitech-hidpp driver. Without the patches the
wheel remains stuck in autocentering mode ("resisting" any attempt to
trun) as well as missing support for any FF action.

Thanks,
Andrey Smirnov

Changes since [v1]:

     - "HID: logitech-hidpp: split g920_get_config()" is changed to
       not rely on devres and be a self contained patch

     - Quirk driven behaviour of "HID: logitech-hidpp: add G920 device
       validation quirk" is replaced with generic validation algorithm
       of "HID: logitech-hidpp: rework device validation"

     - Fix for a poteintial race condition is added in
       "HID: logitech-hidpp: do all FF cleanup in hidpp_ff_destroy()"
       as per suggestion by Benjamin Tissoires

     - Collected Tested-by from Sam Bazely for "HID: logitech-hidpp:
       split g920_get_config()" since that patch didn't change
       significantly since [v1]

     - Specified stable kernel versions I think the patches should
       apply to (hopefully I got that right)

[v1] lore.kernel.org/lkml/20191007051240.4410-1-andrew.smirnov@gmail.com

Andrey Smirnov (3):
  HID: logitech-hidpp: split g920_get_config()
  HID: logitech-hidpp: rework device validation
  HID: logitech-hidpp: do all FF cleanup in hidpp_ff_destroy()

 drivers/hid/hid-logitech-hidpp.c | 237 +++++++++++++++++--------------
 1 file changed, 131 insertions(+), 106 deletions(-)

-- 
2.21.0

[PATCH v2 1/3] HID: logitech-hidpp: split g920_get_config()

[Andrey Smirnov]

Original version of g920_get_config() contained two kind of actions:

    1. Device specific communication to query/set some parameters
       which requires active communication channel with the device,
       or, put in other way, for the call to be sandwiched between
       hid_device_io_start() and hid_device_io_stop().

    2. Input subsystem specific FF controller initialization which, in
       order to access a valid 'struct hid_input' via
       'hid->inputs.next', requires claimed hidinput which means be
       executed after the call to hid_hw_start() with connect_mask
       containing HID_CONNECT_HIDINPUT.

Location of g920_get_config() can only fulfill requirements for #1 and
not #2, which might result in following backtrace:

[   88.312258] logitech-hidpp-device 0003:046D:C262.0005: HID++ 4.2 device connected.
[   88.320298] BUG: kernel NULL pointer dereference, address: 0000000000000018
[   88.320304] #PF: supervisor read access in kernel mode
[   88.320307] #PF: error_code(0x0000) - not-present page
[   88.320309] PGD 0 P4D 0
[   88.320315] Oops: 0000 [#1] SMP PTI
[   88.320320] CPU: 1 PID: 3080 Comm: systemd-udevd Not tainted 5.4.0-rc1+ #31
[   88.320322] Hardware name: Apple Inc. MacBookPro11,1/Mac-189A3D4F975D5FFC, BIOS 149.0.0.0.0 09/17/2018
[   88.320334] RIP: 0010:hidpp_probe+0x61f/0x948 [hid_logitech_hidpp]
[   88.320338] Code: 81 00 00 48 89 ef e8 f0 d6 ff ff 41 89 c6 85 c0 75 b5 0f b6 44 24 28 48 8b 5d 00 88 44 24 1e 89 44 24 0c 48 8b 83 18 1c 00 00 <48> 8b 48 18 48 8b 83 10 19 00 00 48 8b 40 40 48 89 0c 24 0f b7 80
[   88.320341] RSP: 0018:ffffb0a6824aba68 EFLAGS: 00010246
[   88.320345] RAX: 0000000000000000 RBX: ffff93a50756e000 RCX: 0000000000010408
[   88.320347] RDX: 0000000000000000 RSI: ffff93a51f0ad0a0 RDI: 000000000002d0a0
[   88.320350] RBP: ffff93a50416da28 R08: ffff93a50416da70 R09: ffff93a50416da70
[   88.320352] R10: 000000148ae9e60c R11: 00000000000f1525 R12: ffff93a50756e000
[   88.320354] R13: ffff93a50756f8d0 R14: 0000000000000000 R15: ffff93a50756fc38
[   88.320358] FS:  00007f8d8c1e0940(0000) GS:ffff93a51f080000(0000) knlGS:0000000000000000
[   88.320361] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   88.320363] CR2: 0000000000000018 CR3: 00000003996d8003 CR4: 00000000001606e0
[   88.320366] Call Trace:
[   88.320377]  ? _cond_resched+0x15/0x30
[   88.320387]  ? create_pinctrl+0x2f/0x3c0
[   88.320393]  ? kernfs_link_sibling+0x94/0xe0
[   88.320398]  ? _cond_resched+0x15/0x30
[   88.320402]  ? kernfs_activate+0x5f/0x80
[   88.320406]  ? kernfs_add_one+0xe2/0x130
[   88.320411]  hid_device_probe+0x106/0x170
[   88.320419]  really_probe+0x147/0x3c0
[   88.320424]  driver_probe_device+0xb6/0x100
[   88.320428]  device_driver_attach+0x53/0x60
[   88.320433]  __driver_attach+0x8a/0x150
[   88.320437]  ? device_driver_attach+0x60/0x60
[   88.320440]  bus_for_each_dev+0x78/0xc0
[   88.320445]  bus_add_driver+0x14d/0x1f0
[   88.320450]  driver_register+0x6c/0xc0
[   88.320453]  ? 0xffffffffc0d67000
[   88.320457]  __hid_register_driver+0x4c/0x80
[   88.320464]  do_one_initcall+0x46/0x1f4
[   88.320469]  ? _cond_resched+0x15/0x30
[   88.320474]  ? kmem_cache_alloc_trace+0x162/0x220
[   88.320481]  ? do_init_module+0x23/0x230
[   88.320486]  do_init_module+0x5c/0x230
[   88.320491]  load_module+0x26e1/0x2990
[   88.320502]  ? ima_post_read_file+0xf0/0x100
[   88.320508]  ? __do_sys_finit_module+0xaa/0x110
[   88.320512]  __do_sys_finit_module+0xaa/0x110
[   88.320520]  do_syscall_64+0x5b/0x180
[   88.320525]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   88.320528] RIP: 0033:0x7f8d8d1f01fd
[   88.320532] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 5b 8c 0c 00 f7 d8 64 89 01 48
[   88.320535] RSP: 002b:00007ffefa3bb068 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[   88.320539] RAX: ffffffffffffffda RBX: 000055922040cb40 RCX: 00007f8d8d1f01fd
[   88.320541] RDX: 0000000000000000 RSI: 00007f8d8ce4984d RDI: 0000000000000006
[   88.320543] RBP: 0000000000020000 R08: 0000000000000000 R09: 0000000000000007
[   88.320545] R10: 0000000000000006 R11: 0000000000000246 R12: 00007f8d8ce4984d
[   88.320547] R13: 0000000000000000 R14: 000055922040efc0 R15: 000055922040cb40
[   88.320551] Modules linked in: hid_logitech_hidpp(+) fuse rfcomm ccm xt_CHECKSUM xt_MASQUERADE bridge stp llc nf_nat_tftp nf_conntrack_tftp nf_conntrack_netbios_ns nf_conntrack_broadcast xt_CT ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 xt_conntrack ebtable_nat ip6table_nat ip6table_mangle ip6table_raw ip6table_security iptable_nat nf_nat tun iptable_mangle iptable_raw iptable_security nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 libcrc32c ip_set nfnetlink ebtable_filter ebtables ip6table_filter ip6_tables cmac bnep sunrpc dm_crypt nls_utf8 hfsplus intel_rapl_msr intel_rapl_common ath9k_htc ath9k_common x86_pkg_temp_thermal intel_powerclamp b43 ath9k_hw coretemp snd_hda_codec_hdmi cordic kvm_intel snd_hda_codec_cirrus mac80211 snd_hda_codec_generic ledtrig_audio kvm snd_hda_intel snd_intel_nhlt irqbypass snd_hda_codec btusb btrtl snd_hda_core ath btbcm ssb snd_hwdep btintel snd_seq crct10dif_pclmul iTCO_wdt snd_seq_device crc32_pclmul bluetooth mmc_core iTCO_vendor_support joydev cfg80211
[   88.320602]  applesmc ghash_clmulni_intel ecdh_generic snd_pcm input_polldev intel_cstate ecc intel_uncore thunderbolt snd_timer i2c_i801 libarc4 rfkill intel_rapl_perf lpc_ich mei_me pcspkr bcm5974 snd bcma mei soundcore acpi_als sbs kfifo_buf sbshc industrialio apple_bl i915 i2c_algo_bit drm_kms_helper drm uas crc32c_intel usb_storage video hid_apple
[   88.320630] CR2: 0000000000000018
[   88.320633] ---[ end trace 933491c8a4fadeb7 ]---
[   88.320642] RIP: 0010:hidpp_probe+0x61f/0x948 [hid_logitech_hidpp]
[   88.320645] Code: 81 00 00 48 89 ef e8 f0 d6 ff ff 41 89 c6 85 c0 75 b5 0f b6 44 24 28 48 8b 5d 00 88 44 24 1e 89 44 24 0c 48 8b 83 18 1c 00 00 <48> 8b 48 18 48 8b 83 10 19 00 00 48 8b 40 40 48 89 0c 24 0f b7 80
[   88.320647] RSP: 0018:ffffb0a6824aba68 EFLAGS: 00010246
[   88.320650] RAX: 0000000000000000 RBX: ffff93a50756e000 RCX: 0000000000010408
[   88.320652] RDX: 0000000000000000 RSI: ffff93a51f0ad0a0 RDI: 000000000002d0a0
[   88.320655] RBP: ffff93a50416da28 R08: ffff93a50416da70 R09: ffff93a50416da70
[   88.320657] R10: 000000148ae9e60c R11: 00000000000f1525 R12: ffff93a50756e000
[   88.320659] R13: ffff93a50756f8d0 R14: 0000000000000000 R15: ffff93a50756fc38
[   88.320662] FS:  00007f8d8c1e0940(0000) GS:ffff93a51f080000(0000) knlGS:0000000000000000
[   88.320664] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   88.320667] CR2: 0000000000000018 CR3: 00000003996d8003 CR4: 00000000001606e0

To solve this issue:

   1. Split g920_get_config() such that all of the device specific
      communication remains a part of the function and input subsystem
      initialization bits go to hidpp_ff_init()

   2. Move call to hidpp_ff_init() from being a part of
      g920_get_config() to be the last step of .probe(), right after a
      call to hid_hw_start() with connect_mask containing
      HID_CONNECT_HIDINPUT.

Fixes: 91cf9a98ae41 ("HID: logitech-hidpp: make .probe usbhid capable")
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Tested-by: Sam Bazley <sambazley@fastmail.com>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Henrik Rydberg <rydberg@bitmath.org>
Cc: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com>
Cc: Austin Palmer <austinp@valvesoftware.com>
Cc: linux-input@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: stable@vger.kernel.org # 5.2+
---
 drivers/hid/hid-logitech-hidpp.c | 150 ++++++++++++++++++++-----------
 1 file changed, 96 insertions(+), 54 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 1ac1ecc1e67c..85911586b3b6 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1669,6 +1669,7 @@ static void hidpp_touchpad_raw_xy_event(struct hidpp_device *hidpp_dev,
 
 #define HIDPP_FF_EFFECTID_NONE		-1
 #define HIDPP_FF_EFFECTID_AUTOCENTER	-2
+#define HIDPP_AUTOCENTER_PARAMS_LENGTH	18
 
 #define HIDPP_FF_MAX_PARAMS	20
 #define HIDPP_FF_RESERVED_SLOTS	1
@@ -2009,7 +2010,7 @@ static int hidpp_ff_erase_effect(struct input_dev *dev, int effect_id)
 static void hidpp_ff_set_autocenter(struct input_dev *dev, u16 magnitude)
 {
 	struct hidpp_ff_private_data *data = dev->ff->private;
-	u8 params[18];
+	u8 params[HIDPP_AUTOCENTER_PARAMS_LENGTH];
 
 	dbg_hid("Setting autocenter to %d.\n", magnitude);
 
@@ -2081,7 +2082,8 @@ static void hidpp_ff_destroy(struct ff_device *ff)
 	kfree(data->effect_ids);
 }
 
-static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
+static int hidpp_ff_init(struct hidpp_device *hidpp,
+			 struct hidpp_ff_private_data *data)
 {
 	struct hid_device *hid = hidpp->hid_dev;
 	struct hid_input *hidinput;
@@ -2089,9 +2091,7 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
 	const struct usb_device_descriptor *udesc = &(hid_to_usb_dev(hid)->descriptor);
 	const u16 bcdDevice = le16_to_cpu(udesc->bcdDevice);
 	struct ff_device *ff;
-	struct hidpp_report response;
-	struct hidpp_ff_private_data *data;
-	int error, j, num_slots;
+	int error, j, num_slots = data->num_effects;
 	u8 version;
 
 	if (list_empty(&hid->inputs)) {
@@ -2116,27 +2116,17 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
 		for (j = 0; hidpp_ff_effects_v2[j] >= 0; j++)
 			set_bit(hidpp_ff_effects_v2[j], dev->ffbit);
 
-	/* Read number of slots available in device */
-	error = hidpp_send_fap_command_sync(hidpp, feature_index,
-		HIDPP_FF_GET_INFO, NULL, 0, &response);
-	if (error) {
-		if (error < 0)
-			return error;
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, error);
-		return -EPROTO;
-	}
-
-	num_slots = response.fap.params[0] - HIDPP_FF_RESERVED_SLOTS;
-
 	error = input_ff_create(dev, num_slots);
 
 	if (error) {
 		hid_err(dev, "Failed to create FF device!\n");
 		return error;
 	}
-
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	/*
+	 * Create a copy of passed data, so we can transfer memory
+	 * ownership to FF core
+	 */
+	data = kmemdup(data, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 	data->effect_ids = kcalloc(num_slots, sizeof(int), GFP_KERNEL);
@@ -2152,10 +2142,7 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
 	}
 
 	data->hidpp = hidpp;
-	data->feature_index = feature_index;
 	data->version = version;
-	data->slot_autocenter = 0;
-	data->num_effects = num_slots;
 	for (j = 0; j < num_slots; j++)
 		data->effect_ids[j] = -1;
 
@@ -2169,37 +2156,14 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
 	ff->set_autocenter = hidpp_ff_set_autocenter;
 	ff->destroy = hidpp_ff_destroy;
 
-
-	/* reset all forces */
-	error = hidpp_send_fap_command_sync(hidpp, feature_index,
-		HIDPP_FF_RESET_ALL, NULL, 0, &response);
-
-	/* Read current Range */
-	error = hidpp_send_fap_command_sync(hidpp, feature_index,
-		HIDPP_FF_GET_APERTURE, NULL, 0, &response);
-	if (error)
-		hid_warn(hidpp->hid_dev, "Failed to read range from device!\n");
-	data->range = error ? 900 : get_unaligned_be16(&response.fap.params[0]);
-
 	/* Create sysfs interface */
 	error = device_create_file(&(hidpp->hid_dev->dev), &dev_attr_range);
 	if (error)
 		hid_warn(hidpp->hid_dev, "Unable to create sysfs interface for \"range\", errno %d!\n", error);
 
-	/* Read the current gain values */
-	error = hidpp_send_fap_command_sync(hidpp, feature_index,
-		HIDPP_FF_GET_GLOBAL_GAINS, NULL, 0, &response);
-	if (error)
-		hid_warn(hidpp->hid_dev, "Failed to read gain values from device!\n");
-	data->gain = error ? 0xffff : get_unaligned_be16(&response.fap.params[0]);
-	/* ignore boost value at response.fap.params[2] */
-
 	/* init the hardware command queue */
 	atomic_set(&data->workqueue_size, 0);
 
-	/* initialize with zero autocenter to get wheel in usable state */
-	hidpp_ff_set_autocenter(dev, 0);
-
 	hid_info(hid, "Force feedback support loaded (firmware release %d).\n",
 		 version);
 
@@ -2732,24 +2696,93 @@ static int k400_connect(struct hid_device *hdev, bool connected)
 
 #define HIDPP_PAGE_G920_FORCE_FEEDBACK			0x8123
 
-static int g920_get_config(struct hidpp_device *hidpp)
+static int g920_ff_set_autocenter(struct hidpp_device *hidpp,
+				  struct hidpp_ff_private_data *data)
 {
+	struct hidpp_report response;
+	u8 params[HIDPP_AUTOCENTER_PARAMS_LENGTH] = {
+		[1] = HIDPP_FF_EFFECT_SPRING | HIDPP_FF_EFFECT_AUTOSTART,
+	};
+	int ret;
+
+	/* initialize with zero autocenter to get wheel in usable state */
+
+	dbg_hid("Setting autocenter to 0.\n");
+	ret = hidpp_send_fap_command_sync(hidpp, data->feature_index,
+					  HIDPP_FF_DOWNLOAD_EFFECT,
+					  params, ARRAY_SIZE(params),
+					  &response);
+	if (ret)
+		hid_warn(hidpp->hid_dev, "Failed to autocenter device!\n");
+	else
+		data->slot_autocenter = response.fap.params[0];
+
+	return ret;
+}
+
+static int g920_get_config(struct hidpp_device *hidpp,
+			   struct hidpp_ff_private_data *data)
+{
+	struct hidpp_report response;
 	u8 feature_type;
-	u8 feature_index;
 	int ret;
 
+	memset(data, 0, sizeof(*data));
+
 	/* Find feature and store for later use */
 	ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_G920_FORCE_FEEDBACK,
-		&feature_index, &feature_type);
+				     &data->feature_index, &feature_type);
 	if (ret)
 		return ret;
 
-	ret = hidpp_ff_init(hidpp, feature_index);
+	/* Read number of slots available in device */
+	ret = hidpp_send_fap_command_sync(hidpp, data->feature_index,
+					  HIDPP_FF_GET_INFO,
+					  NULL, 0,
+					  &response);
+	if (ret) {
+		if (ret < 0)
+			return ret;
+		hid_err(hidpp->hid_dev,
+			"%s: received protocol error 0x%02x\n", __func__, ret);
+		return -EPROTO;
+	}
+
+	data->num_effects = response.fap.params[0] - HIDPP_FF_RESERVED_SLOTS;
+
+	/* reset all forces */
+	ret = hidpp_send_fap_command_sync(hidpp, data->feature_index,
+					  HIDPP_FF_RESET_ALL,
+					  NULL, 0,
+					  &response);
 	if (ret)
-		hid_warn(hidpp->hid_dev, "Unable to initialize force feedback support, errno %d\n",
-				ret);
+		hid_warn(hidpp->hid_dev, "Failed to reset all forces!\n");
 
-	return 0;
+	ret = hidpp_send_fap_command_sync(hidpp, data->feature_index,
+					  HIDPP_FF_GET_APERTURE,
+					  NULL, 0,
+					  &response);
+	if (ret) {
+		hid_warn(hidpp->hid_dev,
+			 "Failed to read range from device!\n");
+	}
+	data->range = ret ?
+		900 : get_unaligned_be16(&response.fap.params[0]);
+
+	/* Read the current gain values */
+	ret = hidpp_send_fap_command_sync(hidpp, data->feature_index,
+					  HIDPP_FF_GET_GLOBAL_GAINS,
+					  NULL, 0,
+					  &response);
+	if (ret)
+		hid_warn(hidpp->hid_dev,
+			 "Failed to read gain values from device!\n");
+	data->gain = ret ?
+		0xffff : get_unaligned_be16(&response.fap.params[0]);
+
+	/* ignore boost value at response.fap.params[2] */
+
+	return g920_ff_set_autocenter(hidpp, data);
 }
 
 /* -------------------------------------------------------------------------- */
@@ -3512,6 +3545,7 @@ 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);
@@ -3621,7 +3655,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		if (ret)
 			goto hid_hw_init_fail;
 	} else if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) {
-		ret = g920_get_config(hidpp);
+		ret = g920_get_config(hidpp, &data);
 		if (ret)
 			goto hid_hw_init_fail;
 	}
@@ -3643,6 +3677,14 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		goto hid_hw_start_fail;
 	}
 
+	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
+		ret = hidpp_ff_init(hidpp, &data);
+		if (ret)
+			hid_warn(hidpp->hid_dev,
+		     "Unable to initialize force feedback support, errno %d\n",
+				 ret);
+	}
+
 	return ret;
 
 hid_hw_init_fail:
-- 
2.21.0

[PATCH v2 2/3] HID: logitech-hidpp: rework device validation

[Andrey Smirnov]

G920 device only advertises REPORT_ID_HIDPP_LONG and
REPORT_ID_HIDPP_VERY_LONG in its HID report descriptor, so querying
for REPORT_ID_HIDPP_SHORT with optional=false will always fail and
prevent G920 to be recognized as a valid HID++ device.

To fix this and improve some other aspects, modify
hidpp_validate_device() as follows:

  - Inline the code of hidpp_validate_report() to simplify
    distingushing between non-present and invalid report descriptors

  - Drop the check for id >= HID_MAX_IDS || id < 0 since all of our
    IDs are static and known to satisfy that at compile time

  - Change the algorithms to check all possible report
    types (including very long report) and deem the device as a valid
    HID++ device if it supports at least one

  - Treat invalid report length as a hard stop for the validation
    algorithm, meaning that if any of the supported reports has
    invalid length we assume the worst and treat the device as a
    generic HID device.

  - Fold initialization of hidpp->very_long_report_length into
    hidpp_validate_device() since it already fetches very long report
    length and validates its value

Fixes: fe3ee1ec007b ("HID: logitech-hidpp: allow non HID++ devices to be handled by this module")
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204191
Reported-by: Sam Bazely <sambazley@fastmail.com>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Henrik Rydberg <rydberg@bitmath.org>
Cc: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com>
Cc: Austin Palmer <austinp@valvesoftware.com>
Cc: linux-input@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: stable@vger.kernel.org # 5.2+
---
 drivers/hid/hid-logitech-hidpp.c | 54 ++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 85911586b3b6..8c4be991f387 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -3498,34 +3498,45 @@ static int hidpp_get_report_length(struct hid_device *hdev, int id)
 	return report->field[0]->report_count + 1;
 }
 
-static bool hidpp_validate_report(struct hid_device *hdev, int id,
-				  int expected_length, bool optional)
+static bool hidpp_validate_device(struct hid_device *hdev)
 {
-	int report_length;
+	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
+	int id, report_length, supported_reports = 0;
+
+	id = REPORT_ID_HIDPP_SHORT;
+	report_length = hidpp_get_report_length(hdev, id);
+	if (report_length) {
+		if (report_length < HIDPP_REPORT_SHORT_LENGTH)
+			goto bad_device;
 
-	if (id >= HID_MAX_IDS || id < 0) {
-		hid_err(hdev, "invalid HID report id %u\n", id);
-		return false;
+		supported_reports++;
 	}
 
+	id = REPORT_ID_HIDPP_LONG;
 	report_length = hidpp_get_report_length(hdev, id);
-	if (!report_length)
-		return optional;
+	if (report_length) {
+		if (report_length < HIDPP_REPORT_LONG_LENGTH)
+			goto bad_device;
 
-	if (report_length < expected_length) {
-		hid_warn(hdev, "not enough values in hidpp report %d\n", id);
-		return false;
+		supported_reports++;
 	}
 
-	return true;
-}
+	id = REPORT_ID_HIDPP_VERY_LONG;
+	report_length = hidpp_get_report_length(hdev, id);
+	if (report_length) {
+		if (report_length > HIDPP_REPORT_LONG_LENGTH &&
+		    report_length < HIDPP_REPORT_VERY_LONG_MAX_LENGTH)
+			goto bad_device;
 
-static bool hidpp_validate_device(struct hid_device *hdev)
-{
-	return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT,
-				     HIDPP_REPORT_SHORT_LENGTH, false) &&
-	       hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
-				     HIDPP_REPORT_LONG_LENGTH, true);
+		supported_reports++;
+		hidpp->very_long_report_length = report_length;
+	}
+
+	return supported_reports;
+
+bad_device:
+	hid_warn(hdev, "not enough values in hidpp report %d\n", id);
+	return false;
 }
 
 static bool hidpp_application_equals(struct hid_device *hdev,
@@ -3572,11 +3583,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 	}
 
-	hidpp->very_long_report_length =
-		hidpp_get_report_length(hdev, REPORT_ID_HIDPP_VERY_LONG);
-	if (hidpp->very_long_report_length > HIDPP_REPORT_VERY_LONG_MAX_LENGTH)
-		hidpp->very_long_report_length = HIDPP_REPORT_VERY_LONG_MAX_LENGTH;
-
 	if (id->group == HID_GROUP_LOGITECH_DJ_DEVICE)
 		hidpp->quirks |= HIDPP_QUIRK_UNIFYING;
 
-- 
2.21.0

Re: [PATCH v2 2/3] HID: logitech-hidpp: rework device validation

[Benjamin Tissoires]

Hi Andrey,

On Wed, Oct 16, 2019 at 8:30 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> G920 device only advertises REPORT_ID_HIDPP_LONG and
> REPORT_ID_HIDPP_VERY_LONG in its HID report descriptor, so querying
> for REPORT_ID_HIDPP_SHORT with optional=false will always fail and
> prevent G920 to be recognized as a valid HID++ device.
>
> To fix this and improve some other aspects, modify
> hidpp_validate_device() as follows:
>
>   - Inline the code of hidpp_validate_report() to simplify
>     distingushing between non-present and invalid report descriptors
>
>   - Drop the check for id >= HID_MAX_IDS || id < 0 since all of our
>     IDs are static and known to satisfy that at compile time
>
>   - Change the algorithms to check all possible report
>     types (including very long report) and deem the device as a valid
>     HID++ device if it supports at least one
>
>   - Treat invalid report length as a hard stop for the validation
>     algorithm, meaning that if any of the supported reports has
>     invalid length we assume the worst and treat the device as a
>     generic HID device.
>
>   - Fold initialization of hidpp->very_long_report_length into
>     hidpp_validate_device() since it already fetches very long report
>     length and validates its value
>
> Fixes: fe3ee1ec007b ("HID: logitech-hidpp: allow non HID++ devices to be handled by this module")
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204191
> Reported-by: Sam Bazely <sambazley@fastmail.com>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Jiri Kosina <jikos@kernel.org>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Henrik Rydberg <rydberg@bitmath.org>
> Cc: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com>
> Cc: Austin Palmer <austinp@valvesoftware.com>
> Cc: linux-input@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: stable@vger.kernel.org # 5.2+
> ---
>  drivers/hid/hid-logitech-hidpp.c | 54 ++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 85911586b3b6..8c4be991f387 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -3498,34 +3498,45 @@ static int hidpp_get_report_length(struct hid_device *hdev, int id)
>         return report->field[0]->report_count + 1;
>  }
>
> -static bool hidpp_validate_report(struct hid_device *hdev, int id,
> -                                 int expected_length, bool optional)
> +static bool hidpp_validate_device(struct hid_device *hdev)
>  {
> -       int report_length;
> +       struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> +       int id, report_length, supported_reports = 0;
> +
> +       id = REPORT_ID_HIDPP_SHORT;
> +       report_length = hidpp_get_report_length(hdev, id);
> +       if (report_length) {
> +               if (report_length < HIDPP_REPORT_SHORT_LENGTH)
> +                       goto bad_device;
>
> -       if (id >= HID_MAX_IDS || id < 0) {
> -               hid_err(hdev, "invalid HID report id %u\n", id);
> -               return false;
> +               supported_reports++;
>         }
>
> +       id = REPORT_ID_HIDPP_LONG;
>         report_length = hidpp_get_report_length(hdev, id);
> -       if (!report_length)
> -               return optional;
> +       if (report_length) {
> +               if (report_length < HIDPP_REPORT_LONG_LENGTH)
> +                       goto bad_device;
>
> -       if (report_length < expected_length) {
> -               hid_warn(hdev, "not enough values in hidpp report %d\n", id);
> -               return false;
> +               supported_reports++;
>         }
>
> -       return true;
> -}
> +       id = REPORT_ID_HIDPP_VERY_LONG;
> +       report_length = hidpp_get_report_length(hdev, id);
> +       if (report_length) {
> +               if (report_length > HIDPP_REPORT_LONG_LENGTH &&
> +                   report_length < HIDPP_REPORT_VERY_LONG_MAX_LENGTH)

Can you double check the conditions here?
It's late, but I think you inverted the tests as we expect the report
length to be between HIDPP_REPORT_LONG_LENGTH and
HIDPP_REPORT_VERY_LONG_MAX_LENGTH inclusive, while here this creates a
bad_device.

Other than that, I really like the series.

Cheers,
Benjamin

> +                       goto bad_device;
>
> -static bool hidpp_validate_device(struct hid_device *hdev)
> -{
> -       return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT,
> -                                    HIDPP_REPORT_SHORT_LENGTH, false) &&
> -              hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
> -                                    HIDPP_REPORT_LONG_LENGTH, true);
> +               supported_reports++;
> +               hidpp->very_long_report_length = report_length;
> +       }
> +
> +       return supported_reports;
> +
> +bad_device:
> +       hid_warn(hdev, "not enough values in hidpp report %d\n", id);
> +       return false;
>  }
>
>  static bool hidpp_application_equals(struct hid_device *hdev,
> @@ -3572,11 +3583,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                 return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>         }
>
> -       hidpp->very_long_report_length =
> -               hidpp_get_report_length(hdev, REPORT_ID_HIDPP_VERY_LONG);
> -       if (hidpp->very_long_report_length > HIDPP_REPORT_VERY_LONG_MAX_LENGTH)
> -               hidpp->very_long_report_length = HIDPP_REPORT_VERY_LONG_MAX_LENGTH;
> -
>         if (id->group == HID_GROUP_LOGITECH_DJ_DEVICE)
>                 hidpp->quirks |= HIDPP_QUIRK_UNIFYING;
>
> --
> 2.21.0
>

Re: [PATCH v2 2/3] HID: logitech-hidpp: rework device validation

[Andrey Smirnov]

On Wed, Oct 16, 2019 at 12:24 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Hi Andrey,
>
> On Wed, Oct 16, 2019 at 8:30 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> >
> > G920 device only advertises REPORT_ID_HIDPP_LONG and
> > REPORT_ID_HIDPP_VERY_LONG in its HID report descriptor, so querying
> > for REPORT_ID_HIDPP_SHORT with optional=false will always fail and
> > prevent G920 to be recognized as a valid HID++ device.
> >
> > To fix this and improve some other aspects, modify
> > hidpp_validate_device() as follows:
> >
> >   - Inline the code of hidpp_validate_report() to simplify
> >     distingushing between non-present and invalid report descriptors
> >
> >   - Drop the check for id >= HID_MAX_IDS || id < 0 since all of our
> >     IDs are static and known to satisfy that at compile time
> >
> >   - Change the algorithms to check all possible report
> >     types (including very long report) and deem the device as a valid
> >     HID++ device if it supports at least one
> >
> >   - Treat invalid report length as a hard stop for the validation
> >     algorithm, meaning that if any of the supported reports has
> >     invalid length we assume the worst and treat the device as a
> >     generic HID device.
> >
> >   - Fold initialization of hidpp->very_long_report_length into
> >     hidpp_validate_device() since it already fetches very long report
> >     length and validates its value
> >
> > Fixes: fe3ee1ec007b ("HID: logitech-hidpp: allow non HID++ devices to be handled by this module")
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204191
> > Reported-by: Sam Bazely <sambazley@fastmail.com>
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > Cc: Jiri Kosina <jikos@kernel.org>
> > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > Cc: Henrik Rydberg <rydberg@bitmath.org>
> > Cc: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com>
> > Cc: Austin Palmer <austinp@valvesoftware.com>
> > Cc: linux-input@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: stable@vger.kernel.org # 5.2+
> > ---
> >  drivers/hid/hid-logitech-hidpp.c | 54 ++++++++++++++++++--------------
> >  1 file changed, 30 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 85911586b3b6..8c4be991f387 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -3498,34 +3498,45 @@ static int hidpp_get_report_length(struct hid_device *hdev, int id)
> >         return report->field[0]->report_count + 1;
> >  }
> >
> > -static bool hidpp_validate_report(struct hid_device *hdev, int id,
> > -                                 int expected_length, bool optional)
> > +static bool hidpp_validate_device(struct hid_device *hdev)
> >  {
> > -       int report_length;
> > +       struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> > +       int id, report_length, supported_reports = 0;
> > +
> > +       id = REPORT_ID_HIDPP_SHORT;
> > +       report_length = hidpp_get_report_length(hdev, id);
> > +       if (report_length) {
> > +               if (report_length < HIDPP_REPORT_SHORT_LENGTH)
> > +                       goto bad_device;
> >
> > -       if (id >= HID_MAX_IDS || id < 0) {
> > -               hid_err(hdev, "invalid HID report id %u\n", id);
> > -               return false;
> > +               supported_reports++;
> >         }
> >
> > +       id = REPORT_ID_HIDPP_LONG;
> >         report_length = hidpp_get_report_length(hdev, id);
> > -       if (!report_length)
> > -               return optional;
> > +       if (report_length) {
> > +               if (report_length < HIDPP_REPORT_LONG_LENGTH)
> > +                       goto bad_device;
> >
> > -       if (report_length < expected_length) {
> > -               hid_warn(hdev, "not enough values in hidpp report %d\n", id);
> > -               return false;
> > +               supported_reports++;
> >         }
> >
> > -       return true;
> > -}
> > +       id = REPORT_ID_HIDPP_VERY_LONG;
> > +       report_length = hidpp_get_report_length(hdev, id);
> > +       if (report_length) {
> > +               if (report_length > HIDPP_REPORT_LONG_LENGTH &&
> > +                   report_length < HIDPP_REPORT_VERY_LONG_MAX_LENGTH)
>
> Can you double check the conditions here?
> It's late, but I think you inverted the tests as we expect the report
> length to be between HIDPP_REPORT_LONG_LENGTH and
> HIDPP_REPORT_VERY_LONG_MAX_LENGTH inclusive, while here this creates a
> bad_device.

Hmm, I think you are right. Not sure why I didn't catch it on G920
since it support very long reports AFAIR. Will re-spin and double
check in v3. Sorry about that.

Thanks,
Andrey Smirnov

Re: [PATCH v2 2/3] HID: logitech-hidpp: rework device validation

[Benjamin Tissoires]

On Wed, Oct 16, 2019 at 9:38 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> On Wed, Oct 16, 2019 at 12:24 PM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > Hi Andrey,
> >
> > On Wed, Oct 16, 2019 at 8:30 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > >
> > > G920 device only advertises REPORT_ID_HIDPP_LONG and
> > > REPORT_ID_HIDPP_VERY_LONG in its HID report descriptor, so querying
> > > for REPORT_ID_HIDPP_SHORT with optional=false will always fail and
> > > prevent G920 to be recognized as a valid HID++ device.
> > >
> > > To fix this and improve some other aspects, modify
> > > hidpp_validate_device() as follows:
> > >
> > >   - Inline the code of hidpp_validate_report() to simplify
> > >     distingushing between non-present and invalid report descriptors
> > >
> > >   - Drop the check for id >= HID_MAX_IDS || id < 0 since all of our
> > >     IDs are static and known to satisfy that at compile time
> > >
> > >   - Change the algorithms to check all possible report
> > >     types (including very long report) and deem the device as a valid
> > >     HID++ device if it supports at least one
> > >
> > >   - Treat invalid report length as a hard stop for the validation
> > >     algorithm, meaning that if any of the supported reports has
> > >     invalid length we assume the worst and treat the device as a
> > >     generic HID device.
> > >
> > >   - Fold initialization of hidpp->very_long_report_length into
> > >     hidpp_validate_device() since it already fetches very long report
> > >     length and validates its value
> > >
> > > Fixes: fe3ee1ec007b ("HID: logitech-hidpp: allow non HID++ devices to be handled by this module")
> > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204191
> > > Reported-by: Sam Bazely <sambazley@fastmail.com>
> > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > > Cc: Jiri Kosina <jikos@kernel.org>
> > > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > Cc: Henrik Rydberg <rydberg@bitmath.org>
> > > Cc: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com>
> > > Cc: Austin Palmer <austinp@valvesoftware.com>
> > > Cc: linux-input@vger.kernel.org
> > > Cc: linux-kernel@vger.kernel.org
> > > Cc: stable@vger.kernel.org # 5.2+
> > > ---
> > >  drivers/hid/hid-logitech-hidpp.c | 54 ++++++++++++++++++--------------
> > >  1 file changed, 30 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > index 85911586b3b6..8c4be991f387 100644
> > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > @@ -3498,34 +3498,45 @@ static int hidpp_get_report_length(struct hid_device *hdev, int id)
> > >         return report->field[0]->report_count + 1;
> > >  }
> > >
> > > -static bool hidpp_validate_report(struct hid_device *hdev, int id,
> > > -                                 int expected_length, bool optional)
> > > +static bool hidpp_validate_device(struct hid_device *hdev)
> > >  {
> > > -       int report_length;
> > > +       struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> > > +       int id, report_length, supported_reports = 0;
> > > +
> > > +       id = REPORT_ID_HIDPP_SHORT;
> > > +       report_length = hidpp_get_report_length(hdev, id);
> > > +       if (report_length) {
> > > +               if (report_length < HIDPP_REPORT_SHORT_LENGTH)
> > > +                       goto bad_device;
> > >
> > > -       if (id >= HID_MAX_IDS || id < 0) {
> > > -               hid_err(hdev, "invalid HID report id %u\n", id);
> > > -               return false;
> > > +               supported_reports++;
> > >         }
> > >
> > > +       id = REPORT_ID_HIDPP_LONG;
> > >         report_length = hidpp_get_report_length(hdev, id);
> > > -       if (!report_length)
> > > -               return optional;
> > > +       if (report_length) {
> > > +               if (report_length < HIDPP_REPORT_LONG_LENGTH)
> > > +                       goto bad_device;
> > >
> > > -       if (report_length < expected_length) {
> > > -               hid_warn(hdev, "not enough values in hidpp report %d\n", id);
> > > -               return false;
> > > +               supported_reports++;
> > >         }
> > >
> > > -       return true;
> > > -}
> > > +       id = REPORT_ID_HIDPP_VERY_LONG;
> > > +       report_length = hidpp_get_report_length(hdev, id);
> > > +       if (report_length) {
> > > +               if (report_length > HIDPP_REPORT_LONG_LENGTH &&
> > > +                   report_length < HIDPP_REPORT_VERY_LONG_MAX_LENGTH)
> >
> > Can you double check the conditions here?
> > It's late, but I think you inverted the tests as we expect the report
> > length to be between HIDPP_REPORT_LONG_LENGTH and
> > HIDPP_REPORT_VERY_LONG_MAX_LENGTH inclusive, while here this creates a
> > bad_device.
>
> Hmm, I think you are right. Not sure why I didn't catch it on G920
> since it support very long reports AFAIR. Will re-spin and double
> check in v3. Sorry about that.
>

Oh, the issue is that the very long HID++ reports on the G920 are
HIDPP_REPORT_VERY_LONG_MAX_LENGTH long, which means the test will fail
for those.

Cheers,
Benjamin

[PATCH v2 3/3] HID: logitech-hidpp: do all FF cleanup in hidpp_ff_destroy()

[Andrey Smirnov]

All of the FF-related resources belong to corresponding FF device, so
they should be freed as a part of hidpp_ff_destroy() to avoid
potential race condidions.

Fixes: ff21a635dd1a ("HID: logitech-hidpp: Force feedback support for the Logitech G920")
Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Henrik Rydberg <rydberg@bitmath.org>
Cc: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com>
Cc: Austin Palmer <austinp@valvesoftware.com>
Cc: linux-input@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: stable@vger.kernel.org # 4.9+
---
 drivers/hid/hid-logitech-hidpp.c | 33 +++++---------------------------
 1 file changed, 5 insertions(+), 28 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 8c4be991f387..2524b5104573 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2078,7 +2078,12 @@ static DEVICE_ATTR(range, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH, hidpp
 static void hidpp_ff_destroy(struct ff_device *ff)
 {
 	struct hidpp_ff_private_data *data = ff->private;
+	struct hid_device *hid = data->hidpp->hid_dev;
 
+	hid_info(hid, "Unloading HID++ force feedback.\n");
+
+	device_remove_file(&hid->dev, &dev_attr_range);
+	destroy_workqueue(data->wq);
 	kfree(data->effect_ids);
 }
 
@@ -2170,31 +2175,6 @@ static int hidpp_ff_init(struct hidpp_device *hidpp,
 	return 0;
 }
 
-static int hidpp_ff_deinit(struct hid_device *hid)
-{
-	struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
-	struct input_dev *dev = hidinput->input;
-	struct hidpp_ff_private_data *data;
-
-	if (!dev) {
-		hid_err(hid, "Struct input_dev not found!\n");
-		return -EINVAL;
-	}
-
-	hid_info(hid, "Unloading HID++ force feedback.\n");
-	data = dev->ff->private;
-	if (!data) {
-		hid_err(hid, "Private data not found!\n");
-		return -EINVAL;
-	}
-
-	destroy_workqueue(data->wq);
-	device_remove_file(&hid->dev, &dev_attr_range);
-
-	return 0;
-}
-
-
 /* ************************************************************************** */
 /*                                                                            */
 /* Device Support                                                             */
@@ -3713,9 +3693,6 @@ static void hidpp_remove(struct hid_device *hdev)
 
 	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
 
-	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)
-		hidpp_ff_deinit(hdev);
-
 	hid_hw_stop(hdev);
 	cancel_work_sync(&hidpp->work);
 	mutex_destroy(&hidpp->send_mutex);
-- 
2.21.0

Re: [PATCH v2 3/3] HID: logitech-hidpp: do all FF cleanup in hidpp_ff_destroy()

[Andrey Smirnov]

On Thu, Oct 17, 2019 at 7:31 AM Sasha Levin <sashal@kernel.org> wrote:
>
> Hi,
>
> [This is an automated email]
>
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: ff21a635dd1a9 HID: logitech-hidpp: Force feedback support for the Logitech G920.
>
> The bot has tested the following trees: v5.3.6, v4.19.79, v4.14.149, v4.9.196.
>
> v5.3.6: Build OK!
> v4.19.79: Failed to apply! Possible dependencies:
>     91cf9a98ae412 ("HID: logitech-hidpp: make .probe usbhid capable")
>
> v4.14.149: Failed to apply! Possible dependencies:
>     91cf9a98ae412 ("HID: logitech-hidpp: make .probe usbhid capable")
>
> v4.9.196: Failed to apply! Possible dependencies:
>     6bd4e65d521f9 ("HID: logitech-hidpp: remove HIDPP_QUIRK_CONNECT_EVENTS")
>     91cf9a98ae412 ("HID: logitech-hidpp: make .probe usbhid capable")
>     a4bf6153b3177 ("HID: logitech-hidpp: add a sysfs file to tell we support power_supply")
>
>
> NOTE: The patch will not be queued to stable trees until it is upstream.
>
> How should we proceed with this patch?

Looks like I'd have to mark this one as 5.2+ as well. Please disregard
this series in favor of v3 that will be sent out shortly.

Thanks,
Andrey Smirnov