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

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

Thanks,
Gideon

On Thu, Nov 12, 2026 at 11:48 AM Gideon Heinz [off-list ref] 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
[off-list ref] wrote:

quoted

Hi Gideon,

On Thu, Nov 12, 2026 at 02:30:39AM +0100, Gideon Heinz wrote:

quoted

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

...

quoted

 - 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: <redacted>
Reported-by: Gideon Heinz <redacted>
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