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

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