[PATCH v2 00/33] HID: logitech-hidpp: G920 force-feedback fixes, rework, and cleanups

[PATCH v2 00/33] HID: logitech-hidpp: G920 force-feedback fixes, rework, and cleanups

[Dmitry Torokhov]

This series starts from Gideon Heinz's report of Logitech G920
force-feedback initialization timing out in v7.3 and addresses several
additional pre-existing bugs, lifetime races, and cleanup opportunities
found while auditing hid-logitech-hidpp.

The first 14 patches carry Fixes and stable tags. They fix the v7.3
G920 force-feedback initialization timeout by temporarily enabling
device I/O around g920_get_config() in hidpp_input_configured(), switch
hidpp_ff_queue_work() to GFP_ATOMIC since playback, gain, and autocenter
callbacks run under the input event spinlock, drain the G920 FF
workqueue before closing the HID transport on input close so queued
commands still receive their USB interrupt IN replies, synchronously
disable driver work items and disconnect input devices with device I/O
enabled before stopping the transport on removal and probe failure,
avoid storing an ERR_PTR in battery->ps on registration failure, avoid
NULL pointer subtraction in the HID++ 1.0 consumer keys report fixup,
and fix an off-by-one in G920 slot tracking that made the 63rd user
effect in hardware slot 64 unusable and leaked it across reopen.

Additionally in the bug fix section, hidpp_send_message_sync() is
updated to stop retrying non-busy errors (timeouts and transport
failures) so failed commands fail promptly without 15-second stalls,
and response buffers are initialized before publishing send_receive_buf
to eliminate races with concurrent or spurious packets. Battery fixes
correct HID++ 1.0 battery mileage event parsing to read capacity from
the correct report byte and update state, properly propagate negative
transport and timeout errors in ADC measurement (0x1f20) queries, mark
devices online when receiving unified battery (0x1004) events, fix K750
solar battery feature lookup and reconnect handling, and expose the
charge_type property for the battery voltage (0x1001) feature.

The next section prepares transport handling and reworks G920 force
feedback for next. Connect-event work is kept disabled in probe until
hid_connect() completes successfully. Transport error codes are
preserved in __hidpp_send_report(), and hidpp_send_message_sync()
is switched to return standard negative error codes on failure.
G920 capability discovery moves back into hidpp_probe() while device
I/O is active (keeping force feedback optional on failure and
excluding generic USB PID force feedback), device-level wheel settings
are separated from the input FF queue lifetime, the G920 range sysfs
attribute moves to driver dev_groups, and FF workqueue teardown moves
into ff->stop so open evdev handles across USB disconnect or unbind
no longer cause use-after-free.

Furthermore, G920 range store, effect upload, and effect erase are
converted to execute synchronously with proper transport lifetime
handling, avoiding asynchronous command queueing and work flushing
during device close and teardown. Effect slot lookup is streamlined by
indexing slot arrays directly by effect ID instead of performing linear
scans.

The final section modernizes the driver. Multi-byte packet fields are
switched to unaligned accessors, the device serial is stored as a byte
array, and resource management is updated to use guard(mutex),
__free(kfree), alloc_ordered_workqueue(), devm_mutex_init(), and
devm_device_add_group(). Lastly, unused kfifo fields and helpers
are removed.

Note that device removal teardown still has limitations due to how HID
core's hid_device_io_start() and hid_device_io_stop() (via
driver_input_lock) gate both driver raw command/response I/O and
subdriver event forwarding, disabling I/O before .remove() and
hid_disconnect(). For instance, HIDPP_QUIRK_DELAYED_INIT devm-managed
input devices are still unregistered after hid_hw_stop(), and
hidpp_connect_event() publishes hidpp->input before input registration
completes. Resolving these teardown edge cases cleanly will require
follow-up work in HID core and driver input registration.

Thanks.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
Changes in v2:
- Rebased the series on top of linux-next.
- Collected Tested-by tags from Gideon Heinz for 17 v1 patches (omitted
  from patch 4 where hidpp_remove() was significantly altered).
- Reorganized the series to place all 14 stable bug fixes upfront,
  followed by next refactorings and modernizations.
- Updated hidpp_remove() to enable device I/O while draining work
  items and disconnecting hidinput so in-flight transfers and
  force-feedback teardown can receive device replies (noting that
  complete resolution of delayed-init and transport teardown ordering
  requires follow-up work in HID core).
- Stopped retrying non-busy errors (timeouts and negative transport
  failures) in hidpp_send_message_sync().
- Initialized response buffers before publishing send_receive_buf to
  prevent stale response reads on concurrent or spurious events.
- Fixed HID++ 1.0 battery mileage event parsing to read capacity from
  the correct report byte and update battery state.
- Handled negative transport and timeout errors properly in ADC
  measurement (0x1f20) queries.
- Marked devices online when receiving unified battery (0x1004) events.
- Fixed K750 solar battery feature lookup and reconnect handling.
- Exposed charge_type property for battery voltage (0x1001) feature.
- Kept connect work disabled during probe until hid_connect() succeeds.
- Switched hidpp_send_message_sync() to translate RAP and FAP protocol
  error codes into negative Linux error codes (-EBUSY, -EIO, -EPROTO)
  instead of returning positive protocol values, and preserved transport
  error codes in __hidpp_send_report().
- Converted G920 range store, effect upload, and effect erase to
  synchronous execution with slot indexing by effect ID.
- Added G920 force-feedback helper cleanups, removed unused kfifo and
  struct wtp_data fields, and cleaned up minor helpers.
- Link to v1: https://patch.msgid.link/20261109061103.2887636-1-dmitry.torokhov@gmail.com

---
Dmitry Torokhov (33):
      HID: logitech-hidpp: fix G920 force-feedback initialization timeout
      HID: logitech-hidpp: use GFP_ATOMIC in hidpp_ff_queue_work()
      HID: logitech-hidpp: drain G920 FF workqueue before closing transport
      HID: logitech-hidpp: disable works before stopping transport
      HID: logitech-hidpp: do not store ERR_PTR in battery->ps
      HID: logitech-hidpp: avoid NULL subtraction in consumer key report fixup
      HID: logitech-hidpp: fix off-by-one in G920 force-feedback slot tracking
      HID: logitech-hidpp: do not retry timeouts and transport errors in hidpp_send_message_sync()
      HID: logitech-hidpp: initialize response buffer before publishing send_receive_buf
      HID: logitech-hidpp: fix HID++ 1.0 battery mileage event parsing
      HID: logitech-hidpp: handle negative errors in hidpp20_get_adc_measurement_1f20()
      HID: logitech-hidpp: mark device online on unified battery 0x1004 events
      HID: logitech-hidpp: fix K750 solar battery feature lookup and reconnect
      HID: logitech-hidpp: expose charge_type property for battery voltage feature
      HID: logitech-hidpp: keep connect work disabled until hid_connect() completes
      HID: logitech-hidpp: preserve transport error codes in __hidpp_send_report()
      HID: logitech-hidpp: return negative error codes from hidpp_send_message_sync()
      HID: logitech-hidpp: rework G920 force-feedback initialization
      HID: logitech-hidpp: use sysfs_emit() in hidpp_ff_range_show()
      HID: logitech-hidpp: manage G920 range sysfs attribute via dev_groups
      HID: logitech-hidpp: execute G920 range sysfs store synchronously
      HID: logitech-hidpp: shut down G920 force feedback on input unregister
      HID: logitech-hidpp: index G920 force-feedback slots by effect ID
      HID: logitech-hidpp: execute G920 effect upload and erase synchronously
      HID: logitech-hidpp: clean up G920 force-feedback helpers
      HID: logitech-hidpp: use unaligned accessors for multi-byte values
      HID: logitech-hidpp: store device serial as a byte array
      HID: logitech-hidpp: use guard() for send_mutex
      HID: logitech-hidpp: use __free(kfree) for temporary allocations
      HID: logitech-hidpp: use alloc_ordered_workqueue() for FF queue
      HID: logitech-hidpp: manage send_mutex with devm_mutex_init()
      HID: logitech-hidpp: manage ps_attribute_group with devm_device_add_group()
      HID: logitech-hidpp: remove unused kfifo and clean up minor helpers

 drivers/hid/hid-logitech-hidpp.c | 897 ++++++++++++++++++---------------------
 1 file changed, 411 insertions(+), 486 deletions(-)
---
base-commit: 630059a5a632377b9b7b487baccde92b7ae1dd47
change-id: 20261110-hid-logitech-hidpp-6884bfb6a348

Thanks.

-- 
Dmitry

[PATCH v2 01/33] HID: logitech-hidpp: fix G920 force-feedback initialization timeout

[Dmitry Torokhov]

Commit 544315e40172 ("HID: logitech-hidpp: move FF initialization to
.input_configured()") moved Logitech G920 force-feedback querying and
initialization into hidpp_input_configured(), which runs during
hid_connect(). Because hidpp_probe() calls hid_device_io_stop() across
hid_connect(), incoming HID++ responses from the device are blocked
and g920_get_config() times out with -ETIMEDOUT.

Temporarily re-enable HID device I/O around g920_get_config() in
hidpp_input_configured() so the hardware configuration queries receive
their replies during input setup.

Fixes: 544315e40172 ("HID: logitech-hidpp: move FF initialization to .input_configured()")
Reported-by: Gideon Heinz <gideonheinz02@gmail.com>
Closes: https://lore.kernel.org/r/CAPb1+K1-pirdLGZ1wTUw26G6pvhg7rGP3QZ172ys5DB7rNCf5w@mail.gmail.com
Tested-by: Gideon Heinz <gideonheinz02@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 60fc6b86a085..85d8d168ff15 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4096,7 +4096,9 @@ static int hidpp_input_configured(struct hid_device *hdev, struct hid_input *hid
 		if (!list_is_first(&hidinput->list, &hdev->inputs))
 			return 0;
 
+		hid_device_io_start(hdev);
 		ret = g920_get_config(hidpp, &data);
+		hid_device_io_stop(hdev);
 		if (!ret)
 			ret = hidpp_ff_init(hidpp, &data);
 

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 02/33] HID: logitech-hidpp: use GFP_ATOMIC in hidpp_ff_queue_work()

[Dmitry Torokhov]

Force-feedback playback, gain, and autocenter updates are invoked by the
input force-feedback core while holding the input device event spinlock
with interrupts disabled. Allocating work items with GFP_KERNEL in
hidpp_ff_queue_work() can therefore sleep in atomic context.

Allocate work items with GFP_ATOMIC instead so force-feedback commands
can be queued safely while event locks are held.

Fixes: ff21a635dd1a ("HID: logitech-hidpp: Force feedback support for the Logitech G920")
Tested-by: Gideon Heinz <gideonheinz02@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 85d8d168ff15..d9342a3e8122 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2542,9 +2542,10 @@ static void hidpp_ff_work_handler(struct work_struct *w)
 
 static int hidpp_ff_queue_work(struct hidpp_ff_private_data *data, int effect_id, u8 command, u8 *params, u8 size)
 {
-	struct hidpp_ff_work_data *wd = kzalloc_obj(*wd);
+	struct hidpp_ff_work_data *wd;
 	int s;
 
+	wd = kzalloc_obj(*wd, GFP_ATOMIC);
 	if (!wd)
 		return -ENOMEM;
 

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 03/33] HID: logitech-hidpp: drain G920 FF workqueue before closing transport

[Dmitry Torokhov]

Probe no longer keeps the HID transport open indefinitely, so the
underlying USB interrupt IN endpoint only stays active while the HID
transport is held open. When the last input user closes the device
after input core flushes active effects, or when the device is
inhibited with commands still queued, releasing the HID transport
shuts down transport I/O unless another client holds it open. Queued
force-feedback commands then fail to receive their HID++ replies,
causing command timeouts and leaving stale slot mappings behind.

Override the input open and close callbacks once G920 force feedback is
initialized so the force-feedback send queue drains completely before
closing the HID transport.

Fixes: 91cf9a98ae41 ("HID: logitech-hidpp: make .probe usbhid capable")
Tested-by: Gideon Heinz <gideonheinz02@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index d9342a3e8122..d5f53b35ac6f 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2815,6 +2815,22 @@ static void hidpp_ff_destroy(struct ff_device *ff)
 	kfree(data->effect_ids);
 }
 
+static int g920_input_open(struct input_dev *dev)
+{
+	struct hid_device *hid = input_get_drvdata(dev);
+
+	return hid_hw_open(hid);
+}
+
+static void g920_input_close(struct input_dev *dev)
+{
+	struct hid_device *hid = input_get_drvdata(dev);
+	struct hidpp_ff_private_data *data = dev->ff->private;
+
+	flush_workqueue(data->wq);
+	hid_hw_close(hid);
+}
+
 static int hidpp_ff_init(struct hidpp_device *hidpp,
 			 struct hidpp_ff_private_data *data)
 {
@@ -2897,6 +2913,9 @@ static int hidpp_ff_init(struct hidpp_device *hidpp,
 	ff->set_autocenter = hidpp_ff_set_autocenter;
 	ff->destroy = hidpp_ff_destroy;
 
+	dev->open = g920_input_open;
+	dev->close = g920_input_close;
+
 	/* Create sysfs interface */
 	error = device_create_file(&(hidpp->hid_dev->dev), &dev_attr_range);
 	if (error)

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 04/33] HID: logitech-hidpp: disable works before stopping transport

[Dmitry Torokhov]

Device removal and the probe error unwind path cancel the connect-event
and hi-res-reset work items only after closing and stopping the HID
transport, leaving in-flight work handlers able to issue HID++ transfers
or register power-supply and input devices while transport teardown is
underway. Moving cancel_work_sync() ahead of transport shutdown is not
sufficient because incoming reports remain active until the transport
stops and can re-schedule the work items before hid_hw_stop() completes.

Disable both work items synchronously before closing or stopping the
HID transport on device removal and on the post-I/O probe failure path,
and drop the unreachable cancellations from early transport start and
open failure paths where incoming packets have not been enabled yet.

On device removal where HID core enters with device I/O stopped and
driver_input_lock held, temporarily re-enable device I/O to flush
pending work handlers and disconnect input devices before stopping the
transport. Clear HID_CLAIMED_INPUT and hidpp->input upfront so incoming
reports can still wake synchronous commands without dispatching events
to the disconnecting input device.

Fixes: 7bfd2927adca ("HID: hid-logitech-hidpp: Add basic support for Logitech G920")
Fixes: ed80cc4667ac ("HID: logitech-hidpp: Add HIDPP_QUIRK_RESET_HI_RES_SCROLL")
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index d5f53b35ac6f..0e1a9fd98faf 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4805,13 +4805,13 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	return ret;
 
 hid_hw_init_fail:
+	disable_work_sync(&hidpp->work);
+	disable_work_sync(&hidpp->reset_hi_res_work);
 	hid_hw_close(hdev);
 hid_hw_open_fail:
 	hid_hw_stop(hdev);
 hid_hw_start_fail:
 	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
-	cancel_work_sync(&hidpp->work);
-	cancel_work_sync(&hidpp->reset_hi_res_work);
 	mutex_destroy(&hidpp->send_mutex);
 	return ret;
 }
@@ -4819,15 +4819,26 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 static void hidpp_remove(struct hid_device *hdev)
 {
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
+	bool claimed_input;
 
 	if (!hidpp)
 		return hid_hw_stop(hdev);
 
 	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
 
+	claimed_input = hdev->claimed & HID_CLAIMED_INPUT;
+	hdev->claimed &= ~HID_CLAIMED_INPUT;
+	if (claimed_input)
+		hidpp->input = NULL;
+
+	hid_device_io_start(hdev);
+	disable_work_sync(&hidpp->work);
+	disable_work_sync(&hidpp->reset_hi_res_work);
+	if (claimed_input)
+		hidinput_disconnect(hdev);
+	hid_device_io_stop(hdev);
+
 	hid_hw_stop(hdev);
-	cancel_work_sync(&hidpp->work);
-	cancel_work_sync(&hidpp->reset_hi_res_work);
 	mutex_destroy(&hidpp->send_mutex);
 }
 

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 05/33] HID: logitech-hidpp: do not store ERR_PTR in battery->ps

[Dmitry Torokhov]

When power supply registration fails, storing the error pointer directly
in the battery supply field leaves it non-NULL. Callers such as the
connect handler and battery event handlers only check for non-NULL
before notifying the power supply core, causing an error-pointer
dereference oops and blocking retries on subsequent reconnect events.

Capture the registered power supply pointer in a local variable and
publish it to device state only after registration and sysfs linking
succeed.

Fixes: a52ec107fa81 ("HID: logitech-hidpp: create the battery for all types of HID++ devices")
Tested-by: Gideon Heinz <gideonheinz02@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0e1a9fd98faf..6ac41b7d76ee 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4333,6 +4333,7 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 	struct power_supply_desc *desc = &hidpp->battery.desc;
 	enum power_supply_property *battery_props;
 	struct hidpp_battery *battery;
+	struct power_supply *ps;
 	unsigned int num_battery_props;
 	unsigned long n;
 	int ret;
@@ -4413,15 +4414,16 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 	desc->type = POWER_SUPPLY_TYPE_BATTERY;
 	desc->use_for_apm = 0;
 
-	battery->ps = devm_power_supply_register(&hidpp->hid_dev->dev,
-						 &battery->desc,
-						 &cfg);
-	if (IS_ERR(battery->ps))
-		return PTR_ERR(battery->ps);
+	ps = devm_power_supply_register(&hidpp->hid_dev->dev,
+					&battery->desc,
+					&cfg);
+	if (IS_ERR(ps))
+		return PTR_ERR(ps);
 
-	power_supply_powers(battery->ps, &hidpp->hid_dev->dev);
+	power_supply_powers(ps, &hidpp->hid_dev->dev);
+	battery->ps = ps;
 
-	return ret;
+	return 0;
 }
 
 /* Get name + serial for USB and Bluetooth HID++ devices */

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 06/33] HID: logitech-hidpp: avoid NULL subtraction in consumer key report fixup

[Dmitry Torokhov]

Report fixup runs for all 27 MHz devices because the keyboard quirk
is only determined later after parsing the report descriptor. For mice
and other devices without the consumer collection, searching for the
descriptor fragment returns NULL, yet the remaining length calculation
subtracts the base descriptor pointer from NULL before checking the
search result, triggering pointer-subtraction undefined behavior.

Ensure the search result is non-NULL before computing the remaining
descriptor length.

Fixes: 42bc4f3129e6 ("HID: logitech-hidpp: add support for HID++ 1.0 consumer keys reports")
Tested-by: Gideon Heinz <gideonheinz02@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 6ac41b7d76ee..53c171dfcf96 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -3872,11 +3872,9 @@ static u8 *hidpp10_consumer_keys_report_fixup(struct hidpp_device *hidpp,
 		0x26, 0x00	/* LOGICAL_MAX (...                    */
 	};
 	char *consumer_rdesc, *rdesc = (char *)_rdesc;
-	unsigned int size;
 
 	consumer_rdesc = strnstr(rdesc, consumer_rdesc_start, *rsize);
-	size = *rsize - (consumer_rdesc - rdesc);
-	if (consumer_rdesc && size >= 25) {
+	if (consumer_rdesc && *rsize - (consumer_rdesc - rdesc) >= 25) {
 		consumer_rdesc[15] = 0x7f;
 		consumer_rdesc[16] = 0x10;
 		consumer_rdesc[20] = 0x7f;

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 07/33] HID: logitech-hidpp: fix off-by-one in G920 force-feedback slot tracking

[Dmitry Torokhov]

Logitech G920 firmware reports 64 total 1-based hardware effect slots,
reserving one slot for the built-in autocenter spring while allocating
the remaining 63 user effects across 1-based slots 2 through 64. Because
the driver subtracts the reserved slot count upfront before sizing and
bounds-checking its slot mapping table, the 63rd concurrent user effect
is assigned hardware slot 64 and exceeds the stored limit, so its slot
mapping is never recorded or released and remains leaked in wheel memory
across device close and reopen.

Keep the full hardware slot count for table sizing, lookups, and slot
bounds checks, and withhold the reserved slot count only when creating
the input force-feedback device.

Fixes: ff21a635dd1a ("HID: logitech-hidpp: Force feedback support for the Logitech G920")
Tested-by: Gideon Heinz <gideonheinz02@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 53c171dfcf96..0264e4541edf 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2872,7 +2872,7 @@ static int hidpp_ff_init(struct hidpp_device *hidpp,
 		for (j = 0; hidpp_ff_effects_v2[j] >= 0; j++)
 			set_bit(hidpp_ff_effects_v2[j], dev->ffbit);
 
-	error = input_ff_create(dev, num_slots);
+	error = input_ff_create(dev, num_slots - HIDPP_FF_RESERVED_SLOTS);
 
 	if (error) {
 		hid_err(dev, "Failed to create FF device!\n");
@@ -3486,7 +3486,7 @@ static int g920_get_config(struct hidpp_device *hidpp,
 		return -EPROTO;
 	}
 
-	data->num_effects = response.fap.params[0] - HIDPP_FF_RESERVED_SLOTS;
+	data->num_effects = response.fap.params[0];
 
 	/* reset all forces */
 	ret = hidpp_send_fap_command_sync(hidpp, data->feature_index,

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 08/33] HID: logitech-hidpp: do not retry timeouts and transport errors in hidpp_send_message_sync()

[Dmitry Torokhov]

When a transport failure or timeout occurs, the synchronous send helper
clears the response buffer, setting its report ID to zero. Because the
retry loop only inspects valid HID++ short, long, and very long report
IDs to decide whether to return, a cleared buffer misses both checks and
falls through to the busy-retry path. Every transport error and timeout
is therefore misidentified as a busy error and retried three times while
holding the send mutex.

Return immediately when the low-level helper reports success or a
negative error code, and only retry when the device responds with an
explicit busy protocol error.

Fixes: aba7963544d4 ("HID: logitech-hidpp: Do not assume FAP in hidpp_send_message_sync()")
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0264e4541edf..c486488efe99 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -364,6 +364,9 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
 
 	do {
 		ret = __do_hidpp_send_message_sync(hidpp, message, response);
+		if (ret <= 0)
+			break;
+
 		if (response->report_id == REPORT_ID_HIDPP_SHORT &&
 		    ret != HIDPP_ERROR_BUSY)
 			break;

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 09/33] HID: logitech-hidpp: initialize response buffer before publishing send_receive_buf

[Dmitry Torokhov]

__do_hidpp_send_message_sync() publishes send_receive_buf before
initializing the response buffer. hidpp_raw_hidpp_event() runs in IRQ or
softirq context without taking send_mutex whenever the mutex is locked,
reading question and answer from send_receive_buf. If an incoming report
arrives after send_receive_buf is published but before response is
populated, hidpp_match_answer() inspects uninitialized memory.

Initialize response with the outgoing message and clear answer_available
before publishing send_receive_buf with release semantics, clear the
pointer with WRITE_ONCE() on completion, and read it once with acquire
semantics in the event handler.

Fixes: 2f31c5252910 ("HID: Introduce hidpp, a module to handle Logitech hid++ devices")
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index c486488efe99..16dcd79059d5 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -300,14 +300,17 @@ static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp,
 
 	__must_hold(&hidpp->send_mutex);
 
-	hidpp->send_receive_buf = response;
-	hidpp->answer_available = false;
-
 	/*
 	 * So that we can later validate the answer when it arrives
 	 * in hidpp_raw_event
 	 */
 	*response = *message;
+	hidpp->answer_available = false;
+	/*
+	 * Pairs with smp_load_acquire() in hidpp_raw_hidpp_event() to ensure
+	 * the response buffer initialization is visible before publishing.
+	 */
+	smp_store_release(&hidpp->send_receive_buf, response);
 
 	ret = __hidpp_send_report(hidpp->hid_dev, message);
 	if (ret) {
@@ -342,7 +345,7 @@ static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp,
 	ret = 0;
 
 out:
-	hidpp->send_receive_buf = NULL;
+	WRITE_ONCE(hidpp->send_receive_buf, NULL);
 	return ret;
 }
 
@@ -4148,12 +4151,16 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
 	 * previously sent command.
 	 */
 	if (unlikely(mutex_is_locked(&hidpp->send_mutex))) {
-		question = hidpp->send_receive_buf;
-		answer = hidpp->send_receive_buf;
-
+		/*
+		 * Pairs with smp_store_release() in __do_hidpp_send_message_sync()
+		 * to ensure the initialized response buffer is observed.
+		 */
+		question = smp_load_acquire(&hidpp->send_receive_buf);
 		if (!question)
 			return 0;
 
+		answer = question;
+
 		/*
 		 * Check for a correct hidpp20 answer or the corresponding
 		 * error

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 10/33] HID: logitech-hidpp: fix HID++ 1.0 battery mileage event parsing

[Dmitry Torokhov]

In HID++ 1.0 battery mileage event notifications (register 0x0d), the
register address occupies the sub_id field rather than reg_address, so
the payload bytes start at rawbytes[1] (parameter p0). The driver
previously read capacity from report->rap.params[0] (rawbytes[2]),
extracting parameter p1 instead of the battery mileage percentage.
Additionally, when a change was detected, hidpp10_battery_event()
updated level and status but neglected to update battery.capacity,
leaving capacity frozen and triggering spurious power supply updates on
every subsequent event.

Read capacity from report->rawbytes[1] and store the updated capacity in
hidpp->battery.capacity when changed.

Fixes: 7f7ce2a258b4 ("HID: logitech-hidpp: enable HID++ 1.0 battery reporting")
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 16dcd79059d5..0ec2f2fd6dec 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -829,7 +829,7 @@ static int hidpp10_battery_event(struct hidpp_device *hidpp, u8 *data, int size)
 		status = hidpp10_battery_status_map_status(report->rawbytes[2]);
 		break;
 	case HIDPP_REG_BATTERY_MILEAGE:
-		capacity = report->rap.params[0];
+		capacity = report->rawbytes[1];
 		level = hidpp->battery.level;
 		status = hidpp10_battery_mileage_map_status(report->rawbytes[3]);
 		break;
@@ -846,6 +846,7 @@ static int hidpp10_battery_event(struct hidpp_device *hidpp, u8 *data, int size)
 				status == POWER_SUPPLY_STATUS_FULL;
 
 	if (changed) {
+		hidpp->battery.capacity = capacity;
 		hidpp->battery.level = level;
 		hidpp->battery.status = status;
 		if (hidpp->battery.ps)

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 11/33] HID: logitech-hidpp: handle negative errors in hidpp20_get_adc_measurement_1f20()

[Dmitry Torokhov]

When hidpp_send_fap_command_sync() fails with a negative transport or
timeout error (-ETIMEDOUT, -ENODEV, etc.),
hidpp20_get_adc_measurement_1f20() only checked if (ret > 0) for
protocol errors and fell through. It then parsed zeroed response
parameters (0 mV) and returned true, marking a disconnected or
unresponsive device online and triggering spurious "possibly using the
wrong voltage curve" warnings.

Return false whenever ret is non-zero, logging the debug message only
when ret is a positive protocol error.

Fixes: c361982a13c9 ("HID: logitech-hidpp: Add support for ADC measurement feature")
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0ec2f2fd6dec..28402baf04fb 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1952,9 +1952,10 @@ static bool hidpp20_get_adc_measurement_1f20(struct hidpp_device *hidpp,
 					  CMD_ADC_MEASUREMENT_GET_ADC_MEASUREMENT,
 					  NULL, 0, &response);
 
-	if (ret > 0) {
-		hid_dbg(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
+	if (ret) {
+		if (ret > 0)
+			hid_dbg(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
+				__func__, ret);
 		return false;
 	}
 

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 12/33] HID: logitech-hidpp: mark device online on unified battery 0x1004 events

[Dmitry Torokhov]

When a wireless device disconnects, its battery state is marked offline.
However, when the device reconnects and sends an unsolicited unified
battery status event (feature 0x1004), hidpp20_battery_event_1004()
never updated hidpp->battery.online to true and did not include the
online state in the changed check.

As a result, sysfs POWER_SUPPLY_PROP_ONLINE stayed 0, power supply
listeners were not notified of the online transition, and the 0 to 1
online transition check in hidpp_raw_hidpp_event() failed to schedule
reset_hi_res_work to reconfigure high-resolution scrolling on reconnect.

Include !hidpp->battery.online in the changed check and mark
hidpp->battery.online true when receiving a 0x1004 status event.

Fixes: e037acf0b1ae ("HID: logitech-hidpp: add support for Unified Battery (1004) feature")
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 28402baf04fb..f717f0308454 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1761,12 +1761,15 @@ static int hidpp20_battery_event_1004(struct hidpp_device *hidpp,
 	status = hidpp20_unifiedbattery_map_status(hidpp, params[2], params[3]);
 	level = hidpp20_unifiedbattery_map_level(hidpp, params[1]);
 
-	changed = status != hidpp->battery.status ||
+	changed = !hidpp->battery.online ||
+		  status != hidpp->battery.status ||
 		  (state_of_charge != hidpp->battery.capacity &&
 		   hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_PERCENTAGE) ||
 		  (level != hidpp->battery.level &&
 		   hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_LEVEL_STATUS);
 
+	hidpp->battery.online = true;
+
 	if (changed) {
 		hidpp->battery.capacity = state_of_charge;
 		hidpp->battery.status = status;

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 13/33] HID: logitech-hidpp: fix K750 solar battery feature lookup and reconnect

[Dmitry Torokhov]

In hidpp_solar_request_battery_event(), the driver checked whether
hidpp->battery.feature_index == 0xff instead of checking
solar_feature_index, causing it to re-query the root feature table
unnecessarily or misidentify whether the solar feature was resolved.

Furthermore, in hidpp_connect_event(), the HIDPP20_BATTERY block lacked
a branch for HIDPP_QUIRK_CLASS_K750, causing wireless reconnects of a
K750 keyboard to fall through to hidpp20_query_battery_info_1000() which
fails because the K750 does not implement feature 0x1000.

Check solar_feature_index in hidpp_solar_request_battery_event() and
re-enable solar battery reporting via the solar request helper on K750
reconnects.

Fixes: 696ecef9b587 ("HID: logitech-hidpp: add support for battery status for the K750")
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index f717f0308454..cdeb1b3c2f21 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2122,7 +2122,7 @@ static int hidpp_solar_request_battery_event(struct hidpp_device *hidpp)
 	u8 params[2] = { 1, 1 };
 	int ret;
 
-	if (hidpp->battery.feature_index == 0xff) {
+	if (hidpp->battery.solar_feature_index == 0xff) {
 		ret = hidpp_root_get_feature(hidpp,
 					     HIDPP_PAGE_SOLAR_KEYBOARD,
 					     &hidpp->battery.solar_feature_index);
@@ -4582,7 +4582,9 @@ static void hidpp_connect_event(struct work_struct *work)
 		else
 			hidpp10_query_battery_status(hidpp);
 	} else if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_BATTERY) {
-		if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE)
+		if (hidpp->quirks & HIDPP_QUIRK_CLASS_K750)
+			hidpp_solar_request_battery_event(hidpp);
+		else if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE)
 			hidpp20_query_battery_voltage_info(hidpp);
 		else if (hidpp->capabilities & HIDPP_CAPABILITY_UNIFIED_BATTERY)
 			hidpp20_query_battery_info_1004(hidpp);

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 14/33] HID: logitech-hidpp: expose charge_type property for battery voltage feature

[Dmitry Torokhov]

Commit be281368f297 ("hid-logitech-hidpp: read battery voltage from newer
devices") added support for the 0x1001 battery voltage feature, populated
battery->charge_type, and handled POWER_SUPPLY_PROP_CHARGE_TYPE in
hidpp_battery_get_property(). However, it did not add a placeholder slot
in hidpp_battery_props[] or register POWER_SUPPLY_PROP_CHARGE_TYPE in
hidpp_initialize_battery() when HIDPP_CAPABILITY_BATTERY_VOLTAGE is set.
Consequently, the charge_type attribute was never exposed via sysfs.
Furthermore, hidpp20_battery_voltage_event() only checked whether voltage
or status changed before updating cached battery values and notifying
user space, ignoring changes in level and charge_type.

Add a placeholder slot to hidpp_battery_props[], update the fixed
property count in hidpp_initialize_battery(), append
POWER_SUPPLY_PROP_CHARGE_TYPE when HIDPP_CAPABILITY_BATTERY_VOLTAGE is
set, and include level and charge_type in the change detection condition
in hidpp20_battery_voltage_event().

Fixes: be281368f297 ("hid-logitech-hidpp: read battery voltage from newer devices")
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index cdeb1b3c2f21..fb9ad4cfc583 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1553,7 +1553,10 @@ static int hidpp20_battery_voltage_event(struct hidpp_device *hidpp,
 
 	hidpp->battery.online = status != POWER_SUPPLY_STATUS_NOT_CHARGING;
 
-	if (voltage != hidpp->battery.voltage || status != hidpp->battery.status) {
+	if (voltage != hidpp->battery.voltage ||
+	    status != hidpp->battery.status ||
+	    level != hidpp->battery.level ||
+	    charge_type != hidpp->battery.charge_type) {
 		hidpp->battery.voltage = voltage;
 		hidpp->battery.capacity = hidpp20_map_battery_capacity(hidpp->hid_dev,
 								       voltage);
@@ -1795,6 +1798,7 @@ static enum power_supply_property hidpp_battery_props[] = {
 	0, /* placeholder for POWER_SUPPLY_PROP_CAPACITY, */
 	0, /* placeholder for POWER_SUPPLY_PROP_CAPACITY_LEVEL, */
 	0, /* placeholder for POWER_SUPPLY_PROP_VOLTAGE_NOW, */
+	0, /* placeholder for POWER_SUPPLY_PROP_CHARGE_TYPE, */
 };
 
 static int hidpp_battery_get_property(struct power_supply *psy,
@@ -4398,7 +4402,7 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 	if (!battery_props)
 		return -ENOMEM;
 
-	num_battery_props = ARRAY_SIZE(hidpp_battery_props) - 3;
+	num_battery_props = ARRAY_SIZE(hidpp_battery_props) - 4;
 
 	if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_MILEAGE ||
 	    hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_PERCENTAGE ||
@@ -4416,6 +4420,10 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 		battery_props[num_battery_props++] =
 			POWER_SUPPLY_PROP_VOLTAGE_NOW;
 
+	if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE)
+		battery_props[num_battery_props++] =
+				POWER_SUPPLY_PROP_CHARGE_TYPE;
+
 	battery = &hidpp->battery;
 
 	n = atomic_inc_return(&battery_no) - 1;

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 15/33] HID: logitech-hidpp: keep connect work disabled until hid_connect() completes

[Dmitry Torokhov]

During probe, initial feature discovery and device queries run with
device I/O active before hid_connect() exports input and hidraw
interfaces. An unsolicited wireless connect notification (0x41)
received during this window queues connect work (hidpp_connect_event),
which can run while hid_device_io_stop() has paused transport or race
with hid_connect().

Keep connect work disabled from initialization until after hid_connect()
has finished and device I/O is restarted, ensuring that connect work
only executes after interfaces are fully established.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index fb9ad4cfc583..15fe9dcbca6c 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4768,6 +4768,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	}
 
 	INIT_WORK(&hidpp->work, hidpp_connect_event);
+	disable_work(&hidpp->work);
 	INIT_WORK(&hidpp->reset_hi_res_work, hidpp_reset_hi_res_handler);
 	mutex_init(&hidpp->send_mutex);
 	init_waitqueue_head(&hidpp->wait);
@@ -4819,6 +4820,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	/* Check for connected devices now that incoming packets will not be disabled again */
 	hid_device_io_start(hdev);
+	enable_work(&hidpp->work);
 	schedule_work(&hidpp->work);
 	flush_work(&hidpp->work);
 

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 16/33] HID: logitech-hidpp: preserve transport error codes in __hidpp_send_report()

[Dmitry Torokhov]

__hidpp_send_report() returns -1 (-EPERM) whenever the underlying HID
transport write does not match the expected report length. This discards
genuine negative error codes returned by hid_hw_output_report() and
hid_hw_raw_request() (such as -ENODEV, -ESHUTDOWN, or -EPIPE), masking
device disconnection and transport failure from callers.

Preserve negative error codes from the transport call, and return -EIO
instead of -1 when a short write occurs.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 15fe9dcbca6c..3745d95f5bba 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -279,7 +279,7 @@ static int __hidpp_send_report(struct hid_device *hdev,
 			HID_REQ_SET_REPORT);
 	}
 
-	return ret == fields_count ? 0 : -1;
+	return ret == fields_count ? 0 : (ret < 0 ? ret : -EIO);
 }
 
 /*

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 17/33] HID: logitech-hidpp: return negative error codes from hidpp_send_message_sync()

[Dmitry Torokhov]

Low-level synchronous send helpers previously returned 0 on success,
negative error numbers on transport/timeout errors, and positive values
on device protocol errors. Callers across the driver were burdened with
checking if (ret > 0) to convert protocol errors into -EPROTO or -EIO,
leading to scattered, inconsistent error translation.

Translate HID++ 1.0 and HID++ 2.0 error reports directly into standard
negative error codes (-EBUSY, -EIO, -EPROTO) in the core synchronous
send helper, simplify retry handling in hidpp_send_message_sync() to
check ret == -EBUSY, and remove redundant protocol error conversion
boilerplate across all callers.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 158 +++++++++------------------------------
 1 file changed, 37 insertions(+), 121 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 3745d95f5bba..cf42626ab98d 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -287,10 +287,9 @@ static int __hidpp_send_report(struct hid_device *hdev,
  *
  * Must be called with hidpp->send_mutex locked
  *
- * Same return protocol than hidpp_send_message_sync():
- * - success on 0
- * - negative error means transport error
- * - positive value means protocol error
+ * Same return protocol as hidpp_send_message_sync():
+ * - 0 on success
+ * - negative error code on failure
  */
 static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp,
 	struct hidpp_report *message,
@@ -331,6 +330,18 @@ static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp,
 	    response->rap.sub_id == HIDPP_ERROR) {
 		ret = response->rap.params[1];
 		dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
+		switch (ret) {
+		case HIDPP_ERROR_BUSY:
+			ret = -EBUSY;
+			break;
+		case HIDPP_ERROR_RESOURCE_ERROR:
+		case HIDPP_ERROR_UNKNOWN_DEVICE:
+			ret = -EIO;
+			break;
+		default:
+			ret = -EPROTO;
+			break;
+		}
 		goto out;
 	}
 
@@ -339,6 +350,14 @@ static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp,
 	    response->fap.feature_index == HIDPP20_ERROR) {
 		ret = response->fap.params[1];
 		dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
+		switch (ret) {
+		case HIDPP20_ERROR_BUSY:
+			ret = -EBUSY;
+			break;
+		default:
+			ret = -EPROTO;
+			break;
+		}
 		goto out;
 	}
 
@@ -350,11 +369,8 @@ static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp,
 }
 
 /*
- * hidpp_send_message_sync() returns 0 in case of success, and something else
- * in case of a failure.
- *
- * See __do_hidpp_send_message_sync() for a detailed explanation of the returned
- * value.
+ * hidpp_send_message_sync() returns 0 in case of success, and a negative error
+ * code in case of a failure.
  */
 static int hidpp_send_message_sync(struct hidpp_device *hidpp,
 	struct hidpp_report *message,
@@ -367,18 +383,10 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
 
 	do {
 		ret = __do_hidpp_send_message_sync(hidpp, message, response);
-		if (ret <= 0)
+		if (ret != -EBUSY)
 			break;
 
-		if (response->report_id == REPORT_ID_HIDPP_SHORT &&
-		    ret != HIDPP_ERROR_BUSY)
-			break;
-		if ((response->report_id == REPORT_ID_HIDPP_LONG ||
-		     response->report_id == REPORT_ID_HIDPP_VERY_LONG) &&
-		    ret != HIDPP20_ERROR_BUSY)
-			break;
-
-		dbg_hid("%s:got busy hidpp error %02X, retrying\n", __func__, ret);
+		dbg_hid("%s:got busy hidpp error, retrying\n", __func__);
 	} while (--max_retries);
 
 	mutex_unlock(&hidpp->send_mutex);
@@ -387,11 +395,8 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
 }
 
 /*
- * hidpp_send_fap_command_sync() returns 0 in case of success, and something else
- * in case of a failure.
- *
- * See __do_hidpp_send_message_sync() for a detailed explanation of the returned
- * value.
+ * hidpp_send_fap_command_sync() returns 0 in case of success, and a negative error
+ * code in case of a failure.
  */
 static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp,
 	u8 feat_index, u8 funcindex_clientid, u8 *params, int param_count,
@@ -426,11 +431,8 @@ static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp,
 }
 
 /*
- * hidpp_send_rap_command_sync() returns 0 in case of success, and something else
- * in case of a failure.
- *
- * See __do_hidpp_send_message_sync() for a detailed explanation of the returned
- * value.
+ * hidpp_send_rap_command_sync() returns 0 in case of success, and a negative error
+ * code in case of a failure.
  */
 static int hidpp_send_rap_command_sync(struct hidpp_device *hidpp_dev,
 	u8 report_id, u8 sub_id, u8 reg_address, u8 *params, int param_count,
@@ -988,22 +990,14 @@ static int hidpp_root_get_protocol_version(struct hidpp_device *hidpp)
 			CMD_ROOT_GET_PROTOCOL_VERSION | LINUX_KERNEL_SW_ID,
 			ping_data, sizeof(ping_data), &response);
 
-	if (ret == HIDPP_ERROR_INVALID_SUBID) {
+	if (ret == -EPROTO &&
+	    response.rap.sub_id == HIDPP_ERROR &&
+	    response.rap.params[1] == HIDPP_ERROR_INVALID_SUBID) {
 		hidpp->protocol_major = 1;
 		hidpp->protocol_minor = 0;
 		goto print_version;
 	}
 
-	/* the device might not be connected */
-	if (ret == HIDPP_ERROR_RESOURCE_ERROR ||
-	    ret == HIDPP_ERROR_UNKNOWN_DEVICE)
-		return -EIO;
-
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -1091,12 +1085,6 @@ static int hidpp_devicenametype_get_count(struct hidpp_device *hidpp,
 
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 		CMD_GET_DEVICE_NAME_TYPE_GET_COUNT, NULL, 0, &response);
-
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -1115,12 +1103,6 @@ static int hidpp_devicenametype_get_device_name(struct hidpp_device *hidpp,
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 		CMD_GET_DEVICE_NAME_TYPE_GET_DEVICE_NAME, &char_index, 1,
 		&response);
-
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -1273,14 +1255,6 @@ static int hidpp20_batterylevel_get_battery_capacity(struct hidpp_device *hidpp,
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 					  CMD_BATTERY_LEVEL_STATUS_GET_BATTERY_LEVEL_STATUS,
 					  NULL, 0, &response);
-	/* Ignore these intermittent errors */
-	if (ret == HIDPP_ERROR_RESOURCE_ERROR)
-		return -EIO;
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -1302,11 +1276,6 @@ static int hidpp20_batterylevel_get_battery_info(struct hidpp_device *hidpp,
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 					  CMD_BATTERY_LEVEL_STATUS_GET_BATTERY_CAPABILITY,
 					  NULL, 0, &response);
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -1456,12 +1425,6 @@ static int hidpp20_battery_get_battery_voltage(struct hidpp_device *hidpp,
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 					  CMD_BATTERY_VOLTAGE_GET_BATTERY_VOLTAGE,
 					  NULL, 0, &response);
-
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -1604,14 +1567,6 @@ static int hidpp20_unifiedbattery_get_capabilities(struct hidpp_device *hidpp,
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 					  CMD_UNIFIED_BATTERY_GET_CAPABILITIES,
 					  NULL, 0, &response);
-	/* Ignore these intermittent errors */
-	if (ret == HIDPP_ERROR_RESOURCE_ERROR)
-		return -EIO;
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -1694,14 +1649,6 @@ static int hidpp20_unifiedbattery_get_status(struct hidpp_device *hidpp,
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 					  CMD_UNIFIED_BATTERY_GET_STATUS,
 					  NULL, 0, &response);
-	/* Ignore these intermittent errors */
-	if (ret == HIDPP_ERROR_RESOURCE_ERROR)
-		return -EIO;
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -1958,13 +1905,8 @@ static bool hidpp20_get_adc_measurement_1f20(struct hidpp_device *hidpp,
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 					  CMD_ADC_MEASUREMENT_GET_ADC_MEASUREMENT,
 					  NULL, 0, &response);
-
-	if (ret) {
-		if (ret > 0)
-			hid_dbg(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-				__func__, ret);
+	if (ret)
 		return false;
-	}
 
 	*status = hidpp20_map_adc_measurement_1f20(params, voltage);
 	return true;
@@ -2138,11 +2080,6 @@ static int hidpp_solar_request_battery_event(struct hidpp_device *hidpp)
 					  hidpp->battery.solar_feature_index,
 					  CMD_SOLAR_SET_LIGHT_MEASURE,
 					  params, 2, &response);
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -2230,12 +2167,6 @@ static int hidpp_touchpad_fw_items_set(struct hidpp_device *hidpp,
 
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 		CMD_TOUCHPAD_FW_ITEMS_SET, &items->state, 1, &response);
-
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -2300,12 +2231,6 @@ static int hidpp_touchpad_get_raw_info(struct hidpp_device *hidpp,
 
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
 		CMD_TOUCHPAD_GET_RAW_INFO, NULL, 0, &response);
-
-	if (ret > 0) {
-		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
-			__func__, ret);
-		return -EPROTO;
-	}
 	if (ret)
 		return ret;
 
@@ -3493,13 +3418,8 @@ static int g920_get_config(struct hidpp_device *hidpp,
 					  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;
-	}
+	if (ret)
+		return ret;
 
 	data->num_effects = response.fap.params[0];
 
@@ -3702,8 +3622,6 @@ static int hidpp20_reprog_controls_get_count(struct hidpp_device *hidpp)
 
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index, cmd, NULL, 0,
 					  &response);
-	if (ret > 0)
-		return -EPROTO;
 	if (ret)
 		return ret;
 
@@ -3721,8 +3639,6 @@ static int hidpp20_reprog_controls_get_cid_info(struct hidpp_device *hidpp,
 
 	ret = hidpp_send_fap_command_sync(hidpp, feature_index, cmd, &index,
 					  sizeof(index), &response);
-	if (ret > 0)
-		return -EPROTO;
 	if (ret)
 		return ret;
 

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 18/33] HID: logitech-hidpp: rework G920 force-feedback initialization

[Dmitry Torokhov]

Querying G920 force-feedback capabilities inside .input_configured()
requires toggling device I/O mid-registration and conflates device
configuration with input setup. At the same time, the single allocated
state structure mixes per-device wheel settings with the lifetime of the
input force-feedback queue, complicating error handling and cleanup.

Exclude generic USB PID force feedback for validated HID++ devices and
query the G920 force-feedback configuration during probe while device
I/O is already active. Keep both probe query failures and force-feedback
initialization errors non-fatal so basic wheel and pedal inputs remain
usable without force feedback, while ensuring failed initialization
leaves input capabilities clean.

Split the overloaded force-feedback state by lifetime so device-level
G920 wheel settings are devm-managed for the bound device lifetime,
while the input force-feedback queue and slot tracking are allocated
with a flexible array and owned strictly by the force-feedback device.

Tested-by: Gideon Heinz <gideonheinz02@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 210 +++++++++++++++++++--------------------
 1 file changed, 101 insertions(+), 109 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index cf42626ab98d..bad6739c618c 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2341,15 +2341,19 @@ static void hidpp_touchpad_raw_xy_event(struct hidpp_device *hidpp_dev,
 
 struct hidpp_ff_private_data {
 	struct hidpp_device *hidpp;
+	struct workqueue_struct *wq;
+	atomic_t workqueue_size;
+	u8 num_effects;
+	int effect_ids[] __counted_by(num_effects);
+};
+
+struct g920_data {
+	struct hidpp_ff_private_data *ff;
 	u8 feature_index;
-	u8 version;
-	u16 gain;
-	s16 range;
 	u8 slot_autocenter;
 	u8 num_effects;
-	int *effect_ids;
-	struct workqueue_struct *wq;
-	atomic_t workqueue_size;
+	u16 gain;
+	u16 range;
 };
 
 struct hidpp_ff_work_data {
@@ -2413,6 +2417,7 @@ static void hidpp_ff_work_handler(struct work_struct *w)
 {
 	struct hidpp_ff_work_data *wd = container_of(w, struct hidpp_ff_work_data, work);
 	struct hidpp_ff_private_data *data = wd->data;
+	struct g920_data *g920 = data->hidpp->private_data;
 	struct hidpp_report response;
 	u8 slot;
 	int ret;
@@ -2420,7 +2425,7 @@ static void hidpp_ff_work_handler(struct work_struct *w)
 	/* add slot number if needed */
 	switch (wd->effect_id) {
 	case HIDPP_FF_EFFECTID_AUTOCENTER:
-		wd->params[0] = data->slot_autocenter;
+		wd->params[0] = g920->slot_autocenter;
 		break;
 	case HIDPP_FF_EFFECTID_NONE:
 		/* leave slot as zero */
@@ -2432,8 +2437,9 @@ static void hidpp_ff_work_handler(struct work_struct *w)
 	}
 
 	/* send command and wait for reply */
-	ret = hidpp_send_fap_command_sync(data->hidpp, data->feature_index,
-		wd->command, wd->params, wd->size, &response);
+	ret = hidpp_send_fap_command_sync(data->hidpp, g920->feature_index,
+					  wd->command, wd->params, wd->size,
+					  &response);
 
 	if (ret) {
 		hid_err(data->hidpp->hid_dev, "Failed to send command to device!\n");
@@ -2450,7 +2456,7 @@ static void hidpp_ff_work_handler(struct work_struct *w)
 				data->effect_ids[slot-1] = wd->effect_id;
 			else if (wd->effect_id >= HIDPP_FF_EFFECTID_AUTOCENTER)
 				/* autocenter spring uploaded */
-				data->slot_autocenter = slot;
+				g920->slot_autocenter = slot;
 		}
 		break;
 	case HIDPP_FF_DESTROY_EFFECT:
@@ -2461,14 +2467,14 @@ static void hidpp_ff_work_handler(struct work_struct *w)
 				data->effect_ids[slot-1] = -1;
 			else if (wd->effect_id >= HIDPP_FF_EFFECTID_AUTOCENTER)
 				/* autocenter spring destroyed */
-				data->slot_autocenter = 0;
+				g920->slot_autocenter = 0;
 		}
 		break;
 	case HIDPP_FF_SET_GLOBAL_GAINS:
-		data->gain = (wd->params[0] << 8) + wd->params[1];
+		g920->gain = (wd->params[0] << 8) + wd->params[1];
 		break;
 	case HIDPP_FF_SET_APERTURE:
-		data->range = (wd->params[0] << 8) + wd->params[1];
+		g920->range = (wd->params[0] << 8) + wd->params[1];
 		break;
 	default:
 		/* no action needed */
@@ -2715,19 +2721,17 @@ static void hidpp_ff_set_gain(struct input_dev *dev, u16 gain)
 static ssize_t hidpp_ff_range_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct hid_device *hid = to_hid_device(dev);
-	struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
-	struct input_dev *idev = hidinput->input;
-	struct hidpp_ff_private_data *data = idev->ff->private;
+	struct hidpp_device *hidpp = hid_get_drvdata(hid);
+	struct g920_data *g920 = hidpp->private_data;
 
-	return scnprintf(buf, PAGE_SIZE, "%u\n", data->range);
+	return scnprintf(buf, PAGE_SIZE, "%u\n", g920->range);
 }
 
 static ssize_t hidpp_ff_range_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
 {
 	struct hid_device *hid = to_hid_device(dev);
-	struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
-	struct input_dev *idev = hidinput->input;
-	struct hidpp_ff_private_data *data = idev->ff->private;
+	struct hidpp_device *hidpp = hid_get_drvdata(hid);
+	struct g920_data *g920 = hidpp->private_data;
 	u8 params[2];
 	int range = simple_strtoul(buf, NULL, 10);
 
@@ -2736,7 +2740,7 @@ static ssize_t hidpp_ff_range_store(struct device *dev, struct device_attribute
 	params[0] = range >> 8;
 	params[1] = range & 0x00FF;
 
-	hidpp_ff_queue_work(data, -1, HIDPP_FF_SET_APERTURE, params, ARRAY_SIZE(params));
+	hidpp_ff_queue_work(g920->ff, -1, HIDPP_FF_SET_APERTURE, params, ARRAY_SIZE(params));
 
 	return count;
 }
@@ -2746,13 +2750,15 @@ static DEVICE_ATTR(range, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH, hidpp
 static void hidpp_ff_destroy(struct ff_device *ff)
 {
 	struct hidpp_ff_private_data *data = ff->private;
-	struct hid_device *hid = data->hidpp->hid_dev;
+	struct hidpp_device *hidpp = data->hidpp;
+	struct hid_device *hid = hidpp->hid_dev;
+	struct g920_data *g920 = hidpp->private_data;
 
 	hid_info(hid, "Unloading HID++ force feedback.\n");
 
 	device_remove_file(&hid->dev, &dev_attr_range);
 	destroy_workqueue(data->wq);
-	kfree(data->effect_ids);
+	g920->ff = NULL;
 }
 
 static int g920_input_open(struct input_dev *dev)
@@ -2771,16 +2777,14 @@ static void g920_input_close(struct input_dev *dev)
 	hid_hw_close(hid);
 }
 
-static int hidpp_ff_init(struct hidpp_device *hidpp,
-			 struct hidpp_ff_private_data *data)
+static int hidpp_ff_init(struct hidpp_device *hidpp, struct input_dev *dev)
 {
 	struct hid_device *hid = hidpp->hid_dev;
-	struct hid_input *hidinput;
-	struct input_dev *dev;
+	struct g920_data *g920 = hidpp->private_data;
 	struct usb_device_descriptor *udesc;
 	u16 bcdDevice;
 	struct ff_device *ff;
-	int error, j, num_slots = data->num_effects;
+	int error, j, num_slots = g920->num_effects;
 	u8 version;
 
 	if (!hid_is_usb(hid)) {
@@ -2788,17 +2792,22 @@ static int hidpp_ff_init(struct hidpp_device *hidpp,
 		return -ENODEV;
 	}
 
-	if (list_empty(&hid->inputs)) {
-		hid_err(hid, "no inputs found\n");
-		return -ENODEV;
-	}
-	hidinput = list_entry(hid->inputs.next, struct hid_input, list);
-	dev = hidinput->input;
+	struct hidpp_ff_private_data *data __free(kfree) =
+		kzalloc_flex(*data, effect_ids, num_slots);
+	if (!data)
+		return -ENOMEM;
 
-	if (!dev) {
-		hid_err(hid, "Struct input_dev not set!\n");
-		return -EINVAL;
-	}
+	data->hidpp = hidpp;
+	data->num_effects = num_slots;
+	for (j = 0; j < num_slots; j++)
+		data->effect_ids[j] = -1;
+
+	/* init the hardware command queue */
+	atomic_set(&data->workqueue_size, 0);
+
+	data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
+	if (!data->wq)
+		return -ENOMEM;
 
 	/* Get firmware release */
 	udesc = &(hid_to_usb_dev(hid)->descriptor);
@@ -2813,38 +2822,16 @@ static int hidpp_ff_init(struct hidpp_device *hidpp,
 			set_bit(hidpp_ff_effects_v2[j], dev->ffbit);
 
 	error = input_ff_create(dev, num_slots - HIDPP_FF_RESERVED_SLOTS);
-
 	if (error) {
-		hid_err(dev, "Failed to create FF device!\n");
+		hid_err(hid, "Failed to create FF device!\n");
+		bitmap_zero(dev->ffbit, FF_CNT);
+		destroy_workqueue(data->wq);
 		return error;
 	}
-	/*
-	 * Create a copy of passed data, so we can transfer memory
-	 * ownership to FF core
-	 */
-	data = kmemdup(data, sizeof(*data), GFP_KERNEL);
-	if (!data) {
-		error = -ENOMEM;
-		goto err_destroy_ff;
-	}
-	data->effect_ids = kzalloc_objs(int, num_slots);
-	if (!data->effect_ids) {
-		error = -ENOMEM;
-		goto err_free_data;
-	}
-	data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
-	if (!data->wq) {
-		error = -ENOMEM;
-		goto err_free_effect_ids;
-	}
-
-	data->hidpp = hidpp;
-	data->version = version;
-	for (j = 0; j < num_slots; j++)
-		data->effect_ids[j] = -1;
 
 	ff = dev->ff;
-	ff->private = data;
+	g920->ff = data;
+	ff->private = no_free_ptr(data);
 
 	ff->upload = hidpp_ff_upload_effect;
 	ff->erase = hidpp_ff_erase_effect;
@@ -2861,21 +2848,10 @@ static int hidpp_ff_init(struct hidpp_device *hidpp,
 	if (error)
 		hid_warn(hidpp->hid_dev, "Unable to create sysfs interface for \"range\", errno %d!\n", error);
 
-	/* init the hardware command queue */
-	atomic_set(&data->workqueue_size, 0);
-
 	hid_info(hid, "Force feedback support loaded (firmware release %d).\n",
 		 version);
 
 	return 0;
-
-err_free_effect_ids:
-	kfree(data->effect_ids);
-err_free_data:
-	kfree(data);
-err_destroy_ff:
-	input_ff_destroy(dev);
-	return error;
 }
 
 /* ************************************************************************** */
@@ -3376,7 +3352,7 @@ static int k400_connect(struct hid_device *hdev)
 #define HIDPP_PAGE_G920_FORCE_FEEDBACK			0x8123
 
 static int g920_ff_set_autocenter(struct hidpp_device *hidpp,
-				  struct hidpp_ff_private_data *data)
+				  struct g920_data *data)
 {
 	struct hidpp_report response;
 	u8 params[HIDPP_AUTOCENTER_PARAMS_LENGTH] = {
@@ -3391,71 +3367,83 @@ static int g920_ff_set_autocenter(struct hidpp_device *hidpp,
 					  HIDPP_FF_DOWNLOAD_EFFECT,
 					  params, ARRAY_SIZE(params),
 					  &response);
-	if (ret)
+	if (ret) {
 		hid_warn(hidpp->hid_dev, "Failed to autocenter device!\n");
-	else
-		data->slot_autocenter = response.fap.params[0];
+		return ret;
+	}
 
-	return ret;
+	data->slot_autocenter = response.fap.params[0];
+	return 0;
 }
 
-static int g920_get_config(struct hidpp_device *hidpp,
-			   struct hidpp_ff_private_data *data)
+static int g920_get_config(struct hidpp_device *hidpp)
 {
+	struct g920_data data = {};
 	struct hidpp_report response;
 	int ret;
 
-	memset(data, 0, sizeof(*data));
-
 	/* Find feature and store for later use */
 	ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_G920_FORCE_FEEDBACK,
-				     &data->feature_index);
+				     &data.feature_index);
 	if (ret)
 		return ret;
 
 	/* Read number of slots available in device */
-	ret = hidpp_send_fap_command_sync(hidpp, data->feature_index,
+	ret = hidpp_send_fap_command_sync(hidpp, data.feature_index,
 					  HIDPP_FF_GET_INFO,
 					  NULL, 0,
 					  &response);
 	if (ret)
 		return ret;
 
-	data->num_effects = response.fap.params[0];
+	if (response.fap.params[0] <= HIDPP_FF_RESERVED_SLOTS ||
+	    response.fap.params[0] > FF_MAX_EFFECTS + HIDPP_FF_RESERVED_SLOTS) {
+		hid_err(hidpp->hid_dev,
+			"Invalid force feedback slot count %u\n",
+			response.fap.params[0]);
+		return -EINVAL;
+	}
+
+	data.num_effects = response.fap.params[0];
 
 	/* reset all forces */
-	ret = hidpp_send_fap_command_sync(hidpp, data->feature_index,
+	ret = hidpp_send_fap_command_sync(hidpp, data.feature_index,
 					  HIDPP_FF_RESET_ALL,
 					  NULL, 0,
 					  &response);
 	if (ret)
 		hid_warn(hidpp->hid_dev, "Failed to reset all forces!\n");
 
-	ret = hidpp_send_fap_command_sync(hidpp, data->feature_index,
+	ret = hidpp_send_fap_command_sync(hidpp, data.feature_index,
 					  HIDPP_FF_GET_APERTURE,
 					  NULL, 0,
 					  &response);
-	if (ret) {
+	if (ret)
 		hid_warn(hidpp->hid_dev,
 			 "Failed to read range from device!\n");
-	}
-	data->range = ret ?
+	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,
+	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 ?
+	data.gain = ret ?
 		0xffff : get_unaligned_be16(&response.fap.params[0]);
 
 	/* ignore boost value at response.fap.params[2] */
 
-	return g920_ff_set_autocenter(hidpp, data);
+	ret = g920_ff_set_autocenter(hidpp, &data);
+	if (ret)
+		return ret;
+
+	hidpp->private_data = devm_kmemdup(&hidpp->hid_dev->dev, &data,
+					   sizeof(data), GFP_KERNEL);
+	return hidpp->private_data ? 0 : -ENOMEM;
 }
 
 /* -------------------------------------------------------------------------- */
@@ -4040,21 +4028,15 @@ static int hidpp_input_configured(struct hid_device *hdev, struct hid_input *hid
 		return 0;
 
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
-		struct hidpp_ff_private_data data;
-
 		if (!list_is_first(&hidinput->list, &hdev->inputs))
 			return 0;
 
-		hid_device_io_start(hdev);
-		ret = g920_get_config(hidpp, &data);
-		hid_device_io_stop(hdev);
-		if (!ret)
-			ret = hidpp_ff_init(hidpp, &data);
-
-		if (ret) {
-			hid_warn(hidpp->hid_dev,
-				 "Unable to initialize force feedback support, errno %d\n",
-				 ret);
+		if (hidpp->private_data) {
+			ret = hidpp_ff_init(hidpp, input);
+			if (ret)
+				hid_warn(hdev,
+					 "Unable to initialize force feedback support, errno %d\n",
+					 ret);
 		}
 	}
 
@@ -4633,7 +4615,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	struct hidpp_device *hidpp;
 	int ret;
-	unsigned int connect_mask = HID_CONNECT_DEFAULT;
+	unsigned int connect_mask;
 
 	/* report_fixup needs drvdata to be set before we call hid_parse */
 	hidpp = devm_kzalloc_obj(&hdev->dev, *hidpp);
@@ -4723,6 +4705,16 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	else
 		hidpp_non_unifying_init(hidpp);
 
+	if ((hidpp->quirks & HIDPP_QUIRK_CLASS_G920) && hid_is_usb(hdev)) {
+		ret = g920_get_config(hidpp);
+		if (ret)
+			hid_warn(hdev,
+				 "Unable to query force feedback config, errno %d\n",
+				 ret);
+	}
+
+	/* None of the HID++ devices use generic USB PID force-feedback. */
+	connect_mask = HID_CONNECT_DEFAULT & ~HID_CONNECT_FF;
 	if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT)
 		connect_mask &= ~HID_CONNECT_HIDINPUT;
 

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 19/33] HID: logitech-hidpp: use sysfs_emit() in hidpp_ff_range_show()

[Dmitry Torokhov]

Formatting the G920 wheel range sysfs output with scnprintf() passes
buffer sizes manually instead of using the standard sysfs helper. Use
sysfs_emit() in hidpp_ff_range_show() to align with current kernel sysfs
conventions.

Tested-by: Gideon Heinz <gideonheinz02@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index bad6739c618c..03d2e9077675 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2724,7 +2724,7 @@ static ssize_t hidpp_ff_range_show(struct device *dev, struct device_attribute *
 	struct hidpp_device *hidpp = hid_get_drvdata(hid);
 	struct g920_data *g920 = hidpp->private_data;
 
-	return scnprintf(buf, PAGE_SIZE, "%u\n", g920->range);
+	return sysfs_emit(buf, "%u\n", g920->range);
 }
 
 static ssize_t hidpp_ff_range_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 20/33] HID: logitech-hidpp: manage G920 range sysfs attribute via dev_groups

[Dmitry Torokhov]

Creating and removing the G920 range attribute manually inside force
feedback initialization and destruction ties sysfs cleanup to input
device release rather than driver unbind, leaving a dangling attribute
if userspace holds an open event handle across disconnect. Manual sysfs
management also adds avoidable boilerplate.

Register the attribute group through driver dev_groups with a visibility
callback checking initialized G920 force-feedback state, allowing driver
core to create and tear down the attribute cleanly around probe and
remove.

Tested-by: Gideon Heinz <gideonheinz02@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 45 +++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 03d2e9077675..f24fa70d272e 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2747,16 +2747,47 @@ static ssize_t hidpp_ff_range_store(struct device *dev, struct device_attribute
 
 static DEVICE_ATTR(range, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH, hidpp_ff_range_show, hidpp_ff_range_store);
 
+static struct attribute *g920_range_attrs[] = {
+	&dev_attr_range.attr,
+	NULL
+};
+
+static umode_t g920_range_attr_is_visible(struct kobject *kobj,
+					  struct attribute *attr, int idx)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct hid_device *hdev = to_hid_device(dev);
+	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
+	struct g920_data *g920;
+
+	if (!hidpp || !(hidpp->quirks & HIDPP_QUIRK_CLASS_G920))
+		return 0;
+
+	g920 = hidpp->private_data;
+	if (!g920 || !g920->ff)
+		return 0;
+
+	return attr->mode;
+}
+
+static const struct attribute_group g920_range_group = {
+	.attrs = g920_range_attrs,
+	.is_visible = g920_range_attr_is_visible,
+};
+
+static const struct attribute_group *hidpp_dev_groups[] = {
+	&g920_range_group,
+	NULL
+};
+
 static void hidpp_ff_destroy(struct ff_device *ff)
 {
 	struct hidpp_ff_private_data *data = ff->private;
 	struct hidpp_device *hidpp = data->hidpp;
-	struct hid_device *hid = hidpp->hid_dev;
 	struct g920_data *g920 = hidpp->private_data;
 
-	hid_info(hid, "Unloading HID++ force feedback.\n");
+	hid_info(hidpp->hid_dev, "Unloading HID++ force feedback.\n");
 
-	device_remove_file(&hid->dev, &dev_attr_range);
 	destroy_workqueue(data->wq);
 	g920->ff = NULL;
 }
@@ -2843,11 +2874,6 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, struct input_dev *dev)
 	dev->open = g920_input_open;
 	dev->close = g920_input_close;
 
-	/* 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);
-
 	hid_info(hid, "Force feedback support loaded (firmware release %d).\n",
 		 version);
 
@@ -4985,6 +5011,9 @@ static struct hid_driver hidpp_driver = {
 	.input_configured = hidpp_input_configured,
 	.input_mapping = hidpp_input_mapping,
 	.input_mapped = hidpp_input_mapped,
+	.driver = {
+		.dev_groups = hidpp_dev_groups,
+	},
 };
 
 module_hid_driver(hidpp_driver);

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 21/33] HID: logitech-hidpp: execute G920 range sysfs store synchronously

[Dmitry Torokhov]

Writing to the G920 range sysfs attribute currently queues an
asynchronous work item to issue the HIDPP_FF_SET_APERTURE command on
the force-feedback workqueue. Because the sysfs store returns success
immediately without waiting for the hardware transfer to complete, user
space cannot know whether the setting succeeded and a subsequent read of
the range attribute may return stale data before the workqueue drains.

Execute the aperture setting command synchronously within the range
sysfs store method. Open and close the underlying transport around the
command, clamp the input range between 180 and 900 degrees, update the
cached range on success, and return errors directly to user space. Read
and write the cached range with READ_ONCE and WRITE_ONCE to prevent
torn reads.

Because the sysfs store now sends the command directly using the HID++
device handle, the backpointer to the force-feedback private data is no
longer needed in struct g920_data. Drop the unused pointer and simplify
the attribute visibility check.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index f24fa70d272e..f7fefff0aed4 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2348,7 +2348,6 @@ struct hidpp_ff_private_data {
 };
 
 struct g920_data {
-	struct hidpp_ff_private_data *ff;
 	u8 feature_index;
 	u8 slot_autocenter;
 	u8 num_effects;
@@ -2473,9 +2472,6 @@ static void hidpp_ff_work_handler(struct work_struct *w)
 	case HIDPP_FF_SET_GLOBAL_GAINS:
 		g920->gain = (wd->params[0] << 8) + wd->params[1];
 		break;
-	case HIDPP_FF_SET_APERTURE:
-		g920->range = (wd->params[0] << 8) + wd->params[1];
-		break;
 	default:
 		/* no action needed */
 		break;
@@ -2724,7 +2720,7 @@ static ssize_t hidpp_ff_range_show(struct device *dev, struct device_attribute *
 	struct hidpp_device *hidpp = hid_get_drvdata(hid);
 	struct g920_data *g920 = hidpp->private_data;
 
-	return sysfs_emit(buf, "%u\n", g920->range);
+	return sysfs_emit(buf, "%u\n", READ_ONCE(g920->range));
 }
 
 static ssize_t hidpp_ff_range_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
@@ -2732,15 +2728,34 @@ static ssize_t hidpp_ff_range_store(struct device *dev, struct device_attribute
 	struct hid_device *hid = to_hid_device(dev);
 	struct hidpp_device *hidpp = hid_get_drvdata(hid);
 	struct g920_data *g920 = hidpp->private_data;
+	struct hidpp_report response;
 	u8 params[2];
-	int range = simple_strtoul(buf, NULL, 10);
+	u16 range;
+	int error;
 
-	range = clamp(range, 180, 900);
+	error = kstrtou16(buf, 10, &range);
+	if (error)
+		return error;
+
+	range = clamp_val(range, 180, 900);
 
 	params[0] = range >> 8;
 	params[1] = range & 0x00FF;
 
-	hidpp_ff_queue_work(g920->ff, -1, HIDPP_FF_SET_APERTURE, params, ARRAY_SIZE(params));
+	error = hid_hw_open(hid);
+	if (error)
+		return error;
+
+	error = hidpp_send_fap_command_sync(hidpp, g920->feature_index,
+					    HIDPP_FF_SET_APERTURE,
+					    params, ARRAY_SIZE(params),
+					    &response);
+	hid_hw_close(hid);
+
+	if (error)
+		return error;
+
+	WRITE_ONCE(g920->range, range);
 
 	return count;
 }
@@ -2764,7 +2779,7 @@ static umode_t g920_range_attr_is_visible(struct kobject *kobj,
 		return 0;
 
 	g920 = hidpp->private_data;
-	if (!g920 || !g920->ff)
+	if (!g920)
 		return 0;
 
 	return attr->mode;
@@ -2784,12 +2799,10 @@ static void hidpp_ff_destroy(struct ff_device *ff)
 {
 	struct hidpp_ff_private_data *data = ff->private;
 	struct hidpp_device *hidpp = data->hidpp;
-	struct g920_data *g920 = hidpp->private_data;
 
 	hid_info(hidpp->hid_dev, "Unloading HID++ force feedback.\n");
 
 	destroy_workqueue(data->wq);
-	g920->ff = NULL;
 }
 
 static int g920_input_open(struct input_dev *dev)
@@ -2861,7 +2874,6 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, struct input_dev *dev)
 	}
 
 	ff = dev->ff;
-	g920->ff = data;
 	ff->private = no_free_ptr(data);
 
 	ff->upload = hidpp_ff_upload_effect;

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 22/33] HID: logitech-hidpp: shut down G920 force feedback on input unregister

[Dmitry Torokhov]

Force-feedback teardown currently runs only in the force-feedback
destroy callback, which is deferred until the last open event handle
is closed. If userspace holds an open handle across USB disconnect or
driver unbind, device removal frees driver state and destroys the
command mutex while queued force-feedback work is still active, causing
use-after-free errors.

Move workqueue teardown into the force-feedback stop callback so input
unregistration drains queued work while driver and HID state are still
alive. Keep calling stop from destroy as a safety net for registration
failures, guarded so repeated teardown is a safe no-op.

Fixes: 08c453f6d073 ("HID: logitech-hidpp: do all FF cleanup in hidpp_ff_destroy()")
Tested-by: Gideon Heinz <gideonheinz02@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index f7fefff0aed4..ceaa57af6eab 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2795,14 +2795,32 @@ static const struct attribute_group *hidpp_dev_groups[] = {
 	NULL
 };
 
-static void hidpp_ff_destroy(struct ff_device *ff)
+static void hidpp_ff_stop(struct ff_device *ff)
 {
 	struct hidpp_ff_private_data *data = ff->private;
-	struct hidpp_device *hidpp = data->hidpp;
+	struct hidpp_device *hidpp;
+
+	hidpp = data->hidpp;
+	if (!hidpp)
+		return;
 
 	hid_info(hidpp->hid_dev, "Unloading HID++ force feedback.\n");
 
 	destroy_workqueue(data->wq);
+
+	data->wq = NULL;
+	data->hidpp = NULL;
+}
+
+static void hidpp_ff_destroy(struct ff_device *ff)
+{
+	/*
+	 * Workqueue teardown normally runs in ff->stop() when the input device
+	 * is unregistered. Still call it here as a safety net for error paths
+	 * before registration completes; once stopped, it becomes a safe no-op
+	 * that never touches freed driver data.
+	 */
+	hidpp_ff_stop(ff);
 }
 
 static int g920_input_open(struct input_dev *dev)
@@ -2881,6 +2899,7 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, struct input_dev *dev)
 	ff->playback = hidpp_ff_playback;
 	ff->set_gain = hidpp_ff_set_gain;
 	ff->set_autocenter = hidpp_ff_set_autocenter;
+	ff->stop = hidpp_ff_stop;
 	ff->destroy = hidpp_ff_destroy;
 
 	dev->open = g920_input_open;

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 23/33] HID: logitech-hidpp: index G920 force-feedback slots by effect ID

[Dmitry Torokhov]

The G920 driver previously maintained an effect_ids array indexed by
hardware slot minus 1 to store the associated input effect ID. Locating
the hardware slot assigned to an effect ID required iterating through
the array in hidpp_ff_find_effect() on every playback, destroy, or
update request.

Reverse the mapping by storing an array of hardware slot numbers indexed
directly by the 0-based input effect ID. Because input effect IDs are
allocated in the range 0 to num_effects - 1, looking up a slot becomes
an O(1) array access and hidpp_ff_find_effect() can be removed. Sizing
the array as u8 slots[] also allows zero to represent an unassigned slot
because hardware slot numbers returned by the device are non-zero,
eliminating the need to initialize entries to -1.

In g920_get_config(), deduct the reserved autocenter slot count when
storing num_effects so that data.num_effects reflects the number of
user-assignable effect slots and can be passed directly to
input_ff_create().

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index ceaa57af6eab..9526286ba406 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2344,7 +2344,7 @@ struct hidpp_ff_private_data {
 	struct workqueue_struct *wq;
 	atomic_t workqueue_size;
 	u8 num_effects;
-	int effect_ids[] __counted_by(num_effects);
+	u8 slots[] __counted_by(num_effects);
 };
 
 struct g920_data {
@@ -2401,17 +2401,6 @@ static const char *HIDPP_FF_CONDITION_NAMES[] = {
 };
 
 
-static u8 hidpp_ff_find_effect(struct hidpp_ff_private_data *data, int effect_id)
-{
-	int i;
-
-	for (i = 0; i < data->num_effects; i++)
-		if (data->effect_ids[i] == effect_id)
-			return i+1;
-
-	return 0;
-}
-
 static void hidpp_ff_work_handler(struct work_struct *w)
 {
 	struct hidpp_ff_work_data *wd = container_of(w, struct hidpp_ff_work_data, work);
@@ -2431,7 +2420,7 @@ static void hidpp_ff_work_handler(struct work_struct *w)
 		break;
 	default:
 		/* find current slot for effect */
-		wd->params[0] = hidpp_ff_find_effect(data, wd->effect_id);
+		wd->params[0] = data->slots[wd->effect_id];
 		break;
 	}
 
@@ -2449,10 +2438,10 @@ static void hidpp_ff_work_handler(struct work_struct *w)
 	switch (wd->command) {
 	case HIDPP_FF_DOWNLOAD_EFFECT:
 		slot = response.fap.params[0];
-		if (slot > 0 && slot <= data->num_effects) {
+		if (slot > 0) {
 			if (wd->effect_id >= 0)
 				/* regular effect uploaded */
-				data->effect_ids[slot-1] = wd->effect_id;
+				data->slots[wd->effect_id] = slot;
 			else if (wd->effect_id >= HIDPP_FF_EFFECTID_AUTOCENTER)
 				/* autocenter spring uploaded */
 				g920->slot_autocenter = slot;
@@ -2460,10 +2449,10 @@ static void hidpp_ff_work_handler(struct work_struct *w)
 		break;
 	case HIDPP_FF_DESTROY_EFFECT:
 		slot = wd->params[0];
-		if (slot > 0 && slot <= data->num_effects) {
+		if (slot > 0) {
 			if (wd->effect_id >= 0)
 				/* regular effect destroyed */
-				data->effect_ids[slot-1] = -1;
+				data->slots[wd->effect_id] = 0;
 			else if (wd->effect_id >= HIDPP_FF_EFFECTID_AUTOCENTER)
 				/* autocenter spring destroyed */
 				g920->slot_autocenter = 0;
@@ -2855,14 +2844,12 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, struct input_dev *dev)
 	}
 
 	struct hidpp_ff_private_data *data __free(kfree) =
-		kzalloc_flex(*data, effect_ids, num_slots);
+		kzalloc_flex(*data, slots, num_slots);
 	if (!data)
 		return -ENOMEM;
 
 	data->hidpp = hidpp;
 	data->num_effects = num_slots;
-	for (j = 0; j < num_slots; j++)
-		data->effect_ids[j] = -1;
 
 	/* init the hardware command queue */
 	atomic_set(&data->workqueue_size, 0);
@@ -2883,7 +2870,7 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, struct input_dev *dev)
 		for (j = 0; hidpp_ff_effects_v2[j] >= 0; j++)
 			set_bit(hidpp_ff_effects_v2[j], dev->ffbit);
 
-	error = input_ff_create(dev, num_slots - HIDPP_FF_RESERVED_SLOTS);
+	error = input_ff_create(dev, num_slots);
 	if (error) {
 		hid_err(hid, "Failed to create FF device!\n");
 		bitmap_zero(dev->ffbit, FF_CNT);
@@ -3461,7 +3448,7 @@ static int g920_get_config(struct hidpp_device *hidpp)
 		return -EINVAL;
 	}
 
-	data.num_effects = response.fap.params[0];
+	data.num_effects = response.fap.params[0] - HIDPP_FF_RESERVED_SLOTS;
 
 	/* reset all forces */
 	ret = hidpp_send_fap_command_sync(hidpp, data.feature_index,

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 24/33] HID: logitech-hidpp: execute G920 effect upload and erase synchronously

[Dmitry Torokhov]

The input force-feedback subsystem expects upload and erase operations
to execute synchronously in process context, returning errors if the
device rejects the effect or communication fails so that callers can
react accordingly. The G920 driver previously queued these requests onto
an asynchronous workqueue and immediately returned success to the input
core, hiding transfer errors and allowing user space to race ahead
before the hardware effect was actually uploaded or removed.

Convert hidpp_ff_upload_effect() and hidpp_ff_erase_effect() to execute
synchronously using hidpp_send_fap_command_sync(). Before issuing the
hardware command, flush any in-flight playback commands on the workqueue
to ensure ordering. Open and close the underlying transport around the
transfer, update the slot table directly upon completion, and report
errors back to the caller on upload.

Because upload and erase no longer go through the workqueue, drop
effect ID tracking from the asynchronous work item and queue playback,
autocenter, and gain commands with their slot parameters already
populated by the caller.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 126 +++++++++++++++++++--------------------
 1 file changed, 60 insertions(+), 66 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 9526286ba406..20f7e8367ee1 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2332,8 +2332,6 @@ static void hidpp_touchpad_raw_xy_event(struct hidpp_device *hidpp_dev,
 
 #define HIDPP_FF_EFFECT_AUTOSTART	0x80
 
-#define HIDPP_FF_EFFECTID_NONE		-1
-#define HIDPP_FF_EFFECTID_AUTOCENTER	-2
 #define HIDPP_AUTOCENTER_PARAMS_LENGTH	18
 
 #define HIDPP_FF_MAX_PARAMS	20
@@ -2358,7 +2356,6 @@ struct g920_data {
 struct hidpp_ff_work_data {
 	struct work_struct work;
 	struct hidpp_ff_private_data *data;
-	int effect_id;
 	u8 command;
 	u8 params[HIDPP_FF_MAX_PARAMS];
 	u8 size;
@@ -2407,71 +2404,23 @@ static void hidpp_ff_work_handler(struct work_struct *w)
 	struct hidpp_ff_private_data *data = wd->data;
 	struct g920_data *g920 = data->hidpp->private_data;
 	struct hidpp_report response;
-	u8 slot;
 	int ret;
 
-	/* add slot number if needed */
-	switch (wd->effect_id) {
-	case HIDPP_FF_EFFECTID_AUTOCENTER:
-		wd->params[0] = g920->slot_autocenter;
-		break;
-	case HIDPP_FF_EFFECTID_NONE:
-		/* leave slot as zero */
-		break;
-	default:
-		/* find current slot for effect */
-		wd->params[0] = data->slots[wd->effect_id];
-		break;
-	}
-
 	/* send command and wait for reply */
 	ret = hidpp_send_fap_command_sync(data->hidpp, g920->feature_index,
 					  wd->command, wd->params, wd->size,
 					  &response);
-
-	if (ret) {
+	if (ret)
 		hid_err(data->hidpp->hid_dev, "Failed to send command to device!\n");
-		goto out;
-	}
-
-	/* parse return data */
-	switch (wd->command) {
-	case HIDPP_FF_DOWNLOAD_EFFECT:
-		slot = response.fap.params[0];
-		if (slot > 0) {
-			if (wd->effect_id >= 0)
-				/* regular effect uploaded */
-				data->slots[wd->effect_id] = slot;
-			else if (wd->effect_id >= HIDPP_FF_EFFECTID_AUTOCENTER)
-				/* autocenter spring uploaded */
-				g920->slot_autocenter = slot;
-		}
-		break;
-	case HIDPP_FF_DESTROY_EFFECT:
-		slot = wd->params[0];
-		if (slot > 0) {
-			if (wd->effect_id >= 0)
-				/* regular effect destroyed */
-				data->slots[wd->effect_id] = 0;
-			else if (wd->effect_id >= HIDPP_FF_EFFECTID_AUTOCENTER)
-				/* autocenter spring destroyed */
-				g920->slot_autocenter = 0;
-		}
-		break;
-	case HIDPP_FF_SET_GLOBAL_GAINS:
+	else if (wd->command == HIDPP_FF_SET_GLOBAL_GAINS)
 		g920->gain = (wd->params[0] << 8) + wd->params[1];
-		break;
-	default:
-		/* no action needed */
-		break;
-	}
 
-out:
 	atomic_dec(&data->workqueue_size);
 	kfree(wd);
 }
 
-static int hidpp_ff_queue_work(struct hidpp_ff_private_data *data, int effect_id, u8 command, u8 *params, u8 size)
+static int hidpp_ff_queue_work(struct hidpp_ff_private_data *data, u8 command,
+			       const u8 *params, u8 size)
 {
 	struct hidpp_ff_work_data *wd;
 	int s;
@@ -2483,7 +2432,6 @@ static int hidpp_ff_queue_work(struct hidpp_ff_private_data *data, int effect_id
 	INIT_WORK(&wd->work, hidpp_ff_work_handler);
 
 	wd->data = data;
-	wd->effect_id = effect_id;
 	wd->command = command;
 	wd->size = size;
 	memcpy(wd->params, params, size);
@@ -2501,11 +2449,16 @@ static int hidpp_ff_queue_work(struct hidpp_ff_private_data *data, int effect_id
 static int hidpp_ff_upload_effect(struct input_dev *dev, struct ff_effect *effect, struct ff_effect *old)
 {
 	struct hidpp_ff_private_data *data = dev->ff->private;
+	struct hidpp_device *hidpp = data->hidpp;
+	struct hid_device *hid = hidpp->hid_dev;
+	struct g920_data *g920 = hidpp->private_data;
+	struct hidpp_report response;
 	u8 params[20];
 	u8 size;
-	int force;
+	int force, error;
 
 	/* set common parameters */
+	params[0] = old ? data->slots[effect->id] : 0;
 	params[2] = effect->replay.length >> 8;
 	params[3] = effect->replay.length & 255;
 	params[4] = effect->replay.delay >> 8;
@@ -2552,7 +2505,9 @@ static int hidpp_ff_upload_effect(struct input_dev *dev, struct ff_effect *effec
 			params[1] = HIDPP_FF_EFFECT_PERIODIC_TRIANGLE;
 			break;
 		default:
-			hid_err(data->hidpp->hid_dev, "Unexpected periodic waveform type %i!\n", effect->u.periodic.waveform);
+			hid_err(hidpp->hid_dev,
+				"Unexpected periodic waveform type %i!\n",
+				effect->u.periodic.waveform);
 			return -EINVAL;
 		}
 		force = (effect->u.periodic.magnitude * fixp_sin16((effect->direction * 360) >> 16)) >> 15;
@@ -2637,11 +2592,26 @@ static int hidpp_ff_upload_effect(struct input_dev *dev, struct ff_effect *effec
 				effect->u.condition[0].center);
 		break;
 	default:
-		hid_err(data->hidpp->hid_dev, "Unexpected force type %i!\n", effect->type);
+		hid_err(hidpp->hid_dev, "Unexpected force type %i!\n", effect->type);
 		return -EINVAL;
 	}
 
-	return hidpp_ff_queue_work(data, effect->id, HIDPP_FF_DOWNLOAD_EFFECT, params, size);
+	flush_workqueue(data->wq);
+
+	error = hid_hw_open(hid);
+	if (error)
+		return error;
+
+	error = hidpp_send_fap_command_sync(hidpp, g920->feature_index,
+					    HIDPP_FF_DOWNLOAD_EFFECT,
+					    params, size, &response);
+	hid_hw_close(hid);
+
+	if (error)
+		return error;
+
+	data->slots[effect->id] = response.fap.params[0];
+	return 0;
 }
 
 static int hidpp_ff_playback(struct input_dev *dev, int effect_id, int value)
@@ -2649,31 +2619,55 @@ static int hidpp_ff_playback(struct input_dev *dev, int effect_id, int value)
 	struct hidpp_ff_private_data *data = dev->ff->private;
 	u8 params[2];
 
+	params[0] = data->slots[effect_id];
+	if (!dev->ready || !params[0])
+		return 0;
+
 	params[1] = value ? HIDPP_FF_EFFECT_STATE_PLAY : HIDPP_FF_EFFECT_STATE_STOP;
 
-	dbg_hid("St%sing playback of effect %d.\n", value?"art":"opp", effect_id);
+	dbg_hid("St%sing playback of effect %d.\n", value ? "art" : "opp", effect_id);
 
-	return hidpp_ff_queue_work(data, effect_id, HIDPP_FF_SET_EFFECT_STATE, params, ARRAY_SIZE(params));
+	return hidpp_ff_queue_work(data, HIDPP_FF_SET_EFFECT_STATE, params, ARRAY_SIZE(params));
 }
 
 static int hidpp_ff_erase_effect(struct input_dev *dev, int effect_id)
 {
 	struct hidpp_ff_private_data *data = dev->ff->private;
-	u8 slot = 0;
+	struct hidpp_device *hidpp = data->hidpp;
+	struct hid_device *hid = hidpp->hid_dev;
+	struct g920_data *g920 = hidpp->private_data;
+	struct hidpp_report response;
+	u8 slot;
 
 	dbg_hid("Erasing effect %d.\n", effect_id);
 
-	return hidpp_ff_queue_work(data, effect_id, HIDPP_FF_DESTROY_EFFECT, &slot, 1);
+	flush_workqueue(data->wq);
+
+	slot = data->slots[effect_id];
+	data->slots[effect_id] = 0;
+
+	if (slot) {
+		if (hid_hw_open(hid) == 0) {
+			hidpp_send_fap_command_sync(hidpp, g920->feature_index,
+						    HIDPP_FF_DESTROY_EFFECT,
+						    &slot, 1, &response);
+			hid_hw_close(hid);
+		}
+	}
+
+	return 0;
 }
 
 static void hidpp_ff_set_autocenter(struct input_dev *dev, u16 magnitude)
 {
 	struct hidpp_ff_private_data *data = dev->ff->private;
+	struct g920_data *g920 = data->hidpp->private_data;
 	u8 params[HIDPP_AUTOCENTER_PARAMS_LENGTH];
 
 	dbg_hid("Setting autocenter to %d.\n", magnitude);
 
 	/* start a standard spring effect */
+	params[0] = g920->slot_autocenter;
 	params[1] = HIDPP_FF_EFFECT_SPRING | HIDPP_FF_EFFECT_AUTOSTART;
 	/* zero delay and duration */
 	params[2] = params[3] = params[4] = params[5] = 0;
@@ -2685,7 +2679,7 @@ static void hidpp_ff_set_autocenter(struct input_dev *dev, u16 magnitude)
 	/* zero deadband and center */
 	params[10] = params[11] = params[12] = params[13] = 0;
 
-	hidpp_ff_queue_work(data, HIDPP_FF_EFFECTID_AUTOCENTER, HIDPP_FF_DOWNLOAD_EFFECT, params, ARRAY_SIZE(params));
+	hidpp_ff_queue_work(data, HIDPP_FF_DOWNLOAD_EFFECT, params, ARRAY_SIZE(params));
 }
 
 static void hidpp_ff_set_gain(struct input_dev *dev, u16 gain)
@@ -2700,7 +2694,7 @@ static void hidpp_ff_set_gain(struct input_dev *dev, u16 gain)
 	params[2] = 0; /* no boost */
 	params[3] = 0;
 
-	hidpp_ff_queue_work(data, HIDPP_FF_EFFECTID_NONE, HIDPP_FF_SET_GLOBAL_GAINS, params, ARRAY_SIZE(params));
+	hidpp_ff_queue_work(data, HIDPP_FF_SET_GLOBAL_GAINS, params, ARRAY_SIZE(params));
 }
 
 static ssize_t hidpp_ff_range_show(struct device *dev, struct device_attribute *attr, char *buf)

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 25/33] HID: logitech-hidpp: clean up G920 force-feedback helpers

[Dmitry Torokhov]

Perform several minor cleanups across the G920 force-feedback support:

- In hidpp_ff_init(), obtain the firmware version directly in a single
  expression from the USB device descriptor, drop the redundant
  hid_is_usb() check on the already-matched G920 device, and remove
  unused local variables.
- In hidpp_ff_upload_effect(), remove the dead force calculation in the
  periodic effect upload case where magnitude is written directly.
- Zero-initialize stack parameter buffers in effect upload, playback,
  autocenter, and gain helpers so unused parameter bytes sent over USB
  are always clean zeroes.
- Drop the write-only gain field from struct g920_data, remove the
  unused HIDPP_FF_GET_GLOBAL_GAINS query at probe time, and remove the
  gain cache update from the work handler.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 43 +++++-----------------------------------
 1 file changed, 5 insertions(+), 38 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 20f7e8367ee1..1d80e7b7a99e 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2310,7 +2310,6 @@ static void hidpp_touchpad_raw_xy_event(struct hidpp_device *hidpp_dev,
 #define HIDPP_FF_DESTROY_EFFECT		0x41
 #define HIDPP_FF_GET_APERTURE		0x51
 #define HIDPP_FF_SET_APERTURE		0x61
-#define HIDPP_FF_GET_GLOBAL_GAINS	0x71
 #define HIDPP_FF_SET_GLOBAL_GAINS	0x81
 
 #define HIDPP_FF_EFFECT_STATE_GET	0x00
@@ -2349,7 +2348,6 @@ struct g920_data {
 	u8 feature_index;
 	u8 slot_autocenter;
 	u8 num_effects;
-	u16 gain;
 	u16 range;
 };
 
@@ -2412,8 +2410,6 @@ static void hidpp_ff_work_handler(struct work_struct *w)
 					  &response);
 	if (ret)
 		hid_err(data->hidpp->hid_dev, "Failed to send command to device!\n");
-	else if (wd->command == HIDPP_FF_SET_GLOBAL_GAINS)
-		g920->gain = (wd->params[0] << 8) + wd->params[1];
 
 	atomic_dec(&data->workqueue_size);
 	kfree(wd);
@@ -2453,7 +2449,7 @@ static int hidpp_ff_upload_effect(struct input_dev *dev, struct ff_effect *effec
 	struct hid_device *hid = hidpp->hid_dev;
 	struct g920_data *g920 = hidpp->private_data;
 	struct hidpp_report response;
-	u8 params[20];
+	u8 params[20] = {};
 	u8 size;
 	int force, error;
 
@@ -2510,7 +2506,6 @@ static int hidpp_ff_upload_effect(struct input_dev *dev, struct ff_effect *effec
 				effect->u.periodic.waveform);
 			return -EINVAL;
 		}
-		force = (effect->u.periodic.magnitude * fixp_sin16((effect->direction * 360) >> 16)) >> 15;
 		params[6] = effect->u.periodic.magnitude >> 8;
 		params[7] = effect->u.periodic.magnitude & 255;
 		params[8] = effect->u.periodic.offset >> 8;
@@ -2617,7 +2612,7 @@ static int hidpp_ff_upload_effect(struct input_dev *dev, struct ff_effect *effec
 static int hidpp_ff_playback(struct input_dev *dev, int effect_id, int value)
 {
 	struct hidpp_ff_private_data *data = dev->ff->private;
-	u8 params[2];
+	u8 params[2] = {};
 
 	params[0] = data->slots[effect_id];
 	if (!dev->ready || !params[0])
@@ -2662,22 +2657,18 @@ static void hidpp_ff_set_autocenter(struct input_dev *dev, u16 magnitude)
 {
 	struct hidpp_ff_private_data *data = dev->ff->private;
 	struct g920_data *g920 = data->hidpp->private_data;
-	u8 params[HIDPP_AUTOCENTER_PARAMS_LENGTH];
+	u8 params[HIDPP_AUTOCENTER_PARAMS_LENGTH] = {};
 
 	dbg_hid("Setting autocenter to %d.\n", magnitude);
 
 	/* start a standard spring effect */
 	params[0] = g920->slot_autocenter;
 	params[1] = HIDPP_FF_EFFECT_SPRING | HIDPP_FF_EFFECT_AUTOSTART;
-	/* zero delay and duration */
-	params[2] = params[3] = params[4] = params[5] = 0;
 	/* set coeff to 25% of saturation */
 	params[8] = params[14] = magnitude >> 11;
 	params[9] = params[15] = (magnitude >> 3) & 255;
 	params[6] = params[16] = magnitude >> 9;
 	params[7] = params[17] = (magnitude >> 1) & 255;
-	/* zero deadband and center */
-	params[10] = params[11] = params[12] = params[13] = 0;
 
 	hidpp_ff_queue_work(data, HIDPP_FF_DOWNLOAD_EFFECT, params, ARRAY_SIZE(params));
 }
@@ -2685,14 +2676,12 @@ static void hidpp_ff_set_autocenter(struct input_dev *dev, u16 magnitude)
 static void hidpp_ff_set_gain(struct input_dev *dev, u16 gain)
 {
 	struct hidpp_ff_private_data *data = dev->ff->private;
-	u8 params[4];
+	u8 params[4] = {};
 
 	dbg_hid("Setting gain to %d.\n", gain);
 
 	params[0] = gain >> 8;
 	params[1] = gain & 255;
-	params[2] = 0; /* no boost */
-	params[3] = 0;
 
 	hidpp_ff_queue_work(data, HIDPP_FF_SET_GLOBAL_GAINS, params, ARRAY_SIZE(params));
 }
@@ -2826,17 +2815,10 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, struct input_dev *dev)
 {
 	struct hid_device *hid = hidpp->hid_dev;
 	struct g920_data *g920 = hidpp->private_data;
-	struct usb_device_descriptor *udesc;
-	u16 bcdDevice;
 	struct ff_device *ff;
 	int error, j, num_slots = g920->num_effects;
 	u8 version;
 
-	if (!hid_is_usb(hid)) {
-		hid_err(hid, "device is not USB\n");
-		return -ENODEV;
-	}
-
 	struct hidpp_ff_private_data *data __free(kfree) =
 		kzalloc_flex(*data, slots, num_slots);
 	if (!data)
@@ -2853,9 +2835,7 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, struct input_dev *dev)
 		return -ENOMEM;
 
 	/* Get firmware release */
-	udesc = &(hid_to_usb_dev(hid)->descriptor);
-	bcdDevice = le16_to_cpu(udesc->bcdDevice);
-	version = bcdDevice & 255;
+	version = le16_to_cpu(hid_to_usb_dev(hid)->descriptor.bcdDevice) & 0xff;
 
 	/* Set supported force feedback capabilities */
 	for (j = 0; hidpp_ff_effects[j] >= 0; j++)
@@ -3462,19 +3442,6 @@ static int g920_get_config(struct hidpp_device *hidpp)
 	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] */
-
 	ret = g920_ff_set_autocenter(hidpp, &data);
 	if (ret)
 		return ret;

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 26/33] HID: logitech-hidpp: use unaligned accessors for multi-byte values

[Dmitry Torokhov]

Several paths in the Logitech HID++ driver manually split and reassemble
16-bit protocol values using shifts and masks instead of using standard
kernel helpers, which obscures endianness intent and adds unnecessary
noise.

Replace open-coded 16-bit big-endian packing and unpacking with
get_unaligned_be16() and put_unaligned_be16().

Tested-by: Gideon Heinz <gideonheinz02@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 84 +++++++++++++++-------------------------
 1 file changed, 31 insertions(+), 53 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 1d80e7b7a99e..df0bcaaf8647 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -960,7 +960,9 @@ static int hidpp_root_get_feature(struct hidpp_device *hidpp, u16 feature,
 {
 	struct hidpp_report response;
 	int ret;
-	u8 params[2] = { feature >> 8, feature & 0x00FF };
+	u8 params[2];
+
+	put_unaligned_be16(feature, params);
 
 	ret = hidpp_send_fap_command_sync(hidpp,
 			HIDPP_PAGE_ROOT_IDX,
@@ -2108,7 +2110,7 @@ static int hidpp_solar_battery_event(struct hidpp_device *hidpp,
 
 	switch (function) {
 	case EVENT_SOLAR_BATTERY_LIGHT_MEASURE:
-		lux = (report->fap.params[1] << 8) | report->fap.params[2];
+		lux = get_unaligned_be16(&report->fap.params[1]);
 		if (lux > 200)
 			status = POWER_SUPPLY_STATUS_CHARGING;
 		else
@@ -2410,7 +2412,6 @@ static void hidpp_ff_work_handler(struct work_struct *w)
 					  &response);
 	if (ret)
 		hid_err(data->hidpp->hid_dev, "Failed to send command to device!\n");
-
 	atomic_dec(&data->workqueue_size);
 	kfree(wd);
 }
@@ -2455,23 +2456,18 @@ static int hidpp_ff_upload_effect(struct input_dev *dev, struct ff_effect *effec
 
 	/* set common parameters */
 	params[0] = old ? data->slots[effect->id] : 0;
-	params[2] = effect->replay.length >> 8;
-	params[3] = effect->replay.length & 255;
-	params[4] = effect->replay.delay >> 8;
-	params[5] = effect->replay.delay & 255;
+	put_unaligned_be16(effect->replay.length, &params[2]);
+	put_unaligned_be16(effect->replay.delay, &params[4]);
 
 	switch (effect->type) {
 	case FF_CONSTANT:
 		force = (effect->u.constant.level * fixp_sin16((effect->direction * 360) >> 16)) >> 15;
 		params[1] = HIDPP_FF_EFFECT_CONSTANT;
-		params[6] = force >> 8;
-		params[7] = force & 255;
+		put_unaligned_be16(force, &params[6]);
 		params[8] = effect->u.constant.envelope.attack_level >> 7;
-		params[9] = effect->u.constant.envelope.attack_length >> 8;
-		params[10] = effect->u.constant.envelope.attack_length & 255;
+		put_unaligned_be16(effect->u.constant.envelope.attack_length, &params[9]);
 		params[11] = effect->u.constant.envelope.fade_level >> 7;
-		params[12] = effect->u.constant.envelope.fade_length >> 8;
-		params[13] = effect->u.constant.envelope.fade_length & 255;
+		put_unaligned_be16(effect->u.constant.envelope.fade_length, &params[12]);
 		size = 14;
 		dbg_hid("Uploading constant force level=%d in dir %d = %d\n",
 				effect->u.constant.level,
@@ -2506,20 +2502,14 @@ static int hidpp_ff_upload_effect(struct input_dev *dev, struct ff_effect *effec
 				effect->u.periodic.waveform);
 			return -EINVAL;
 		}
-		params[6] = effect->u.periodic.magnitude >> 8;
-		params[7] = effect->u.periodic.magnitude & 255;
-		params[8] = effect->u.periodic.offset >> 8;
-		params[9] = effect->u.periodic.offset & 255;
-		params[10] = effect->u.periodic.period >> 8;
-		params[11] = effect->u.periodic.period & 255;
-		params[12] = effect->u.periodic.phase >> 8;
-		params[13] = effect->u.periodic.phase & 255;
+		put_unaligned_be16(effect->u.periodic.magnitude, &params[6]);
+		put_unaligned_be16(effect->u.periodic.offset, &params[8]);
+		put_unaligned_be16(effect->u.periodic.period, &params[10]);
+		put_unaligned_be16(effect->u.periodic.phase, &params[12]);
 		params[14] = effect->u.periodic.envelope.attack_level >> 7;
-		params[15] = effect->u.periodic.envelope.attack_length >> 8;
-		params[16] = effect->u.periodic.envelope.attack_length & 255;
+		put_unaligned_be16(effect->u.periodic.envelope.attack_length, &params[15]);
 		params[17] = effect->u.periodic.envelope.fade_level >> 7;
-		params[18] = effect->u.periodic.envelope.fade_length >> 8;
-		params[19] = effect->u.periodic.envelope.fade_length & 255;
+		put_unaligned_be16(effect->u.periodic.envelope.fade_length, &params[18]);
 		size = 20;
 		dbg_hid("Uploading periodic force mag=%d/dir=%d, offset=%d, period=%d ms, phase=%d\n",
 				effect->u.periodic.magnitude, effect->direction,
@@ -2536,17 +2526,13 @@ static int hidpp_ff_upload_effect(struct input_dev *dev, struct ff_effect *effec
 	case FF_RAMP:
 		params[1] = HIDPP_FF_EFFECT_RAMP;
 		force = (effect->u.ramp.start_level * fixp_sin16((effect->direction * 360) >> 16)) >> 15;
-		params[6] = force >> 8;
-		params[7] = force & 255;
+		put_unaligned_be16(force, &params[6]);
 		force = (effect->u.ramp.end_level * fixp_sin16((effect->direction * 360) >> 16)) >> 15;
-		params[8] = force >> 8;
-		params[9] = force & 255;
+		put_unaligned_be16(force, &params[8]);
 		params[10] = effect->u.ramp.envelope.attack_level >> 7;
-		params[11] = effect->u.ramp.envelope.attack_length >> 8;
-		params[12] = effect->u.ramp.envelope.attack_length & 255;
+		put_unaligned_be16(effect->u.ramp.envelope.attack_length, &params[11]);
 		params[13] = effect->u.ramp.envelope.fade_level >> 7;
-		params[14] = effect->u.ramp.envelope.fade_length >> 8;
-		params[15] = effect->u.ramp.envelope.fade_length & 255;
+		put_unaligned_be16(effect->u.ramp.envelope.fade_length, &params[14]);
 		size = 16;
 		dbg_hid("Uploading ramp force level=%d -> %d in dir %d = %d\n",
 				effect->u.ramp.start_level,
@@ -2563,18 +2549,12 @@ static int hidpp_ff_upload_effect(struct input_dev *dev, struct ff_effect *effec
 	case FF_SPRING:
 	case FF_DAMPER:
 		params[1] = HIDPP_FF_CONDITION_CMDS[effect->type - FF_SPRING];
-		params[6] = effect->u.condition[0].left_saturation >> 9;
-		params[7] = (effect->u.condition[0].left_saturation >> 1) & 255;
-		params[8] = effect->u.condition[0].left_coeff >> 8;
-		params[9] = effect->u.condition[0].left_coeff & 255;
-		params[10] = effect->u.condition[0].deadband >> 9;
-		params[11] = (effect->u.condition[0].deadband >> 1) & 255;
-		params[12] = effect->u.condition[0].center >> 8;
-		params[13] = effect->u.condition[0].center & 255;
-		params[14] = effect->u.condition[0].right_coeff >> 8;
-		params[15] = effect->u.condition[0].right_coeff & 255;
-		params[16] = effect->u.condition[0].right_saturation >> 9;
-		params[17] = (effect->u.condition[0].right_saturation >> 1) & 255;
+		put_unaligned_be16(effect->u.condition[0].left_saturation >> 1, &params[6]);
+		put_unaligned_be16(effect->u.condition[0].left_coeff, &params[8]);
+		put_unaligned_be16(effect->u.condition[0].deadband >> 1, &params[10]);
+		put_unaligned_be16(effect->u.condition[0].center, &params[12]);
+		put_unaligned_be16(effect->u.condition[0].right_coeff, &params[14]);
+		put_unaligned_be16(effect->u.condition[0].right_saturation >> 1, &params[16]);
 		size = 18;
 		dbg_hid("Uploading %s force left coeff=%d, left sat=%d, right coeff=%d, right sat=%d\n",
 				HIDPP_FF_CONDITION_NAMES[effect->type - FF_SPRING],
@@ -2665,10 +2645,10 @@ static void hidpp_ff_set_autocenter(struct input_dev *dev, u16 magnitude)
 	params[0] = g920->slot_autocenter;
 	params[1] = HIDPP_FF_EFFECT_SPRING | HIDPP_FF_EFFECT_AUTOSTART;
 	/* set coeff to 25% of saturation */
-	params[8] = params[14] = magnitude >> 11;
-	params[9] = params[15] = (magnitude >> 3) & 255;
-	params[6] = params[16] = magnitude >> 9;
-	params[7] = params[17] = (magnitude >> 1) & 255;
+	put_unaligned_be16(magnitude >> 3, &params[8]);
+	put_unaligned_be16(magnitude >> 3, &params[14]);
+	put_unaligned_be16(magnitude >> 1, &params[6]);
+	put_unaligned_be16(magnitude >> 1, &params[16]);
 
 	hidpp_ff_queue_work(data, HIDPP_FF_DOWNLOAD_EFFECT, params, ARRAY_SIZE(params));
 }
@@ -2680,8 +2660,7 @@ static void hidpp_ff_set_gain(struct input_dev *dev, u16 gain)
 
 	dbg_hid("Setting gain to %d.\n", gain);
 
-	params[0] = gain >> 8;
-	params[1] = gain & 255;
+	put_unaligned_be16(gain, params);
 
 	hidpp_ff_queue_work(data, HIDPP_FF_SET_GLOBAL_GAINS, params, ARRAY_SIZE(params));
 }
@@ -2711,8 +2690,7 @@ static ssize_t hidpp_ff_range_store(struct device *dev, struct device_attribute
 
 	range = clamp_val(range, 180, 900);
 
-	params[0] = range >> 8;
-	params[1] = range & 0x00FF;
+	put_unaligned_be16(range, params);
 
 	error = hid_hw_open(hid);
 	if (error)

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 27/33] HID: logitech-hidpp: store device serial as a byte array

[Dmitry Torokhov]

Device serial numbers are read from 4-byte protocol payloads only to be
formatted as raw byte sequences with %4phD. Storing them as u32 requires
unaligned pointer casts and dereferences that depend on endian-neutral
memory layout, and the FAP-based serial helper reads from the RAP union
member instead of the matching FAP payload.

Represent device serial numbers as 4-byte arrays, copy the payload bytes
with a fixed serial length constant, and read the FAP device information
response from the FAP payload directly.

Tested-by: Gideon Heinz <gideonheinz02@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index df0bcaaf8647..181b791b3e0c 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -48,6 +48,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_REPORT_SHORT_LENGTH		7
 #define HIDPP_REPORT_LONG_LENGTH		20
 #define HIDPP_REPORT_VERY_LONG_MAX_LENGTH	64
+#define HIDPP_SERIAL_LENGTH			4
 
 #define HIDPP_REPORT_SHORT_SUPPORTED		BIT(0)
 #define HIDPP_REPORT_LONG_SUPPORTED		BIT(1)
@@ -898,7 +899,8 @@ static char *hidpp_unifying_get_name(struct hidpp_device *hidpp_dev)
 	return name;
 }
 
-static int hidpp_unifying_get_serial(struct hidpp_device *hidpp, u32 *serial)
+static int hidpp_unifying_get_serial(struct hidpp_device *hidpp,
+				     u8 serial[HIDPP_SERIAL_LENGTH])
 {
 	struct hidpp_report response;
 	int ret;
@@ -912,11 +914,7 @@ static int hidpp_unifying_get_serial(struct hidpp_device *hidpp, u32 *serial)
 	if (ret)
 		return ret;
 
-	/*
-	 * We don't care about LE or BE, we will output it as a string
-	 * with %4phD, so we need to keep the order.
-	 */
-	*serial = *((u32 *)&response.rap.params[1]);
+	memcpy(serial, &response.rap.params[1], HIDPP_SERIAL_LENGTH);
 	return 0;
 }
 
@@ -924,14 +922,14 @@ static int hidpp_unifying_init(struct hidpp_device *hidpp)
 {
 	struct hid_device *hdev = hidpp->hid_dev;
 	const char *name;
-	u32 serial;
+	u8 serial[HIDPP_SERIAL_LENGTH];
 	int ret;
 
-	ret = hidpp_unifying_get_serial(hidpp, &serial);
+	ret = hidpp_unifying_get_serial(hidpp, serial);
 	if (ret)
 		return ret;
 
-	snprintf(hdev->uniq, sizeof(hdev->uniq), "%4phD", &serial);
+	snprintf(hdev->uniq, sizeof(hdev->uniq), "%4phD", serial);
 	dbg_hid("HID++ Unifying: Got serial: %s\n", hdev->uniq);
 
 	name = hidpp_unifying_get_name(hidpp);
@@ -1031,7 +1029,8 @@ static int hidpp_root_get_protocol_version(struct hidpp_device *hidpp)
 
 #define CMD_GET_DEVICE_INFO				0x00
 
-static int hidpp_get_serial(struct hidpp_device *hidpp, u32 *serial)
+static int hidpp_get_serial(struct hidpp_device *hidpp,
+			    u8 serial[HIDPP_SERIAL_LENGTH])
 {
 	struct hidpp_report response;
 	u8 feature_index;
@@ -1048,22 +1047,21 @@ static int hidpp_get_serial(struct hidpp_device *hidpp, u32 *serial)
 	if (ret)
 		return ret;
 
-	/* See hidpp_unifying_get_serial() */
-	*serial = *((u32 *)&response.rap.params[1]);
+	memcpy(serial, &response.fap.params[1], HIDPP_SERIAL_LENGTH);
 	return 0;
 }
 
 static int hidpp_serial_init(struct hidpp_device *hidpp)
 {
 	struct hid_device *hdev = hidpp->hid_dev;
-	u32 serial;
+	u8 serial[HIDPP_SERIAL_LENGTH];
 	int ret;
 
-	ret = hidpp_get_serial(hidpp, &serial);
+	ret = hidpp_get_serial(hidpp, serial);
 	if (ret)
 		return ret;
 
-	snprintf(hdev->uniq, sizeof(hdev->uniq), "%4phD", &serial);
+	snprintf(hdev->uniq, sizeof(hdev->uniq), "%4phD", serial);
 	dbg_hid("HID++ DeviceInformation: Got serial: %s\n", hdev->uniq);
 
 	return 0;

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 28/33] HID: logitech-hidpp: use guard() for send_mutex

[Dmitry Torokhov]

Manual mutex_lock() and mutex_unlock() calls force early loop exits to
break to a shared unlock path instead of returning directly. Convert
hidpp_send_message_sync() to use guard(mutex) so lock release is bound
to the function scope and busy-retry exits can return immediately.

Tested-by: Gideon Heinz <gideonheinz02@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 181b791b3e0c..8f2a9d3e0f74 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -380,19 +380,17 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
 	int ret;
 	int max_retries = 3;
 
-	mutex_lock(&hidpp->send_mutex);
+	guard(mutex)(&hidpp->send_mutex);
 
 	do {
 		ret = __do_hidpp_send_message_sync(hidpp, message, response);
 		if (ret != -EBUSY)
-			break;
+			return ret;
 
 		dbg_hid("%s:got busy hidpp error, retrying\n", __func__);
 	} while (--max_retries);
 
-	mutex_unlock(&hidpp->send_mutex);
 	return ret;
-
 }
 
 /*

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 29/33] HID: logitech-hidpp: use __free(kfree) for temporary allocations

[Dmitry Torokhov]

Managing short-lived heap allocations manually with explicit kfree()
calls adds boilerplate cleanup paths and forces callers to hold
temporary return variables across calls.

Convert temporary message and device-name allocations in command helpers
and device initialization paths to use __free(kfree) with no_free_ptr()
ownership transfer where the buffer is returned to callers.

Tested-by: Gideon Heinz <gideonheinz02@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 44 +++++++++++++---------------------------
 1 file changed, 14 insertions(+), 30 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 8f2a9d3e0f74..67b4f1112168 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -401,18 +401,15 @@ static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp,
 	u8 feat_index, u8 funcindex_clientid, u8 *params, int param_count,
 	struct hidpp_report *response)
 {
-	struct hidpp_report *message;
-	int ret;
-
-	if (param_count > sizeof(message->fap.params)) {
+	if (param_count > sizeof_field(struct hidpp_report, fap.params)) {
 		hid_dbg(hidpp->hid_dev,
 			"Invalid number of parameters passed to command (%d != %llu)\n",
 			param_count,
-			(unsigned long long) sizeof(message->fap.params));
+			(unsigned long long)sizeof_field(struct hidpp_report, fap.params));
 		return -EINVAL;
 	}
 
-	message = kzalloc_obj(struct hidpp_report);
+	struct hidpp_report *message __free(kfree) = kzalloc_obj(*message);
 	if (!message)
 		return -ENOMEM;
 
@@ -424,9 +421,7 @@ static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp,
 	message->fap.funcindex_clientid = funcindex_clientid | LINUX_KERNEL_SW_ID;
 	memcpy(&message->fap.params, params, param_count);
 
-	ret = hidpp_send_message_sync(hidpp, message, response);
-	kfree(message);
-	return ret;
+	return hidpp_send_message_sync(hidpp, message, response);
 }
 
 /*
@@ -437,8 +432,7 @@ static int hidpp_send_rap_command_sync(struct hidpp_device *hidpp_dev,
 	u8 report_id, u8 sub_id, u8 reg_address, u8 *params, int param_count,
 	struct hidpp_report *response)
 {
-	struct hidpp_report *message;
-	int ret, max_count;
+	int max_count;
 
 	/* Send as long report if short reports are not supported. */
 	if (report_id == REPORT_ID_HIDPP_SHORT &&
@@ -462,7 +456,7 @@ static int hidpp_send_rap_command_sync(struct hidpp_device *hidpp_dev,
 	if (param_count > max_count)
 		return -EINVAL;
 
-	message = kzalloc_obj(struct hidpp_report);
+	struct hidpp_report *message __free(kfree) = kzalloc_obj(*message);
 	if (!message)
 		return -ENOMEM;
 	message->report_id = report_id;
@@ -470,9 +464,7 @@ static int hidpp_send_rap_command_sync(struct hidpp_device *hidpp_dev,
 	message->rap.reg_address = reg_address;
 	memcpy(&message->rap.params, params, param_count);
 
-	ret = hidpp_send_message_sync(hidpp_dev, message, response);
-	kfree(message);
-	return ret;
+	return hidpp_send_message_sync(hidpp_dev, message, response);
 }
 
 static inline bool hidpp_match_answer(struct hidpp_report *question,
@@ -919,7 +911,6 @@ static int hidpp_unifying_get_serial(struct hidpp_device *hidpp,
 static int hidpp_unifying_init(struct hidpp_device *hidpp)
 {
 	struct hid_device *hdev = hidpp->hid_dev;
-	const char *name;
 	u8 serial[HIDPP_SERIAL_LENGTH];
 	int ret;
 
@@ -930,14 +921,13 @@ static int hidpp_unifying_init(struct hidpp_device *hidpp)
 	snprintf(hdev->uniq, sizeof(hdev->uniq), "%4phD", serial);
 	dbg_hid("HID++ Unifying: Got serial: %s\n", hdev->uniq);
 
-	name = hidpp_unifying_get_name(hidpp);
+	char *name __free(kfree) = hidpp_unifying_get_name(hidpp);
 	if (!name)
 		return -EIO;
 
 	snprintf(hdev->name, sizeof(hdev->name), "%s", name);
 	dbg_hid("HID++ Unifying: Got name: %s\n", name);
 
-	kfree(name);
 	return 0;
 }
 
@@ -1131,7 +1121,6 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp)
 {
 	u8 feature_index;
 	u8 __name_length;
-	char *name;
 	unsigned index = 0;
 	int ret;
 
@@ -1145,7 +1134,7 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp)
 	if (ret)
 		return NULL;
 
-	name = kzalloc(__name_length + 1, GFP_KERNEL);
+	char *name __free(kfree) = kzalloc(__name_length + 1, GFP_KERNEL);
 	if (!name)
 		return NULL;
 
@@ -1153,17 +1142,15 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp)
 		ret = hidpp_devicenametype_get_device_name(hidpp,
 			feature_index, index, name + index,
 			__name_length - index);
-		if (ret <= 0) {
-			kfree(name);
+		if (ret <= 0)
 			return NULL;
-		}
 		index += ret;
 	}
 
 	/* include the terminating '\0' */
 	hidpp_prefix_name(&name, __name_length + 1);
 
-	return name;
+	return no_free_ptr(name);
 }
 
 /* -------------------------------------------------------------------------- */
@@ -4328,17 +4315,15 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 static void hidpp_non_unifying_init(struct hidpp_device *hidpp)
 {
 	struct hid_device *hdev = hidpp->hid_dev;
-	char *name;
 
 	/* Bluetooth devices already have their serialnr set */
 	if (hid_is_usb(hdev))
 		hidpp_serial_init(hidpp);
 
-	name = hidpp_get_device_name(hidpp);
+	char *name __free(kfree) = hidpp_get_device_name(hidpp);
 	if (name) {
 		dbg_hid("HID++: Got name: %s\n", name);
 		snprintf(hdev->name, sizeof(hdev->name), "%s", name);
-		kfree(name);
 	}
 }
 
@@ -4385,7 +4370,7 @@ static void hidpp_connect_event(struct work_struct *work)
 	struct hidpp_device *hidpp = container_of(work, struct hidpp_device, work);
 	struct hid_device *hdev = hidpp->hid_dev;
 	struct input_dev *input;
-	char *name, *devm_name;
+	char *devm_name;
 	int ret;
 
 	/* Get device version to check if it is connected */
@@ -4443,11 +4428,10 @@ static void hidpp_connect_event(struct work_struct *work)
 	}
 
 	if (hidpp->name == hdev->name && hidpp->protocol_major >= 2) {
-		name = hidpp_get_device_name(hidpp);
+		char *name __free(kfree) = hidpp_get_device_name(hidpp);
 		if (name) {
 			devm_name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
 						   "%s", name);
-			kfree(name);
 			if (!devm_name)
 				return;
 

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 30/33] HID: logitech-hidpp: use alloc_ordered_workqueue() for FF queue

[Dmitry Torokhov]

create_singlethread_workqueue() is a deprecated wrapper around
alloc_ordered_workqueue() that unconditionally enables WQ_MEM_RECLAIM,
allocating a rescuer thread. Force-feedback packet submission does not
run on a memory-reclaim path, so allocate the send queue with
alloc_ordered_workqueue() and zero flags to preserve strict in-order
execution without unnecessary rescuer overhead.

Tested-by: Gideon Heinz <gideonheinz02@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 67b4f1112168..b6c7a951cc94 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2791,7 +2791,7 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, struct input_dev *dev)
 	/* init the hardware command queue */
 	atomic_set(&data->workqueue_size, 0);
 
-	data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
+	data->wq = alloc_ordered_workqueue("hidpp-ff-sendqueue", 0);
 	if (!data->wq)
 		return -ENOMEM;
 

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 31/33] HID: logitech-hidpp: manage send_mutex with devm_mutex_init()

[Dmitry Torokhov]

Manually initializing and destroying the HID++ command mutex in probe
and remove destroys the mutex before devres tears down managed child
resources such as the delayed input device and battery power supply.
Switching to devm_mutex_init() eliminates manual teardown boilerplate
while keeping the mutex valid throughout child device teardown.

Initialize the command mutex with devm_mutex_init() during probe,
returning early on allocation failure, and drop manual mutex
destruction from the error path and device removal handler.

Tested-by: Gideon Heinz <gideonheinz02@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index b6c7a951cc94..be3c2649b0e3 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4631,9 +4631,12 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	INIT_WORK(&hidpp->work, hidpp_connect_event);
 	disable_work(&hidpp->work);
 	INIT_WORK(&hidpp->reset_hi_res_work, hidpp_reset_hi_res_handler);
-	mutex_init(&hidpp->send_mutex);
 	init_waitqueue_head(&hidpp->wait);
 
+	ret = devm_mutex_init(&hdev->dev, &hidpp->send_mutex);
+	if (ret)
+		return ret;
+
 	/* indicates we are handling the battery properties in the kernel */
 	ret = sysfs_create_group(&hdev->dev.kobj, &ps_attribute_group);
 	if (ret)
@@ -4710,7 +4713,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	hid_hw_stop(hdev);
 hid_hw_start_fail:
 	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
-	mutex_destroy(&hidpp->send_mutex);
 	return ret;
 }
 
@@ -4737,7 +4739,6 @@ static void hidpp_remove(struct hid_device *hdev)
 	hid_device_io_stop(hdev);
 
 	hid_hw_stop(hdev);
-	mutex_destroy(&hidpp->send_mutex);
 }
 
 #define LDJ_DEVICE(product) \

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 32/33] HID: logitech-hidpp: manage ps_attribute_group with devm_device_add_group()

[Dmitry Torokhov]

Managing the power-supply sysfs attribute group manually requires
explicit cleanup in both probe failure paths and device removal, even
though all surrounding device state uses devres.

Switch to devm_device_add_group() while keeping synchronous creation
before connecting subdrivers and non-fatal warning behavior on failure.
Drop the manual removal calls, eliminate the empty start-failure unwind
label in probe, and simplify device removal accordingly.

Tested-by: Gideon Heinz <gideonheinz02@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index be3c2649b0e3..019b917c3b90 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4638,7 +4638,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 
 	/* indicates we are handling the battery properties in the kernel */
-	ret = sysfs_create_group(&hdev->dev.kobj, &ps_attribute_group);
+	ret = devm_device_add_group(&hdev->dev, &ps_attribute_group);
 	if (ret)
 		hid_warn(hdev, "Cannot allocate sysfs group for %s\n",
 			 hdev->name);
@@ -4652,7 +4652,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	ret = hid_hw_start(hdev, 0);
 	if (ret) {
 		hid_err(hdev, "hw start failed\n");
-		goto hid_hw_start_fail;
+		return ret;
 	}
 
 	ret = hid_hw_open(hdev);
@@ -4711,8 +4711,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	hid_hw_close(hdev);
 hid_hw_open_fail:
 	hid_hw_stop(hdev);
-hid_hw_start_fail:
-	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
 	return ret;
 }
 
@@ -4721,22 +4719,19 @@ static void hidpp_remove(struct hid_device *hdev)
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 	bool claimed_input;
 
-	if (!hidpp)
-		return hid_hw_stop(hdev);
-
-	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
-
-	claimed_input = hdev->claimed & HID_CLAIMED_INPUT;
-	hdev->claimed &= ~HID_CLAIMED_INPUT;
-	if (claimed_input)
-		hidpp->input = NULL;
-
-	hid_device_io_start(hdev);
-	disable_work_sync(&hidpp->work);
-	disable_work_sync(&hidpp->reset_hi_res_work);
-	if (claimed_input)
-		hidinput_disconnect(hdev);
-	hid_device_io_stop(hdev);
+	if (hidpp) {
+		claimed_input = hdev->claimed & HID_CLAIMED_INPUT;
+		hdev->claimed &= ~HID_CLAIMED_INPUT;
+		if (claimed_input)
+			hidpp->input = NULL;
+
+		hid_device_io_start(hdev);
+		disable_work_sync(&hidpp->work);
+		disable_work_sync(&hidpp->reset_hi_res_work);
+		if (claimed_input)
+			hidinput_disconnect(hdev);
+		hid_device_io_stop(hdev);
+	}
 
 	hid_hw_stop(hdev);
 }

-- 
2.56.0.542.g3ef89b7020-goog

[PATCH v2 33/33] HID: logitech-hidpp: remove unused kfifo and clean up minor helpers

[Dmitry Torokhov]

Remove unused linux/kfifo.h header inclusion and the unused
delayed_work_fifo field from struct hidpp_device. Also drop the unused
finger_count and button_feature_index fields from struct wtp_data.

Drop the redundant input dev.parent assignment in hidpp_allocate_input()
which is already performed by devm_input_allocate_device(). Also drop
the stale ret format specifier in the hidpp_connect_event() input device
allocation failure log message.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 019b917c3b90..6529c7b10b8f 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -18,7 +18,6 @@
 #include <linux/slab.h>
 #include <linux/sched.h>
 #include <linux/sched/clock.h>
-#include <linux/kfifo.h>
 #include <linux/input/mt.h>
 #include <linux/workqueue.h>
 #include <linux/atomic.h>
@@ -198,7 +197,6 @@ struct hidpp_device {
 
 	struct work_struct work;
 	struct work_struct reset_hi_res_work;
-	struct kfifo delayed_work_fifo;
 	struct input_dev *delayed_input;
 
 	unsigned long quirks;
@@ -2847,9 +2845,7 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, struct input_dev *dev)
 
 struct wtp_data {
 	u16 x_size, y_size;
-	u8 finger_count;
 	u8 mt_feature_index;
-	u8 button_feature_index;
 	u8 maxcontacts;
 	bool flip_y;
 	unsigned int resolution;
@@ -4360,7 +4356,6 @@ static struct input_dev *hidpp_allocate_input(struct hid_device *hdev)
 	input_dev->id.vendor  = hdev->vendor;
 	input_dev->id.product = hdev->product;
 	input_dev->id.version = hdev->version;
-	input_dev->dev.parent = &hdev->dev;
 
 	return input_dev;
 }
@@ -4474,7 +4469,7 @@ static void hidpp_connect_event(struct work_struct *work)
 
 	input = hidpp_allocate_input(hdev);
 	if (!input) {
-		hid_err(hdev, "cannot allocate new input device: %d\n", ret);
+		hid_err(hdev, "cannot allocate new input device\n");
 		return;
 	}
 

-- 
2.56.0.542.g3ef89b7020-goog

Re: [PATCH v2 00/33] HID: logitech-hidpp: G920 force-feedback fixes, rework, and cleanups

[Gideon Heinz]

Hi Dmitry,

I tested v2 of the series on my G920 (046d:c262). Everything on my
list works except one scenario that takes the whole machine down, with
and without your series (last item).

Setup: Linux 7.3.1-1-cachyos (clang build). The pre-image of patch 1
(blob 60fc6b86a085) is identical to v7.4-rc2's hid-logitech-hidpp.c, so
I applied all 33 patches to that file (every step matched the blob hash
recorded in the patch) and built it as an out-of-tree module against
the 7.3.1 headers, loaded with insmod. The only local change is a
3-line fallback for devm_kzalloc_obj(), which the 7.3.1 headers lack.
No full kernel build.

Results:

 - Initialization: "Force feedback support loaded (firmware release 4)"
   on every plug-in (8 in total), no -110.

 - fftest: all effect types are listed and the effects play and
   produce force. After exiting with -1 the log stays clean for 2+
   minutes.

 - Input: wheel rotation, buttons and the hat all register in evtest.
   I use other pedals, so the G920 pedals are untested.

 - range sysfs: with nothing holding the device open, writes take
   ~67 ms and read back correctly (540, then 900). On v1 my
   closed-device writes each ended in a "Failed to send command to
   device!" about 15 s later. With evtest open it behaves the same.
   100 reads back as 180 and 2000 as 900.

 - Replug x4: FF loads cleanly each time.

 - Unplug with evtest open: evtest exits with "No such device",
   "Unloading HID++ force feedback" and nothing else in dmesg.

 - Unplug with fftest open (6 effects loaded): 6 "Failed to send
   command to device!" lines within 1.7 ms of each other (v1 gave 12),
   then "Unloading HID++ force feedback" 19 ms after the last. No
   warnings, oops or hangs.

 - rmmod with fftest holding the device open: rmmod returned after
   2.99 s with "Unloading HID++ force feedback" and nothing else in
   dmesg. When I then exited fftest (entering -1), the machine went
   down: the first time with a garbled panic screen that I couldn't
   read, and on later attempts as a total hang with no output. The
   same sequence on stock Arch 7.2.11-arch1-1 with the unpatched
   driver also hangs the whole system, so this predates the series.

   To reproduce: with the module loaded and the G920 plugged in, start
   fftest on its event node and leave it at the effect prompt, run
   "rmmod hid_logitech_hidpp" from another terminal, then enter -1 in
   fftest.

   I could not recover a trace yet and will send one if I manage to.

   My guess, from reading the code and not verified by a trace:
   hidpp_ff_create() stores hidpp_ff_destroy() and the other FF
   callbacks in the ff_device as pointers into the module. With fftest
   holding the evdev open, the input_dev outlives the driver, so when
   fftest closes, the final input_dev release calls ff->destroy in
   module code that rmmod has already freed.

Not tested: the battery patches (10-14, a wired G920 can't exercise
them), in-game FFB, and sysfs driver unbind.

Happy to test patches or send the full logs.

Gideon

Re: [PATCH v2 00/33] HID: logitech-hidpp: G920 force-feedback fixes, rework, and cleanups

[Dmitry Torokhov]

Hi Gideon,

On Thu, Nov 12, 2026 at 02:30:39AM +0100, Gideon Heinz wrote:
> Hi Dmitry,
>
> I tested v2 of the series on my G920 (046d:c262). Everything on my
> list works except one scenario that takes the whole machine down, with
> and without your series (last item).
...
>  - rmmod with fftest holding the device open: rmmod returned after
>    2.99 s with "Unloading HID++ force feedback" and nothing else in
>    dmesg. When I then exited fftest (entering -1), the machine went
>    down: the first time with a garbled panic screen that I couldn't
>    read, and on later attempts as a total hang with no output. The
>    same sequence on stock Arch 7.2.11-arch1-1 with the unpatched
>    driver also hangs the whole system, so this predates the series.
>
>    To reproduce: with the module loaded and the G920 plugged in, start
>    fftest on its event node and leave it at the effect prompt, run
>    "rmmod hid_logitech_hidpp" from another terminal, then enter -1 in
>    fftest.
>
>    I could not recover a trace yet and will send one if I manage to.
>
>    My guess, from reading the code and not verified by a trace:
>    hidpp_ff_create() stores hidpp_ff_destroy() and the other FF
>    callbacks in the ff_device as pointers into the module. With fftest
>    holding the evdev open, the input_dev outlives the driver, so when
>    fftest closes, the final input_dev release calls ff->destroy in
>    module code that rmmod has already freed.

Thank you very much for the thorough testing!

Your diagnosis is spot on. Opening /dev/input/eventX pins the evdev cdev
(and through it input_dev), but does not hold a module reference on the
underlying HID driver. When rmmod unloads hid_logitech_hidpp while fftest
still has the file descriptor open, __input_unregister_device() disconnects
evdev (flushing effects, closing the device, and marking evdev dead so all
subsequent ioctls and writes return -ENODEV) and runs ff->stop(), after
which rmmod frees the module text. When fftest finally exits and closes the
file descriptor, input_dev_release() calls input_ff_destroy(), which
invokes ff->destroy() pointing into the already-unmapped module text.

Rather than working around this in individual drivers with a separate
ff->stop() callback, input core should call input_ff_destroy() directly in
__input_unregister_device() once all handlers are disconnected and the
device is removed from sysfs. At that point nothing touches dev->ff
anymore, and calling input_ff_destroy() there guarantees ff->destroy()
always runs while the driver and its module are still loaded (while the
second input_ff_destroy() call in input_dev_release() becomes a no-op once
dev->ff is NULL).

Could you try the input core patch below (with patch 22/33 "HID:
logitech-hidpp: shut down G920 force feedback on input unregister" dropped
from the hid-logitech-hidpp series, since ff->stop is removed and
hidpp_ff_destroy() will now run on input unregistration)?

Alternatively, if you prefer to test with your out-of-tree module build
without rebuilding the kernel, you can test the equivalent effect by
setting:

	ff->destroy = NULL;

inside hidpp_ff_stop() in hid-logitech-hidpp.c.

Thanks.

---
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Subject: [PATCH] Input: destroy force-feedback device on input unregistration

When an input device is unregistered while userspace holds an open file
descriptor to its event node, input_dev_release() is deferred until the
last file descriptor is closed. Because input_ff_destroy() was only
called from input_dev_release(), force-feedback teardown (ff->destroy
and freeing ff->private) could run after the parent driver had already
unbound, freed its device state, or even been unloaded via rmmod,
causing use-after-free errors or page faults in unmapped module text.

Commit ef166ce08801 ("Input: stop force-feedback timer when
unregistering input devices") added a separate ff->stop callback for
ff-memless, but left ff->destroy invoked from input_dev_release().

Once __input_unregister_device() has disconnected all input handlers
(which flushes effects, closes the device, and marks evdev dead) and
deleted the device from sysfs, nothing accesses dev->ff anymore. Call
input_ff_destroy() directly in __input_unregister_device() so that
ff->destroy() always runs synchronously while the driver and its module
are still active, and remove the now-redundant ff->stop callback. Keep
input_ff_destroy() in input_dev_release() (where it is a no-op once
dev->ff is NULL) to handle input_free_device() on devices that were
never registered.

Fixes: 509ca1a93836 ("Input: implement new force feedback interface")
Cc: stable@vger.kernel.org
Reported-by: Gideon Heinz <gideonheinz02@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/ff-core.c    |  2 +-
 drivers/input/ff-memless.c | 23 -----------------------
 drivers/input/input.c      |  5 ++---
 include/linux/input.h      |  3 ---
 4 files changed, 3 insertions(+), 30 deletions(-)

diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
index 0e5d1d1ceb46..133b4622dc49 100644
--- a/drivers/input/ff-core.c
+++ b/drivers/input/ff-core.c
@@ -338,7 +338,7 @@ EXPORT_SYMBOL_GPL(input_ff_create);
  *
  * This function is only needed in error path as input core will
  * automatically free force feedback structures when device is
- * destroyed.
+ * unregistered or destroyed.
  */
 void input_ff_destroy(struct input_dev *dev)
 {
diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
index d1fefd1dfc0d..3069436148ec 100644
--- a/drivers/input/ff-memless.c
+++ b/drivers/input/ff-memless.c
@@ -483,32 +483,10 @@ static void ml_ff_destroy(struct ff_device *ff)
 {
 	struct ml_device *ml = ff->private;
 
-	/*
-	 * The timer is normally shut down in ml_ff_stop() when the device
-	 * is unregistered. However, we still shut it down here as a safety
-	 * net and for cases where the device was never registered (e.g.
-	 * error paths during probe).
-	 */
 	timer_shutdown_sync(&ml->timer);
-
 	kfree(ml->private);
 }
 
-static void ml_ff_stop(struct ff_device *ff)
-{
-	struct ml_device *ml = ff->private;
-
-	/*
-	 * Even though we stop all playing effects when tearing down an
-	 * input device (by the way of evdev calling input_flush_device()
-	 * that calls into input_ff_flush() that stops and erases all
-	 * effects), we do not actually shutdown the timer, and therefore
-	 * we should do it here to prevent it firing after the input
-	 * device is unregistered and its associated resources are freed.
-	 */
-	timer_shutdown_sync(&ml->timer);
-}
-
 /**
  * input_ff_create_memless() - create memoryless force-feedback device
  * @dev: input device supporting force-feedback
@@ -543,7 +521,6 @@ int input_ff_create_memless(struct input_dev *dev, void *data,
 	ff->playback = ml_ff_playback;
 	ff->set_gain = ml_ff_set_gain;
 	ff->destroy = ml_ff_destroy;
-	ff->stop = ml_ff_stop;
 
 	/* we can emulate periodic effects with RUMBLE */
 	if (test_bit(FF_RUMBLE, ff->ffbit)) {
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 210635ef147e..7be69deed8eb 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -2286,10 +2286,9 @@ static void __input_unregister_device(struct input_dev *dev)
 		input_wakeup_procfs_readers();
 	}
 
-	if (dev->ff && dev->ff->stop)
-		dev->ff->stop(dev->ff);
-
 	device_del(&dev->dev);
+
+	input_ff_destroy(dev);
 }
 
 static void devm_input_device_unregister(struct device *dev, void *res)
diff --git a/include/linux/input.h b/include/linux/input.h
index 3381608127f7..02047cc45d04 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -549,8 +549,6 @@ extern const struct class input_class;
  * @set_autocenter: Called to auto-center device
  * @destroy: called by input core when parent input device is being
  *	destroyed
- * @stop: called by input core when parent input device is being
- *	unregistered
  * @private: driver-specific data, will be freed automatically
  * @ffbit: bitmap of force feedback capabilities truly supported by
  *	device (not emulated like ones in input_dev->ffbit)
@@ -579,7 +577,6 @@ struct ff_device {
 	void (*set_autocenter)(struct input_dev *dev, u16 magnitude);
 
 	void (*destroy)(struct ff_device *);
-	void (*stop)(struct ff_device *);
 
 	void *private;
 

-- 
Dmitry

Re: [PATCH v2 00/33] HID: logitech-hidpp: G920 force-feedback fixes, rework, and cleanups

[Gideon Heinz]

Hi Dmitry,

I tested the out-of-tree equivalent you suggested, ff->destroy = NULL;
in hidpp_ff_stop(), on top of the 33-patch v2 module (G920 046d:c262,
7.3.1-1-cachyos).

Same reproduction as before: fftest holding the event node at the
effect prompt, rmmod hid_logitech_hidpp from another terminal, then
entering -1 in fftest.
This time fftest exits normally, the machine stays up, and there are
no errors in dmesg.
The only message is the single “Unloading HID++ force feedback.” at rmmod time.

I’ll try your input core patch on a rebuilt kernel later and report back

Thanks,
Gideon

On Thu, Nov 12, 2026 at 3:43 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Gideon,
>
> On Thu, Nov 12, 2026 at 02:30:39AM +0100, Gideon Heinz wrote:
> > Hi Dmitry,
> >
> > I tested v2 of the series on my G920 (046d:c262). Everything on my
> > list works except one scenario that takes the whole machine down, with
> > and without your series (last item).
> ...
> >  - rmmod with fftest holding the device open: rmmod returned after
> >    2.99 s with "Unloading HID++ force feedback" and nothing else in
> >    dmesg. When I then exited fftest (entering -1), the machine went
> >    down: the first time with a garbled panic screen that I couldn't
> >    read, and on later attempts as a total hang with no output. The
> >    same sequence on stock Arch 7.2.11-arch1-1 with the unpatched
> >    driver also hangs the whole system, so this predates the series.
> >
> >    To reproduce: with the module loaded and the G920 plugged in, start
> >    fftest on its event node and leave it at the effect prompt, run
> >    "rmmod hid_logitech_hidpp" from another terminal, then enter -1 in
> >    fftest.
> >
> >    I could not recover a trace yet and will send one if I manage to.
> >
> >    My guess, from reading the code and not verified by a trace:
> >    hidpp_ff_create() stores hidpp_ff_destroy() and the other FF
> >    callbacks in the ff_device as pointers into the module. With fftest
> >    holding the evdev open, the input_dev outlives the driver, so when
> >    fftest closes, the final input_dev release calls ff->destroy in
> >    module code that rmmod has already freed.
>
> Thank you very much for the thorough testing!
>
> Your diagnosis is spot on. Opening /dev/input/eventX pins the evdev cdev
> (and through it input_dev), but does not hold a module reference on the
> underlying HID driver. When rmmod unloads hid_logitech_hidpp while fftest
> still has the file descriptor open, __input_unregister_device() disconnects
> evdev (flushing effects, closing the device, and marking evdev dead so all
> subsequent ioctls and writes return -ENODEV) and runs ff->stop(), after
> which rmmod frees the module text. When fftest finally exits and closes the
> file descriptor, input_dev_release() calls input_ff_destroy(), which
> invokes ff->destroy() pointing into the already-unmapped module text.
>
> Rather than working around this in individual drivers with a separate
> ff->stop() callback, input core should call input_ff_destroy() directly in
> __input_unregister_device() once all handlers are disconnected and the
> device is removed from sysfs. At that point nothing touches dev->ff
> anymore, and calling input_ff_destroy() there guarantees ff->destroy()
> always runs while the driver and its module are still loaded (while the
> second input_ff_destroy() call in input_dev_release() becomes a no-op once
> dev->ff is NULL).
>
> Could you try the input core patch below (with patch 22/33 "HID:
> logitech-hidpp: shut down G920 force feedback on input unregister" dropped
> from the hid-logitech-hidpp series, since ff->stop is removed and
> hidpp_ff_destroy() will now run on input unregistration)?
>
> Alternatively, if you prefer to test with your out-of-tree module build
> without rebuilding the kernel, you can test the equivalent effect by
> setting:
>
>         ff->destroy = NULL;
>
> inside hidpp_ff_stop() in hid-logitech-hidpp.c.
>
> Thanks.
>
> ---
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Subject: [PATCH] Input: destroy force-feedback device on input unregistration
>
> When an input device is unregistered while userspace holds an open file
> descriptor to its event node, input_dev_release() is deferred until the
> last file descriptor is closed. Because input_ff_destroy() was only
> called from input_dev_release(), force-feedback teardown (ff->destroy
> and freeing ff->private) could run after the parent driver had already
> unbound, freed its device state, or even been unloaded via rmmod,
> causing use-after-free errors or page faults in unmapped module text.
>
> Commit ef166ce08801 ("Input: stop force-feedback timer when
> unregistering input devices") added a separate ff->stop callback for
> ff-memless, but left ff->destroy invoked from input_dev_release().
>
> Once __input_unregister_device() has disconnected all input handlers
> (which flushes effects, closes the device, and marks evdev dead) and
> deleted the device from sysfs, nothing accesses dev->ff anymore. Call
> input_ff_destroy() directly in __input_unregister_device() so that
> ff->destroy() always runs synchronously while the driver and its module
> are still active, and remove the now-redundant ff->stop callback. Keep
> input_ff_destroy() in input_dev_release() (where it is a no-op once
> dev->ff is NULL) to handle input_free_device() on devices that were
> never registered.
>
> Fixes: 509ca1a93836 ("Input: implement new force feedback interface")
> Cc: stable@vger.kernel.org
> Reported-by: Gideon Heinz <gideonheinz02@gmail.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/ff-core.c    |  2 +-
>  drivers/input/ff-memless.c | 23 -----------------------
>  drivers/input/input.c      |  5 ++---
>  include/linux/input.h      |  3 ---
>  4 files changed, 3 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
> index 0e5d1d1ceb46..133b4622dc49 100644
> --- a/drivers/input/ff-core.c
> +++ b/drivers/input/ff-core.c
> @@ -338,7 +338,7 @@ EXPORT_SYMBOL_GPL(input_ff_create);
>   *
>   * This function is only needed in error path as input core will
>   * automatically free force feedback structures when device is
> - * destroyed.
> + * unregistered or destroyed.
>   */
>  void input_ff_destroy(struct input_dev *dev)
>  {
> diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
> index d1fefd1dfc0d..3069436148ec 100644
> --- a/drivers/input/ff-memless.c
> +++ b/drivers/input/ff-memless.c
> @@ -483,32 +483,10 @@ static void ml_ff_destroy(struct ff_device *ff)
>  {
>         struct ml_device *ml = ff->private;
>
> -       /*
> -        * The timer is normally shut down in ml_ff_stop() when the device
> -        * is unregistered. However, we still shut it down here as a safety
> -        * net and for cases where the device was never registered (e.g.
> -        * error paths during probe).
> -        */
>         timer_shutdown_sync(&ml->timer);
> -
>         kfree(ml->private);
>  }
>
> -static void ml_ff_stop(struct ff_device *ff)
> -{
> -       struct ml_device *ml = ff->private;
> -
> -       /*
> -        * Even though we stop all playing effects when tearing down an
> -        * input device (by the way of evdev calling input_flush_device()
> -        * that calls into input_ff_flush() that stops and erases all
> -        * effects), we do not actually shutdown the timer, and therefore
> -        * we should do it here to prevent it firing after the input
> -        * device is unregistered and its associated resources are freed.
> -        */
> -       timer_shutdown_sync(&ml->timer);
> -}
> -
>  /**
>   * input_ff_create_memless() - create memoryless force-feedback device
>   * @dev: input device supporting force-feedback
> @@ -543,7 +521,6 @@ int input_ff_create_memless(struct input_dev *dev, void *data,
>         ff->playback = ml_ff_playback;
>         ff->set_gain = ml_ff_set_gain;
>         ff->destroy = ml_ff_destroy;
> -       ff->stop = ml_ff_stop;
>
>         /* we can emulate periodic effects with RUMBLE */
>         if (test_bit(FF_RUMBLE, ff->ffbit)) {
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 210635ef147e..7be69deed8eb 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -2286,10 +2286,9 @@ static void __input_unregister_device(struct input_dev *dev)
>                 input_wakeup_procfs_readers();
>         }
>
> -       if (dev->ff && dev->ff->stop)
> -               dev->ff->stop(dev->ff);
> -
>         device_del(&dev->dev);
> +
> +       input_ff_destroy(dev);
>  }
>
>  static void devm_input_device_unregister(struct device *dev, void *res)
> diff --git a/include/linux/input.h b/include/linux/input.h
> index 3381608127f7..02047cc45d04 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -549,8 +549,6 @@ extern const struct class input_class;
>   * @set_autocenter: Called to auto-center device
>   * @destroy: called by input core when parent input device is being
>   *     destroyed
> - * @stop: called by input core when parent input device is being
> - *     unregistered
>   * @private: driver-specific data, will be freed automatically
>   * @ffbit: bitmap of force feedback capabilities truly supported by
>   *     device (not emulated like ones in input_dev->ffbit)
> @@ -579,7 +577,6 @@ struct ff_device {
>         void (*set_autocenter)(struct input_dev *dev, u16 magnitude);
>
>         void (*destroy)(struct ff_device *);
> -       void (*stop)(struct ff_device *);
>
>         void *private;
>
>
> --
> Dmitry

Re: [PATCH v2 00/33] HID: logitech-hidpp: G920 force-feedback fixes, rework, and cleanups

[Gideon Heinz]

Hi Dmitry,

I tested your input core patch (“Input: destroy force-feedback device
on input unregistration”) on a real kernel: CachyOS 7.3.4 (stable,
which already has an equivalent G920 init fix) plus only your patch,
with the stock hid-logitech-hidpp driver. The hidpp series is not
applied, since it doesn’t apply to 7.3.4 as is.

Same reproduction as before: fftest holding the G920 event node at the
effect prompt, rmmod hid_logitech_hidpp from another terminal, then
fftest exiting. The machine no longer goes down. The module unloaded
cleanly and the system stayed responsive.

One observation: with the stock driver, rmmod blocked for about 3
minutes. After “Unloading HID++ force feedback.” dmesg shows 12
“Failed to send command to device!” lines, 15.36 s apart (the 3 x 5 s
retry per command), then rmmod returned. I assume these are the queued
effect erases timing out because the transport is already closed, and
that the series’ changes to hidpp_send_message_sync() and the
synchronous erase would remove this wait, but I have not tested that
combination on this kernel.

Tested-by: Gideon Heinz gideonheinz02@gmail.com

Thanks,
Gideon

On Thu, Nov 12, 2026 at 11:48 AM Gideon Heinz <gideonheinz02@gmail.com> wrote:
>
> Hi Dmitry,
>
> I tested the out-of-tree equivalent you suggested, ff->destroy = NULL;
> in hidpp_ff_stop(), on top of the 33-patch v2 module (G920 046d:c262,
> 7.3.1-1-cachyos).
>
> Same reproduction as before: fftest holding the event node at the
> effect prompt, rmmod hid_logitech_hidpp from another terminal, then
> entering -1 in fftest.
> This time fftest exits normally, the machine stays up, and there are
> no errors in dmesg.
> The only message is the single “Unloading HID++ force feedback.” at rmmod time.
>
> I’ll try your input core patch on a rebuilt kernel later and report back
>
> Thanks,
> Gideon
>
> On Thu, Nov 12, 2026 at 3:43 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Hi Gideon,
> >
> > On Thu, Nov 12, 2026 at 02:30:39AM +0100, Gideon Heinz wrote:
> > > Hi Dmitry,
> > >
> > > I tested v2 of the series on my G920 (046d:c262). Everything on my
> > > list works except one scenario that takes the whole machine down, with
> > > and without your series (last item).
> > ...
> > >  - rmmod with fftest holding the device open: rmmod returned after
> > >    2.99 s with "Unloading HID++ force feedback" and nothing else in
> > >    dmesg. When I then exited fftest (entering -1), the machine went
> > >    down: the first time with a garbled panic screen that I couldn't
> > >    read, and on later attempts as a total hang with no output. The
> > >    same sequence on stock Arch 7.2.11-arch1-1 with the unpatched
> > >    driver also hangs the whole system, so this predates the series.
> > >
> > >    To reproduce: with the module loaded and the G920 plugged in, start
> > >    fftest on its event node and leave it at the effect prompt, run
> > >    "rmmod hid_logitech_hidpp" from another terminal, then enter -1 in
> > >    fftest.
> > >
> > >    I could not recover a trace yet and will send one if I manage to.
> > >
> > >    My guess, from reading the code and not verified by a trace:
> > >    hidpp_ff_create() stores hidpp_ff_destroy() and the other FF
> > >    callbacks in the ff_device as pointers into the module. With fftest
> > >    holding the evdev open, the input_dev outlives the driver, so when
> > >    fftest closes, the final input_dev release calls ff->destroy in
> > >    module code that rmmod has already freed.
> >
> > Thank you very much for the thorough testing!
> >
> > Your diagnosis is spot on. Opening /dev/input/eventX pins the evdev cdev
> > (and through it input_dev), but does not hold a module reference on the
> > underlying HID driver. When rmmod unloads hid_logitech_hidpp while fftest
> > still has the file descriptor open, __input_unregister_device() disconnects
> > evdev (flushing effects, closing the device, and marking evdev dead so all
> > subsequent ioctls and writes return -ENODEV) and runs ff->stop(), after
> > which rmmod frees the module text. When fftest finally exits and closes the
> > file descriptor, input_dev_release() calls input_ff_destroy(), which
> > invokes ff->destroy() pointing into the already-unmapped module text.
> >
> > Rather than working around this in individual drivers with a separate
> > ff->stop() callback, input core should call input_ff_destroy() directly in
> > __input_unregister_device() once all handlers are disconnected and the
> > device is removed from sysfs. At that point nothing touches dev->ff
> > anymore, and calling input_ff_destroy() there guarantees ff->destroy()
> > always runs while the driver and its module are still loaded (while the
> > second input_ff_destroy() call in input_dev_release() becomes a no-op once
> > dev->ff is NULL).
> >
> > Could you try the input core patch below (with patch 22/33 "HID:
> > logitech-hidpp: shut down G920 force feedback on input unregister" dropped
> > from the hid-logitech-hidpp series, since ff->stop is removed and
> > hidpp_ff_destroy() will now run on input unregistration)?
> >
> > Alternatively, if you prefer to test with your out-of-tree module build
> > without rebuilding the kernel, you can test the equivalent effect by
> > setting:
> >
> >         ff->destroy = NULL;
> >
> > inside hidpp_ff_stop() in hid-logitech-hidpp.c.
> >
> > Thanks.
> >
> > ---
> > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Subject: [PATCH] Input: destroy force-feedback device on input unregistration
> >
> > When an input device is unregistered while userspace holds an open file
> > descriptor to its event node, input_dev_release() is deferred until the
> > last file descriptor is closed. Because input_ff_destroy() was only
> > called from input_dev_release(), force-feedback teardown (ff->destroy
> > and freeing ff->private) could run after the parent driver had already
> > unbound, freed its device state, or even been unloaded via rmmod,
> > causing use-after-free errors or page faults in unmapped module text.
> >
> > Commit ef166ce08801 ("Input: stop force-feedback timer when
> > unregistering input devices") added a separate ff->stop callback for
> > ff-memless, but left ff->destroy invoked from input_dev_release().
> >
> > Once __input_unregister_device() has disconnected all input handlers
> > (which flushes effects, closes the device, and marks evdev dead) and
> > deleted the device from sysfs, nothing accesses dev->ff anymore. Call
> > input_ff_destroy() directly in __input_unregister_device() so that
> > ff->destroy() always runs synchronously while the driver and its module
> > are still active, and remove the now-redundant ff->stop callback. Keep
> > input_ff_destroy() in input_dev_release() (where it is a no-op once
> > dev->ff is NULL) to handle input_free_device() on devices that were
> > never registered.
> >
> > Fixes: 509ca1a93836 ("Input: implement new force feedback interface")
> > Cc: stable@vger.kernel.org
> > Reported-by: Gideon Heinz <gideonheinz02@gmail.com>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/input/ff-core.c    |  2 +-
> >  drivers/input/ff-memless.c | 23 -----------------------
> >  drivers/input/input.c      |  5 ++---
> >  include/linux/input.h      |  3 ---
> >  4 files changed, 3 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
> > index 0e5d1d1ceb46..133b4622dc49 100644
> > --- a/drivers/input/ff-core.c
> > +++ b/drivers/input/ff-core.c
> > @@ -338,7 +338,7 @@ EXPORT_SYMBOL_GPL(input_ff_create);
> >   *
> >   * This function is only needed in error path as input core will
> >   * automatically free force feedback structures when device is
> > - * destroyed.
> > + * unregistered or destroyed.
> >   */
> >  void input_ff_destroy(struct input_dev *dev)
> >  {
> > diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
> > index d1fefd1dfc0d..3069436148ec 100644
> > --- a/drivers/input/ff-memless.c
> > +++ b/drivers/input/ff-memless.c
> > @@ -483,32 +483,10 @@ static void ml_ff_destroy(struct ff_device *ff)
> >  {
> >         struct ml_device *ml = ff->private;
> >
> > -       /*
> > -        * The timer is normally shut down in ml_ff_stop() when the device
> > -        * is unregistered. However, we still shut it down here as a safety
> > -        * net and for cases where the device was never registered (e.g.
> > -        * error paths during probe).
> > -        */
> >         timer_shutdown_sync(&ml->timer);
> > -
> >         kfree(ml->private);
> >  }
> >
> > -static void ml_ff_stop(struct ff_device *ff)
> > -{
> > -       struct ml_device *ml = ff->private;
> > -
> > -       /*
> > -        * Even though we stop all playing effects when tearing down an
> > -        * input device (by the way of evdev calling input_flush_device()
> > -        * that calls into input_ff_flush() that stops and erases all
> > -        * effects), we do not actually shutdown the timer, and therefore
> > -        * we should do it here to prevent it firing after the input
> > -        * device is unregistered and its associated resources are freed.
> > -        */
> > -       timer_shutdown_sync(&ml->timer);
> > -}
> > -
> >  /**
> >   * input_ff_create_memless() - create memoryless force-feedback device
> >   * @dev: input device supporting force-feedback
> > @@ -543,7 +521,6 @@ int input_ff_create_memless(struct input_dev *dev, void *data,
> >         ff->playback = ml_ff_playback;
> >         ff->set_gain = ml_ff_set_gain;
> >         ff->destroy = ml_ff_destroy;
> > -       ff->stop = ml_ff_stop;
> >
> >         /* we can emulate periodic effects with RUMBLE */
> >         if (test_bit(FF_RUMBLE, ff->ffbit)) {
> > diff --git a/drivers/input/input.c b/drivers/input/input.c
> > index 210635ef147e..7be69deed8eb 100644
> > --- a/drivers/input/input.c
> > +++ b/drivers/input/input.c
> > @@ -2286,10 +2286,9 @@ static void __input_unregister_device(struct input_dev *dev)
> >                 input_wakeup_procfs_readers();
> >         }
> >
> > -       if (dev->ff && dev->ff->stop)
> > -               dev->ff->stop(dev->ff);
> > -
> >         device_del(&dev->dev);
> > +
> > +       input_ff_destroy(dev);
> >  }
> >
> >  static void devm_input_device_unregister(struct device *dev, void *res)
> > diff --git a/include/linux/input.h b/include/linux/input.h
> > index 3381608127f7..02047cc45d04 100644
> > --- a/include/linux/input.h
> > +++ b/include/linux/input.h
> > @@ -549,8 +549,6 @@ extern const struct class input_class;
> >   * @set_autocenter: Called to auto-center device
> >   * @destroy: called by input core when parent input device is being
> >   *     destroyed
> > - * @stop: called by input core when parent input device is being
> > - *     unregistered
> >   * @private: driver-specific data, will be freed automatically
> >   * @ffbit: bitmap of force feedback capabilities truly supported by
> >   *     device (not emulated like ones in input_dev->ffbit)
> > @@ -579,7 +577,6 @@ struct ff_device {
> >         void (*set_autocenter)(struct input_dev *dev, u16 magnitude);
> >
> >         void (*destroy)(struct ff_device *);
> > -       void (*stop)(struct ff_device *);
> >
> >         void *private;
> >
> >
> > --
> > Dmitry

Re: [PATCH v2 00/33] HID: logitech-hidpp: G920 force-feedback fixes, rework, and cleanups

[Gideon Heinz]

Hi Dmitry,

A follow-up to my last mail. I ran the rmmod test a second time with
the same result: no crash, and rmmod blocked for 3m7s, again with 12
“Failed to send command to device!” lines 15.36 s apart. This time the
kernel printed a hung task warning, which shows where it waits:

INFO: task rmmod:6286 blocked for more than 122 seconds.
      Tainted: G S  7.3.4-1-cachyos #1
task:rmmod state:D
Call Trace:
 schedule+0x6f/0xb0
 schedule_timeout+0x31/0x140
 wait_for_completion+0xc7/0x1f0
 __flush_workqueue+0x198/0x420
 drain_workqueue+0x4f/0x150
 destroy_workqueue+0x56/0x390
 hidpp_ff_destroy+0x42/0x50 [hid_logitech_hidpp]
 input_ff_destroy+0x33/0x70
 input_unregister_device+0x48/0x70
 hidinput_disconnect+0x45/0xc0
 hid_hw_stop+0x8c/0xc0
 hidpp_remove+0x39/0x60 [hid_logitech_hidpp]
 hid_device_remove+0x4a/0xa0
 device_release_driver_internal+0x1c2/0x320
 driver_detach+0x8f/0xd0
 bus_remove_driver+0x102/0x190
 hid_unregister_driver+0x22/0xb0
 __se_sys_delete_module+0x291/0x3d0

So with your patch the final teardown now runs from
input_unregister_device() while the module is still loaded, which is
what we wanted. The 3 minute wait is destroy_workqueue() draining the
queued commands, each of which times out after the driver’s 3 x 5 s
retry. I have not tested whether the series’ changes to the send path
and removal order shorten this on this kernel.

Thanks,
Gideon

On Thu, Nov 12, 2026 at 3:30 PM Gideon Heinz <gideonheinz02@gmail.com> wrote:
>
> Hi Dmitry,
>
> I tested your input core patch (“Input: destroy force-feedback device
> on input unregistration”) on a real kernel: CachyOS 7.3.4 (stable,
> which already has an equivalent G920 init fix) plus only your patch,
> with the stock hid-logitech-hidpp driver. The hidpp series is not
> applied, since it doesn’t apply to 7.3.4 as is.
>
> Same reproduction as before: fftest holding the G920 event node at the
> effect prompt, rmmod hid_logitech_hidpp from another terminal, then
> fftest exiting. The machine no longer goes down. The module unloaded
> cleanly and the system stayed responsive.
>
> One observation: with the stock driver, rmmod blocked for about 3
> minutes. After “Unloading HID++ force feedback.” dmesg shows 12
> “Failed to send command to device!” lines, 15.36 s apart (the 3 x 5 s
> retry per command), then rmmod returned. I assume these are the queued
> effect erases timing out because the transport is already closed, and
> that the series’ changes to hidpp_send_message_sync() and the
> synchronous erase would remove this wait, but I have not tested that
> combination on this kernel.
>
> Tested-by: Gideon Heinz gideonheinz02@gmail.com
>
> Thanks,
> Gideon
>
> On Thu, Nov 12, 2026 at 11:48 AM Gideon Heinz <gideonheinz02@gmail.com> wrote:
> >
> > Hi Dmitry,
> >
> > I tested the out-of-tree equivalent you suggested, ff->destroy = NULL;
> > in hidpp_ff_stop(), on top of the 33-patch v2 module (G920 046d:c262,
> > 7.3.1-1-cachyos).
> >
> > Same reproduction as before: fftest holding the event node at the
> > effect prompt, rmmod hid_logitech_hidpp from another terminal, then
> > entering -1 in fftest.
> > This time fftest exits normally, the machine stays up, and there are
> > no errors in dmesg.
> > The only message is the single “Unloading HID++ force feedback.” at rmmod time.
> >
> > I’ll try your input core patch on a rebuilt kernel later and report back
> >
> > Thanks,
> > Gideon
> >
> > On Thu, Nov 12, 2026 at 3:43 AM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > Hi Gideon,
> > >
> > > On Thu, Nov 12, 2026 at 02:30:39AM +0100, Gideon Heinz wrote:
> > > > Hi Dmitry,
> > > >
> > > > I tested v2 of the series on my G920 (046d:c262). Everything on my
> > > > list works except one scenario that takes the whole machine down, with
> > > > and without your series (last item).
> > > ...
> > > >  - rmmod with fftest holding the device open: rmmod returned after
> > > >    2.99 s with "Unloading HID++ force feedback" and nothing else in
> > > >    dmesg. When I then exited fftest (entering -1), the machine went
> > > >    down: the first time with a garbled panic screen that I couldn't
> > > >    read, and on later attempts as a total hang with no output. The
> > > >    same sequence on stock Arch 7.2.11-arch1-1 with the unpatched
> > > >    driver also hangs the whole system, so this predates the series.
> > > >
> > > >    To reproduce: with the module loaded and the G920 plugged in, start
> > > >    fftest on its event node and leave it at the effect prompt, run
> > > >    "rmmod hid_logitech_hidpp" from another terminal, then enter -1 in
> > > >    fftest.
> > > >
> > > >    I could not recover a trace yet and will send one if I manage to.
> > > >
> > > >    My guess, from reading the code and not verified by a trace:
> > > >    hidpp_ff_create() stores hidpp_ff_destroy() and the other FF
> > > >    callbacks in the ff_device as pointers into the module. With fftest
> > > >    holding the evdev open, the input_dev outlives the driver, so when
> > > >    fftest closes, the final input_dev release calls ff->destroy in
> > > >    module code that rmmod has already freed.
> > >
> > > Thank you very much for the thorough testing!
> > >
> > > Your diagnosis is spot on. Opening /dev/input/eventX pins the evdev cdev
> > > (and through it input_dev), but does not hold a module reference on the
> > > underlying HID driver. When rmmod unloads hid_logitech_hidpp while fftest
> > > still has the file descriptor open, __input_unregister_device() disconnects
> > > evdev (flushing effects, closing the device, and marking evdev dead so all
> > > subsequent ioctls and writes return -ENODEV) and runs ff->stop(), after
> > > which rmmod frees the module text. When fftest finally exits and closes the
> > > file descriptor, input_dev_release() calls input_ff_destroy(), which
> > > invokes ff->destroy() pointing into the already-unmapped module text.
> > >
> > > Rather than working around this in individual drivers with a separate
> > > ff->stop() callback, input core should call input_ff_destroy() directly in
> > > __input_unregister_device() once all handlers are disconnected and the
> > > device is removed from sysfs. At that point nothing touches dev->ff
> > > anymore, and calling input_ff_destroy() there guarantees ff->destroy()
> > > always runs while the driver and its module are still loaded (while the
> > > second input_ff_destroy() call in input_dev_release() becomes a no-op once
> > > dev->ff is NULL).
> > >
> > > Could you try the input core patch below (with patch 22/33 "HID:
> > > logitech-hidpp: shut down G920 force feedback on input unregister" dropped
> > > from the hid-logitech-hidpp series, since ff->stop is removed and
> > > hidpp_ff_destroy() will now run on input unregistration)?
> > >
> > > Alternatively, if you prefer to test with your out-of-tree module build
> > > without rebuilding the kernel, you can test the equivalent effect by
> > > setting:
> > >
> > >         ff->destroy = NULL;
> > >
> > > inside hidpp_ff_stop() in hid-logitech-hidpp.c.
> > >
> > > Thanks.
> > >
> > > ---
> > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > Subject: [PATCH] Input: destroy force-feedback device on input unregistration
> > >
> > > When an input device is unregistered while userspace holds an open file
> > > descriptor to its event node, input_dev_release() is deferred until the
> > > last file descriptor is closed. Because input_ff_destroy() was only
> > > called from input_dev_release(), force-feedback teardown (ff->destroy
> > > and freeing ff->private) could run after the parent driver had already
> > > unbound, freed its device state, or even been unloaded via rmmod,
> > > causing use-after-free errors or page faults in unmapped module text.
> > >
> > > Commit ef166ce08801 ("Input: stop force-feedback timer when
> > > unregistering input devices") added a separate ff->stop callback for
> > > ff-memless, but left ff->destroy invoked from input_dev_release().
> > >
> > > Once __input_unregister_device() has disconnected all input handlers
> > > (which flushes effects, closes the device, and marks evdev dead) and
> > > deleted the device from sysfs, nothing accesses dev->ff anymore. Call
> > > input_ff_destroy() directly in __input_unregister_device() so that
> > > ff->destroy() always runs synchronously while the driver and its module
> > > are still active, and remove the now-redundant ff->stop callback. Keep
> > > input_ff_destroy() in input_dev_release() (where it is a no-op once
> > > dev->ff is NULL) to handle input_free_device() on devices that were
> > > never registered.
> > >
> > > Fixes: 509ca1a93836 ("Input: implement new force feedback interface")
> > > Cc: stable@vger.kernel.org
> > > Reported-by: Gideon Heinz <gideonheinz02@gmail.com>
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > ---
> > >  drivers/input/ff-core.c    |  2 +-
> > >  drivers/input/ff-memless.c | 23 -----------------------
> > >  drivers/input/input.c      |  5 ++---
> > >  include/linux/input.h      |  3 ---
> > >  4 files changed, 3 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
> > > index 0e5d1d1ceb46..133b4622dc49 100644
> > > --- a/drivers/input/ff-core.c
> > > +++ b/drivers/input/ff-core.c
> > > @@ -338,7 +338,7 @@ EXPORT_SYMBOL_GPL(input_ff_create);
> > >   *
> > >   * This function is only needed in error path as input core will
> > >   * automatically free force feedback structures when device is
> > > - * destroyed.
> > > + * unregistered or destroyed.
> > >   */
> > >  void input_ff_destroy(struct input_dev *dev)
> > >  {
> > > diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
> > > index d1fefd1dfc0d..3069436148ec 100644
> > > --- a/drivers/input/ff-memless.c
> > > +++ b/drivers/input/ff-memless.c
> > > @@ -483,32 +483,10 @@ static void ml_ff_destroy(struct ff_device *ff)
> > >  {
> > >         struct ml_device *ml = ff->private;
> > >
> > > -       /*
> > > -        * The timer is normally shut down in ml_ff_stop() when the device
> > > -        * is unregistered. However, we still shut it down here as a safety
> > > -        * net and for cases where the device was never registered (e.g.
> > > -        * error paths during probe).
> > > -        */
> > >         timer_shutdown_sync(&ml->timer);
> > > -
> > >         kfree(ml->private);
> > >  }
> > >
> > > -static void ml_ff_stop(struct ff_device *ff)
> > > -{
> > > -       struct ml_device *ml = ff->private;
> > > -
> > > -       /*
> > > -        * Even though we stop all playing effects when tearing down an
> > > -        * input device (by the way of evdev calling input_flush_device()
> > > -        * that calls into input_ff_flush() that stops and erases all
> > > -        * effects), we do not actually shutdown the timer, and therefore
> > > -        * we should do it here to prevent it firing after the input
> > > -        * device is unregistered and its associated resources are freed.
> > > -        */
> > > -       timer_shutdown_sync(&ml->timer);
> > > -}
> > > -
> > >  /**
> > >   * input_ff_create_memless() - create memoryless force-feedback device
> > >   * @dev: input device supporting force-feedback
> > > @@ -543,7 +521,6 @@ int input_ff_create_memless(struct input_dev *dev, void *data,
> > >         ff->playback = ml_ff_playback;
> > >         ff->set_gain = ml_ff_set_gain;
> > >         ff->destroy = ml_ff_destroy;
> > > -       ff->stop = ml_ff_stop;
> > >
> > >         /* we can emulate periodic effects with RUMBLE */
> > >         if (test_bit(FF_RUMBLE, ff->ffbit)) {
> > > diff --git a/drivers/input/input.c b/drivers/input/input.c
> > > index 210635ef147e..7be69deed8eb 100644
> > > --- a/drivers/input/input.c
> > > +++ b/drivers/input/input.c
> > > @@ -2286,10 +2286,9 @@ static void __input_unregister_device(struct input_dev *dev)
> > >                 input_wakeup_procfs_readers();
> > >         }
> > >
> > > -       if (dev->ff && dev->ff->stop)
> > > -               dev->ff->stop(dev->ff);
> > > -
> > >         device_del(&dev->dev);
> > > +
> > > +       input_ff_destroy(dev);
> > >  }
> > >
> > >  static void devm_input_device_unregister(struct device *dev, void *res)
> > > diff --git a/include/linux/input.h b/include/linux/input.h
> > > index 3381608127f7..02047cc45d04 100644
> > > --- a/include/linux/input.h
> > > +++ b/include/linux/input.h
> > > @@ -549,8 +549,6 @@ extern const struct class input_class;
> > >   * @set_autocenter: Called to auto-center device
> > >   * @destroy: called by input core when parent input device is being
> > >   *     destroyed
> > > - * @stop: called by input core when parent input device is being
> > > - *     unregistered
> > >   * @private: driver-specific data, will be freed automatically
> > >   * @ffbit: bitmap of force feedback capabilities truly supported by
> > >   *     device (not emulated like ones in input_dev->ffbit)
> > > @@ -579,7 +577,6 @@ struct ff_device {
> > >         void (*set_autocenter)(struct input_dev *dev, u16 magnitude);
> > >
> > >         void (*destroy)(struct ff_device *);
> > > -       void (*stop)(struct ff_device *);
> > >
> > >         void *private;
> > >
> > >
> > > --
> > > Dmitry

Re: [PATCH v2 00/33] HID: logitech-hidpp: G920 force-feedback fixes, rework, and cleanups

[Gideon Heinz]

Hi Dmitry,

One more result, this time the combination you asked for: your input
core patch in the kernel, plus the v2 series without patch 22/33 as
the driver.

Setup: CachyOS 7.3.4 kernel with only your “Input: destroy
force-feedback device on input unregistration” patch built in. On top
of that, hid-logitech-hidpp built out of tree against the 7.3.4
headers from your v2 base (630059a5a632) with patches 1-21 and 23-33
(7.3.4 already has a different G920 init fix, so I could not apply the
series to the in-tree driver directly).

Same reproduction: fftest holding the G920 event node at the effect
prompt, rmmod hid_logitech_hidpp, then exiting fftest. Result: rmmod
returns in 98 ms, there are no “Failed to send command to device!”
lines at all, no hung task warning, and fftest exits normally with the
machine staying up. For comparison, your input patch with the stock
7.3.4 driver took about 3 minutes and printed 12 failures, as in my
previous mail.

If you would like me to add a Tested-by for the combination (input
patch + series without 22/33), please say so.

Thanks,
Gideon

On Thu, Nov 12, 2026 at 3:41 PM Gideon Heinz <gideonheinz02@gmail.com> wrote:
>
> Hi Dmitry,
>
> A follow-up to my last mail. I ran the rmmod test a second time with
> the same result: no crash, and rmmod blocked for 3m7s, again with 12
> “Failed to send command to device!” lines 15.36 s apart. This time the
> kernel printed a hung task warning, which shows where it waits:
>
> INFO: task rmmod:6286 blocked for more than 122 seconds.
>       Tainted: G S  7.3.4-1-cachyos #1
> task:rmmod state:D
> Call Trace:
>  schedule+0x6f/0xb0
>  schedule_timeout+0x31/0x140
>  wait_for_completion+0xc7/0x1f0
>  __flush_workqueue+0x198/0x420
>  drain_workqueue+0x4f/0x150
>  destroy_workqueue+0x56/0x390
>  hidpp_ff_destroy+0x42/0x50 [hid_logitech_hidpp]
>  input_ff_destroy+0x33/0x70
>  input_unregister_device+0x48/0x70
>  hidinput_disconnect+0x45/0xc0
>  hid_hw_stop+0x8c/0xc0
>  hidpp_remove+0x39/0x60 [hid_logitech_hidpp]
>  hid_device_remove+0x4a/0xa0
>  device_release_driver_internal+0x1c2/0x320
>  driver_detach+0x8f/0xd0
>  bus_remove_driver+0x102/0x190
>  hid_unregister_driver+0x22/0xb0
>  __se_sys_delete_module+0x291/0x3d0
>
> So with your patch the final teardown now runs from
> input_unregister_device() while the module is still loaded, which is
> what we wanted. The 3 minute wait is destroy_workqueue() draining the
> queued commands, each of which times out after the driver’s 3 x 5 s
> retry. I have not tested whether the series’ changes to the send path
> and removal order shorten this on this kernel.
>
> Thanks,
> Gideon
>
> On Thu, Nov 12, 2026 at 3:30 PM Gideon Heinz <gideonheinz02@gmail.com> wrote:
> >
> > Hi Dmitry,
> >
> > I tested your input core patch (“Input: destroy force-feedback device
> > on input unregistration”) on a real kernel: CachyOS 7.3.4 (stable,
> > which already has an equivalent G920 init fix) plus only your patch,
> > with the stock hid-logitech-hidpp driver. The hidpp series is not
> > applied, since it doesn’t apply to 7.3.4 as is.
> >
> > Same reproduction as before: fftest holding the G920 event node at the
> > effect prompt, rmmod hid_logitech_hidpp from another terminal, then
> > fftest exiting. The machine no longer goes down. The module unloaded
> > cleanly and the system stayed responsive.
> >
> > One observation: with the stock driver, rmmod blocked for about 3
> > minutes. After “Unloading HID++ force feedback.” dmesg shows 12
> > “Failed to send command to device!” lines, 15.36 s apart (the 3 x 5 s
> > retry per command), then rmmod returned. I assume these are the queued
> > effect erases timing out because the transport is already closed, and
> > that the series’ changes to hidpp_send_message_sync() and the
> > synchronous erase would remove this wait, but I have not tested that
> > combination on this kernel.
> >
> > Tested-by: Gideon Heinz gideonheinz02@gmail.com
> >
> > Thanks,
> > Gideon
> >
> > On Thu, Nov 12, 2026 at 11:48 AM Gideon Heinz <gideonheinz02@gmail.com> wrote:
> > >
> > > Hi Dmitry,
> > >
> > > I tested the out-of-tree equivalent you suggested, ff->destroy = NULL;
> > > in hidpp_ff_stop(), on top of the 33-patch v2 module (G920 046d:c262,
> > > 7.3.1-1-cachyos).
> > >
> > > Same reproduction as before: fftest holding the event node at the
> > > effect prompt, rmmod hid_logitech_hidpp from another terminal, then
> > > entering -1 in fftest.
> > > This time fftest exits normally, the machine stays up, and there are
> > > no errors in dmesg.
> > > The only message is the single “Unloading HID++ force feedback.” at rmmod time.
> > >
> > > I’ll try your input core patch on a rebuilt kernel later and report back
> > >
> > > Thanks,
> > > Gideon
> > >
> > > On Thu, Nov 12, 2026 at 3:43 AM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > Hi Gideon,
> > > >
> > > > On Thu, Nov 12, 2026 at 02:30:39AM +0100, Gideon Heinz wrote:
> > > > > Hi Dmitry,
> > > > >
> > > > > I tested v2 of the series on my G920 (046d:c262). Everything on my
> > > > > list works except one scenario that takes the whole machine down, with
> > > > > and without your series (last item).
> > > > ...
> > > > >  - rmmod with fftest holding the device open: rmmod returned after
> > > > >    2.99 s with "Unloading HID++ force feedback" and nothing else in
> > > > >    dmesg. When I then exited fftest (entering -1), the machine went
> > > > >    down: the first time with a garbled panic screen that I couldn't
> > > > >    read, and on later attempts as a total hang with no output. The
> > > > >    same sequence on stock Arch 7.2.11-arch1-1 with the unpatched
> > > > >    driver also hangs the whole system, so this predates the series.
> > > > >
> > > > >    To reproduce: with the module loaded and the G920 plugged in, start
> > > > >    fftest on its event node and leave it at the effect prompt, run
> > > > >    "rmmod hid_logitech_hidpp" from another terminal, then enter -1 in
> > > > >    fftest.
> > > > >
> > > > >    I could not recover a trace yet and will send one if I manage to.
> > > > >
> > > > >    My guess, from reading the code and not verified by a trace:
> > > > >    hidpp_ff_create() stores hidpp_ff_destroy() and the other FF
> > > > >    callbacks in the ff_device as pointers into the module. With fftest
> > > > >    holding the evdev open, the input_dev outlives the driver, so when
> > > > >    fftest closes, the final input_dev release calls ff->destroy in
> > > > >    module code that rmmod has already freed.
> > > >
> > > > Thank you very much for the thorough testing!
> > > >
> > > > Your diagnosis is spot on. Opening /dev/input/eventX pins the evdev cdev
> > > > (and through it input_dev), but does not hold a module reference on the
> > > > underlying HID driver. When rmmod unloads hid_logitech_hidpp while fftest
> > > > still has the file descriptor open, __input_unregister_device() disconnects
> > > > evdev (flushing effects, closing the device, and marking evdev dead so all
> > > > subsequent ioctls and writes return -ENODEV) and runs ff->stop(), after
> > > > which rmmod frees the module text. When fftest finally exits and closes the
> > > > file descriptor, input_dev_release() calls input_ff_destroy(), which
> > > > invokes ff->destroy() pointing into the already-unmapped module text.
> > > >
> > > > Rather than working around this in individual drivers with a separate
> > > > ff->stop() callback, input core should call input_ff_destroy() directly in
> > > > __input_unregister_device() once all handlers are disconnected and the
> > > > device is removed from sysfs. At that point nothing touches dev->ff
> > > > anymore, and calling input_ff_destroy() there guarantees ff->destroy()
> > > > always runs while the driver and its module are still loaded (while the
> > > > second input_ff_destroy() call in input_dev_release() becomes a no-op once
> > > > dev->ff is NULL).
> > > >
> > > > Could you try the input core patch below (with patch 22/33 "HID:
> > > > logitech-hidpp: shut down G920 force feedback on input unregister" dropped
> > > > from the hid-logitech-hidpp series, since ff->stop is removed and
> > > > hidpp_ff_destroy() will now run on input unregistration)?
> > > >
> > > > Alternatively, if you prefer to test with your out-of-tree module build
> > > > without rebuilding the kernel, you can test the equivalent effect by
> > > > setting:
> > > >
> > > >         ff->destroy = NULL;
> > > >
> > > > inside hidpp_ff_stop() in hid-logitech-hidpp.c.
> > > >
> > > > Thanks.
> > > >
> > > > ---
> > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > Subject: [PATCH] Input: destroy force-feedback device on input unregistration
> > > >
> > > > When an input device is unregistered while userspace holds an open file
> > > > descriptor to its event node, input_dev_release() is deferred until the
> > > > last file descriptor is closed. Because input_ff_destroy() was only
> > > > called from input_dev_release(), force-feedback teardown (ff->destroy
> > > > and freeing ff->private) could run after the parent driver had already
> > > > unbound, freed its device state, or even been unloaded via rmmod,
> > > > causing use-after-free errors or page faults in unmapped module text.
> > > >
> > > > Commit ef166ce08801 ("Input: stop force-feedback timer when
> > > > unregistering input devices") added a separate ff->stop callback for
> > > > ff-memless, but left ff->destroy invoked from input_dev_release().
> > > >
> > > > Once __input_unregister_device() has disconnected all input handlers
> > > > (which flushes effects, closes the device, and marks evdev dead) and
> > > > deleted the device from sysfs, nothing accesses dev->ff anymore. Call
> > > > input_ff_destroy() directly in __input_unregister_device() so that
> > > > ff->destroy() always runs synchronously while the driver and its module
> > > > are still active, and remove the now-redundant ff->stop callback. Keep
> > > > input_ff_destroy() in input_dev_release() (where it is a no-op once
> > > > dev->ff is NULL) to handle input_free_device() on devices that were
> > > > never registered.
> > > >
> > > > Fixes: 509ca1a93836 ("Input: implement new force feedback interface")
> > > > Cc: stable@vger.kernel.org
> > > > Reported-by: Gideon Heinz <gideonheinz02@gmail.com>
> > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > ---
> > > >  drivers/input/ff-core.c    |  2 +-
> > > >  drivers/input/ff-memless.c | 23 -----------------------
> > > >  drivers/input/input.c      |  5 ++---
> > > >  include/linux/input.h      |  3 ---
> > > >  4 files changed, 3 insertions(+), 30 deletions(-)
> > > >
> > > > diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
> > > > index 0e5d1d1ceb46..133b4622dc49 100644
> > > > --- a/drivers/input/ff-core.c
> > > > +++ b/drivers/input/ff-core.c
> > > > @@ -338,7 +338,7 @@ EXPORT_SYMBOL_GPL(input_ff_create);
> > > >   *
> > > >   * This function is only needed in error path as input core will
> > > >   * automatically free force feedback structures when device is
> > > > - * destroyed.
> > > > + * unregistered or destroyed.
> > > >   */
> > > >  void input_ff_destroy(struct input_dev *dev)
> > > >  {
> > > > diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
> > > > index d1fefd1dfc0d..3069436148ec 100644
> > > > --- a/drivers/input/ff-memless.c
> > > > +++ b/drivers/input/ff-memless.c
> > > > @@ -483,32 +483,10 @@ static void ml_ff_destroy(struct ff_device *ff)
> > > >  {
> > > >         struct ml_device *ml = ff->private;
> > > >
> > > > -       /*
> > > > -        * The timer is normally shut down in ml_ff_stop() when the device
> > > > -        * is unregistered. However, we still shut it down here as a safety
> > > > -        * net and for cases where the device was never registered (e.g.
> > > > -        * error paths during probe).
> > > > -        */
> > > >         timer_shutdown_sync(&ml->timer);
> > > > -
> > > >         kfree(ml->private);
> > > >  }
> > > >
> > > > -static void ml_ff_stop(struct ff_device *ff)
> > > > -{
> > > > -       struct ml_device *ml = ff->private;
> > > > -
> > > > -       /*
> > > > -        * Even though we stop all playing effects when tearing down an
> > > > -        * input device (by the way of evdev calling input_flush_device()
> > > > -        * that calls into input_ff_flush() that stops and erases all
> > > > -        * effects), we do not actually shutdown the timer, and therefore
> > > > -        * we should do it here to prevent it firing after the input
> > > > -        * device is unregistered and its associated resources are freed.
> > > > -        */
> > > > -       timer_shutdown_sync(&ml->timer);
> > > > -}
> > > > -
> > > >  /**
> > > >   * input_ff_create_memless() - create memoryless force-feedback device
> > > >   * @dev: input device supporting force-feedback
> > > > @@ -543,7 +521,6 @@ int input_ff_create_memless(struct input_dev *dev, void *data,
> > > >         ff->playback = ml_ff_playback;
> > > >         ff->set_gain = ml_ff_set_gain;
> > > >         ff->destroy = ml_ff_destroy;
> > > > -       ff->stop = ml_ff_stop;
> > > >
> > > >         /* we can emulate periodic effects with RUMBLE */
> > > >         if (test_bit(FF_RUMBLE, ff->ffbit)) {
> > > > diff --git a/drivers/input/input.c b/drivers/input/input.c
> > > > index 210635ef147e..7be69deed8eb 100644
> > > > --- a/drivers/input/input.c
> > > > +++ b/drivers/input/input.c
> > > > @@ -2286,10 +2286,9 @@ static void __input_unregister_device(struct input_dev *dev)
> > > >                 input_wakeup_procfs_readers();
> > > >         }
> > > >
> > > > -       if (dev->ff && dev->ff->stop)
> > > > -               dev->ff->stop(dev->ff);
> > > > -
> > > >         device_del(&dev->dev);
> > > > +
> > > > +       input_ff_destroy(dev);
> > > >  }
> > > >
> > > >  static void devm_input_device_unregister(struct device *dev, void *res)
> > > > diff --git a/include/linux/input.h b/include/linux/input.h
> > > > index 3381608127f7..02047cc45d04 100644
> > > > --- a/include/linux/input.h
> > > > +++ b/include/linux/input.h
> > > > @@ -549,8 +549,6 @@ extern const struct class input_class;
> > > >   * @set_autocenter: Called to auto-center device
> > > >   * @destroy: called by input core when parent input device is being
> > > >   *     destroyed
> > > > - * @stop: called by input core when parent input device is being
> > > > - *     unregistered
> > > >   * @private: driver-specific data, will be freed automatically
> > > >   * @ffbit: bitmap of force feedback capabilities truly supported by
> > > >   *     device (not emulated like ones in input_dev->ffbit)
> > > > @@ -579,7 +577,6 @@ struct ff_device {
> > > >         void (*set_autocenter)(struct input_dev *dev, u16 magnitude);
> > > >
> > > >         void (*destroy)(struct ff_device *);
> > > > -       void (*stop)(struct ff_device *);
> > > >
> > > >         void *private;
> > > >
> > > >
> > > > --
> > > > Dmitry