[PATCH 0/3] Logitech G920 fixes

[PATCH 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

Andrey Smirnov (3):
  HID: logitech-hidpp: use devres to manage FF private data
  HID: logitech-hidpp: split g920_get_config()
  HID: logitech-hidpp: add G920 device validation quirk

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

-- 
2.21.0

[PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Andrey Smirnov]

To simplify resource management in commit that follows as well as to
save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch
driver code to use devres to manage the life-cycle of FF private data.

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: Sam Bazely <sambazley@fastmail.com>
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
---
 drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++---------------
 1 file changed, 29 insertions(+), 24 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0179f7ed77e5..58eb928224e5 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff)
 	struct hidpp_ff_private_data *data = ff->private;
 
 	kfree(data->effect_ids);
+	/*
+	 * Set private to NULL to prevent input_ff_destroy() from
+	 * freeing our devres allocated memory
+	 */
+	ff->private = NULL;
 }
 
 static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
@@ -2090,7 +2095,7 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
 	const u16 bcdDevice = le16_to_cpu(udesc->bcdDevice);
 	struct ff_device *ff;
 	struct hidpp_report response;
-	struct hidpp_ff_private_data *data;
+	struct hidpp_ff_private_data *data = hidpp->private_data;
 	int error, j, num_slots;
 	u8 version;
 
@@ -2129,18 +2134,13 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
 		return error;
 	}
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
 	data->effect_ids = kcalloc(num_slots, sizeof(int), GFP_KERNEL);
-	if (!data->effect_ids) {
-		kfree(data);
+	if (!data->effect_ids)
 		return -ENOMEM;
-	}
+
 	data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
 	if (!data->wq) {
 		kfree(data->effect_ids);
-		kfree(data);
 		return -ENOMEM;
 	}
 
@@ -2199,28 +2199,15 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
 	return 0;
 }
 
-static int hidpp_ff_deinit(struct hid_device *hid)
+static void 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;
-	}
+	struct hidpp_device *hidpp = hid_get_drvdata(hid);
+	struct hidpp_ff_private_data *data = hidpp->private_data;
 
 	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;
 }
 
 
@@ -2725,6 +2712,20 @@ static int k400_connect(struct hid_device *hdev, bool connected)
 
 #define HIDPP_PAGE_G920_FORCE_FEEDBACK			0x8123
 
+static int g920_allocate(struct hid_device *hdev)
+{
+	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
+	struct hidpp_ff_private_data *data;
+
+	data = devm_kzalloc(&hdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	hidpp->private_data = data;
+
+	return 0;
+}
+
 static int g920_get_config(struct hidpp_device *hidpp)
 {
 	u8 feature_type;
@@ -3561,6 +3562,10 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		ret = k400_allocate(hdev);
 		if (ret)
 			return ret;
+	} else if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
+		ret = g920_allocate(hdev);
+		if (ret)
+			return ret;
 	}
 
 	INIT_WORK(&hidpp->work, delayed_work_cb);
-- 
2.21.0

[PATCH 2/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 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>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Henrik Rydberg <rydberg@bitmath.org>
Cc: Sam Bazely <sambazley@fastmail.com>
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
---
 drivers/hid/hid-logitech-hidpp.c | 134 ++++++++++++++++++++-----------
 1 file changed, 85 insertions(+), 49 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 58eb928224e5..cadf36d6c6f3 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);
 
@@ -2086,7 +2087,7 @@ static void hidpp_ff_destroy(struct ff_device *ff)
 	ff->private = NULL;
 }
 
-static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
+static int hidpp_ff_init(struct hidpp_device *hidpp)
 {
 	struct hid_device *hid = hidpp->hid_dev;
 	struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
@@ -2094,9 +2095,8 @@ 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 = hidpp->private_data;
-	int error, j, num_slots;
+	int error, j, num_slots = data->num_effects;
 	u8 version;
 
 	if (!dev) {
@@ -2114,19 +2114,6 @@ 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) {
@@ -2145,10 +2132,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;
 
@@ -2162,37 +2146,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);
 
@@ -2712,6 +2673,30 @@ static int k400_connect(struct hid_device *hdev, bool connected)
 
 #define HIDPP_PAGE_G920_FORCE_FEEDBACK			0x8123
 
+static int g920_ff_set_autocenter(struct hidpp_device *hidpp)
+{
+	struct hidpp_report response;
+	struct hidpp_ff_private_data *data = hidpp->private_data;
+	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_allocate(struct hid_device *hdev)
 {
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
@@ -2728,22 +2713,65 @@ static int g920_allocate(struct hid_device *hdev)
 
 static int g920_get_config(struct hidpp_device *hidpp)
 {
+	struct hidpp_ff_private_data *data = hidpp->private_data;
+	struct hidpp_report response;
 	u8 feature_type;
-	u8 feature_index;
 	int ret;
 
 	/* 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);
 }
 
 /* -------------------------------------------------------------------------- */
@@ -3641,6 +3669,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);
+		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 3/3] HID: logitech-hidpp: add G920 device validation quirk

[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.

Modify hidpp_validate_device() to check only REPORT_ID_HIDPP_LONG with
optional=false on G920 to fix this.

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: Sam Bazely <sambazley@fastmail.com>
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
---
 drivers/hid/hid-logitech-hidpp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index cadf36d6c6f3..f415bf398e17 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -3511,6 +3511,12 @@ static bool hidpp_validate_report(struct hid_device *hdev, int id,
 
 static bool hidpp_validate_device(struct hid_device *hdev)
 {
+	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
+
+	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)
+		return hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
+					     HIDPP_REPORT_SHORT_LENGTH, false);
+
 	return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT,
 				     HIDPP_REPORT_SHORT_LENGTH, false) &&
 	       hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
-- 
2.21.0

Re: [PATCH 0/3] Logitech G920 fixes

[Sam Bazley]

On Sun, Oct 06, 2019 at 10:12:37PM -0700, Andrey Smirnov wrote:
> 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
> 
> Andrey Smirnov (3):
>   HID: logitech-hidpp: use devres to manage FF private data
>   HID: logitech-hidpp: split g920_get_config()
>   HID: logitech-hidpp: add G920 device validation quirk
> 
>  drivers/hid/hid-logitech-hidpp.c | 193 +++++++++++++++++++------------
>  1 file changed, 120 insertions(+), 73 deletions(-)
> 
> -- 
> 2.21.0
> 

Tested-by: Sam Bazley <sambazley@fastmail.com>

Thanks again Andrey!

Re: [PATCH 0/3] Logitech G920 fixes

[Sam Bazley]

On Sun, Oct 06, 2019 at 10:12:37PM -0700, Andrey Smirnov wrote:
> 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
> 
> Andrey Smirnov (3):
>   HID: logitech-hidpp: use devres to manage FF private data
>   HID: logitech-hidpp: split g920_get_config()
>   HID: logitech-hidpp: add G920 device validation quirk
> 
>  drivers/hid/hid-logitech-hidpp.c | 193 +++++++++++++++++++------------
>  1 file changed, 120 insertions(+), 73 deletions(-)
> 
> -- 
> 2.21.0
> 

All seems to work now. Thanks again Andrey!

Tested-by: Sam Bazley <sambazley@fastmail.com>

Re: [PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Benjamin Tissoires]

Hi Andrey,

On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> To simplify resource management in commit that follows as well as to
> save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch
> driver code to use devres to manage the life-cycle of FF private data.
>
> 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: Sam Bazely <sambazley@fastmail.com>
> 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

This patch doesn't seem to fix any error, is there a reason to send it
to stable? (besides as a dependency of the rest of the series).

> ---
>  drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++---------------
>  1 file changed, 29 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 0179f7ed77e5..58eb928224e5 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff)
>         struct hidpp_ff_private_data *data = ff->private;
>
>         kfree(data->effect_ids);

Is there any reasons we can not also devm alloc data->effect_ids?

> +       /*
> +        * Set private to NULL to prevent input_ff_destroy() from
> +        * freeing our devres allocated memory

Ouch. There is something wrong here: input_ff_destroy() calls
kfree(ff->private), when the data has not been allocated by
input_ff_create(). This seems to lack a little bit of symmetry.

> +        */
> +       ff->private = NULL;
>  }
>
>  static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
> @@ -2090,7 +2095,7 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
>         const u16 bcdDevice = le16_to_cpu(udesc->bcdDevice);
>         struct ff_device *ff;
>         struct hidpp_report response;
> -       struct hidpp_ff_private_data *data;
> +       struct hidpp_ff_private_data *data = hidpp->private_data;
>         int error, j, num_slots;
>         u8 version;
>
> @@ -2129,18 +2134,13 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
>                 return error;
>         }
>
> -       data = kzalloc(sizeof(*data), GFP_KERNEL);
> -       if (!data)
> -               return -ENOMEM;
>         data->effect_ids = kcalloc(num_slots, sizeof(int), GFP_KERNEL);
> -       if (!data->effect_ids) {
> -               kfree(data);
> +       if (!data->effect_ids)
>                 return -ENOMEM;
> -       }
> +
>         data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
>         if (!data->wq) {
>                 kfree(data->effect_ids);
> -               kfree(data);
>                 return -ENOMEM;
>         }
>
> @@ -2199,28 +2199,15 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
>         return 0;
>  }
>
> -static int hidpp_ff_deinit(struct hid_device *hid)
> +static void 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;
> -       }
> +       struct hidpp_device *hidpp = hid_get_drvdata(hid);
> +       struct hidpp_ff_private_data *data = hidpp->private_data;
>
>         hid_info(hid, "Unloading HID++ force feedback.\n");
> -       data = dev->ff->private;
> -       if (!data) {

I am pretty sure we might need to keep a test on data not being null.

> -               hid_err(hid, "Private data not found!\n");
> -               return -EINVAL;
> -       }
>
>         destroy_workqueue(data->wq);
>         device_remove_file(&hid->dev, &dev_attr_range);
> -
> -       return 0;
>  }

This whole hunk seems unrelated to the devm change.
Can you extract a patch that just stores hidpp_ff_private_data in
hidpp->private_data and then cleans up hidpp_ff_deinit() before
switching it to devm? (or maybe not, see below)

After a few more thoughts, I don't think this mixing of devm and non
devm is a good idea:
we could say that the hidpp_ff_private_data and its effect_ids are
both children of the input_dev, not the hid_device. And we would
expect the whole thing to simplify with devm, but it's not, because ff
is not supposed to be used with devm.

I have a feeling the whole ff logic is wrong in term of where things
should be cleaned up, but I can not come up with a good hint on where
to start. For example, destroy_workqueue() is called in
hidpp_ff_deinit() where it might be racy, and maybe we should call it
in hidpp_ff_destroy()...

Last, you should base this patch on top of the for-next branch, we
recently merged a fix for the FF drivers in the HID subsystem:
https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?h=for-next&id=d9d4b1e46d9543a82c23f6df03f4ad697dab361b

Would it be too complex to drop this patch from the series and do a
proper follow up cleanup series that might not need to go to stable?

Cheers,
Benjamin

>
>
> @@ -2725,6 +2712,20 @@ static int k400_connect(struct hid_device *hdev, bool connected)
>
>  #define HIDPP_PAGE_G920_FORCE_FEEDBACK                 0x8123
>
> +static int g920_allocate(struct hid_device *hdev)
> +{
> +       struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> +       struct hidpp_ff_private_data *data;
> +
> +       data = devm_kzalloc(&hdev->dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       hidpp->private_data = data;
> +
> +       return 0;
> +}
> +
>  static int g920_get_config(struct hidpp_device *hidpp)
>  {
>         u8 feature_type;
> @@ -3561,6 +3562,10 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                 ret = k400_allocate(hdev);
>                 if (ret)
>                         return ret;
> +       } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
> +               ret = g920_allocate(hdev);
> +               if (ret)
> +                       return ret;
>         }
>
>         INIT_WORK(&hidpp->work, delayed_work_cb);
> --
> 2.21.0
>

Re: [PATCH 0/3] Logitech G920 fixes

[Benjamin Tissoires]

Hi,

Finally, someone who takes care of the G920!
Note that when I sent my first initial version that Hans reused, I
surely broke it (looking at your patch 3/3), but no one cared to test
it :(

On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> 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.

So, you are talking about regressions, and for that I would like to be
able to push the patches to stable.

However, I would need more information:
- patch 3/3 seems simple enough to go in stable, and is clearly a
regression from the recent series. Can you put it in first and add
stable@vger.kernel.org in a CC field (and possibly with a Fixes tag as
well)?
- I am not sure which patch fixes the wheel remains stuck in
autocentering mode. Is it patch 2/3?
- was the "wheel remains stuck in autocentering mode" bug present from
on of the recent patch or was it always there since we introduced
support in hid-logitech-hidpp, but the game would need to unlock the
wheel first?

So all in all, can you:
- drop the first patch and push it in a later series
- re-order the patches: 3/3 then 2/3
- tell me when the wheel is not stuck when the series is applied
(after 3/3 or 2/3), and make a note in the commit message with that
information
- take into account my comments in the first patch you submitted (that
I just sent).

Cheers,
Benjamin


>
> Thanks,
> Andrey Smirnov
>
> Andrey Smirnov (3):
>   HID: logitech-hidpp: use devres to manage FF private data
>   HID: logitech-hidpp: split g920_get_config()
>   HID: logitech-hidpp: add G920 device validation quirk
>
>  drivers/hid/hid-logitech-hidpp.c | 193 +++++++++++++++++++------------
>  1 file changed, 120 insertions(+), 73 deletions(-)
>
> --
> 2.21.0
>

Re: [PATCH 3/3] HID: logitech-hidpp: add G920 device validation quirk

[Benjamin Tissoires]

On Mon, Oct 7, 2019 at 7:13 AM 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.
>
> Modify hidpp_validate_device() to check only REPORT_ID_HIDPP_LONG with
> optional=false on G920 to fix this.
>
> 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: Sam Bazely <sambazley@fastmail.com>
> 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
> ---
>  drivers/hid/hid-logitech-hidpp.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index cadf36d6c6f3..f415bf398e17 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -3511,6 +3511,12 @@ static bool hidpp_validate_report(struct hid_device *hdev, int id,
>
>  static bool hidpp_validate_device(struct hid_device *hdev)
>  {
> +       struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> +
> +       if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)
> +               return hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
> +                                            HIDPP_REPORT_SHORT_LENGTH, false);
> +

with https://patchwork.kernel.org/patch/11184749/ we also have a need
for such a trick for BLE mice.

I wonder if we should not have a more common way of validating the devices

This can probably be handled later, as your patch fixes the current devices.

Cheers,
Benjamin

>         return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT,
>                                      HIDPP_REPORT_SHORT_LENGTH, false) &&
>                hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
> --
> 2.21.0
>

Re: [PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Andrey Smirnov]

On Fri, Oct 11, 2019 at 7:52 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Hi Andrey,
>
> On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> >
> > To simplify resource management in commit that follows as well as to
> > save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch
> > driver code to use devres to manage the life-cycle of FF private data.
> >
> > 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: Sam Bazely <sambazley@fastmail.com>
> > 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
>
> This patch doesn't seem to fix any error, is there a reason to send it
> to stable? (besides as a dependency of the rest of the series).
>

Dependency is the only reason for this patch, but it is a pretty big
reason. Prior to patches 1/3 and 2/3 FF private data was both
allocated and passed off to FF layer via ff->private = data in a span
of a few lines of code within hidpp_ff_init()/g920_get_config().
After, said pair is effectively split into two different functions,
both needing access to FF private data, but called quite far apart in
hidpp_probe(). Alternatives to patch 1/3 would be to either make sure
that every error path in hidpp_prob() after the call to
g920_allocate() is aware of allocated FF data, which seems like a
nightmare, or, to create a temporary FF data, fill it in
g920_get_config() and memdup() it in hidpp_ff_init(). Is that (the
latter) the path that you prefer to take?

> > ---
> >  drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++---------------
> >  1 file changed, 29 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 0179f7ed77e5..58eb928224e5 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff)
> >         struct hidpp_ff_private_data *data = ff->private;
> >
> >         kfree(data->effect_ids);
>
> Is there any reasons we can not also devm alloc data->effect_ids?

No, but I was trying to limit the scope of this patch.

>
> > +       /*
> > +        * Set private to NULL to prevent input_ff_destroy() from
> > +        * freeing our devres allocated memory
>
> Ouch. There is something wrong here: input_ff_destroy() calls
> kfree(ff->private), when the data has not been allocated by
> input_ff_create(). This seems to lack a little bit of symmetry.
>

I agree, I think it's a wart in FF API design.

> > +        */
> > +       ff->private = NULL;
> >  }
> >
> >  static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
> > @@ -2090,7 +2095,7 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
> >         const u16 bcdDevice = le16_to_cpu(udesc->bcdDevice);
> >         struct ff_device *ff;
> >         struct hidpp_report response;
> > -       struct hidpp_ff_private_data *data;
> > +       struct hidpp_ff_private_data *data = hidpp->private_data;
> >         int error, j, num_slots;
> >         u8 version;
> >
> > @@ -2129,18 +2134,13 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
> >                 return error;
> >         }
> >
> > -       data = kzalloc(sizeof(*data), GFP_KERNEL);
> > -       if (!data)
> > -               return -ENOMEM;
> >         data->effect_ids = kcalloc(num_slots, sizeof(int), GFP_KERNEL);
> > -       if (!data->effect_ids) {
> > -               kfree(data);
> > +       if (!data->effect_ids)
> >                 return -ENOMEM;
> > -       }
> > +
> >         data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
> >         if (!data->wq) {
> >                 kfree(data->effect_ids);
> > -               kfree(data);
> >                 return -ENOMEM;
> >         }
> >
> > @@ -2199,28 +2199,15 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
> >         return 0;
> >  }
> >
> > -static int hidpp_ff_deinit(struct hid_device *hid)
> > +static void 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;
> > -       }
> > +       struct hidpp_device *hidpp = hid_get_drvdata(hid);
> > +       struct hidpp_ff_private_data *data = hidpp->private_data;
> >
> >         hid_info(hid, "Unloading HID++ force feedback.\n");
> > -       data = dev->ff->private;
> > -       if (!data) {
>
> I am pretty sure we might need to keep a test on data not being null.
>

OK, sure. Could you be more explicit in your reasoning next time
though? I am assuming this is because hid_hw_stop() might be called
before?

> > -               hid_err(hid, "Private data not found!\n");
> > -               return -EINVAL;
> > -       }
> >
> >         destroy_workqueue(data->wq);
> >         device_remove_file(&hid->dev, &dev_attr_range);
> > -
> > -       return 0;
> >  }
>
> This whole hunk seems unrelated to the devm change.
> Can you extract a patch that just stores hidpp_ff_private_data in
> hidpp->private_data and then cleans up hidpp_ff_deinit() before
> switching it to devm? (or maybe not, see below)

Well it appears you are against the idea of leveraging devres in this
series, so discussing the fate of said hunk seems moot.

>
> After a few more thoughts, I don't think this mixing of devm and non
> devm is a good idea:
> we could say that the hidpp_ff_private_data and its effect_ids are
> both children of the input_dev, not the hid_device. And we would
> expect the whole thing to simplify with devm, but it's not, because ff
> is not supposed to be used with devm.
>
> I have a feeling the whole ff logic is wrong in term of where things
> should be cleaned up, but I can not come up with a good hint on where
> to start. For example, destroy_workqueue() is called in
> hidpp_ff_deinit() where it might be racy, and maybe we should call it
> in hidpp_ff_destroy()...
>

Yeah, it probably should be moved to hidpp_ff_destroy(). Out of scope
for this series though, I'll deal with it in a separate submission.

> Last, you should base this patch on top of the for-next branch, we
> recently merged a fix for the FF drivers in the HID subsystem:
> https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?h=for-next&id=d9d4b1e46d9543a82c23f6df03f4ad697dab361b
>

Sure will do.

> Would it be too complex to drop this patch from the series and do a
> proper follow up cleanup series that might not need to go to stable?
>

No it's alright. I'll submit a v2 of this series with only two patches
and send a follow up after.

Thanks,
Andrey Smirnov

Re: [PATCH 0/3] Logitech G920 fixes

[Andrey Smirnov]

On Fri, Oct 11, 2019 at 7:53 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Hi,
>
> Finally, someone who takes care of the G920!
> Note that when I sent my first initial version that Hans reused, I
> surely broke it (looking at your patch 3/3), but no one cared to test
> it :(
>
> On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> >
> > 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.
>
> So, you are talking about regressions, and for that I would like to be
> able to push the patches to stable.
>
> However, I would need more information:
> - patch 3/3 seems simple enough to go in stable, and is clearly a
> regression from the recent series. Can you put it in first and add
> stable@vger.kernel.org in a CC field (and possibly with a Fixes tag as
> well)?

It patch 3/3 on purpose because applying it by itself, without fix in
2/3 in place would lead to a segfault and a non working wheel. Maybe
that FF for-next fix you pointed out can prevent that from happening,
but as is the series is pretty atomic and can't be divided.

Patch 3/3 already has stable in CC and Fixes tag.

> - I am not sure which patch fixes the wheel remains stuck in
> autocentering mode. Is it patch 2/3?

There's no specific patch that does that. There were two G920
regressions in the driver and both need to be fixed for wheel to be
configured properly. The specific code that releases the wheel is in
g920_ff_set_autocenter().

> - was the "wheel remains stuck in autocentering mode" bug present from
> on of the recent patch or was it always there since we introduced
> support in hid-logitech-hidpp, but the game would need to unlock the
> wheel first?

The wheel worked as expected prior to

fe3ee1ec007b ("HID: logitech-hidpp: allow non HID++ devices to be
handled by this module")
91cf9a98ae41 ("HID: logitech-hidpp: make .probe usbhid capable")

It's not the game that needs to unlock the wheel, but the driver itself.

Thanks,
Andrey Smirnov

Re: [PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Dmitry Torokhov]

On Fri, Oct 11, 2019 at 04:52:04PM +0200, Benjamin Tissoires wrote:
> Hi Andrey,
> 
> On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> >
> > To simplify resource management in commit that follows as well as to
> > save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch
> > driver code to use devres to manage the life-cycle of FF private data.
> >
> > 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: Sam Bazely <sambazley@fastmail.com>
> > 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
> 
> This patch doesn't seem to fix any error, is there a reason to send it
> to stable? (besides as a dependency of the rest of the series).
> 
> > ---
> >  drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++---------------
> >  1 file changed, 29 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 0179f7ed77e5..58eb928224e5 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff)
> >         struct hidpp_ff_private_data *data = ff->private;
> >
> >         kfree(data->effect_ids);
> 
> Is there any reasons we can not also devm alloc data->effect_ids?
> 
> > +       /*
> > +        * Set private to NULL to prevent input_ff_destroy() from
> > +        * freeing our devres allocated memory
> 
> Ouch. There is something wrong here: input_ff_destroy() calls
> kfree(ff->private), when the data has not been allocated by
> input_ff_create(). This seems to lack a little bit of symmetry.

Yeah, ff and ff-memless essentially take over the private data assigned
to them. They were done before devm and the lifetime of the "private"
data pieces was tied to the lifetime of the input device to simplify
error handling and teardown.

Maybe we should clean it up a bit... I'm open to suggestions.

In this case maybe best way is to get rid of hidpp_ff_destroy() and not
set ff->private and rely on devm to free the buffers. One can get to
device private data from ff methods via input_get_drvdata() since they
all (except destroy) are passed input device pointer.

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Benjamin Tissoires]

Hi Andrey,

On Fri, Oct 11, 2019 at 8:19 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> On Fri, Oct 11, 2019 at 7:52 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > Hi Andrey,
> >
> > On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > >
> > > To simplify resource management in commit that follows as well as to
> > > save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch
> > > driver code to use devres to manage the life-cycle of FF private data.
> > >
> > > 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: Sam Bazely <sambazley@fastmail.com>
> > > 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
> >
> > This patch doesn't seem to fix any error, is there a reason to send it
> > to stable? (besides as a dependency of the rest of the series).
> >
>
> Dependency is the only reason for this patch, but it is a pretty big
> reason. Prior to patches 1/3 and 2/3 FF private data was both
> allocated and passed off to FF layer via ff->private = data in a span
> of a few lines of code within hidpp_ff_init()/g920_get_config().
> After, said pair is effectively split into two different functions,
> both needing access to FF private data, but called quite far apart in
> hidpp_probe(). Alternatives to patch 1/3 would be to either make sure
> that every error path in hidpp_prob() after the call to
> g920_allocate() is aware of allocated FF data, which seems like a
> nightmare, or, to create a temporary FF data, fill it in
> g920_get_config() and memdup() it in hidpp_ff_init(). Is that (the
> latter) the path that you prefer to take?

Hmm, I don't have a strong opinion on that. The point I don't like
with the devres version is that it seems like a half-backed solution,
as part of the driver would use devres while parts for the same
purpose will not. I do not consider your code untrusted, but this is
usually a reasonable source of leakages, so as a rule a thumb, devres
should be used in a all or nothing fashion.

Basically, both alternative solutions would be OK with me, as long as
the scope of each patch is reduced to the minimum (and having more
than one is OK too).

>
> > > ---
> > >  drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++---------------
> > >  1 file changed, 29 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > index 0179f7ed77e5..58eb928224e5 100644
> > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > @@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff)
> > >         struct hidpp_ff_private_data *data = ff->private;
> > >
> > >         kfree(data->effect_ids);
> >
> > Is there any reasons we can not also devm alloc data->effect_ids?
>
> No, but I was trying to limit the scope of this patch.
>
> >
> > > +       /*
> > > +        * Set private to NULL to prevent input_ff_destroy() from
> > > +        * freeing our devres allocated memory
> >
> > Ouch. There is something wrong here: input_ff_destroy() calls
> > kfree(ff->private), when the data has not been allocated by
> > input_ff_create(). This seems to lack a little bit of symmetry.
> >
>
> I agree, I think it's a wart in FF API design.

Yep, see Dmitry's answer for ideas :)

>
> > > +        */
> > > +       ff->private = NULL;
> > >  }
> > >
> > >  static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
> > > @@ -2090,7 +2095,7 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
> > >         const u16 bcdDevice = le16_to_cpu(udesc->bcdDevice);
> > >         struct ff_device *ff;
> > >         struct hidpp_report response;
> > > -       struct hidpp_ff_private_data *data;
> > > +       struct hidpp_ff_private_data *data = hidpp->private_data;
> > >         int error, j, num_slots;
> > >         u8 version;
> > >
> > > @@ -2129,18 +2134,13 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
> > >                 return error;
> > >         }
> > >
> > > -       data = kzalloc(sizeof(*data), GFP_KERNEL);
> > > -       if (!data)
> > > -               return -ENOMEM;
> > >         data->effect_ids = kcalloc(num_slots, sizeof(int), GFP_KERNEL);
> > > -       if (!data->effect_ids) {
> > > -               kfree(data);
> > > +       if (!data->effect_ids)
> > >                 return -ENOMEM;
> > > -       }
> > > +
> > >         data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
> > >         if (!data->wq) {
> > >                 kfree(data->effect_ids);
> > > -               kfree(data);
> > >                 return -ENOMEM;
> > >         }
> > >
> > > @@ -2199,28 +2199,15 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
> > >         return 0;
> > >  }
> > >
> > > -static int hidpp_ff_deinit(struct hid_device *hid)
> > > +static void 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;
> > > -       }
> > > +       struct hidpp_device *hidpp = hid_get_drvdata(hid);
> > > +       struct hidpp_ff_private_data *data = hidpp->private_data;
> > >
> > >         hid_info(hid, "Unloading HID++ force feedback.\n");
> > > -       data = dev->ff->private;
> > > -       if (!data) {
> >
> > I am pretty sure we might need to keep a test on data not being null.
> >
>
> OK, sure. Could you be more explicit in your reasoning next time
> though? I am assuming this is because hid_hw_stop() might be called
> before?

Honestly, I don't have a good reason for it. I have seen enough of
automatic static/dynamic checks with the same patterns to let my guts
express that we need to check on the value of a pointer stored in
private_data before dereferencing it.

If you are absolutely sure this is not need, a simple comment in the
code is enough :)

>
> > > -               hid_err(hid, "Private data not found!\n");
> > > -               return -EINVAL;
> > > -       }
> > >
> > >         destroy_workqueue(data->wq);
> > >         device_remove_file(&hid->dev, &dev_attr_range);
> > > -
> > > -       return 0;
> > >  }
> >
> > This whole hunk seems unrelated to the devm change.
> > Can you extract a patch that just stores hidpp_ff_private_data in
> > hidpp->private_data and then cleans up hidpp_ff_deinit() before
> > switching it to devm? (or maybe not, see below)
>
> Well it appears you are against the idea of leveraging devres in this
> series, so discussing the fate of said hunk seems moot.

Well, I really value your work and I am very happy of it. It's just
that for a patch/series aimed at stable, I rather have the patch
series following the stable rules, which are that we should fix one
thing only, and have the most simplest patch possible. I truly believe
adding devres to cleanup the error path is the thing to do, but maybe
not in this series.

>
> >
> > After a few more thoughts, I don't think this mixing of devm and non
> > devm is a good idea:
> > we could say that the hidpp_ff_private_data and its effect_ids are
> > both children of the input_dev, not the hid_device. And we would
> > expect the whole thing to simplify with devm, but it's not, because ff
> > is not supposed to be used with devm.
> >
> > I have a feeling the whole ff logic is wrong in term of where things
> > should be cleaned up, but I can not come up with a good hint on where
> > to start. For example, destroy_workqueue() is called in
> > hidpp_ff_deinit() where it might be racy, and maybe we should call it
> > in hidpp_ff_destroy()...
> >
>
> Yeah, it probably should be moved to hidpp_ff_destroy(). Out of scope
> for this series though, I'll deal with it in a separate submission.

As per Dmitry's suggestion of removing hidpp_ff_destroy() maybe we
should keep it that way, I am not entirely sure about how the races
can happen (I don't think I even have one FF device I could test
against).

>
> > Last, you should base this patch on top of the for-next branch, we
> > recently merged a fix for the FF drivers in the HID subsystem:
> > https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?h=for-next&id=d9d4b1e46d9543a82c23f6df03f4ad697dab361b
> >
>
> Sure will do.

thanks \o/

>
> > Would it be too complex to drop this patch from the series and do a
> > proper follow up cleanup series that might not need to go to stable?
> >
>
> No it's alright. I'll submit a v2 of this series with only two patches
> and send a follow up after.
>

And thanks a lot again.

Cheers,
Benjamin

Re: [PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Benjamin Tissoires]

On Fri, Oct 11, 2019 at 8:26 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Fri, Oct 11, 2019 at 04:52:04PM +0200, Benjamin Tissoires wrote:
> > Hi Andrey,
> >
> > On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > >
> > > To simplify resource management in commit that follows as well as to
> > > save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch
> > > driver code to use devres to manage the life-cycle of FF private data.
> > >
> > > 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: Sam Bazely <sambazley@fastmail.com>
> > > 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
> >
> > This patch doesn't seem to fix any error, is there a reason to send it
> > to stable? (besides as a dependency of the rest of the series).
> >
> > > ---
> > >  drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++---------------
> > >  1 file changed, 29 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > index 0179f7ed77e5..58eb928224e5 100644
> > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > @@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff)
> > >         struct hidpp_ff_private_data *data = ff->private;
> > >
> > >         kfree(data->effect_ids);
> >
> > Is there any reasons we can not also devm alloc data->effect_ids?
> >
> > > +       /*
> > > +        * Set private to NULL to prevent input_ff_destroy() from
> > > +        * freeing our devres allocated memory
> >
> > Ouch. There is something wrong here: input_ff_destroy() calls
> > kfree(ff->private), when the data has not been allocated by
> > input_ff_create(). This seems to lack a little bit of symmetry.
>
> Yeah, ff and ff-memless essentially take over the private data assigned
> to them. They were done before devm and the lifetime of the "private"
> data pieces was tied to the lifetime of the input device to simplify
> error handling and teardown.

Yeah, that stealing of the pointer is not good :)
But OTOH, it helps

>
> Maybe we should clean it up a bit... I'm open to suggestions.

The problem I had when doing the review was that there is no easy way
to have a "devm_input_ff_create_()", because the way it's built is
already "devres-compatible": the destroy gets called by input core.

So I don't have a good answer to simplify in a transparent manner
without breaking the API.

>
> In this case maybe best way is to get rid of hidpp_ff_destroy() and not
> set ff->private and rely on devm to free the buffers. One can get to
> device private data from ff methods via input_get_drvdata() since they
> all (except destroy) are passed input device pointer.

Sounds like a good idea. However, it seems there might be a race when
removing the workqueue:
the workqueue gets deleted in hidpp_remove, when the input node will
be freed by devres, so after the call of hidpp_remove.

So we should probably keep hidpp_ff_destroy() to clean up the ff bits,
and instead move the content of hidpp_ff_deinit() into
hidpp_ff_destroy() so we ensure proper ordering.

Andrey, note that ensuring the workqueue gets freed after the call of
input_destroy_device is something that should definitively go into
stable as this is a potential race problem.

Cheers,
Benjamin

Re: [PATCH 3/3] HID: logitech-hidpp: add G920 device validation quirk

[Andrey Smirnov]

On Fri, Oct 11, 2019 at 7:56 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Mon, Oct 7, 2019 at 7:13 AM 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.
> >
> > Modify hidpp_validate_device() to check only REPORT_ID_HIDPP_LONG with
> > optional=false on G920 to fix this.
> >
> > 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: Sam Bazely <sambazley@fastmail.com>
> > 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
> > ---
> >  drivers/hid/hid-logitech-hidpp.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index cadf36d6c6f3..f415bf398e17 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -3511,6 +3511,12 @@ static bool hidpp_validate_report(struct hid_device *hdev, int id,
> >
> >  static bool hidpp_validate_device(struct hid_device *hdev)
> >  {
> > +       struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> > +
> > +       if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)
> > +               return hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
> > +                                            HIDPP_REPORT_SHORT_LENGTH, false);
> > +
>
> with https://patchwork.kernel.org/patch/11184749/ we also have a need
> for such a trick for BLE mice.
>
> I wonder if we should not have a more common way of validating the devices
>

What about just checking for:

hidpp_validate_report(REPORT_ID_HIDPP_SHORT,
                                    HIDPP_REPORT_SHORT_LENGTH, true) ||
hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
                                    HIDPP_REPORT_LONG_LENGTH, true);

and probably dropping the "optional" argument for
hidpp_validate_report()? Original code allows there to be devices
supporting shorts reports only, but it seems that devices that support
only long reports are legitimate too, so maybe the only "invalid"
combination is if both are invalid length or missing?

Thanks,
Andrey Smirnov

Re: [PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Dmitry Torokhov]

On Fri, Oct 11, 2019 at 09:25:52PM +0200, Benjamin Tissoires wrote:
> On Fri, Oct 11, 2019 at 8:26 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Fri, Oct 11, 2019 at 04:52:04PM +0200, Benjamin Tissoires wrote:
> > > Hi Andrey,
> > >
> > > On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > > >
> > > > To simplify resource management in commit that follows as well as to
> > > > save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch
> > > > driver code to use devres to manage the life-cycle of FF private data.
> > > >
> > > > 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: Sam Bazely <sambazley@fastmail.com>
> > > > 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
> > >
> > > This patch doesn't seem to fix any error, is there a reason to send it
> > > to stable? (besides as a dependency of the rest of the series).
> > >
> > > > ---
> > > >  drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++---------------
> > > >  1 file changed, 29 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > > index 0179f7ed77e5..58eb928224e5 100644
> > > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > > @@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff)
> > > >         struct hidpp_ff_private_data *data = ff->private;
> > > >
> > > >         kfree(data->effect_ids);
> > >
> > > Is there any reasons we can not also devm alloc data->effect_ids?
> > >
> > > > +       /*
> > > > +        * Set private to NULL to prevent input_ff_destroy() from
> > > > +        * freeing our devres allocated memory
> > >
> > > Ouch. There is something wrong here: input_ff_destroy() calls
> > > kfree(ff->private), when the data has not been allocated by
> > > input_ff_create(). This seems to lack a little bit of symmetry.
> >
> > Yeah, ff and ff-memless essentially take over the private data assigned
> > to them. They were done before devm and the lifetime of the "private"
> > data pieces was tied to the lifetime of the input device to simplify
> > error handling and teardown.
> 
> Yeah, that stealing of the pointer is not good :)
> But OTOH, it helps
> 
> >
> > Maybe we should clean it up a bit... I'm open to suggestions.
> 
> The problem I had when doing the review was that there is no easy way
> to have a "devm_input_ff_create_()", because the way it's built is
> already "devres-compatible": the destroy gets called by input core.

I do not think we want devm_input_ff_create() explicitly, I think the
fact that you can "build up" an input device by allocating it, then
adding slots, poller, ff support, etc, and input core cleans it up is
all good. It is just the ownership if the driver-private data block is
not very obvious and is not compatible with allocating via devm.

> 
> So I don't have a good answer to simplify in a transparent manner
> without breaking the API.
> 
> >
> > In this case maybe best way is to get rid of hidpp_ff_destroy() and not
> > set ff->private and rely on devm to free the buffers. One can get to
> > device private data from ff methods via input_get_drvdata() since they
> > all (except destroy) are passed input device pointer.
> 
> Sounds like a good idea. However, it seems there might be a race when
> removing the workqueue:
> the workqueue gets deleted in hidpp_remove, when the input node will
> be freed by devres, so after the call of hidpp_remove.

Yeah, well, that is a common issue with mixing devm and normal resources
(and workqueue here is that "normal" resource), and we should either:

- not use devm
- use devm_add_action_or_reset() to work in custom actions that work
  freeing of non-managed resources into devm flow.

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Dmitry Torokhov]

On Fri, Oct 11, 2019 at 01:33:03PM -0700, Dmitry Torokhov wrote:
> On Fri, Oct 11, 2019 at 09:25:52PM +0200, Benjamin Tissoires wrote:
> > On Fri, Oct 11, 2019 at 8:26 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > On Fri, Oct 11, 2019 at 04:52:04PM +0200, Benjamin Tissoires wrote:
> > > > Hi Andrey,
> > > >
> > > > On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > > > >
> > > > > To simplify resource management in commit that follows as well as to
> > > > > save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch
> > > > > driver code to use devres to manage the life-cycle of FF private data.
> > > > >
> > > > > 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: Sam Bazely <sambazley@fastmail.com>
> > > > > 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
> > > >
> > > > This patch doesn't seem to fix any error, is there a reason to send it
> > > > to stable? (besides as a dependency of the rest of the series).
> > > >
> > > > > ---
> > > > >  drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++---------------
> > > > >  1 file changed, 29 insertions(+), 24 deletions(-)
> > > > >
> > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > > > index 0179f7ed77e5..58eb928224e5 100644
> > > > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > > > @@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff)
> > > > >         struct hidpp_ff_private_data *data = ff->private;
> > > > >
> > > > >         kfree(data->effect_ids);
> > > >
> > > > Is there any reasons we can not also devm alloc data->effect_ids?
> > > >
> > > > > +       /*
> > > > > +        * Set private to NULL to prevent input_ff_destroy() from
> > > > > +        * freeing our devres allocated memory
> > > >
> > > > Ouch. There is something wrong here: input_ff_destroy() calls
> > > > kfree(ff->private), when the data has not been allocated by
> > > > input_ff_create(). This seems to lack a little bit of symmetry.
> > >
> > > Yeah, ff and ff-memless essentially take over the private data assigned
> > > to them. They were done before devm and the lifetime of the "private"
> > > data pieces was tied to the lifetime of the input device to simplify
> > > error handling and teardown.
> > 
> > Yeah, that stealing of the pointer is not good :)
> > But OTOH, it helps
> > 
> > >
> > > Maybe we should clean it up a bit... I'm open to suggestions.
> > 
> > The problem I had when doing the review was that there is no easy way
> > to have a "devm_input_ff_create_()", because the way it's built is
> > already "devres-compatible": the destroy gets called by input core.
> 
> I do not think we want devm_input_ff_create() explicitly, I think the
> fact that you can "build up" an input device by allocating it, then
> adding slots, poller, ff support, etc, and input core cleans it up is
> all good. It is just the ownership if the driver-private data block is
> not very obvious and is not compatible with allocating via devm.
> 
> > 
> > So I don't have a good answer to simplify in a transparent manner
> > without breaking the API.
> > 
> > >
> > > In this case maybe best way is to get rid of hidpp_ff_destroy() and not
> > > set ff->private and rely on devm to free the buffers. One can get to
> > > device private data from ff methods via input_get_drvdata() since they
> > > all (except destroy) are passed input device pointer.
> > 
> > Sounds like a good idea. However, it seems there might be a race when
> > removing the workqueue:
> > the workqueue gets deleted in hidpp_remove, when the input node will
> > be freed by devres, so after the call of hidpp_remove.
> 
> Yeah, well, that is a common issue with mixing devm and normal resources
> (and workqueue here is that "normal" resource), and we should either:
> 
> - not use devm
> - use devm_add_action_or_reset() to work in custom actions that work
>   freeing of non-managed resources into devm flow.

Actually, there is a door #3: use system workqueue. After all the work
that Tejun done on workqueues it is very rare that one actually needs
a dedicated workqueue (as works usually execute on one if the system
worker threads that are shared with other workqueues anyway).

-- 
Dmitry

Re: [PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Andrey Smirnov]

On Fri, Oct 11, 2019 at 12:26 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Fri, Oct 11, 2019 at 8:26 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Fri, Oct 11, 2019 at 04:52:04PM +0200, Benjamin Tissoires wrote:
> > > Hi Andrey,
> > >
> > > On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > > >
> > > > To simplify resource management in commit that follows as well as to
> > > > save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch
> > > > driver code to use devres to manage the life-cycle of FF private data.
> > > >
> > > > 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: Sam Bazely <sambazley@fastmail.com>
> > > > 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
> > >
> > > This patch doesn't seem to fix any error, is there a reason to send it
> > > to stable? (besides as a dependency of the rest of the series).
> > >
> > > > ---
> > > >  drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++---------------
> > > >  1 file changed, 29 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > > index 0179f7ed77e5..58eb928224e5 100644
> > > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > > @@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff)
> > > >         struct hidpp_ff_private_data *data = ff->private;
> > > >
> > > >         kfree(data->effect_ids);
> > >
> > > Is there any reasons we can not also devm alloc data->effect_ids?
> > >
> > > > +       /*
> > > > +        * Set private to NULL to prevent input_ff_destroy() from
> > > > +        * freeing our devres allocated memory
> > >
> > > Ouch. There is something wrong here: input_ff_destroy() calls
> > > kfree(ff->private), when the data has not been allocated by
> > > input_ff_create(). This seems to lack a little bit of symmetry.
> >
> > Yeah, ff and ff-memless essentially take over the private data assigned
> > to them. They were done before devm and the lifetime of the "private"
> > data pieces was tied to the lifetime of the input device to simplify
> > error handling and teardown.
>
> Yeah, that stealing of the pointer is not good :)
> But OTOH, it helps
>
> >
> > Maybe we should clean it up a bit... I'm open to suggestions.
>
> The problem I had when doing the review was that there is no easy way
> to have a "devm_input_ff_create_()", because the way it's built is
> already "devres-compatible": the destroy gets called by input core.
>
> So I don't have a good answer to simplify in a transparent manner
> without breaking the API.
>
> >
> > In this case maybe best way is to get rid of hidpp_ff_destroy() and not
> > set ff->private and rely on devm to free the buffers. One can get to
> > device private data from ff methods via input_get_drvdata() since they
> > all (except destroy) are passed input device pointer.
>
> Sounds like a good idea. However, it seems there might be a race when
> removing the workqueue:
> the workqueue gets deleted in hidpp_remove, when the input node will
> be freed by devres, so after the call of hidpp_remove.
>
> So we should probably keep hidpp_ff_destroy() to clean up the ff bits,
> and instead move the content of hidpp_ff_deinit() into
> hidpp_ff_destroy() so we ensure proper ordering.
>
> Andrey, note that ensuring the workqueue gets freed after the call of
> input_destroy_device is something that should definitively go into
> stable as this is a potential race problem.

Sure, I'll add this to the series as a separate patch.

Thanks,
Andrey Smirnov

Re: [PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Andrey Smirnov]

On Fri, Oct 11, 2019 at 1:33 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Fri, Oct 11, 2019 at 09:25:52PM +0200, Benjamin Tissoires wrote:
> > On Fri, Oct 11, 2019 at 8:26 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > On Fri, Oct 11, 2019 at 04:52:04PM +0200, Benjamin Tissoires wrote:
> > > > Hi Andrey,
> > > >
> > > > On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > > > >
> > > > > To simplify resource management in commit that follows as well as to
> > > > > save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch
> > > > > driver code to use devres to manage the life-cycle of FF private data.
> > > > >
> > > > > 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: Sam Bazely <sambazley@fastmail.com>
> > > > > 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
> > > >
> > > > This patch doesn't seem to fix any error, is there a reason to send it
> > > > to stable? (besides as a dependency of the rest of the series).
> > > >
> > > > > ---
> > > > >  drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++---------------
> > > > >  1 file changed, 29 insertions(+), 24 deletions(-)
> > > > >
> > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > > > index 0179f7ed77e5..58eb928224e5 100644
> > > > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > > > @@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff)
> > > > >         struct hidpp_ff_private_data *data = ff->private;
> > > > >
> > > > >         kfree(data->effect_ids);
> > > >
> > > > Is there any reasons we can not also devm alloc data->effect_ids?
> > > >
> > > > > +       /*
> > > > > +        * Set private to NULL to prevent input_ff_destroy() from
> > > > > +        * freeing our devres allocated memory
> > > >
> > > > Ouch. There is something wrong here: input_ff_destroy() calls
> > > > kfree(ff->private), when the data has not been allocated by
> > > > input_ff_create(). This seems to lack a little bit of symmetry.
> > >
> > > Yeah, ff and ff-memless essentially take over the private data assigned
> > > to them. They were done before devm and the lifetime of the "private"
> > > data pieces was tied to the lifetime of the input device to simplify
> > > error handling and teardown.
> >
> > Yeah, that stealing of the pointer is not good :)
> > But OTOH, it helps
> >
> > >
> > > Maybe we should clean it up a bit... I'm open to suggestions.
> >
> > The problem I had when doing the review was that there is no easy way
> > to have a "devm_input_ff_create_()", because the way it's built is
> > already "devres-compatible": the destroy gets called by input core.
>
> I do not think we want devm_input_ff_create() explicitly, I think the
> fact that you can "build up" an input device by allocating it, then
> adding slots, poller, ff support, etc, and input core cleans it up is
> all good. It is just the ownership if the driver-private data block is
> not very obvious and is not compatible with allocating via devm.
>

What do you think of creating input_ff_create_with_private() which
would take a pointer to private memory and set a new
"borrowed_private" flag in struct ff_device which input_ff_destroy()
can check to determine if it needs to free ->private or not?

Thanks,
Andrey Smirnov

Re: [PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Dmitry Torokhov]

On Fri, Oct 11, 2019 at 02:02:30PM -0700, Andrey Smirnov wrote:
> On Fri, Oct 11, 2019 at 1:33 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Fri, Oct 11, 2019 at 09:25:52PM +0200, Benjamin Tissoires wrote:
> > > On Fri, Oct 11, 2019 at 8:26 PM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > On Fri, Oct 11, 2019 at 04:52:04PM +0200, Benjamin Tissoires wrote:
> > > > > Hi Andrey,
> > > > >
> > > > > On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > > > > >
> > > > > > To simplify resource management in commit that follows as well as to
> > > > > > save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch
> > > > > > driver code to use devres to manage the life-cycle of FF private data.
> > > > > >
> > > > > > 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: Sam Bazely <sambazley@fastmail.com>
> > > > > > 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
> > > > >
> > > > > This patch doesn't seem to fix any error, is there a reason to send it
> > > > > to stable? (besides as a dependency of the rest of the series).
> > > > >
> > > > > > ---
> > > > > >  drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++---------------
> > > > > >  1 file changed, 29 insertions(+), 24 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > > > > index 0179f7ed77e5..58eb928224e5 100644
> > > > > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > > > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > > > > @@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff)
> > > > > >         struct hidpp_ff_private_data *data = ff->private;
> > > > > >
> > > > > >         kfree(data->effect_ids);
> > > > >
> > > > > Is there any reasons we can not also devm alloc data->effect_ids?
> > > > >
> > > > > > +       /*
> > > > > > +        * Set private to NULL to prevent input_ff_destroy() from
> > > > > > +        * freeing our devres allocated memory
> > > > >
> > > > > Ouch. There is something wrong here: input_ff_destroy() calls
> > > > > kfree(ff->private), when the data has not been allocated by
> > > > > input_ff_create(). This seems to lack a little bit of symmetry.
> > > >
> > > > Yeah, ff and ff-memless essentially take over the private data assigned
> > > > to them. They were done before devm and the lifetime of the "private"
> > > > data pieces was tied to the lifetime of the input device to simplify
> > > > error handling and teardown.
> > >
> > > Yeah, that stealing of the pointer is not good :)
> > > But OTOH, it helps
> > >
> > > >
> > > > Maybe we should clean it up a bit... I'm open to suggestions.
> > >
> > > The problem I had when doing the review was that there is no easy way
> > > to have a "devm_input_ff_create_()", because the way it's built is
> > > already "devres-compatible": the destroy gets called by input core.
> >
> > I do not think we want devm_input_ff_create() explicitly, I think the
> > fact that you can "build up" an input device by allocating it, then
> > adding slots, poller, ff support, etc, and input core cleans it up is
> > all good. It is just the ownership if the driver-private data block is
> > not very obvious and is not compatible with allocating via devm.
> >
> 
> What do you think of creating input_ff_create_with_private() which
> would take a pointer to private memory and set a new
> "borrowed_private" flag in struct ff_device which input_ff_destroy()
> can check to determine if it needs to free ->private or not?

From my quick check we only have 3 users of ff->private in the tree:

dtor@dtor-ws:~/kernel/master (next)$ git grep -l "[^a-z]ff\->private" -- drivers/
drivers/hid/hid-logitech-hidpp.c
drivers/hid/usbhid/hid-pidff.c
drivers/input/ff-core.c
drivers/input/ff-memless.c

and I really wonder if we actually need to have this pointer and
maintain it, given that logitech driver appears to not needing it that
much...

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Dmitry Torokhov]

On Fri, Oct 11, 2019 at 02:02:30PM -0700, Andrey Smirnov wrote:
> On Fri, Oct 11, 2019 at 1:33 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Fri, Oct 11, 2019 at 09:25:52PM +0200, Benjamin Tissoires wrote:
> > > On Fri, Oct 11, 2019 at 8:26 PM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > On Fri, Oct 11, 2019 at 04:52:04PM +0200, Benjamin Tissoires wrote:
> > > > > Hi Andrey,
> > > > >
> > > > > On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > > > > >
> > > > > > To simplify resource management in commit that follows as well as to
> > > > > > save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch
> > > > > > driver code to use devres to manage the life-cycle of FF private data.
> > > > > >
> > > > > > 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: Sam Bazely <sambazley@fastmail.com>
> > > > > > 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
> > > > >
> > > > > This patch doesn't seem to fix any error, is there a reason to send it
> > > > > to stable? (besides as a dependency of the rest of the series).
> > > > >
> > > > > > ---
> > > > > >  drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++---------------
> > > > > >  1 file changed, 29 insertions(+), 24 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > > > > index 0179f7ed77e5..58eb928224e5 100644
> > > > > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > > > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > > > > @@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff)
> > > > > >         struct hidpp_ff_private_data *data = ff->private;
> > > > > >
> > > > > >         kfree(data->effect_ids);
> > > > >
> > > > > Is there any reasons we can not also devm alloc data->effect_ids?
> > > > >
> > > > > > +       /*
> > > > > > +        * Set private to NULL to prevent input_ff_destroy() from
> > > > > > +        * freeing our devres allocated memory
> > > > >
> > > > > Ouch. There is something wrong here: input_ff_destroy() calls
> > > > > kfree(ff->private), when the data has not been allocated by
> > > > > input_ff_create(). This seems to lack a little bit of symmetry.
> > > >
> > > > Yeah, ff and ff-memless essentially take over the private data assigned
> > > > to them. They were done before devm and the lifetime of the "private"
> > > > data pieces was tied to the lifetime of the input device to simplify
> > > > error handling and teardown.
> > >
> > > Yeah, that stealing of the pointer is not good :)
> > > But OTOH, it helps
> > >
> > > >
> > > > Maybe we should clean it up a bit... I'm open to suggestions.
> > >
> > > The problem I had when doing the review was that there is no easy way
> > > to have a "devm_input_ff_create_()", because the way it's built is
> > > already "devres-compatible": the destroy gets called by input core.
> >
> > I do not think we want devm_input_ff_create() explicitly, I think the
> > fact that you can "build up" an input device by allocating it, then
> > adding slots, poller, ff support, etc, and input core cleans it up is
> > all good. It is just the ownership if the driver-private data block is
> > not very obvious and is not compatible with allocating via devm.
> >
> 
> What do you think of creating input_ff_create_with_private() which
> would take a pointer to private memory and set a new
> "borrowed_private" flag in struct ff_device which input_ff_destroy()
> can check to determine if it needs to free ->private or not?

>From my quick check we only have 3 users of ff->private in the tree:

dtor@dtor-ws:~/kernel/master (next)$ git grep -l "[^a-z]ff\->private" -- drivers/
drivers/hid/hid-logitech-hidpp.c
drivers/hid/usbhid/hid-pidff.c
drivers/input/ff-core.c
drivers/input/ff-memless.c

and I really wonder if we actually need to have this pointer and
maintain it, given that logitech driver appears to not needing it that
much...

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Dmitry Torokhov]

On Fri, Oct 11, 2019 at 01:35:09PM -0700, Dmitry Torokhov wrote:
> On Fri, Oct 11, 2019 at 01:33:03PM -0700, Dmitry Torokhov wrote:
> > On Fri, Oct 11, 2019 at 09:25:52PM +0200, Benjamin Tissoires wrote:
> > > On Fri, Oct 11, 2019 at 8:26 PM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > On Fri, Oct 11, 2019 at 04:52:04PM +0200, Benjamin Tissoires wrote:
> > > > > Hi Andrey,
> > > > >
> > > > > On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > > > > >
> > > > > > To simplify resource management in commit that follows as well as to
> > > > > > save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch
> > > > > > driver code to use devres to manage the life-cycle of FF private data.
> > > > > >
> > > > > > 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: Sam Bazely <sambazley@fastmail.com>
> > > > > > 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
> > > > >
> > > > > This patch doesn't seem to fix any error, is there a reason to send it
> > > > > to stable? (besides as a dependency of the rest of the series).
> > > > >
> > > > > > ---
> > > > > >  drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++---------------
> > > > > >  1 file changed, 29 insertions(+), 24 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > > > > index 0179f7ed77e5..58eb928224e5 100644
> > > > > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > > > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > > > > @@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff)
> > > > > >         struct hidpp_ff_private_data *data = ff->private;
> > > > > >
> > > > > >         kfree(data->effect_ids);
> > > > >
> > > > > Is there any reasons we can not also devm alloc data->effect_ids?
> > > > >
> > > > > > +       /*
> > > > > > +        * Set private to NULL to prevent input_ff_destroy() from
> > > > > > +        * freeing our devres allocated memory
> > > > >
> > > > > Ouch. There is something wrong here: input_ff_destroy() calls
> > > > > kfree(ff->private), when the data has not been allocated by
> > > > > input_ff_create(). This seems to lack a little bit of symmetry.
> > > >
> > > > Yeah, ff and ff-memless essentially take over the private data assigned
> > > > to them. They were done before devm and the lifetime of the "private"
> > > > data pieces was tied to the lifetime of the input device to simplify
> > > > error handling and teardown.
> > > 
> > > Yeah, that stealing of the pointer is not good :)
> > > But OTOH, it helps
> > > 
> > > >
> > > > Maybe we should clean it up a bit... I'm open to suggestions.
> > > 
> > > The problem I had when doing the review was that there is no easy way
> > > to have a "devm_input_ff_create_()", because the way it's built is
> > > already "devres-compatible": the destroy gets called by input core.
> > 
> > I do not think we want devm_input_ff_create() explicitly, I think the
> > fact that you can "build up" an input device by allocating it, then
> > adding slots, poller, ff support, etc, and input core cleans it up is
> > all good. It is just the ownership if the driver-private data block is
> > not very obvious and is not compatible with allocating via devm.
> > 
> > > 
> > > So I don't have a good answer to simplify in a transparent manner
> > > without breaking the API.
> > > 
> > > >
> > > > In this case maybe best way is to get rid of hidpp_ff_destroy() and not
> > > > set ff->private and rely on devm to free the buffers. One can get to
> > > > device private data from ff methods via input_get_drvdata() since they
> > > > all (except destroy) are passed input device pointer.
> > > 
> > > Sounds like a good idea. However, it seems there might be a race when
> > > removing the workqueue:
> > > the workqueue gets deleted in hidpp_remove, when the input node will
> > > be freed by devres, so after the call of hidpp_remove.
> > 
> > Yeah, well, that is a common issue with mixing devm and normal resources
> > (and workqueue here is that "normal" resource), and we should either:
> > 
> > - not use devm
> > - use devm_add_action_or_reset() to work in custom actions that work
> >   freeing of non-managed resources into devm flow.
> 
> Actually, there is a door #3: use system workqueue. After all the work
> that Tejun done on workqueues it is very rare that one actually needs
> a dedicated workqueue (as works usually execute on one if the system
> worker threads that are shared with other workqueues anyway).

And additional note about devm:

I think all HID input drivers that are using devm in probe, but do not
have proper remove() function (and maybe even some with remove) are
broken: hid_device_remove() calls hid_hw_stop() which potentially will
shut off the transport. This happens before devm starts unwinding, so
we still can be trying to communicate with the device in question, but
the transport is gone.

io_started/driver_input_lock is broken on removal as well as we release
the lock when driver may very well be still talking to the device in
devm teardown actions.

I think we have similar kind of issues in other buses as well (i2c, spi,
etc). For example, in i2c we remove the device from power domain before
we actually complete devm unwinding.

Thanks.

-- 
Dmitry

Re: [PATCH 3/3] HID: logitech-hidpp: add G920 device validation quirk

[Benjamin Tissoires]

On Fri, Oct 11, 2019 at 9:39 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> On Fri, Oct 11, 2019 at 7:56 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > On Mon, Oct 7, 2019 at 7:13 AM 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.
> > >
> > > Modify hidpp_validate_device() to check only REPORT_ID_HIDPP_LONG with
> > > optional=false on G920 to fix this.
> > >
> > > 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: Sam Bazely <sambazley@fastmail.com>
> > > 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
> > > ---
> > >  drivers/hid/hid-logitech-hidpp.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > index cadf36d6c6f3..f415bf398e17 100644
> > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > @@ -3511,6 +3511,12 @@ static bool hidpp_validate_report(struct hid_device *hdev, int id,
> > >
> > >  static bool hidpp_validate_device(struct hid_device *hdev)
> > >  {
> > > +       struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> > > +
> > > +       if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)
> > > +               return hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
> > > +                                            HIDPP_REPORT_SHORT_LENGTH, false);
> > > +
> >
> > with https://patchwork.kernel.org/patch/11184749/ we also have a need
> > for such a trick for BLE mice.
> >
> > I wonder if we should not have a more common way of validating the devices
> >
>
> What about just checking for:
>
> hidpp_validate_report(REPORT_ID_HIDPP_SHORT,
>                                     HIDPP_REPORT_SHORT_LENGTH, true) ||
> hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
>                                     HIDPP_REPORT_LONG_LENGTH, true);
>
> and probably dropping the "optional" argument for
> hidpp_validate_report()? Original code allows there to be devices
> supporting shorts reports only, but it seems that devices that support
> only long reports are legitimate too, so maybe the only "invalid"
> combination is if both are invalid length or missing?

Well, the problem is we also want to detect 2 things:
- devices that do not have any of the HID++ collections, and handle
them as generic ones (the second mouse/keyboard collection in the
gaming mice should still be exported by the driver, or this will kill
the macros / rebinding capabilities
- malicious devices that pretends to have a HID++ collection but want
to trigger a buffer overflow by having a shorter than expected report
length

Point 2 above should still be fine, but point 1 is why we have the
enforcement of the HID++ short report in the first place.

Cheers,
Benjamin

>
> Thanks,
> Andrey Smirnov

Re: [PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Benjamin Tissoires]

On Fri, Oct 11, 2019 at 10:33 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Fri, Oct 11, 2019 at 09:25:52PM +0200, Benjamin Tissoires wrote:
> > On Fri, Oct 11, 2019 at 8:26 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > On Fri, Oct 11, 2019 at 04:52:04PM +0200, Benjamin Tissoires wrote:
> > > > Hi Andrey,
> > > >
> > > > On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > > > >
> > > > > To simplify resource management in commit that follows as well as to
> > > > > save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch
> > > > > driver code to use devres to manage the life-cycle of FF private data.
> > > > >
> > > > > 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: Sam Bazely <sambazley@fastmail.com>
> > > > > 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
> > > >
> > > > This patch doesn't seem to fix any error, is there a reason to send it
> > > > to stable? (besides as a dependency of the rest of the series).
> > > >
> > > > > ---
> > > > >  drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++---------------
> > > > >  1 file changed, 29 insertions(+), 24 deletions(-)
> > > > >
> > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > > > index 0179f7ed77e5..58eb928224e5 100644
> > > > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > > > @@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff)
> > > > >         struct hidpp_ff_private_data *data = ff->private;
> > > > >
> > > > >         kfree(data->effect_ids);
> > > >
> > > > Is there any reasons we can not also devm alloc data->effect_ids?
> > > >
> > > > > +       /*
> > > > > +        * Set private to NULL to prevent input_ff_destroy() from
> > > > > +        * freeing our devres allocated memory
> > > >
> > > > Ouch. There is something wrong here: input_ff_destroy() calls
> > > > kfree(ff->private), when the data has not been allocated by
> > > > input_ff_create(). This seems to lack a little bit of symmetry.
> > >
> > > Yeah, ff and ff-memless essentially take over the private data assigned
> > > to them. They were done before devm and the lifetime of the "private"
> > > data pieces was tied to the lifetime of the input device to simplify
> > > error handling and teardown.
> >
> > Yeah, that stealing of the pointer is not good :)
> > But OTOH, it helps
> >
> > >
> > > Maybe we should clean it up a bit... I'm open to suggestions.
> >
> > The problem I had when doing the review was that there is no easy way
> > to have a "devm_input_ff_create_()", because the way it's built is
> > already "devres-compatible": the destroy gets called by input core.
>
> I do not think we want devm_input_ff_create() explicitly, I think the
> fact that you can "build up" an input device by allocating it, then
> adding slots, poller, ff support, etc, and input core cleans it up is
> all good. It is just the ownership if the driver-private data block is
> not very obvious and is not compatible with allocating via devm.

Yep, that's what I meant: input_ff_create() already handles its
cleanup, so there is no point in devm_input_ff_create() as the input
core should clean it up for us.

Cheers,
Benjamin

>
> >
> > So I don't have a good answer to simplify in a transparent manner
> > without breaking the API.
> >
> > >
> > > In this case maybe best way is to get rid of hidpp_ff_destroy() and not
> > > set ff->private and rely on devm to free the buffers. One can get to
> > > device private data from ff methods via input_get_drvdata() since they
> > > all (except destroy) are passed input device pointer.
> >
> > Sounds like a good idea. However, it seems there might be a race when
> > removing the workqueue:
> > the workqueue gets deleted in hidpp_remove, when the input node will
> > be freed by devres, so after the call of hidpp_remove.
>
> Yeah, well, that is a common issue with mixing devm and normal resources
> (and workqueue here is that "normal" resource), and we should either:
>
> - not use devm
> - use devm_add_action_or_reset() to work in custom actions that work
>   freeing of non-managed resources into devm flow.
>
> Thanks.
>
> --
> Dmitry

Re: [PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Benjamin Tissoires]

On Fri, Oct 11, 2019 at 11:34 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Fri, Oct 11, 2019 at 01:35:09PM -0700, Dmitry Torokhov wrote:
> > On Fri, Oct 11, 2019 at 01:33:03PM -0700, Dmitry Torokhov wrote:
> > > On Fri, Oct 11, 2019 at 09:25:52PM +0200, Benjamin Tissoires wrote:
> > > > On Fri, Oct 11, 2019 at 8:26 PM Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > > >
> > > > > On Fri, Oct 11, 2019 at 04:52:04PM +0200, Benjamin Tissoires wrote:
> > > > > > Hi Andrey,
> > > > > >
> > > > > > On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > > > > > >
> > > > > > > To simplify resource management in commit that follows as well as to
> > > > > > > save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch
> > > > > > > driver code to use devres to manage the life-cycle of FF private data.
> > > > > > >
> > > > > > > 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: Sam Bazely <sambazley@fastmail.com>
> > > > > > > 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
> > > > > >
> > > > > > This patch doesn't seem to fix any error, is there a reason to send it
> > > > > > to stable? (besides as a dependency of the rest of the series).
> > > > > >
> > > > > > > ---
> > > > > > >  drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++---------------
> > > > > > >  1 file changed, 29 insertions(+), 24 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > > > > > index 0179f7ed77e5..58eb928224e5 100644
> > > > > > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > > > > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > > > > > @@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff)
> > > > > > >         struct hidpp_ff_private_data *data = ff->private;
> > > > > > >
> > > > > > >         kfree(data->effect_ids);
> > > > > >
> > > > > > Is there any reasons we can not also devm alloc data->effect_ids?
> > > > > >
> > > > > > > +       /*
> > > > > > > +        * Set private to NULL to prevent input_ff_destroy() from
> > > > > > > +        * freeing our devres allocated memory
> > > > > >
> > > > > > Ouch. There is something wrong here: input_ff_destroy() calls
> > > > > > kfree(ff->private), when the data has not been allocated by
> > > > > > input_ff_create(). This seems to lack a little bit of symmetry.
> > > > >
> > > > > Yeah, ff and ff-memless essentially take over the private data assigned
> > > > > to them. They were done before devm and the lifetime of the "private"
> > > > > data pieces was tied to the lifetime of the input device to simplify
> > > > > error handling and teardown.
> > > >
> > > > Yeah, that stealing of the pointer is not good :)
> > > > But OTOH, it helps
> > > >
> > > > >
> > > > > Maybe we should clean it up a bit... I'm open to suggestions.
> > > >
> > > > The problem I had when doing the review was that there is no easy way
> > > > to have a "devm_input_ff_create_()", because the way it's built is
> > > > already "devres-compatible": the destroy gets called by input core.
> > >
> > > I do not think we want devm_input_ff_create() explicitly, I think the
> > > fact that you can "build up" an input device by allocating it, then
> > > adding slots, poller, ff support, etc, and input core cleans it up is
> > > all good. It is just the ownership if the driver-private data block is
> > > not very obvious and is not compatible with allocating via devm.
> > >
> > > >
> > > > So I don't have a good answer to simplify in a transparent manner
> > > > without breaking the API.
> > > >
> > > > >
> > > > > In this case maybe best way is to get rid of hidpp_ff_destroy() and not
> > > > > set ff->private and rely on devm to free the buffers. One can get to
> > > > > device private data from ff methods via input_get_drvdata() since they
> > > > > all (except destroy) are passed input device pointer.
> > > >
> > > > Sounds like a good idea. However, it seems there might be a race when
> > > > removing the workqueue:
> > > > the workqueue gets deleted in hidpp_remove, when the input node will
> > > > be freed by devres, so after the call of hidpp_remove.
> > >
> > > Yeah, well, that is a common issue with mixing devm and normal resources
> > > (and workqueue here is that "normal" resource), and we should either:
> > >
> > > - not use devm
> > > - use devm_add_action_or_reset() to work in custom actions that work
> > >   freeing of non-managed resources into devm flow.
> >
> > Actually, there is a door #3: use system workqueue. After all the work
> > that Tejun done on workqueues it is very rare that one actually needs
> > a dedicated workqueue (as works usually execute on one if the system
> > worker threads that are shared with other workqueues anyway).
>
> And additional note about devm:
>
> I think all HID input drivers that are using devm in probe, but do not
> have proper remove() function (and maybe even some with remove) are
> broken: hid_device_remove() calls hid_hw_stop() which potentially will
> shut off the transport. This happens before devm starts unwinding, so
> we still can be trying to communicate with the device in question, but
> the transport is gone.

Well, that is by design. A driver is supposed to call hid_hw_start()
at the very end of its .probe(). And the supposed rule is that in the
specific .remove(), you are to call first hid_hw_stop() to stop the
transport layer underneath. That also means that in the HID subsystem,
at least, you are not supposed to talk to the device during the devm
teardown of the allocated data.

If you really need to communicate with the device during tear down,
then you are supposed to write your own .remove, in which you control
where the hid_hw_stop() happens.

We might have overlooked one or two, but I think we are on a good basis for now.

>
> io_started/driver_input_lock is broken on removal as well as we release
> the lock when driver may very well be still talking to the device in
> devm teardown actions.

Again, this is not supposed to happen. Once hid_hw_stop() is called,
we do not have access to the transport, so drivers can't talk to the
device. So releasing/clearing the locks is supposed to be safe now.

>
> I think we have similar kind of issues in other buses as well (i2c, spi,
> etc). For example, in i2c we remove the device from power domain before
> we actually complete devm unwinding.
>

I agree that this looks bad.

I would need to have a better look at it on Monday. Time to go on week
end (this jet lag doesn't help me to go to sleep...)

Cheers,
Benjamin

Re: [PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Dmitry Torokhov]

On Sat, Oct 12, 2019 at 12:48:42AM +0200, Benjamin Tissoires wrote:
> On Fri, Oct 11, 2019 at 11:34 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Fri, Oct 11, 2019 at 01:35:09PM -0700, Dmitry Torokhov wrote:
> > > On Fri, Oct 11, 2019 at 01:33:03PM -0700, Dmitry Torokhov wrote:
> > > > On Fri, Oct 11, 2019 at 09:25:52PM +0200, Benjamin Tissoires wrote:
> > > > > On Fri, Oct 11, 2019 at 8:26 PM Dmitry Torokhov
> > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > >
> > > > > > On Fri, Oct 11, 2019 at 04:52:04PM +0200, Benjamin Tissoires wrote:
> > > > > > > Hi Andrey,
> > > > > > >
> > > > > > > On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > > > > > > >
> > > > > > > > To simplify resource management in commit that follows as well as to
> > > > > > > > save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch
> > > > > > > > driver code to use devres to manage the life-cycle of FF private data.
> > > > > > > >
> > > > > > > > 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: Sam Bazely <sambazley@fastmail.com>
> > > > > > > > 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
> > > > > > >
> > > > > > > This patch doesn't seem to fix any error, is there a reason to send it
> > > > > > > to stable? (besides as a dependency of the rest of the series).
> > > > > > >
> > > > > > > > ---
> > > > > > > >  drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++---------------
> > > > > > > >  1 file changed, 29 insertions(+), 24 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > > > > > > index 0179f7ed77e5..58eb928224e5 100644
> > > > > > > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > > > > > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > > > > > > @@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff)
> > > > > > > >         struct hidpp_ff_private_data *data = ff->private;
> > > > > > > >
> > > > > > > >         kfree(data->effect_ids);
> > > > > > >
> > > > > > > Is there any reasons we can not also devm alloc data->effect_ids?
> > > > > > >
> > > > > > > > +       /*
> > > > > > > > +        * Set private to NULL to prevent input_ff_destroy() from
> > > > > > > > +        * freeing our devres allocated memory
> > > > > > >
> > > > > > > Ouch. There is something wrong here: input_ff_destroy() calls
> > > > > > > kfree(ff->private), when the data has not been allocated by
> > > > > > > input_ff_create(). This seems to lack a little bit of symmetry.
> > > > > >
> > > > > > Yeah, ff and ff-memless essentially take over the private data assigned
> > > > > > to them. They were done before devm and the lifetime of the "private"
> > > > > > data pieces was tied to the lifetime of the input device to simplify
> > > > > > error handling and teardown.
> > > > >
> > > > > Yeah, that stealing of the pointer is not good :)
> > > > > But OTOH, it helps
> > > > >
> > > > > >
> > > > > > Maybe we should clean it up a bit... I'm open to suggestions.
> > > > >
> > > > > The problem I had when doing the review was that there is no easy way
> > > > > to have a "devm_input_ff_create_()", because the way it's built is
> > > > > already "devres-compatible": the destroy gets called by input core.
> > > >
> > > > I do not think we want devm_input_ff_create() explicitly, I think the
> > > > fact that you can "build up" an input device by allocating it, then
> > > > adding slots, poller, ff support, etc, and input core cleans it up is
> > > > all good. It is just the ownership if the driver-private data block is
> > > > not very obvious and is not compatible with allocating via devm.
> > > >
> > > > >
> > > > > So I don't have a good answer to simplify in a transparent manner
> > > > > without breaking the API.
> > > > >
> > > > > >
> > > > > > In this case maybe best way is to get rid of hidpp_ff_destroy() and not
> > > > > > set ff->private and rely on devm to free the buffers. One can get to
> > > > > > device private data from ff methods via input_get_drvdata() since they
> > > > > > all (except destroy) are passed input device pointer.
> > > > >
> > > > > Sounds like a good idea. However, it seems there might be a race when
> > > > > removing the workqueue:
> > > > > the workqueue gets deleted in hidpp_remove, when the input node will
> > > > > be freed by devres, so after the call of hidpp_remove.
> > > >
> > > > Yeah, well, that is a common issue with mixing devm and normal resources
> > > > (and workqueue here is that "normal" resource), and we should either:
> > > >
> > > > - not use devm
> > > > - use devm_add_action_or_reset() to work in custom actions that work
> > > >   freeing of non-managed resources into devm flow.
> > >
> > > Actually, there is a door #3: use system workqueue. After all the work
> > > that Tejun done on workqueues it is very rare that one actually needs
> > > a dedicated workqueue (as works usually execute on one if the system
> > > worker threads that are shared with other workqueues anyway).
> >
> > And additional note about devm:
> >
> > I think all HID input drivers that are using devm in probe, but do not
> > have proper remove() function (and maybe even some with remove) are
> > broken: hid_device_remove() calls hid_hw_stop() which potentially will
> > shut off the transport. This happens before devm starts unwinding, so
> > we still can be trying to communicate with the device in question, but
> > the transport is gone.
> 
> Well, that is by design. A driver is supposed to call hid_hw_start()
> at the very end of its .probe(). And the supposed rule is that in the
> specific .remove(), you are to call first hid_hw_stop() to stop the
> transport layer underneath. That also means that in the HID subsystem,
> at least, you are not supposed to talk to the device during the devm
> teardown of the allocated data.
> 
> If you really need to communicate with the device during tear down,
> then you are supposed to write your own .remove, in which you control
> where the hid_hw_stop() happens.
> 
> We might have overlooked one or two, but I think we are on a good basis for now.

You have to be _very_ careful there. For example, we can take a look at
hid-elan.c. If you notice, it uses devm_led_classdev_register() to
create "mute" led and it needs to talk over HID to control it;s
brightness/state. So the driver has custom remove() and calls
hid_hw_stop() from it. But the LED will be unregistered much later (in
the depth of the driver core) so users of LED subsystem are free to send
requests through and the driver will try to talk to the device even
after hid_hw_stop() is called and the io_started/driver_input_lock is
reset/released.

I am sure there are more such examples.

> 
> >
> > io_started/driver_input_lock is broken on removal as well as we release
> > the lock when driver may very well be still talking to the device in
> > devm teardown actions.
> 
> Again, this is not supposed to happen. Once hid_hw_stop() is called,
> we do not have access to the transport, so drivers can't talk to the
> device. So releasing/clearing the locks is supposed to be safe now.

Except that it is hard to enforce once you throw in devm.

> 
> >
> > I think we have similar kind of issues in other buses as well (i2c, spi,
> > etc). For example, in i2c we remove the device from power domain before
> > we actually complete devm unwinding.
> >
> 
> I agree that this looks bad.
> 
> I would need to have a better look at it on Monday. Time to go on week
> end (this jet lag doesn't help me to go to sleep...)

I wonder if every bus should open a new devm group for device and
manually release it after calling ->remove(). That would ensure that all
devm resouces allocated by drivers will be freed before we start
executing bus-specific code.

Thanks.

-- 
Dmitry

Re: [PATCH 3/3] HID: logitech-hidpp: add G920 device validation quirk

[Andrey Smirnov]

On Fri, Oct 11, 2019 at 3:33 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Fri, Oct 11, 2019 at 9:39 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> >
> > On Fri, Oct 11, 2019 at 7:56 AM Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > >
> > > On Mon, Oct 7, 2019 at 7:13 AM 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.
> > > >
> > > > Modify hidpp_validate_device() to check only REPORT_ID_HIDPP_LONG with
> > > > optional=false on G920 to fix this.
> > > >
> > > > 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: Sam Bazely <sambazley@fastmail.com>
> > > > 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
> > > > ---
> > > >  drivers/hid/hid-logitech-hidpp.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > > index cadf36d6c6f3..f415bf398e17 100644
> > > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > > @@ -3511,6 +3511,12 @@ static bool hidpp_validate_report(struct hid_device *hdev, int id,
> > > >
> > > >  static bool hidpp_validate_device(struct hid_device *hdev)
> > > >  {
> > > > +       struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> > > > +
> > > > +       if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)
> > > > +               return hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
> > > > +                                            HIDPP_REPORT_SHORT_LENGTH, false);
> > > > +
> > >
> > > with https://patchwork.kernel.org/patch/11184749/ we also have a need
> > > for such a trick for BLE mice.
> > >
> > > I wonder if we should not have a more common way of validating the devices
> > >
> >
> > What about just checking for:
> >
> > hidpp_validate_report(REPORT_ID_HIDPP_SHORT,
> >                                     HIDPP_REPORT_SHORT_LENGTH, true) ||
> > hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
> >                                     HIDPP_REPORT_LONG_LENGTH, true);
> >
> > and probably dropping the "optional" argument for
> > hidpp_validate_report()? Original code allows there to be devices
> > supporting shorts reports only, but it seems that devices that support
> > only long reports are legitimate too, so maybe the only "invalid"
> > combination is if both are invalid length or missing?
>
> Well, the problem is we also want to detect 2 things:
> - devices that do not have any of the HID++ collections, and handle
> them as generic ones (the second mouse/keyboard collection in the
> gaming mice should still be exported by the driver, or this will kill
> the macros / rebinding capabilities
> - malicious devices that pretends to have a HID++ collection but want
> to trigger a buffer overflow by having a shorter than expected report
> length
>
> Point 2 above should still be fine, but point 1 is why we have the
> enforcement of the HID++ short report in the first place.
>

It sounds like the result of hidpp_validate_report() can't really be
contained in a bool. If we modify it to return -EINVAL for bogus
report length, -ENOTSUPP if report ID is not supported and 0 if
everything is valid we should be able to capture all valid permutation
by checking for with

int id_short = hidpp_validate_report(ID_SHORT);
int id_long  = hidpp_validate_report(ID_LONG);

return (!id_short && !id_long) || (id_short == -ENOTSUPP && !id_long)
|| (id_long == -ENOTSUPP && !id_short)

no?

Thanks,
Andrey Smirnov

Re: [PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Benjamin Tissoires]

On Sat, Oct 12, 2019 at 1:24 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Sat, Oct 12, 2019 at 12:48:42AM +0200, Benjamin Tissoires wrote:
> > On Fri, Oct 11, 2019 at 11:34 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > On Fri, Oct 11, 2019 at 01:35:09PM -0700, Dmitry Torokhov wrote:
> > > > On Fri, Oct 11, 2019 at 01:33:03PM -0700, Dmitry Torokhov wrote:
> > > > > On Fri, Oct 11, 2019 at 09:25:52PM +0200, Benjamin Tissoires wrote:
> > > > > > On Fri, Oct 11, 2019 at 8:26 PM Dmitry Torokhov
> > > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > > >
> > > > > > > On Fri, Oct 11, 2019 at 04:52:04PM +0200, Benjamin Tissoires wrote:
> > > > > > > > Hi Andrey,
> > > > > > > >
> > > > > > > > On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > To simplify resource management in commit that follows as well as to
> > > > > > > > > save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch
> > > > > > > > > driver code to use devres to manage the life-cycle of FF private data.
> > > > > > > > >
> > > > > > > > > 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: Sam Bazely <sambazley@fastmail.com>
> > > > > > > > > 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
> > > > > > > >
> > > > > > > > This patch doesn't seem to fix any error, is there a reason to send it
> > > > > > > > to stable? (besides as a dependency of the rest of the series).
> > > > > > > >
> > > > > > > > > ---
> > > > > > > > >  drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++---------------
> > > > > > > > >  1 file changed, 29 insertions(+), 24 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > > > > > > > index 0179f7ed77e5..58eb928224e5 100644
> > > > > > > > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > > > > > > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > > > > > > > @@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff)
> > > > > > > > >         struct hidpp_ff_private_data *data = ff->private;
> > > > > > > > >
> > > > > > > > >         kfree(data->effect_ids);
> > > > > > > >
> > > > > > > > Is there any reasons we can not also devm alloc data->effect_ids?
> > > > > > > >
> > > > > > > > > +       /*
> > > > > > > > > +        * Set private to NULL to prevent input_ff_destroy() from
> > > > > > > > > +        * freeing our devres allocated memory
> > > > > > > >
> > > > > > > > Ouch. There is something wrong here: input_ff_destroy() calls
> > > > > > > > kfree(ff->private), when the data has not been allocated by
> > > > > > > > input_ff_create(). This seems to lack a little bit of symmetry.
> > > > > > >
> > > > > > > Yeah, ff and ff-memless essentially take over the private data assigned
> > > > > > > to them. They were done before devm and the lifetime of the "private"
> > > > > > > data pieces was tied to the lifetime of the input device to simplify
> > > > > > > error handling and teardown.
> > > > > >
> > > > > > Yeah, that stealing of the pointer is not good :)
> > > > > > But OTOH, it helps
> > > > > >
> > > > > > >
> > > > > > > Maybe we should clean it up a bit... I'm open to suggestions.
> > > > > >
> > > > > > The problem I had when doing the review was that there is no easy way
> > > > > > to have a "devm_input_ff_create_()", because the way it's built is
> > > > > > already "devres-compatible": the destroy gets called by input core.
> > > > >
> > > > > I do not think we want devm_input_ff_create() explicitly, I think the
> > > > > fact that you can "build up" an input device by allocating it, then
> > > > > adding slots, poller, ff support, etc, and input core cleans it up is
> > > > > all good. It is just the ownership if the driver-private data block is
> > > > > not very obvious and is not compatible with allocating via devm.
> > > > >
> > > > > >
> > > > > > So I don't have a good answer to simplify in a transparent manner
> > > > > > without breaking the API.
> > > > > >
> > > > > > >
> > > > > > > In this case maybe best way is to get rid of hidpp_ff_destroy() and not
> > > > > > > set ff->private and rely on devm to free the buffers. One can get to
> > > > > > > device private data from ff methods via input_get_drvdata() since they
> > > > > > > all (except destroy) are passed input device pointer.
> > > > > >
> > > > > > Sounds like a good idea. However, it seems there might be a race when
> > > > > > removing the workqueue:
> > > > > > the workqueue gets deleted in hidpp_remove, when the input node will
> > > > > > be freed by devres, so after the call of hidpp_remove.
> > > > >
> > > > > Yeah, well, that is a common issue with mixing devm and normal resources
> > > > > (and workqueue here is that "normal" resource), and we should either:
> > > > >
> > > > > - not use devm
> > > > > - use devm_add_action_or_reset() to work in custom actions that work
> > > > >   freeing of non-managed resources into devm flow.
> > > >
> > > > Actually, there is a door #3: use system workqueue. After all the work
> > > > that Tejun done on workqueues it is very rare that one actually needs
> > > > a dedicated workqueue (as works usually execute on one if the system
> > > > worker threads that are shared with other workqueues anyway).
> > >
> > > And additional note about devm:
> > >
> > > I think all HID input drivers that are using devm in probe, but do not
> > > have proper remove() function (and maybe even some with remove) are
> > > broken: hid_device_remove() calls hid_hw_stop() which potentially will
> > > shut off the transport. This happens before devm starts unwinding, so
> > > we still can be trying to communicate with the device in question, but
> > > the transport is gone.
> >
> > Well, that is by design. A driver is supposed to call hid_hw_start()
> > at the very end of its .probe(). And the supposed rule is that in the
> > specific .remove(), you are to call first hid_hw_stop() to stop the
> > transport layer underneath. That also means that in the HID subsystem,
> > at least, you are not supposed to talk to the device during the devm
> > teardown of the allocated data.
> >
> > If you really need to communicate with the device during tear down,
> > then you are supposed to write your own .remove, in which you control
> > where the hid_hw_stop() happens.
> >
> > We might have overlooked one or two, but I think we are on a good basis for now.
>
> You have to be _very_ careful there. For example, we can take a look at
> hid-elan.c. If you notice, it uses devm_led_classdev_register() to
> create "mute" led and it needs to talk over HID to control it;s
> brightness/state. So the driver has custom remove() and calls
> hid_hw_stop() from it. But the LED will be unregistered much later (in
> the depth of the driver core) so users of LED subsystem are free to send
> requests through and the driver will try to talk to the device even
> after hid_hw_stop() is called and the io_started/driver_input_lock is
> reset/released.
>
> I am sure there are more such examples.

Yep, this is problematic. There is no guard in
elan_mute_led_set_brigtness() which tells us that the bus has been
stopped, so we likely have an issue here.

Note that a .remove() that just calls hid_hw_stop() should be removed,
as hid core can do it for us.

>
> >
> > >
> > > io_started/driver_input_lock is broken on removal as well as we release
> > > the lock when driver may very well be still talking to the device in
> > > devm teardown actions.
> >
> > Again, this is not supposed to happen. Once hid_hw_stop() is called,
> > we do not have access to the transport, so drivers can't talk to the
> > device. So releasing/clearing the locks is supposed to be safe now.
>
> Except that it is hard to enforce once you throw in devm.
>
> >
> > >
> > > I think we have similar kind of issues in other buses as well (i2c, spi,
> > > etc). For example, in i2c we remove the device from power domain before
> > > we actually complete devm unwinding.
> > >
> >
> > I agree that this looks bad.
> >
> > I would need to have a better look at it on Monday. Time to go on week
> > end (this jet lag doesn't help me to go to sleep...)
>
> I wonder if every bus should open a new devm group for device and
> manually release it after calling ->remove(). That would ensure that all
> devm resouces allocated by drivers will be freed before we start
> executing bus-specific code.
>

That would be indeed useful. There is no reasons I can think of for a
resource to be created during the .probe() of a device that should
stick around after its .remove().

In the Elan case above, it won't solve all of the issues, as there
will still be a tiny window where the resource will get access to the
bus when it has been stopped.
Maybe adding an other group when we call hid_hw_start() that will be
freed by hid_hw_stop() before the actual stop to the bus could come to
the rescue....

Cheers,
Benjamin

Re: [PATCH 3/3] HID: logitech-hidpp: add G920 device validation quirk

[Benjamin Tissoires]

On Sat, Oct 12, 2019 at 1:33 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> On Fri, Oct 11, 2019 at 3:33 PM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > On Fri, Oct 11, 2019 at 9:39 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > >
> > > On Fri, Oct 11, 2019 at 7:56 AM Benjamin Tissoires
> > > <benjamin.tissoires@redhat.com> wrote:
> > > >
> > > > On Mon, Oct 7, 2019 at 7:13 AM 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.
> > > > >
> > > > > Modify hidpp_validate_device() to check only REPORT_ID_HIDPP_LONG with
> > > > > optional=false on G920 to fix this.
> > > > >
> > > > > 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: Sam Bazely <sambazley@fastmail.com>
> > > > > 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
> > > > > ---
> > > > >  drivers/hid/hid-logitech-hidpp.c | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > > > index cadf36d6c6f3..f415bf398e17 100644
> > > > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > > > @@ -3511,6 +3511,12 @@ static bool hidpp_validate_report(struct hid_device *hdev, int id,
> > > > >
> > > > >  static bool hidpp_validate_device(struct hid_device *hdev)
> > > > >  {
> > > > > +       struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> > > > > +
> > > > > +       if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)
> > > > > +               return hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
> > > > > +                                            HIDPP_REPORT_SHORT_LENGTH, false);
> > > > > +
> > > >
> > > > with https://patchwork.kernel.org/patch/11184749/ we also have a need
> > > > for such a trick for BLE mice.
> > > >
> > > > I wonder if we should not have a more common way of validating the devices
> > > >
> > >
> > > What about just checking for:
> > >
> > > hidpp_validate_report(REPORT_ID_HIDPP_SHORT,
> > >                                     HIDPP_REPORT_SHORT_LENGTH, true) ||
> > > hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
> > >                                     HIDPP_REPORT_LONG_LENGTH, true);
> > >
> > > and probably dropping the "optional" argument for
> > > hidpp_validate_report()? Original code allows there to be devices
> > > supporting shorts reports only, but it seems that devices that support
> > > only long reports are legitimate too, so maybe the only "invalid"
> > > combination is if both are invalid length or missing?
> >
> > Well, the problem is we also want to detect 2 things:
> > - devices that do not have any of the HID++ collections, and handle
> > them as generic ones (the second mouse/keyboard collection in the
> > gaming mice should still be exported by the driver, or this will kill
> > the macros / rebinding capabilities
> > - malicious devices that pretends to have a HID++ collection but want
> > to trigger a buffer overflow by having a shorter than expected report
> > length
> >
> > Point 2 above should still be fine, but point 1 is why we have the
> > enforcement of the HID++ short report in the first place.
> >
>
> It sounds like the result of hidpp_validate_report() can't really be
> contained in a bool. If we modify it to return -EINVAL for bogus
> report length, -ENOTSUPP if report ID is not supported and 0 if
> everything is valid we should be able to capture all valid permutation
> by checking for with
>
> int id_short = hidpp_validate_report(ID_SHORT);
> int id_long  = hidpp_validate_report(ID_LONG);
>
> return (!id_short && !id_long) || (id_short == -ENOTSUPP && !id_long)
> || (id_long == -ENOTSUPP && !id_short)
>
> no?

Sounds like a good idea.

There is a few changes I'd like to do on this proposal:
- ideally, we should also check on very long HID++ reports, but as
d71b18f7c79993 ("HID: logitech-hidpp: do not hardcode very long report
length") mentioned, we should probably ensure that those very long
reports are at least bigger than HIDPP_REPORT_LONG_LENGTH and shorter
than HIDPP_REPORT_VERY_LONG_MAX_LENGTH.

- the boolean operation has a risk of being quite hard to follow if we
now have 3 IDs to check. So we would need a few comments for each
operation to explain which is which.

- maybe we should exit earlier if any of the id_short, id_long or
id_very_long is -EINVAL, as this should be detected has a hard failure

- the choice of the return codes should be changed:
  * ENOTSUPP is defined for the NFSv3 protocol, and I think ENOENT (no
such file or directory) or ENXIO (No such device or address) might be
better
  * EINVAL is for an invalid argument of a function, and here the
argument is correct, but the device is not. Maybe EBADR (Invalid
request descriptor)?

If we can get this patch in stable soon, this would also help of the
BLE series Mazin is currently working on.

Cheers,
Benjamin

Re: [PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Andrey Smirnov]

On Sun, Oct 13, 2019 at 8:54 PM Sasha Levin <sashal@kernel.org> wrote:
>
> Hi,
>
> [This is an automated email]
>
> This commit has been processed because it contains a -stable tag.
> The stable tag indicates that it's relevant for the following trees: all
>
> The bot has tested the following trees: v5.3.5, v5.2.20, v4.19.78, v4.14.148, v4.9.196, v4.4.196.
>
> v5.3.5: Build OK!
> v5.2.20: Build OK!
> v4.19.78: Failed to apply! Possible dependencies:
>     43cd97af70c65 ("HID: logitech: Stop setting drvdata to NULL on probe failure and remove")
>
> v4.14.148: Failed to apply! Possible dependencies:
>     43cd97af70c65 ("HID: logitech: Stop setting drvdata to NULL on probe failure and remove")
>
> v4.9.196: Failed to apply! Possible dependencies:
>     43cd97af70c65 ("HID: logitech: Stop setting drvdata to NULL on probe failure and remove")
>
> v4.4.196: Failed to apply! Possible dependencies:
>     43cd97af70c65 ("HID: logitech: Stop setting drvdata to NULL on probe failure and remove")
>     6c44b15e1c907 ("HID: logitech: check the return value of create_singlethread_workqueue")
>     7bfd2927adcac ("HID: hid-logitech-hidpp: Add basic support for Logitech G920")
>     7f4b49fef6ffb ("HID: hid-logitech-hidpp: Add range sysfs for Logitech G920")
>     af2e628d6be7a ("HID: logitech-hidpp: limit visibility of init/deinit functions")
>     ff21a635dd1a9 ("HID: logitech-hidpp: Force feedback support for the Logitech G920")
>
>
> NOTE: The patch will not be queued to stable trees until it is upstream.
>
> How should we proceed with this patch?

Please ignore this series, since it will be superseded by upcoming v2

Thanks,
Andrey Smirnov