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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox