From: sashiko-bot@kernel.org
To: "Dmitry Torokhov" <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org
Subject: Re: [PATCH 04/12] HID: rmi: fix device lifecycle, suspend, and reset work handling
Date: Sat, 14 Nov 2026 06:21:03 +0000 [thread overview]
Message-ID: <sashiko-outbox-214211@kernel.org> (raw)
In-Reply-To: <20261113-hid-rmi-v1-4-afc43d97d766@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] HID: rmi: race between rmi_event() and rmi_suspend() queuing
reset_work
- [Medium] HID: rmi: disabled IRQ on hid_hw_open() failure in
rmi_post_resume()
--
commit d4708cefad45f4f15a5ff3b156b07b3725726356
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
HID: rmi: fix device lifecycle, suspend, and reset work handling
This commit gates reset work scheduling and power management callbacks on
RMI_STARTED and RMI_SUSPENDED, ensures rmi_driver_resume() runs even if
rmi_reset_attn_mode() fails, and uses disable_work_sync() in rmi_remove().
> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
> index d9181fd66c5c8..8783edb7fd6a1 100644
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c
> @@ -31,6 +31,7 @@
> #define RMI_READ_REQUEST_PENDING 0
> #define RMI_READ_DATA_PENDING 1
> #define RMI_STARTED 2
> +#define RMI_SUSPENDED 3
>
> /* device flags */
> #define RMI_DEVICE BIT(0)
> @@ -428,6 +429,8 @@ static int rmi_event(struct hid_device *hdev, struct hid_field *field,
> struct rmi_data *data = hid_get_drvdata(hdev);
>
> if ((data->device_flags & RMI_DEVICE) &&
> + test_bit(RMI_STARTED, &data->flags) &&
> + !test_bit(RMI_SUSPENDED, &data->flags) &&
> (field->application == HID_GD_POINTER ||
> field->application == HID_GD_MOUSE)) {
> if (data->device_flags & RMI_DEVICE_HAS_PHYS_BUTTONS) {
> @@ -469,12 +472,16 @@ static int rmi_suspend(struct hid_device *hdev, pm_message_t message)
> struct rmi_device *rmi_dev = data->xport.rmi_dev;
> int ret;
>
> - if (!(data->device_flags & RMI_DEVICE))
> + if (!test_bit(RMI_STARTED, &data->flags))
> return 0;
>
> + set_bit(RMI_SUSPENDED, &data->flags);
> + cancel_work_sync(&data->reset_work);
> +
[Severity: Medium]
Can rmi_event() still queue data->reset_work after cancel_work_sync() returns
in rmi_suspend()?
Low-level HID transport drivers such as i2c-hid and usbhid invoke
hid_driver_suspend() before disabling hardware interrupts or stopping input
URBs, and without holding hdev->driver_input_lock:
CPU0 CPU1
rmi_event()
!test_bit(RMI_SUSPENDED, ...)
rmi_suspend()
set_bit(RMI_SUSPENDED, ...)
cancel_work_sync(&data->reset_work)
schedule_work(&data->reset_work)
rmi_driver_suspend()
kworker
rmi_reset_work()
rmi_reset_attn_mode() <-- runs while
suspended
Neither rmi_reset_work() nor rmi_reset_attn_mode() checks RMI_SUSPENDED, so
reset_work will issue bus transfers and reconfigure RMI functions concurrently
with rmi_driver_suspend() or while the device is suspended. Using
disable_work_sync() in rmi_suspend() paired with enable_work() on error and in
rmi_post_resume() would close this race.
> ret = rmi_driver_suspend(rmi_dev, false);
> if (ret) {
> hid_warn(hdev, "Failed to suspend device: %d\n", ret);
> + clear_bit(RMI_SUSPENDED, &data->flags);
> return ret;
> }
>
> @@ -486,27 +493,31 @@ static int rmi_post_resume(struct hid_device *hdev)
> struct rmi_data *data = hid_get_drvdata(hdev);
> struct rmi_device *rmi_dev = data->xport.rmi_dev;
> int ret;
> + int err;
>
> - if (!(data->device_flags & RMI_DEVICE))
> + if (!test_bit(RMI_STARTED, &data->flags))
> return 0;
>
> /* Make sure the HID device is ready to receive events */
> ret = hid_hw_open(hdev);
> if (ret)
> - return ret;
> + goto out;
>
> ret = rmi_reset_attn_mode(hdev);
> if (ret)
> - goto out;
> + hid_warn(hdev, "Failed to reset attention mode: %d\n", ret);
>
> - ret = rmi_driver_resume(rmi_dev, false);
> - if (ret) {
> - hid_warn(hdev, "Failed to resume device: %d\n", ret);
> - goto out;
> + err = rmi_driver_resume(rmi_dev, false);
> + if (err) {
> + hid_warn(hdev, "Failed to resume device: %d\n", err);
> + if (!ret)
> + ret = err;
> }
>
> -out:
> hid_hw_close(hdev);
> +
> +out:
> + clear_bit(RMI_SUSPENDED, &data->flags);
> return ret;
> }
[Severity: Medium]
Does jumping to out when hid_hw_open() fails leave data->rmi_irq disabled?
If hid_hw_open() returns an error (for example, -EINTR from
mutex_lock_killable(&hdev->ll_open_lock)), jumping to out clears RMI_SUSPENDED
without calling rmi_driver_resume(). Because rmi_suspend() disabled the
transport interrupt via rmi_driver_suspend() -> rmi_disable_irq(), the device
is left active after resume with data->rmi_irq permanently disabled, dropping
all subsequent attention reports.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-hid-rmi-v1-0-afc43d97d766@gmail.com?part=4
next prev parent reply other threads:[~2026-11-14 6:21 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-11-14 5:59 [PATCH 00/12] HID: rmi: Fix report handling, device lifecycle, and clean up code Dmitry Torokhov
2026-11-14 5:59 ` [PATCH 01/12] HID: rmi: return error on short output report writes Dmitry Torokhov
2026-11-14 5:59 ` [PATCH 02/12] HID: rmi: only strip trailing 0xff bytes from attention reports Dmitry Torokhov
2026-11-14 5:59 ` [PATCH 03/12] HID: rmi: validate read data report and protocol transfer bounds Dmitry Torokhov
2026-11-14 5:59 ` [PATCH 04/12] HID: rmi: fix device lifecycle, suspend, and reset work handling Dmitry Torokhov
2026-11-14 6:21 ` sashiko-bot [this message]
2026-11-14 5:59 ` [PATCH 05/12] HID: rmi: re-select page 0 across device resets Dmitry Torokhov
2026-11-14 5:59 ` [PATCH 06/12] HID: rmi: avoid GFP_ATOMIC allocation with hard IRQs disabled Dmitry Torokhov
2026-11-14 5:59 ` [PATCH 07/12] HID: rmi: do not mutate shared platform data template Dmitry Torokhov
2026-11-14 5:59 ` [PATCH 08/12] HID: rmi: use put_unaligned_le16() to populate output reports Dmitry Torokhov
2026-11-14 5:59 ` [PATCH 09/12] HID: rmi: use scope-based cleanup for mode feature buffer Dmitry Torokhov
2026-11-14 6:00 ` [PATCH 10/12] HID: rmi: switch to scoped mutex guards Dmitry Torokhov
2026-11-14 6:00 ` [PATCH 11/12] HID: rmi: remove dead code and reorder report write helper Dmitry Torokhov
2026-11-14 6:00 ` [PATCH 12/12] HID: rmi: clean up coding style and logging Dmitry Torokhov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=sashiko-outbox-214211@kernel.org \
--to=sashiko-bot@kernel.org \
--cc=dmitry.torokhov@gmail.com \
--cc=linux-input@vger.kernel.org \
--cc=sashiko-reviews@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.