From: "Derek J. Clark" <derekjohn.clark@gmail.com>
To: Andrei Aldea <andrei1998@gmail.com>,
Jiri Kosina <jikos@kernel.org>,
Benjamin Tissoires <bentiss@kernel.org>
Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
Lee Jones <lee@kernel.org>, Pavel Machek <pavel@kernel.org>,
linux-leds@vger.kernel.org
Subject: Re: [PATCH 10/15] HID: hid-oxp: handle controller reinitialization across suspend
Date: Thu, 10 Sep 2026 13:14:54 -0700 [thread overview]
Message-ID: <5063b29c-e686-472c-8b80-c6c68def0d0e@gmail.com> (raw)
In-Reply-To: <20260910032115.28669-11-andrei1998@gmail.com>
On 9/9/26 20:21, Andrei Aldea wrote:
> A Gen2 monocolor reply uses the same command value as the asynchronous MCU
> reset notification. Treating every such report as a reset can schedule a
> spurious controller reinitialization.
>
> Track an outstanding monocolor write by command and zone under a per-HID
> spinlock, and consume its matching acknowledgment before considering the
> report a reset notification. Clear pending reply state during suspend and
> teardown. A missing reply does not change legacy transport success
> semantics.
>
> For system suspend, disable and drain initialized configuration work and
> reject new output while the device is suspended. Re-enable work on resume
> and queue a fallback reinitialization after the documented MCU reset
> interval. A qualifying reset notification can still bring that work
> forward. Leave runtime autosuspend unchanged.
>
> Fixes: 2f424f28fb39 ("HID: hid-oxp: Add Second Generation Gamepad Mode Switch")
> Assisted-by: LLM
> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
> Signed-off-by: Andrei Aldea <andrei1998@gmail.com>
> ---
> drivers/hid/hid-oxp.c | 116 ++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 107 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
> index 7b36687..e26e6a9 100644
> --- a/drivers/hid/hid-oxp.c
> +++ b/drivers/hid/hid-oxp.c
> @@ -16,6 +16,7 @@
> #include <linux/kstrtox.h>
> #include <linux/led-class-multicolor.h>
> #include <linux/mutex.h>
> +#include <linux/spinlock.h>
> #include <linux/sysfs.h>
> #include <linux/types.h>
> #include <linux/workqueue.h>
> @@ -24,6 +25,7 @@
>
> #define OXP_PACKET_SIZE 64
> #define OXP_STATUS_HEADER_SIZE 6
> +#define OXP_STATUS_ACK 0x20
>
> #define GEN1_MESSAGE_ID 0xff
> #define GEN2_MESSAGE_ID 0x3f
> @@ -185,6 +187,10 @@ struct oxp_hid_cfg {
> struct hid_device *hdev;
> struct mutex cfg_mutex; /*ensure single synchronous output report*/
> struct mutex rgb_mutex; /*serialize complete RGB transactions*/
> + spinlock_t rgb_reply_lock;
> + u8 rgb_reply_command;
> + u8 rgb_reply_zone;
> + bool rgb_reply_pending;
> u8 rgb_brightness;
> u8 gamepad_mode;
> u8 rumble_intensity;
> @@ -193,6 +199,7 @@ struct oxp_hid_cfg {
> u8 rgb_en;
> bool rgb_work_initialized;
> bool gen2_work_initialized;
> + bool suspended;
> bool removing;
> };
>
> @@ -371,7 +378,7 @@ static void oxp_mcu_init_fn(struct work_struct *work)
> u8 gp_mode_data[3] = { OXP_GP_MODE_DEBUG, 0x01, 0x02 };
> int ret;
>
> - if (READ_ONCE(cfg->removing))
> + if (READ_ONCE(cfg->suspended) || READ_ONCE(cfg->removing))
> return;
>
> /* Re-apply the button mapping */
> @@ -410,6 +417,7 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
> struct oxp_hid_cfg *cfg = hid_get_drvdata(hdev);
> struct led_classdev_mc *led_mc = cfg->led_mc;
> struct oxp_gen_2_rgb_report *rgb_rep;
> + bool solicited = false;
>
> if (size < OXP_STATUS_HEADER_SIZE)
> return 0;
> @@ -417,11 +425,26 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
> if (data[0] != OXP_FID_GEN2_STATUS_EVENT)
> return 0;
>
> + /* A monocolor acknowledgment is not an MCU reset notification. */
> + if (data[5] == OXP_STATUS_ACK) {
> + scoped_guard(spinlock_irqsave, &cfg->rgb_reply_lock) {
> + if (cfg->rgb_reply_pending &&
> + data[3] == cfg->rgb_reply_command &&
> + data[4] == cfg->rgb_reply_zone) {
> + cfg->rgb_reply_pending = false;
> + solicited = true;
> + }
> + }
> + if (solicited)
> + return 0;
> + }
> +
> /* Sent ~6s after resume event, indicating the MCU has fully reset.
> * Re-apply our settings after this has been received.
> */
> if (data[3] == OXP_EFFECT_MONO_TRUE) {
> if (READ_ONCE(cfg->gen2_work_initialized) &&
> + !READ_ONCE(cfg->suspended) &&
> !READ_ONCE(cfg->removing))
> mod_delayed_work(system_dfl_wq, &cfg->oxp_mcu_init,
> msecs_to_jiffies(50));
> @@ -489,6 +512,7 @@ static int mcu_property_out(struct oxp_hid_cfg *cfg, u8 *header, size_t header_s
> size_t data_size, u8 *footer, size_t footer_size)
> {
> unsigned char *dmabuf __free(kfree) = kzalloc(OXP_PACKET_SIZE, GFP_KERNEL);
> + bool rgb_write;
> int ret;
>
> if (!dmabuf)
> @@ -500,6 +524,19 @@ static int mcu_property_out(struct oxp_hid_cfg *cfg, u8 *header, size_t header_s
> guard(mutex)(&cfg->cfg_mutex);
> if (READ_ONCE(cfg->removing))
> return -ENODEV;
> + if (READ_ONCE(cfg->suspended))
> + return -EHOSTDOWN;
> +
> + rgb_write = header_size && data_size > 1 &&
> + header[0] == OXP_FID_GEN2_STATUS_EVENT &&
> + data[0] == OXP_EFFECT_MONO_TRUE;
> + if (rgb_write) {
> + scoped_guard(spinlock_irqsave, &cfg->rgb_reply_lock) {
> + cfg->rgb_reply_command = data[0];
> + cfg->rgb_reply_zone = data[1];
> + cfg->rgb_reply_pending = true;
> + }
> + }
>
> memcpy(dmabuf, header, header_size);
> memcpy(dmabuf + header_size, data, data_size);
> @@ -509,12 +546,18 @@ static int mcu_property_out(struct oxp_hid_cfg *cfg, u8 *header, size_t header_s
> dev_dbg(&cfg->hdev->dev, "raw data: [%*ph]\n", OXP_PACKET_SIZE, dmabuf);
>
> ret = hid_hw_output_report(cfg->hdev, dmabuf, OXP_PACKET_SIZE);
> - if (ret < 0)
> - return ret;
> -
> /* MCU takes 200ms to be ready for another command. */
> msleep(200);
> - return ret == OXP_PACKET_SIZE ? 0 : -EIO;
> + if (ret >= 0)
> + ret = ret == OXP_PACKET_SIZE ? 0 : -EIO;
> +
> + if (rgb_write) {
> + scoped_guard(spinlock_irqsave, &cfg->rgb_reply_lock) {
> + cfg->rgb_reply_pending = false;
> + }
> + }
> +
> + return ret;
> }
>
> static int oxp_gen_1_property_out(struct oxp_hid_cfg *cfg, enum oxp_function_index fid, u8 *data,
> @@ -748,7 +791,7 @@ static void oxp_btn_queue_fn(struct work_struct *work)
> struct oxp_hid_cfg, oxp_btn_queue);
> int ret;
>
> - if (READ_ONCE(cfg->removing))
> + if (READ_ONCE(cfg->suspended) || READ_ONCE(cfg->removing))
> return;
>
> ret = oxp_set_buttons(cfg);
> @@ -837,7 +880,7 @@ static ssize_t map_button_store(struct device *dev,
> default:
> return -EINVAL;
> }
> - if (!READ_ONCE(cfg->removing))
> + if (!READ_ONCE(cfg->suspended) && !READ_ONCE(cfg->removing))
> mod_delayed_work(system_dfl_wq, &cfg->oxp_btn_queue,
> msecs_to_jiffies(50));
> return count;
> @@ -1413,7 +1456,7 @@ static void oxp_rgb_queue_fn(struct work_struct *work)
> u8 val = 4 * brightness / max_brightness;
> int ret;
>
> - if (READ_ONCE(cfg->removing))
> + if (READ_ONCE(cfg->suspended) || READ_ONCE(cfg->removing))
> return;
>
> guard(mutex)(&cfg->rgb_mutex);
> @@ -1442,7 +1485,7 @@ static void oxp_rgb_brightness_set(struct led_classdev *led_cdev,
> struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(led_cdev);
> struct oxp_hid_cfg *cfg = container_of(mc_cdev, struct oxp_hid_cfg, cdev);
>
> - if (READ_ONCE(cfg->removing))
> + if (READ_ONCE(cfg->suspended) || READ_ONCE(cfg->removing))
> return;
>
> led_cdev->brightness = brightness;
> @@ -1554,6 +1597,9 @@ static void oxp_drain_output(struct oxp_hid_cfg *cfg)
> static void oxp_quiesce_work(struct oxp_hid_cfg *cfg)
> {
> WRITE_ONCE(cfg->removing, true);
> + scoped_guard(spinlock_irqsave, &cfg->rgb_reply_lock) {
> + cfg->rgb_reply_pending = false;
> + }
> if (cfg->rgb_work_initialized)
> disable_delayed_work_sync(&cfg->oxp_rgb_queue);
> if (cfg->gen2_work_initialized) {
> @@ -1584,6 +1630,7 @@ static int oxp_cfg_probe(struct hid_device *hdev, u16 up)
> cfg->hdev = hdev;
> mutex_init(&cfg->cfg_mutex);
> mutex_init(&cfg->rgb_mutex);
> + spin_lock_init(&cfg->rgb_reply_lock);
>
> /* Clear drvdata after registered callback objects have been released. */
> hid_set_drvdata(hdev, cfg);
> @@ -1714,6 +1761,54 @@ static void oxp_hid_remove(struct hid_device *hdev)
> hid_hw_stop(hdev);
> }
>
> +static int __maybe_unused oxp_hid_suspend(struct hid_device *hdev,
> + pm_message_t message)
> +{
> + struct oxp_hid_cfg *cfg = hid_get_drvdata(hdev);
> +
> + if (!cfg || PMSG_IS_AUTO(message))
> + return 0;
> +
> + WRITE_ONCE(cfg->suspended, true);
> + scoped_guard(spinlock_irqsave, &cfg->rgb_reply_lock) {
> + cfg->rgb_reply_pending = false;
> + }
> + if (cfg->rgb_work_initialized)
> + disable_delayed_work_sync(&cfg->oxp_rgb_queue);
> + if (cfg->gen2_work_initialized) {
> + disable_delayed_work_sync(&cfg->oxp_btn_queue);
> + disable_delayed_work_sync(&cfg->oxp_mcu_init);
> + }
> + oxp_drain_output(cfg);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused oxp_hid_resume(struct hid_device *hdev)
> +{
> + struct oxp_hid_cfg *cfg = hid_get_drvdata(hdev);
> +
> + if (!cfg || !READ_ONCE(cfg->suspended) ||
> + READ_ONCE(cfg->removing))
> + return 0;
> +
> + if (cfg->rgb_work_initialized)
> + enable_delayed_work(&cfg->oxp_rgb_queue);
> + if (cfg->gen2_work_initialized) {
> + enable_delayed_work(&cfg->oxp_btn_queue);
> + enable_delayed_work(&cfg->oxp_mcu_init);
> + }
> + WRITE_ONCE(cfg->suspended, false);
> + if (!cfg->gen2_work_initialized)
> + return 0;
> +
> + /* Allow the controller MCU to finish rebooting before restoring state. */
> + queue_delayed_work(system_dfl_wq, &cfg->oxp_mcu_init,
> + msecs_to_jiffies(6500));
> +
> + return 0;
> +}
> +
> static const struct hid_device_id oxp_devices[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_CRSC, USB_DEVICE_ID_ONEXPLAYER_GEN1) },
> { HID_USB_DEVICE(USB_VENDOR_ID_WCH, USB_DEVICE_ID_ONEXPLAYER_GEN2) },
> @@ -1727,6 +1822,9 @@ static struct hid_driver hid_oxp = {
> .probe = oxp_hid_probe,
> .remove = oxp_hid_remove,
> .raw_event = oxp_hid_raw_event,
> + .suspend = pm_ptr(oxp_hid_suspend),
> + .resume = pm_ptr(oxp_hid_resume),
> + .reset_resume = pm_ptr(oxp_hid_resume),
> };
> module_hid_driver(hid_oxp);
>
Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
next prev parent reply other threads:[~2026-09-10 20:14 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-10 3:21 [PATCH 00/15] HID: hid-oxp: fix and extend X2-family controller support Andrei Aldea
2026-09-10 3:21 ` [PATCH 01/15] HID: hid-oxp: fix default M1 and M2 key mappings Andrei Aldea
2026-09-10 20:03 ` Derek J. Clark
2026-09-10 3:21 ` [PATCH 02/15] HID: hid-oxp: validate input report lengths before decoding Andrei Aldea
2026-09-10 20:04 ` Derek J. Clark
2026-09-10 3:21 ` [PATCH 03/15] HID: hid-oxp: retain fractional brightness when reading RGB status Andrei Aldea
2026-09-10 20:05 ` Derek J. Clark
2026-09-10 3:21 ` [PATCH 04/15] HID: hid-oxp: reject invalid Gen2 RGB status values Andrei Aldea
2026-09-10 20:06 ` Derek J. Clark
2026-09-10 3:21 ` [PATCH 05/15] HID: hid-oxp: fix multicolor LED intensity scaling Andrei Aldea
2026-09-10 20:07 ` Derek J. Clark
2026-09-10 3:21 ` [PATCH 06/15] HID: hid-oxp: serialize complete RGB updates Andrei Aldea
2026-09-10 20:07 ` Derek J. Clark
2026-09-10 3:21 ` [PATCH 07/15] HID: hid-oxp: select brightness policy for the new RGB effect Andrei Aldea
2026-09-10 20:11 ` Derek J. Clark
2026-09-10 3:21 ` [PATCH 08/15] HID: hid-oxp: stop configuration work during teardown Andrei Aldea
2026-09-10 20:12 ` Derek J. Clark
2026-09-10 3:21 ` [PATCH 09/15] HID: hid-oxp: keep configuration state per HID interface Andrei Aldea
2026-09-10 20:13 ` Derek J. Clark
2026-09-10 3:21 ` [PATCH 10/15] HID: hid-oxp: handle controller reinitialization across suspend Andrei Aldea
2026-09-10 20:14 ` Derek J. Clark [this message]
2026-09-10 3:21 ` [PATCH 11/15] HID: hid-oxp: group declarations and protocol definitions Andrei Aldea
2026-09-10 20:15 ` Derek J. Clark
2026-09-10 3:21 ` [PATCH 12/15] HID: hid-oxp: support three-page button maps on X2 controllers Andrei Aldea
2026-09-10 20:16 ` Derek J. Clark
2026-09-10 3:21 ` [PATCH 13/15] HID: hid-oxp: represent RGB LEDs with a common array Andrei Aldea
2026-09-10 20:17 ` Derek J. Clark
2026-09-10 3:21 ` [PATCH 14/15] HID: hid-oxp: add Gen3 joystick ring RGB support Andrei Aldea
2026-09-10 20:19 ` Derek J. Clark
2026-09-10 3:21 ` [PATCH 15/15] HID: hid-oxp: add X2 auxiliary RGB zones Andrei Aldea
2026-09-10 20:20 ` Derek J. Clark
2026-09-10 20:24 ` [PATCH 00/15] HID: hid-oxp: fix and extend X2-family controller support Derek J. Clark
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=5063b29c-e686-472c-8b80-c6c68def0d0e@gmail.com \
--to=derekjohn.clark@gmail.com \
--cc=andrei1998@gmail.com \
--cc=bentiss@kernel.org \
--cc=jikos@kernel.org \
--cc=lee@kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=pavel@kernel.org \
/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
all inboxes | Powered by JetHome®