* [PATCH 01/15] HID: hid-oxp: fix default M1 and M2 key mappings
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 ` Andrei Aldea
2026-09-10 3:21 ` [PATCH 02/15] HID: hid-oxp: validate input report lengths before decoding Andrei Aldea
` (13 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Andrei Aldea @ 2026-09-10 3:21 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Derek J. Clark; +Cc: linux-input, linux-kernel
The default button map intends to assign F15 and F16 to M1 and M2, but
the selected mapping table indexes resolve to F16 and F17. Use indexes
47 and 48 so the programmed usages match the existing comments.
Assisted-by: LLM
Fixes: e4c850a6e750 ("HID: hid-oxp: Add Button Mapping Interface")
Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Signed-off-by: Andrei Aldea <andrei1998@gmail.com>
---
drivers/hid/hid-oxp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index d2ded6b..3f2be80 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -607,9 +607,9 @@ static void oxp_set_defaults_bmap_2(struct oxp_bmap_page_2 *bmap)
bmap->btn_dright.button_idx = BUTTON_DRIGHT;
bmap->btn_dright.mapping_idx = 15;
bmap->btn_m1.button_idx = BUTTON_M1;
- bmap->btn_m1.mapping_idx = 48; /* KEY_F15 */
+ bmap->btn_m1.mapping_idx = 47; /* KEY_F15 */
bmap->btn_m2.button_idx = BUTTON_M2;
- bmap->btn_m2.mapping_idx = 49; /* KEY_F16 */
+ bmap->btn_m2.mapping_idx = 48; /* KEY_F16 */
}
static void oxp_page_fill_data(char *buf, const struct oxp_button_idx *buttons,
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 02/15] HID: hid-oxp: validate input report lengths before decoding
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 3:21 ` Andrei Aldea
2026-09-10 3:34 ` sashiko-bot
2026-09-10 3:21 ` [PATCH 03/15] HID: hid-oxp: retain fractional brightness when reading RGB status Andrei Aldea
` (12 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Andrei Aldea @ 2026-09-10 3:21 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Derek J. Clark
Cc: linux-input, linux-kernel, Lee Jones, Pavel Machek, linux-leds
Check the short Gen2 status header before reading its command, and require
a complete RGB status report before reading either generation's fields.
Use the supplied input length for the debug dump as well, so a short
report cannot cause an out-of-bounds read while logging.
Fixes: 84910c459d65 ("HID: hid-oxp: Add OneXPlayer configuration driver")
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 | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index 3f2be80..0c895bd 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -23,6 +23,7 @@
#include "hid-ids.h"
#define OXP_PACKET_SIZE 64
+#define OXP_STATUS_HEADER_SIZE 6
#define GEN1_MESSAGE_ID 0xff
#define GEN2_MESSAGE_ID 0x3f
@@ -318,6 +319,9 @@ static int oxp_hid_raw_event_gen_1(struct hid_device *hdev,
struct led_classdev_mc *led_mc = drvdata.led_mc;
struct oxp_gen_1_rgb_report *rgb_rep;
+ if (size < sizeof(*rgb_rep))
+ return 0;
+
if (data[1] != OXP_FID_GEN1_RGB_REPLY)
return 0;
@@ -391,6 +395,9 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
struct led_classdev_mc *led_mc = drvdata.led_mc;
struct oxp_gen_2_rgb_report *rgb_rep;
+ if (size < OXP_STATUS_HEADER_SIZE)
+ return 0;
+
if (data[0] != OXP_FID_GEN2_STATUS_EVENT)
return 0;
@@ -404,6 +411,8 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
if (data[3] != OXP_GET_PROPERTY)
return 0;
+ if (size < sizeof(*rgb_rep))
+ return 0;
rgb_rep = (struct oxp_gen_2_rgb_report *)data;
/* Ensure we save monocolor as the list value */
@@ -435,7 +444,7 @@ static int oxp_hid_raw_event(struct hid_device *hdev, struct hid_report *report,
{
u16 up = get_usage_page(hdev);
- dev_dbg(&hdev->dev, "raw event data: [%*ph]\n", OXP_PACKET_SIZE, data);
+ dev_dbg(&hdev->dev, "raw event data: [%*ph]\n", size, data);
switch (up) {
case GEN1_USAGE_PAGE:
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 02/15] HID: hid-oxp: validate input report lengths before decoding
2026-09-10 3:21 ` [PATCH 02/15] HID: hid-oxp: validate input report lengths before decoding Andrei Aldea
@ 2026-09-10 3:34 ` sashiko-bot
0 siblings, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-09-10 3:34 UTC (permalink / raw)
To: Andrei Aldea; +Cc: lee, dmitry.torokhov, linux-input, linux-leds
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Shared driver state variables are concurrently modified from atomic context and read from process context without protection, enabling a TOCTOU out-of-bounds read.
--
commit c83e5978640f43b909fcea30940f12727c190fcd
Author: Andrei Aldea <andrei1998@gmail.com>
Subject: HID: hid-oxp: validate input report lengths before decoding
This commit adds length checks to ensure short USB reports do not result in
out-of-bounds reads when decoding RGB status and printing debug information.
> diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
> index 3f2be8059ddb1..0c895bd778bbf 100644
> --- a/drivers/hid/hid-oxp.c
> +++ b/drivers/hid/hid-oxp.c
[ ... ]
> @@ -404,6 +411,8 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
>
> if (data[3] != OXP_GET_PROPERTY)
> return 0;
> + if (size < sizeof(*rgb_rep))
> + return 0;
>
> rgb_rep = (struct oxp_gen_2_rgb_report *)data;
> /* Ensure we save monocolor as the list value */
[Severity: High]
This isn't a bug introduced by this patch, but can the lockless assignment of
drvdata.rgb_effect here cause a Time-Of-Check to Time-Of-Use (TOCTOU)
out-of-bounds read in effect_show()?
In drivers/hid/hid-oxp.c, oxp_hid_raw_event_gen_2() updates the effect
locklessly from the raw event handler:
drvdata.rgb_effect = rgb_rep->effect == OXP_EFFECT_MONO_TRUE ?
OXP_EFFECT_MONO_LIST :
rgb_rep->effect;
Concurrently, a userspace process reading the effect sysfs attribute calls
effect_show(), which performs two separate lockless reads of
drvdata.rgb_effect:
static ssize_t effect_show(...)
{
...
if (drvdata.rgb_effect >= ARRAY_SIZE(oxp_rgb_effect_text))
return -EINVAL;
return sysfs_emit(buf, "%s\n", oxp_rgb_effect_text[drvdata.rgb_effect]);
}
If a malicious device sends continuous status event reports, can the value of
drvdata.rgb_effect change to an out-of-bounds value immediately after the
bounds check but before the array access? This would cause sysfs_emit() to
dereference a garbage pointer.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260910032115.28669-1-andrei1998@gmail.com?part=2
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 03/15] HID: hid-oxp: retain fractional brightness when reading RGB status
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 3:21 ` [PATCH 02/15] HID: hid-oxp: validate input report lengths before decoding Andrei Aldea
@ 2026-09-10 3:21 ` Andrei Aldea
2026-09-10 3:29 ` sashiko-bot
2026-09-10 3:21 ` [PATCH 04/15] HID: hid-oxp: reject invalid Gen2 RGB status values Andrei Aldea
` (11 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Andrei Aldea @ 2026-09-10 3:21 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Derek J. Clark
Cc: linux-input, linux-kernel, Lee Jones, Pavel Machek, linux-leds
Multiply the hardware brightness level by the LED brightness range before
dividing by four. Dividing first reported every intermediate hardware
level as zero instead of 25, 50 or 75 percent.
Fixes: 84910c459d65 ("HID: hid-oxp: Add OneXPlayer configuration driver")
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 | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index 0c895bd..86a559c 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -334,8 +334,8 @@ static int oxp_hid_raw_event_gen_1(struct hid_device *hdev,
drvdata.rgb_en = rgb_rep->enabled == 0 ? OXP_FEAT_DISABLED :
OXP_FEAT_ENABLED;
drvdata.rgb_brightness = rgb_rep->brightness;
- led_mc->led_cdev.brightness = rgb_rep->brightness / 4 *
- led_mc->led_cdev.max_brightness;
+ led_mc->led_cdev.brightness = rgb_rep->brightness *
+ led_mc->led_cdev.max_brightness / 4;
/* If monocolor had less than 100% brightness on the previous boot,
* there will be no reliable way to determine the real intensity.
* Since intensity scaling is used with a hardware brightness set at max,
@@ -423,8 +423,8 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
drvdata.rgb_en = rgb_rep->enabled == 0 ? OXP_FEAT_DISABLED :
OXP_FEAT_ENABLED;
drvdata.rgb_brightness = rgb_rep->brightness;
- led_mc->led_cdev.brightness = rgb_rep->brightness / 4 *
- led_mc->led_cdev.max_brightness;
+ led_mc->led_cdev.brightness = rgb_rep->brightness *
+ led_mc->led_cdev.max_brightness / 4;
/* If monocolor had less than 100% brightness on the previous boot,
* there will be no reliable way to determine the real intensity.
* Since intensity scaling is used with a hardware brightness set at max,
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 04/15] HID: hid-oxp: reject invalid Gen2 RGB status values
2026-09-10 3:21 [PATCH 00/15] HID: hid-oxp: fix and extend X2-family controller support Andrei Aldea
` (2 preceding siblings ...)
2026-09-10 3:21 ` [PATCH 03/15] HID: hid-oxp: retain fractional brightness when reading RGB status Andrei Aldea
@ 2026-09-10 3:21 ` Andrei Aldea
2026-09-10 3:32 ` sashiko-bot
2026-09-10 3:21 ` [PATCH 05/15] HID: hid-oxp: fix multicolor LED intensity scaling Andrei Aldea
` (10 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Andrei Aldea @ 2026-09-10 3:21 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Derek J. Clark
Cc: linux-input, linux-kernel, Lee Jones, Pavel Machek, linux-leds
Do not replace the cached RGB state with a Gen2 status report whose enable,
speed or hardware brightness fields are outside the supported ranges.
Fixes: 252c4bf1d931 ("HID: hid-oxp: Add Second Generation RGB Control")
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 | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index 86a559c..c3cfa95 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -415,6 +415,10 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
return 0;
rgb_rep = (struct oxp_gen_2_rgb_report *)data;
+ if (rgb_rep->enabled > OXP_FEAT_ENABLED || rgb_rep->speed > 9 ||
+ rgb_rep->brightness > 4)
+ return 0;
+
/* Ensure we save monocolor as the list value */
drvdata.rgb_effect = rgb_rep->effect == OXP_EFFECT_MONO_TRUE ?
OXP_EFFECT_MONO_LIST :
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 05/15] HID: hid-oxp: fix multicolor LED intensity scaling
2026-09-10 3:21 [PATCH 00/15] HID: hid-oxp: fix and extend X2-family controller support Andrei Aldea
` (3 preceding siblings ...)
2026-09-10 3:21 ` [PATCH 04/15] HID: hid-oxp: reject invalid Gen2 RGB status values Andrei Aldea
@ 2026-09-10 3:21 ` Andrei Aldea
2026-09-10 3:33 ` sashiko-bot
2026-09-10 3:21 ` [PATCH 06/15] HID: hid-oxp: serialize complete RGB updates Andrei Aldea
` (9 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Andrei Aldea @ 2026-09-10 3:21 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Derek J. Clark
Cc: linux-input, linux-kernel, Lee Jones, Pavel Machek, linux-leds
The controller accepts eight-bit RGB components, but the sub-LED channel
maximum currently inherits the 0-100 overall brightness range. This
prevents userspace from selecting channel intensities above 100.
Set each channel maximum to 255 and use the multicolor LED core helper to
scale components with the requested brightness. This also replaces the
private truncating calculation with the LED core's rounded calculation.
Keep the existing Gen1 and Gen2 color packet layouts unchanged.
Fixes: 84910c459d65 ("HID: hid-oxp: Add OneXPlayer configuration driver")
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 | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index c3cfa95..bee1317 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -1088,7 +1088,6 @@ static ssize_t oxp_rgb_status_show(void)
static int oxp_rgb_color_set(void)
{
- u8 max_br = drvdata.led_mc->led_cdev.max_brightness;
u8 br = drvdata.led_mc->led_cdev.brightness;
u16 up = get_usage_page(drvdata.hdev);
u8 green, red, blue;
@@ -1096,9 +1095,10 @@ static int oxp_rgb_color_set(void)
u8 *data;
int i;
- red = br * drvdata.led_mc->subled_info[0].intensity / max_br;
- green = br * drvdata.led_mc->subled_info[1].intensity / max_br;
- blue = br * drvdata.led_mc->subled_info[2].intensity / max_br;
+ led_mc_calc_color_components(drvdata.led_mc, br);
+ red = drvdata.led_mc->subled_info[0].brightness;
+ green = drvdata.led_mc->subled_info[1].brightness;
+ blue = drvdata.led_mc->subled_info[2].brightness;
switch (up) {
case GEN1_USAGE_PAGE:
@@ -1383,16 +1383,19 @@ static struct mc_subled oxp_rgb_subled_info[] = {
{
.color_index = LED_COLOR_ID_RED,
.intensity = 0x24,
+ .max_intensity = 0xff,
.channel = 0x1,
},
{
.color_index = LED_COLOR_ID_GREEN,
.intensity = 0x22,
+ .max_intensity = 0xff,
.channel = 0x2,
},
{
.color_index = LED_COLOR_ID_BLUE,
.intensity = 0x99,
+ .max_intensity = 0xff,
.channel = 0x3,
},
};
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 06/15] HID: hid-oxp: serialize complete RGB updates
2026-09-10 3:21 [PATCH 00/15] HID: hid-oxp: fix and extend X2-family controller support Andrei Aldea
` (4 preceding siblings ...)
2026-09-10 3:21 ` [PATCH 05/15] HID: hid-oxp: fix multicolor LED intensity scaling Andrei Aldea
@ 2026-09-10 3:21 ` Andrei Aldea
2026-09-10 3:32 ` sashiko-bot
2026-09-10 3:21 ` [PATCH 07/15] HID: hid-oxp: select brightness policy for the new RGB effect Andrei Aldea
` (8 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Andrei Aldea @ 2026-09-10 3:21 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Derek J. Clark
Cc: linux-input, linux-kernel, Lee Jones, Pavel Machek, linux-leds
The transport mutex protects one report at a time, but changing an RGB
effect requires a status command followed by a color or effect command.
Serialize whole RGB transactions so sysfs and delayed brightness work do
not interleave these command sequences.
Fixes: 84910c459d65 ("HID: hid-oxp: Add OneXPlayer configuration driver")
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 | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index bee1317..8d07816 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -181,6 +181,7 @@ static struct oxp_hid_cfg {
struct led_classdev_mc *led_mc;
struct hid_device *hdev;
struct mutex cfg_mutex; /*ensure single synchronous output report*/
+ struct mutex rgb_mutex; /*serialize complete RGB transactions*/
u8 rgb_brightness;
u8 gamepad_mode;
u8 rumble_intensity;
@@ -1074,6 +1075,8 @@ static ssize_t oxp_rgb_status_show(void)
u16 up = get_usage_page(drvdata.hdev);
u8 *data;
+ guard(mutex)(&drvdata.rgb_mutex);
+
switch (up) {
case GEN1_USAGE_PAGE:
data = (u8[1]) { OXP_GET_PROPERTY };
@@ -1191,6 +1194,8 @@ static ssize_t enabled_store(struct device *dev, struct device_attribute *attr,
return ret;
val = ret;
+ guard(mutex)(&drvdata.rgb_mutex);
+
ret = oxp_rgb_status_store(val, drvdata.rgb_speed,
drvdata.rgb_brightness);
if (ret)
@@ -1244,6 +1249,8 @@ static ssize_t effect_store(struct device *dev, struct device_attribute *attr,
val = ret;
+ guard(mutex)(&drvdata.rgb_mutex);
+
ret = oxp_rgb_status_store(drvdata.rgb_en, drvdata.rgb_speed,
drvdata.rgb_brightness);
if (ret)
@@ -1302,6 +1309,8 @@ static ssize_t speed_store(struct device *dev, struct device_attribute *attr,
if (val > 9)
return -EINVAL;
+ guard(mutex)(&drvdata.rgb_mutex);
+
ret = oxp_rgb_status_store(drvdata.rgb_en, val, drvdata.rgb_brightness);
if (ret)
return ret;
@@ -1340,6 +1349,8 @@ static void oxp_rgb_queue_fn(struct work_struct *work)
u8 val = 4 * brightness / max_brightness;
int ret;
+ guard(mutex)(&drvdata.rgb_mutex);
+
if (drvdata.rgb_brightness != val) {
ret = oxp_rgb_status_store(drvdata.rgb_en, drvdata.rgb_speed, val);
if (ret)
@@ -1470,6 +1481,7 @@ static int oxp_cfg_probe(struct hid_device *hdev, u16 up)
hid_set_drvdata(hdev, &drvdata);
mutex_init(&drvdata.cfg_mutex);
+ mutex_init(&drvdata.rgb_mutex);
drvdata.hdev = hdev;
if (up == GEN2_USAGE_PAGE && oxp_hybrid_mcu_device())
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 07/15] HID: hid-oxp: select brightness policy for the new RGB effect
2026-09-10 3:21 [PATCH 00/15] HID: hid-oxp: fix and extend X2-family controller support Andrei Aldea
` (5 preceding siblings ...)
2026-09-10 3:21 ` [PATCH 06/15] HID: hid-oxp: serialize complete RGB updates Andrei Aldea
@ 2026-09-10 3:21 ` Andrei Aldea
2026-09-10 3:33 ` sashiko-bot
2026-09-10 3:21 ` [PATCH 08/15] HID: hid-oxp: stop configuration work during teardown Andrei Aldea
` (7 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Andrei Aldea @ 2026-09-10 3:21 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Derek J. Clark
Cc: linux-input, linux-kernel, Lee Jones, Pavel Machek, linux-leds
Set the requested effect before constructing its status command, since
monocolor uses maximum hardware brightness and software intensity scaling.
Previously entering or leaving monocolor used the old effect's brightness
policy. Restore the cached effect if either output command fails.
Fixes: 84910c459d65 ("HID: hid-oxp: Add OneXPlayer configuration driver")
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 | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index 8d07816..9c22c8e 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -1240,6 +1240,7 @@ static DEVICE_ATTR_RO(enabled_index);
static ssize_t effect_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
+ u8 old_effect;
int ret;
u8 val;
@@ -1250,15 +1251,21 @@ static ssize_t effect_store(struct device *dev, struct device_attribute *attr,
val = ret;
guard(mutex)(&drvdata.rgb_mutex);
+ old_effect = drvdata.rgb_effect;
+ drvdata.rgb_effect = val;
ret = oxp_rgb_status_store(drvdata.rgb_en, drvdata.rgb_speed,
drvdata.rgb_brightness);
- if (ret)
+ if (ret) {
+ drvdata.rgb_effect = old_effect;
return ret;
+ }
ret = oxp_rgb_effect_set(val);
- if (ret)
+ if (ret) {
+ drvdata.rgb_effect = old_effect;
return ret;
+ }
return count;
}
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 07/15] HID: hid-oxp: select brightness policy for the new RGB effect
2026-09-10 3:21 ` [PATCH 07/15] HID: hid-oxp: select brightness policy for the new RGB effect Andrei Aldea
@ 2026-09-10 3:33 ` sashiko-bot
0 siblings, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-09-10 3:33 UTC (permalink / raw)
To: Andrei Aldea; +Cc: linux-leds, dmitry.torokhov, lee, linux-input
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Incomplete hardware state rollback in effect_store() leaves the hardware brightness out of sync with software.
--
commit 75feb6ea7d7e2f2f49294cf7e04996cd656c16bc
Author: Andrei Aldea <andrei1998@gmail.com>
HID: hid-oxp: select brightness policy for the new RGB effect
This commit updates the effect_store() function to set the requested effect
before constructing its status command to handle monocolor software intensity
scaling. It also adds logic to restore the cached software effect if either
output command fails.
> diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
> index 8d07816132166..9c22c8e0703a7 100644
> --- a/drivers/hid/hid-oxp.c
> +++ b/drivers/hid/hid-oxp.c
> @@ -1250,15 +1251,21 @@ static ssize_t effect_store(struct device *dev, struct device_attribute *attr,
> val = ret;
>
> guard(mutex)(&drvdata.rgb_mutex);
> + old_effect = drvdata.rgb_effect;
> + drvdata.rgb_effect = val;
>
> ret = oxp_rgb_status_store(drvdata.rgb_en, drvdata.rgb_speed,
> drvdata.rgb_brightness);
> - if (ret)
> + if (ret) {
> + drvdata.rgb_effect = old_effect;
> return ret;
> + }
>
> ret = oxp_rgb_effect_set(val);
> - if (ret)
> + if (ret) {
> + drvdata.rgb_effect = old_effect;
[Severity: Medium]
Does this error path leave the hardware desynchronized from the software state?
If oxp_rgb_effect_set() fails, the software state (drvdata.rgb_effect) is
rolled back to old_effect, but the hardware has already successfully processed
the oxp_rgb_status_store() command using the new effect's policy.
Should this error path also send a corrective command via
oxp_rgb_status_store() to restore the original hardware status?
> return ret;
> + }
>
> return count;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260910032115.28669-1-andrei1998@gmail.com?part=7
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 08/15] HID: hid-oxp: stop configuration work during teardown
2026-09-10 3:21 [PATCH 00/15] HID: hid-oxp: fix and extend X2-family controller support Andrei Aldea
` (6 preceding siblings ...)
2026-09-10 3:21 ` [PATCH 07/15] HID: hid-oxp: select brightness policy for the new RGB effect Andrei Aldea
@ 2026-09-10 3:21 ` Andrei Aldea
2026-09-10 3:32 ` sashiko-bot
2026-09-10 3:21 ` [PATCH 09/15] HID: hid-oxp: keep configuration state per HID interface Andrei Aldea
` (6 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Andrei Aldea @ 2026-09-10 3:21 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Derek J. Clark
Cc: linux-input, linux-kernel, Lee Jones, Pavel Machek, linux-leds
Configuration callbacks can queue delayed RGB, button-mapping, and MCU
initialization work. Normal removal previously closed the HID transport
without synchronously stopping that work, while a later configuration
probe failure could leave work queued as devres released its objects.
Track which work items have been initialized, reject new output once
teardown begins, disable initialized work synchronously, and drain an
in-flight transport write before closing the device. Use the same
shutdown path for normal removal and every probe-error unwind after work
can have been exposed.
This also avoids operating on uninitialized work for HID interfaces that
do not own configuration state.
Fixes: 84910c459d65 ("HID: hid-oxp: Add OneXPlayer configuration driver")
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 | 94 +++++++++++++++++++++++++++++++++++--------
1 file changed, 77 insertions(+), 17 deletions(-)
diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index 9c22c8e..eef57c3 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -188,6 +188,9 @@ static struct oxp_hid_cfg {
u8 rgb_effect;
u8 rgb_speed;
u8 rgb_en;
+ bool rgb_work_initialized;
+ bool gen2_work_initialized;
+ bool removing;
} drvdata;
#define OXP_FILL_PAGE_SLOT(page, btn) \
@@ -320,7 +323,7 @@ static int oxp_hid_raw_event_gen_1(struct hid_device *hdev,
struct led_classdev_mc *led_mc = drvdata.led_mc;
struct oxp_gen_1_rgb_report *rgb_rep;
- if (size < sizeof(*rgb_rep))
+ if (size < sizeof(*rgb_rep) || !led_mc)
return 0;
if (data[1] != OXP_FID_GEN1_RGB_REPLY)
@@ -360,6 +363,9 @@ 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(drvdata.removing))
+ return;
+
/* Re-apply the button mapping */
ret = oxp_set_buttons();
if (ret)
@@ -406,13 +412,16 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
* Re-apply our settings after this has been received.
*/
if (data[3] == OXP_EFFECT_MONO_TRUE) {
- mod_delayed_work(system_dfl_wq, &drvdata.oxp_mcu_init, msecs_to_jiffies(50));
+ if (READ_ONCE(drvdata.gen2_work_initialized) &&
+ !READ_ONCE(drvdata.removing))
+ mod_delayed_work(system_dfl_wq, &drvdata.oxp_mcu_init,
+ msecs_to_jiffies(50));
return 0;
}
if (data[3] != OXP_GET_PROPERTY)
return 0;
- if (size < sizeof(*rgb_rep))
+ if (size < sizeof(*rgb_rep) || !led_mc)
return 0;
rgb_rep = (struct oxp_gen_2_rgb_report *)data;
@@ -449,6 +458,9 @@ static int oxp_hid_raw_event(struct hid_device *hdev, struct hid_report *report,
{
u16 up = get_usage_page(hdev);
+ if (!hid_get_drvdata(hdev) || READ_ONCE(drvdata.removing))
+ return 0;
+
dev_dbg(&hdev->dev, "raw event data: [%*ph]\n", size, data);
switch (up) {
@@ -476,6 +488,9 @@ static int mcu_property_out(u8 *header, size_t header_size, u8 *data,
return -EINVAL;
guard(mutex)(&drvdata.cfg_mutex);
+ if (READ_ONCE(drvdata.removing))
+ return -ENODEV;
+
memcpy(dmabuf, header, header_size);
memcpy(dmabuf + header_size, data, data_size);
if (footer_size)
@@ -717,6 +732,9 @@ static void oxp_btn_queue_fn(struct work_struct *work)
{
int ret;
+ if (READ_ONCE(drvdata.removing))
+ return;
+
ret = oxp_set_buttons();
if (ret)
dev_err(&drvdata.hdev->dev,
@@ -802,7 +820,9 @@ static ssize_t map_button_store(struct device *dev,
default:
return -EINVAL;
}
- mod_delayed_work(system_dfl_wq, &drvdata.oxp_btn_queue, msecs_to_jiffies(50));
+ if (!READ_ONCE(drvdata.removing))
+ mod_delayed_work(system_dfl_wq, &drvdata.oxp_btn_queue,
+ msecs_to_jiffies(50));
return count;
}
@@ -1356,6 +1376,9 @@ static void oxp_rgb_queue_fn(struct work_struct *work)
u8 val = 4 * brightness / max_brightness;
int ret;
+ if (READ_ONCE(drvdata.removing))
+ return;
+
guard(mutex)(&drvdata.rgb_mutex);
if (drvdata.rgb_brightness != val) {
@@ -1379,6 +1402,9 @@ static void oxp_rgb_queue_fn(struct work_struct *work)
static void oxp_rgb_brightness_set(struct led_classdev *led_cdev,
enum led_brightness brightness)
{
+ if (READ_ONCE(drvdata.removing))
+ return;
+
led_cdev->brightness = brightness;
mod_delayed_work(system_dfl_wq, &drvdata.oxp_rgb_queue, msecs_to_jiffies(50));
}
@@ -1480,6 +1506,24 @@ static bool oxp_hybrid_mcu_device(void)
return quirks->hybrid_mcu;
}
+static void oxp_drain_output(void)
+{
+ /* Wait for any in-flight sysfs output before closing the transport. */
+ guard(mutex)(&drvdata.cfg_mutex);
+}
+
+static void oxp_quiesce_work(void)
+{
+ WRITE_ONCE(drvdata.removing, true);
+ if (drvdata.rgb_work_initialized)
+ disable_delayed_work_sync(&drvdata.oxp_rgb_queue);
+ if (drvdata.gen2_work_initialized) {
+ disable_delayed_work_sync(&drvdata.oxp_btn_queue);
+ disable_delayed_work_sync(&drvdata.oxp_mcu_init);
+ }
+ oxp_drain_output();
+}
+
static int oxp_cfg_probe(struct hid_device *hdev, u16 up)
{
struct oxp_bmap_page_1 *bmap_1;
@@ -1490,6 +1534,7 @@ static int oxp_cfg_probe(struct hid_device *hdev, u16 up)
mutex_init(&drvdata.cfg_mutex);
mutex_init(&drvdata.rgb_mutex);
drvdata.hdev = hdev;
+ drvdata.removing = false;
if (up == GEN2_USAGE_PAGE && oxp_hybrid_mcu_device())
goto skip_rgb;
@@ -1497,16 +1542,21 @@ static int oxp_cfg_probe(struct hid_device *hdev, u16 up)
drvdata.led_mc = &oxp_cdev_rgb;
INIT_DELAYED_WORK(&drvdata.oxp_rgb_queue, oxp_rgb_queue_fn);
+ drvdata.rgb_work_initialized = true;
ret = devm_led_classdev_multicolor_register(&hdev->dev, &oxp_cdev_rgb);
- if (ret)
- return dev_err_probe(&hdev->dev, ret,
+ if (ret) {
+ dev_err_probe(&hdev->dev, ret,
"Failed to create RGB device\n");
+ goto err_quiesce;
+ }
ret = devm_device_add_group(drvdata.led_mc->led_cdev.dev,
&oxp_rgb_attr_group);
- if (ret)
- return dev_err_probe(drvdata.led_mc->led_cdev.dev, ret,
+ if (ret) {
+ dev_err_probe(drvdata.led_mc->led_cdev.dev, ret,
"Failed to create RGB configuration attributes\n");
+ goto err_quiesce;
+ }
ret = oxp_rgb_status_show();
if (ret)
@@ -1519,14 +1569,18 @@ static int oxp_cfg_probe(struct hid_device *hdev, u16 up)
skip_rgb:
bmap_1 = devm_kzalloc(&hdev->dev, sizeof(struct oxp_bmap_page_1), GFP_KERNEL);
- if (!bmap_1)
- return dev_err_probe(&hdev->dev, -ENOMEM,
+ if (!bmap_1) {
+ ret = dev_err_probe(&hdev->dev, -ENOMEM,
"Unable to allocate button map page 1\n");
+ goto err_quiesce;
+ }
bmap_2 = devm_kzalloc(&hdev->dev, sizeof(struct oxp_bmap_page_2), GFP_KERNEL);
- if (!bmap_2)
- return dev_err_probe(&hdev->dev, -ENOMEM,
+ if (!bmap_2) {
+ ret = dev_err_probe(&hdev->dev, -ENOMEM,
"Unable to allocate button map page 2\n");
+ goto err_quiesce;
+ }
drvdata.bmap_1 = bmap_1;
drvdata.bmap_2 = bmap_2;
@@ -1537,14 +1591,21 @@ skip_rgb:
drvdata.rumble_intensity = 5;
INIT_DELAYED_WORK(&drvdata.oxp_mcu_init, oxp_mcu_init_fn);
+ WRITE_ONCE(drvdata.gen2_work_initialized, true);
mod_delayed_work(system_dfl_wq, &drvdata.oxp_mcu_init, msecs_to_jiffies(50));
ret = devm_device_add_group(&hdev->dev, &oxp_cfg_attrs_group);
- if (ret)
- return dev_err_probe(&hdev->dev, ret,
+ if (ret) {
+ dev_err_probe(&hdev->dev, ret,
"Failed to attach configuration attributes\n");
+ goto err_quiesce;
+ }
return 0;
+
+err_quiesce:
+ oxp_quiesce_work();
+ return ret;
}
static int oxp_hid_probe(struct hid_device *hdev,
@@ -1587,9 +1648,8 @@ static int oxp_hid_probe(struct hid_device *hdev,
static void oxp_hid_remove(struct hid_device *hdev)
{
- cancel_delayed_work(&drvdata.oxp_rgb_queue);
- cancel_delayed_work(&drvdata.oxp_btn_queue);
- cancel_delayed_work(&drvdata.oxp_mcu_init);
+ if (hid_get_drvdata(hdev))
+ oxp_quiesce_work();
hid_hw_close(hdev);
hid_hw_stop(hdev);
}
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 08/15] HID: hid-oxp: stop configuration work during teardown
2026-09-10 3:21 ` [PATCH 08/15] HID: hid-oxp: stop configuration work during teardown Andrei Aldea
@ 2026-09-10 3:32 ` sashiko-bot
0 siblings, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-09-10 3:32 UTC (permalink / raw)
To: Andrei Aldea; +Cc: dmitry.torokhov, linux-leds, lee, linux-input
> Configuration callbacks can queue delayed RGB, button-mapping, and MCU
> initialization work. Normal removal previously closed the HID transport
> without synchronously stopping that work, while a later configuration
> probe failure could leave work queued as devres released its objects.
>
> Track which work items have been initialized, reject new output once
> teardown begins, disable initialized work synchronously, and drain an
> in-flight transport write before closing the device. Use the same
> shutdown path for normal removal and every probe-error unwind after work
> can have been exposed.
>
> This also avoids operating on uninitialized work for HID interfaces that
> do not own configuration state.
>
> Fixes: 84910c459d65 ("HID: hid-oxp: Add OneXPlayer configuration driver")
> Assisted-by: LLM
> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
> Signed-off-by: Andrei Aldea <andrei1998@gmail.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260910032115.28669-1-andrei1998@gmail.com?part=8
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 09/15] HID: hid-oxp: keep configuration state per HID interface
2026-09-10 3:21 [PATCH 00/15] HID: hid-oxp: fix and extend X2-family controller support Andrei Aldea
` (7 preceding siblings ...)
2026-09-10 3:21 ` [PATCH 08/15] HID: hid-oxp: stop configuration work during teardown Andrei Aldea
@ 2026-09-10 3:21 ` Andrei Aldea
2026-09-10 3:35 ` sashiko-bot
2026-09-10 3:21 ` [PATCH 10/15] HID: hid-oxp: handle controller reinitialization across suspend Andrei Aldea
` (5 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Andrei Aldea @ 2026-09-10 3:21 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Derek J. Clark
Cc: linux-input, linux-kernel, Lee Jones, Pavel Machek, linux-leds
Replace the shared driver state and mutable LED object with devm-managed
state owned by each configuration HID. Resolve callbacks through their
HID, LED or embedded work object instead of the last interface probed.
Hybrid devices have distinct Gen1 RGB and Gen2 controller interfaces.
Sharing the transport pointer and work state lets one overwrite the other.
Keep the HID drvdata pointer valid until LED and sysfs objects have been
released, then clear it before freeing the configuration allocation.
Fixes: 252c4bf1d931 ("HID: hid-oxp: Add Second Generation RGB Control")
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 | 498 +++++++++++++++++++++++-------------------
1 file changed, 279 insertions(+), 219 deletions(-)
diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index eef57c3..7b36687 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -172,7 +172,10 @@ struct oxp_bmap_page_2 {
struct oxp_button_idx btn_m2;
} __packed;
-static struct oxp_hid_cfg {
+/* Hybrid devices expose RGB and controller configuration on separate HIDs. */
+struct oxp_hid_cfg {
+ struct led_classdev_mc cdev;
+ struct mc_subled subled_info[3];
struct delayed_work oxp_rgb_queue;
struct delayed_work oxp_btn_queue;
struct oxp_bmap_page_1 *bmap_1;
@@ -191,7 +194,7 @@ static struct oxp_hid_cfg {
bool rgb_work_initialized;
bool gen2_work_initialized;
bool removing;
-} drvdata;
+};
#define OXP_FILL_PAGE_SLOT(page, btn) \
{ .button_idx = (page)->btn.button_idx, \
@@ -320,7 +323,8 @@ static int oxp_hid_raw_event_gen_1(struct hid_device *hdev,
struct hid_report *report, u8 *data,
int size)
{
- struct led_classdev_mc *led_mc = drvdata.led_mc;
+ struct oxp_hid_cfg *cfg = hid_get_drvdata(hdev);
+ struct led_classdev_mc *led_mc = cfg->led_mc;
struct oxp_gen_1_rgb_report *rgb_rep;
if (size < sizeof(*rgb_rep) || !led_mc)
@@ -331,13 +335,13 @@ static int oxp_hid_raw_event_gen_1(struct hid_device *hdev,
rgb_rep = (struct oxp_gen_1_rgb_report *)data;
/* Ensure we save monocolor as the list value */
- drvdata.rgb_effect = rgb_rep->effect == OXP_EFFECT_MONO_TRUE ?
+ cfg->rgb_effect = rgb_rep->effect == OXP_EFFECT_MONO_TRUE ?
OXP_EFFECT_MONO_LIST :
rgb_rep->effect;
- drvdata.rgb_speed = rgb_rep->speed;
- drvdata.rgb_en = rgb_rep->enabled == 0 ? OXP_FEAT_DISABLED :
+ cfg->rgb_speed = rgb_rep->speed;
+ cfg->rgb_en = rgb_rep->enabled == 0 ? OXP_FEAT_DISABLED :
OXP_FEAT_ENABLED;
- drvdata.rgb_brightness = rgb_rep->brightness;
+ cfg->rgb_brightness = rgb_rep->brightness;
led_mc->led_cdev.brightness = rgb_rep->brightness *
led_mc->led_cdev.max_brightness / 4;
/* If monocolor had less than 100% brightness on the previous boot,
@@ -354,44 +358,48 @@ static int oxp_hid_raw_event_gen_1(struct hid_device *hdev,
return 0;
}
-static int oxp_gen_2_property_out(enum oxp_function_index fid, u8 *data, u8 data_size);
-static int oxp_set_buttons(void);
-static int oxp_rumble_intensity_set(u8 intensity);
+static int oxp_gen_2_property_out(struct oxp_hid_cfg *cfg,
+ enum oxp_function_index fid, u8 *data,
+ u8 data_size);
+static int oxp_set_buttons(struct oxp_hid_cfg *cfg);
+static int oxp_rumble_intensity_set(struct oxp_hid_cfg *cfg, u8 intensity);
static void oxp_mcu_init_fn(struct work_struct *work)
{
+ struct oxp_hid_cfg *cfg = container_of(to_delayed_work(work),
+ struct oxp_hid_cfg, oxp_mcu_init);
u8 gp_mode_data[3] = { OXP_GP_MODE_DEBUG, 0x01, 0x02 };
int ret;
- if (READ_ONCE(drvdata.removing))
+ if (READ_ONCE(cfg->removing))
return;
/* Re-apply the button mapping */
- ret = oxp_set_buttons();
+ ret = oxp_set_buttons(cfg);
if (ret)
- dev_err(&drvdata.hdev->dev,
+ dev_err(&cfg->hdev->dev,
"Error: Failed to set button mapping: %i\n", ret);
/* Cycle the gamepad mode */
- ret = oxp_gen_2_property_out(OXP_FID_GEN2_TOGGLE_MODE, gp_mode_data, 3);
+ ret = oxp_gen_2_property_out(cfg, OXP_FID_GEN2_TOGGLE_MODE, gp_mode_data, 3);
if (ret)
- dev_err(&drvdata.hdev->dev,
+ dev_err(&cfg->hdev->dev,
"Error: Failed to set gamepad mode: %i\n", ret);
/* Remainder only applies for xinput mode */
- if (drvdata.gamepad_mode == OXP_GP_MODE_DEBUG)
+ if (cfg->gamepad_mode == OXP_GP_MODE_DEBUG)
return;
gp_mode_data[0] = OXP_GP_MODE_XINPUT;
- ret = oxp_gen_2_property_out(OXP_FID_GEN2_TOGGLE_MODE, gp_mode_data, 3);
+ ret = oxp_gen_2_property_out(cfg, OXP_FID_GEN2_TOGGLE_MODE, gp_mode_data, 3);
if (ret)
- dev_err(&drvdata.hdev->dev,
+ dev_err(&cfg->hdev->dev,
"Error: Failed to set gamepad mode: %i\n", ret);
/* Set vibration level */
- ret = oxp_rumble_intensity_set(drvdata.rumble_intensity);
+ ret = oxp_rumble_intensity_set(cfg, cfg->rumble_intensity);
if (ret)
- dev_err(&drvdata.hdev->dev,
+ dev_err(&cfg->hdev->dev,
"Error: Failed to set rumble intensity: %i\n", ret);
}
@@ -399,7 +407,8 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
struct hid_report *report, u8 *data,
int size)
{
- struct led_classdev_mc *led_mc = drvdata.led_mc;
+ 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;
if (size < OXP_STATUS_HEADER_SIZE)
@@ -412,9 +421,9 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
* Re-apply our settings after this has been received.
*/
if (data[3] == OXP_EFFECT_MONO_TRUE) {
- if (READ_ONCE(drvdata.gen2_work_initialized) &&
- !READ_ONCE(drvdata.removing))
- mod_delayed_work(system_dfl_wq, &drvdata.oxp_mcu_init,
+ if (READ_ONCE(cfg->gen2_work_initialized) &&
+ !READ_ONCE(cfg->removing))
+ mod_delayed_work(system_dfl_wq, &cfg->oxp_mcu_init,
msecs_to_jiffies(50));
return 0;
}
@@ -430,13 +439,13 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
return 0;
/* Ensure we save monocolor as the list value */
- drvdata.rgb_effect = rgb_rep->effect == OXP_EFFECT_MONO_TRUE ?
+ cfg->rgb_effect = rgb_rep->effect == OXP_EFFECT_MONO_TRUE ?
OXP_EFFECT_MONO_LIST :
rgb_rep->effect;
- drvdata.rgb_speed = rgb_rep->speed;
- drvdata.rgb_en = rgb_rep->enabled == 0 ? OXP_FEAT_DISABLED :
+ cfg->rgb_speed = rgb_rep->speed;
+ cfg->rgb_en = rgb_rep->enabled == 0 ? OXP_FEAT_DISABLED :
OXP_FEAT_ENABLED;
- drvdata.rgb_brightness = rgb_rep->brightness;
+ cfg->rgb_brightness = rgb_rep->brightness;
led_mc->led_cdev.brightness = rgb_rep->brightness *
led_mc->led_cdev.max_brightness / 4;
/* If monocolor had less than 100% brightness on the previous boot,
@@ -456,9 +465,10 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
static int oxp_hid_raw_event(struct hid_device *hdev, struct hid_report *report,
u8 *data, int size)
{
+ struct oxp_hid_cfg *cfg = hid_get_drvdata(hdev);
u16 up = get_usage_page(hdev);
- if (!hid_get_drvdata(hdev) || READ_ONCE(drvdata.removing))
+ if (!cfg || READ_ONCE(cfg->removing))
return 0;
dev_dbg(&hdev->dev, "raw event data: [%*ph]\n", size, data);
@@ -475,7 +485,7 @@ static int oxp_hid_raw_event(struct hid_device *hdev, struct hid_report *report,
return 0;
}
-static int mcu_property_out(u8 *header, size_t header_size, u8 *data,
+static int mcu_property_out(struct oxp_hid_cfg *cfg, u8 *header, size_t header_size, u8 *data,
size_t data_size, u8 *footer, size_t footer_size)
{
unsigned char *dmabuf __free(kfree) = kzalloc(OXP_PACKET_SIZE, GFP_KERNEL);
@@ -487,8 +497,8 @@ static int mcu_property_out(u8 *header, size_t header_size, u8 *data,
if (header_size + data_size + footer_size > OXP_PACKET_SIZE)
return -EINVAL;
- guard(mutex)(&drvdata.cfg_mutex);
- if (READ_ONCE(drvdata.removing))
+ guard(mutex)(&cfg->cfg_mutex);
+ if (READ_ONCE(cfg->removing))
return -ENODEV;
memcpy(dmabuf, header, header_size);
@@ -496,9 +506,9 @@ static int mcu_property_out(u8 *header, size_t header_size, u8 *data,
if (footer_size)
memcpy(dmabuf + OXP_PACKET_SIZE - footer_size, footer, footer_size);
- dev_dbg(&drvdata.hdev->dev, "raw data: [%*ph]\n", OXP_PACKET_SIZE, dmabuf);
+ dev_dbg(&cfg->hdev->dev, "raw data: [%*ph]\n", OXP_PACKET_SIZE, dmabuf);
- ret = hid_hw_output_report(drvdata.hdev, dmabuf, OXP_PACKET_SIZE);
+ ret = hid_hw_output_report(cfg->hdev, dmabuf, OXP_PACKET_SIZE);
if (ret < 0)
return ret;
@@ -507,16 +517,16 @@ static int mcu_property_out(u8 *header, size_t header_size, u8 *data,
return ret == OXP_PACKET_SIZE ? 0 : -EIO;
}
-static int oxp_gen_1_property_out(enum oxp_function_index fid, u8 *data,
+static int oxp_gen_1_property_out(struct oxp_hid_cfg *cfg, enum oxp_function_index fid, u8 *data,
u8 data_size)
{
u8 header[] = { fid, GEN1_MESSAGE_ID };
size_t header_size = ARRAY_SIZE(header);
- return mcu_property_out(header, header_size, data, data_size, NULL, 0);
+ return mcu_property_out(cfg, header, header_size, data, data_size, NULL, 0);
}
-static int oxp_gen_2_property_out(enum oxp_function_index fid, u8 *data,
+static int oxp_gen_2_property_out(struct oxp_hid_cfg *cfg, enum oxp_function_index fid, u8 *data,
u8 data_size)
{
u8 header[] = { fid, GEN2_MESSAGE_ID, 0x01 };
@@ -524,7 +534,7 @@ static int oxp_gen_2_property_out(enum oxp_function_index fid, u8 *data,
size_t header_size = ARRAY_SIZE(header);
size_t footer_size = ARRAY_SIZE(footer);
- return mcu_property_out(header, header_size, data, data_size, footer,
+ return mcu_property_out(cfg, header, header_size, data, data_size, footer,
footer_size);
}
@@ -532,7 +542,8 @@ static ssize_t gamepad_mode_store(struct device *dev,
struct device_attribute *attr, const char *buf,
size_t count)
{
- u16 up = get_usage_page(drvdata.hdev);
+ struct oxp_hid_cfg *cfg = dev_get_drvdata(dev);
+ u16 up = get_usage_page(cfg->hdev);
u8 data[3] = { 0x00, 0x01, 0x02 };
int ret = -EINVAL;
int i;
@@ -551,17 +562,17 @@ static ssize_t gamepad_mode_store(struct device *dev,
data[0] = ret;
- ret = oxp_gen_2_property_out(OXP_FID_GEN2_TOGGLE_MODE, data, 3);
+ ret = oxp_gen_2_property_out(cfg, OXP_FID_GEN2_TOGGLE_MODE, data, 3);
if (ret)
return ret;
- drvdata.gamepad_mode = data[0];
+ cfg->gamepad_mode = data[0];
- if (drvdata.gamepad_mode == OXP_GP_MODE_DEBUG)
+ if (cfg->gamepad_mode == OXP_GP_MODE_DEBUG)
return count;
/* Re-apply rumble settings as switching gamepad mode will override */
- ret = oxp_rumble_intensity_set(drvdata.rumble_intensity);
+ ret = oxp_rumble_intensity_set(cfg, cfg->rumble_intensity);
if (ret)
return ret;
@@ -571,7 +582,9 @@ static ssize_t gamepad_mode_store(struct device *dev,
static ssize_t gamepad_mode_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- return sysfs_emit(buf, "%s\n", oxp_gamepad_mode_text[drvdata.gamepad_mode]);
+ struct oxp_hid_cfg *cfg = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%s\n", oxp_gamepad_mode_text[cfg->gamepad_mode]);
}
static DEVICE_ATTR_RW(gamepad_mode);
@@ -656,60 +669,61 @@ static void oxp_page_fill_data(char *buf, const struct oxp_button_idx *buttons,
}
}
-static int oxp_set_buttons(void)
+static int oxp_set_buttons(struct oxp_hid_cfg *cfg)
{
u8 page_1[59] = { 0x02, 0x38, 0x20, 0x01, 0x01 };
u8 page_2[59] = { 0x02, 0x38, 0x20, 0x02, 0x01 };
- u16 up = get_usage_page(drvdata.hdev);
+ u16 up = get_usage_page(cfg->hdev);
int ret;
if (up != GEN2_USAGE_PAGE)
return -EINVAL;
const struct oxp_button_idx p1[] = {
- OXP_FILL_PAGE_SLOT(drvdata.bmap_1, btn_a),
- OXP_FILL_PAGE_SLOT(drvdata.bmap_1, btn_b),
- OXP_FILL_PAGE_SLOT(drvdata.bmap_1, btn_x),
- OXP_FILL_PAGE_SLOT(drvdata.bmap_1, btn_y),
- OXP_FILL_PAGE_SLOT(drvdata.bmap_1, btn_lb),
- OXP_FILL_PAGE_SLOT(drvdata.bmap_1, btn_rb),
- OXP_FILL_PAGE_SLOT(drvdata.bmap_1, btn_lt),
- OXP_FILL_PAGE_SLOT(drvdata.bmap_1, btn_rt),
- OXP_FILL_PAGE_SLOT(drvdata.bmap_1, btn_start),
+ OXP_FILL_PAGE_SLOT(cfg->bmap_1, btn_a),
+ OXP_FILL_PAGE_SLOT(cfg->bmap_1, btn_b),
+ OXP_FILL_PAGE_SLOT(cfg->bmap_1, btn_x),
+ OXP_FILL_PAGE_SLOT(cfg->bmap_1, btn_y),
+ OXP_FILL_PAGE_SLOT(cfg->bmap_1, btn_lb),
+ OXP_FILL_PAGE_SLOT(cfg->bmap_1, btn_rb),
+ OXP_FILL_PAGE_SLOT(cfg->bmap_1, btn_lt),
+ OXP_FILL_PAGE_SLOT(cfg->bmap_1, btn_rt),
+ OXP_FILL_PAGE_SLOT(cfg->bmap_1, btn_start),
};
const struct oxp_button_idx p2[] = {
- OXP_FILL_PAGE_SLOT(drvdata.bmap_2, btn_select),
- OXP_FILL_PAGE_SLOT(drvdata.bmap_2, btn_l3),
- OXP_FILL_PAGE_SLOT(drvdata.bmap_2, btn_r3),
- OXP_FILL_PAGE_SLOT(drvdata.bmap_2, btn_dup),
- OXP_FILL_PAGE_SLOT(drvdata.bmap_2, btn_ddown),
- OXP_FILL_PAGE_SLOT(drvdata.bmap_2, btn_dleft),
- OXP_FILL_PAGE_SLOT(drvdata.bmap_2, btn_dright),
- OXP_FILL_PAGE_SLOT(drvdata.bmap_2, btn_m1),
- OXP_FILL_PAGE_SLOT(drvdata.bmap_2, btn_m2),
+ OXP_FILL_PAGE_SLOT(cfg->bmap_2, btn_select),
+ OXP_FILL_PAGE_SLOT(cfg->bmap_2, btn_l3),
+ OXP_FILL_PAGE_SLOT(cfg->bmap_2, btn_r3),
+ OXP_FILL_PAGE_SLOT(cfg->bmap_2, btn_dup),
+ OXP_FILL_PAGE_SLOT(cfg->bmap_2, btn_ddown),
+ OXP_FILL_PAGE_SLOT(cfg->bmap_2, btn_dleft),
+ OXP_FILL_PAGE_SLOT(cfg->bmap_2, btn_dright),
+ OXP_FILL_PAGE_SLOT(cfg->bmap_2, btn_m1),
+ OXP_FILL_PAGE_SLOT(cfg->bmap_2, btn_m2),
};
oxp_page_fill_data(page_1, p1, ARRAY_SIZE(p1));
oxp_page_fill_data(page_2, p2, ARRAY_SIZE(p2));
- ret = oxp_gen_2_property_out(OXP_FID_GEN2_KEY_STATE, page_1, ARRAY_SIZE(page_1));
+ ret = oxp_gen_2_property_out(cfg, OXP_FID_GEN2_KEY_STATE, page_1, ARRAY_SIZE(page_1));
if (ret)
return ret;
- return oxp_gen_2_property_out(OXP_FID_GEN2_KEY_STATE, page_2, ARRAY_SIZE(page_2));
+ return oxp_gen_2_property_out(cfg, OXP_FID_GEN2_KEY_STATE, page_2, ARRAY_SIZE(page_2));
}
-static void oxp_reset_buttons(void)
+static void oxp_reset_buttons(struct oxp_hid_cfg *cfg)
{
- oxp_set_defaults_bmap_1(drvdata.bmap_1);
- oxp_set_defaults_bmap_2(drvdata.bmap_2);
+ oxp_set_defaults_bmap_1(cfg->bmap_1);
+ oxp_set_defaults_bmap_2(cfg->bmap_2);
}
static ssize_t reset_buttons_store(struct device *dev,
struct device_attribute *attr, const char *buf,
size_t count)
{
+ struct oxp_hid_cfg *cfg = dev_get_drvdata(dev);
int val, ret;
ret = kstrtoint(buf, 10, &val);
@@ -719,8 +733,8 @@ static ssize_t reset_buttons_store(struct device *dev,
if (val != 1)
return -EINVAL;
- oxp_reset_buttons();
- ret = oxp_set_buttons();
+ oxp_reset_buttons(cfg);
+ ret = oxp_set_buttons(cfg);
if (ret)
return ret;
@@ -730,14 +744,16 @@ static DEVICE_ATTR_WO(reset_buttons);
static void oxp_btn_queue_fn(struct work_struct *work)
{
+ struct oxp_hid_cfg *cfg = container_of(to_delayed_work(work),
+ struct oxp_hid_cfg, oxp_btn_queue);
int ret;
- if (READ_ONCE(drvdata.removing))
+ if (READ_ONCE(cfg->removing))
return;
- ret = oxp_set_buttons();
+ ret = oxp_set_buttons(cfg);
if (ret)
- dev_err(&drvdata.hdev->dev,
+ dev_err(&cfg->hdev->dev,
"Error: Failed to write button mapping: %i\n", ret);
}
@@ -756,6 +772,7 @@ static ssize_t map_button_store(struct device *dev,
struct device_attribute *attr, const char *buf,
size_t count, u8 index)
{
+ struct oxp_hid_cfg *cfg = dev_get_drvdata(dev);
int idx;
idx = oxp_button_idx_from_str(buf);
@@ -764,64 +781,64 @@ static ssize_t map_button_store(struct device *dev,
switch (index) {
case BUTTON_A:
- drvdata.bmap_1->btn_a.mapping_idx = idx;
+ cfg->bmap_1->btn_a.mapping_idx = idx;
break;
case BUTTON_B:
- drvdata.bmap_1->btn_b.mapping_idx = idx;
+ cfg->bmap_1->btn_b.mapping_idx = idx;
break;
case BUTTON_X:
- drvdata.bmap_1->btn_x.mapping_idx = idx;
+ cfg->bmap_1->btn_x.mapping_idx = idx;
break;
case BUTTON_Y:
- drvdata.bmap_1->btn_y.mapping_idx = idx;
+ cfg->bmap_1->btn_y.mapping_idx = idx;
break;
case BUTTON_LB:
- drvdata.bmap_1->btn_lb.mapping_idx = idx;
+ cfg->bmap_1->btn_lb.mapping_idx = idx;
break;
case BUTTON_RB:
- drvdata.bmap_1->btn_rb.mapping_idx = idx;
+ cfg->bmap_1->btn_rb.mapping_idx = idx;
break;
case BUTTON_LT:
- drvdata.bmap_1->btn_lt.mapping_idx = idx;
+ cfg->bmap_1->btn_lt.mapping_idx = idx;
break;
case BUTTON_RT:
- drvdata.bmap_1->btn_rt.mapping_idx = idx;
+ cfg->bmap_1->btn_rt.mapping_idx = idx;
break;
case BUTTON_START:
- drvdata.bmap_1->btn_start.mapping_idx = idx;
+ cfg->bmap_1->btn_start.mapping_idx = idx;
break;
case BUTTON_SELECT:
- drvdata.bmap_2->btn_select.mapping_idx = idx;
+ cfg->bmap_2->btn_select.mapping_idx = idx;
break;
case BUTTON_L3:
- drvdata.bmap_2->btn_l3.mapping_idx = idx;
+ cfg->bmap_2->btn_l3.mapping_idx = idx;
break;
case BUTTON_R3:
- drvdata.bmap_2->btn_r3.mapping_idx = idx;
+ cfg->bmap_2->btn_r3.mapping_idx = idx;
break;
case BUTTON_DUP:
- drvdata.bmap_2->btn_dup.mapping_idx = idx;
+ cfg->bmap_2->btn_dup.mapping_idx = idx;
break;
case BUTTON_DDOWN:
- drvdata.bmap_2->btn_ddown.mapping_idx = idx;
+ cfg->bmap_2->btn_ddown.mapping_idx = idx;
break;
case BUTTON_DLEFT:
- drvdata.bmap_2->btn_dleft.mapping_idx = idx;
+ cfg->bmap_2->btn_dleft.mapping_idx = idx;
break;
case BUTTON_DRIGHT:
- drvdata.bmap_2->btn_dright.mapping_idx = idx;
+ cfg->bmap_2->btn_dright.mapping_idx = idx;
break;
case BUTTON_M1:
- drvdata.bmap_2->btn_m1.mapping_idx = idx;
+ cfg->bmap_2->btn_m1.mapping_idx = idx;
break;
case BUTTON_M2:
- drvdata.bmap_2->btn_m2.mapping_idx = idx;
+ cfg->bmap_2->btn_m2.mapping_idx = idx;
break;
default:
return -EINVAL;
}
- if (!READ_ONCE(drvdata.removing))
- mod_delayed_work(system_dfl_wq, &drvdata.oxp_btn_queue,
+ if (!READ_ONCE(cfg->removing))
+ mod_delayed_work(system_dfl_wq, &cfg->oxp_btn_queue,
msecs_to_jiffies(50));
return count;
}
@@ -830,62 +847,63 @@ static ssize_t map_button_show(struct device *dev,
struct device_attribute *attr, char *buf,
u8 index)
{
+ struct oxp_hid_cfg *cfg = dev_get_drvdata(dev);
u8 i;
switch (index) {
case BUTTON_A:
- i = drvdata.bmap_1->btn_a.mapping_idx;
+ i = cfg->bmap_1->btn_a.mapping_idx;
break;
case BUTTON_B:
- i = drvdata.bmap_1->btn_b.mapping_idx;
+ i = cfg->bmap_1->btn_b.mapping_idx;
break;
case BUTTON_X:
- i = drvdata.bmap_1->btn_x.mapping_idx;
+ i = cfg->bmap_1->btn_x.mapping_idx;
break;
case BUTTON_Y:
- i = drvdata.bmap_1->btn_y.mapping_idx;
+ i = cfg->bmap_1->btn_y.mapping_idx;
break;
case BUTTON_LB:
- i = drvdata.bmap_1->btn_lb.mapping_idx;
+ i = cfg->bmap_1->btn_lb.mapping_idx;
break;
case BUTTON_RB:
- i = drvdata.bmap_1->btn_rb.mapping_idx;
+ i = cfg->bmap_1->btn_rb.mapping_idx;
break;
case BUTTON_LT:
- i = drvdata.bmap_1->btn_lt.mapping_idx;
+ i = cfg->bmap_1->btn_lt.mapping_idx;
break;
case BUTTON_RT:
- i = drvdata.bmap_1->btn_rt.mapping_idx;
+ i = cfg->bmap_1->btn_rt.mapping_idx;
break;
case BUTTON_START:
- i = drvdata.bmap_1->btn_start.mapping_idx;
+ i = cfg->bmap_1->btn_start.mapping_idx;
break;
case BUTTON_SELECT:
- i = drvdata.bmap_2->btn_select.mapping_idx;
+ i = cfg->bmap_2->btn_select.mapping_idx;
break;
case BUTTON_L3:
- i = drvdata.bmap_2->btn_l3.mapping_idx;
+ i = cfg->bmap_2->btn_l3.mapping_idx;
break;
case BUTTON_R3:
- i = drvdata.bmap_2->btn_r3.mapping_idx;
+ i = cfg->bmap_2->btn_r3.mapping_idx;
break;
case BUTTON_DUP:
- i = drvdata.bmap_2->btn_dup.mapping_idx;
+ i = cfg->bmap_2->btn_dup.mapping_idx;
break;
case BUTTON_DDOWN:
- i = drvdata.bmap_2->btn_ddown.mapping_idx;
+ i = cfg->bmap_2->btn_ddown.mapping_idx;
break;
case BUTTON_DLEFT:
- i = drvdata.bmap_2->btn_dleft.mapping_idx;
+ i = cfg->bmap_2->btn_dleft.mapping_idx;
break;
case BUTTON_DRIGHT:
- i = drvdata.bmap_2->btn_dright.mapping_idx;
+ i = cfg->bmap_2->btn_dright.mapping_idx;
break;
case BUTTON_M1:
- i = drvdata.bmap_2->btn_m1.mapping_idx;
+ i = cfg->bmap_2->btn_m1.mapping_idx;
break;
case BUTTON_M2:
- i = drvdata.bmap_2->btn_m2.mapping_idx;
+ i = cfg->bmap_2->btn_m2.mapping_idx;
break;
default:
return -EINVAL;
@@ -913,7 +931,7 @@ static ssize_t button_mapping_options_show(struct device *dev,
}
static DEVICE_ATTR_RO(button_mapping_options);
-static int oxp_rumble_intensity_set(u8 intensity)
+static int oxp_rumble_intensity_set(struct oxp_hid_cfg *cfg, u8 intensity)
{
u8 header[15] = { 0x02, 0x38, 0x02, 0xe3, 0x39, 0xe3, 0x39, 0xe3,
0x39, 0x01, intensity, 0x05, 0xe3, 0x39, 0xe3 };
@@ -926,13 +944,14 @@ static int oxp_rumble_intensity_set(u8 intensity)
memcpy(data, header, header_size);
memcpy(data + data_size - footer_size, footer, footer_size);
- return oxp_gen_2_property_out(OXP_FID_GEN2_RUMBLE_SET, data, data_size);
+ return oxp_gen_2_property_out(cfg, OXP_FID_GEN2_RUMBLE_SET, data, data_size);
}
static ssize_t rumble_intensity_store(struct device *dev,
struct device_attribute *attr, const char *buf,
size_t count)
{
+ struct oxp_hid_cfg *cfg = dev_get_drvdata(dev);
int ret;
u8 val;
@@ -943,11 +962,11 @@ static ssize_t rumble_intensity_store(struct device *dev,
if (val < 0 || val > 5)
return -EINVAL;
- ret = oxp_rumble_intensity_set(val);
+ ret = oxp_rumble_intensity_set(cfg, val);
if (ret)
return ret;
- drvdata.rumble_intensity = val;
+ cfg->rumble_intensity = val;
return count;
}
@@ -955,7 +974,9 @@ static ssize_t rumble_intensity_store(struct device *dev,
static ssize_t rumble_intensity_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- return sysfs_emit(buf, "%i\n", drvdata.rumble_intensity);
+ struct oxp_hid_cfg *cfg = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%i\n", cfg->rumble_intensity);
}
static DEVICE_ATTR_RW(rumble_intensity);
@@ -1066,9 +1087,9 @@ static const struct attribute_group oxp_cfg_attrs_group = {
.attrs = oxp_cfg_attrs,
};
-static int oxp_rgb_status_store(u8 enabled, u8 speed, u8 brightness)
+static int oxp_rgb_status_store(struct oxp_hid_cfg *cfg, u8 enabled, u8 speed, u8 brightness)
{
- u16 up = get_usage_page(drvdata.hdev);
+ u16 up = get_usage_page(cfg->hdev);
u8 *data;
/* Always default to max brightness and use intensity scaling when in
@@ -1077,51 +1098,51 @@ static int oxp_rgb_status_store(u8 enabled, u8 speed, u8 brightness)
switch (up) {
case GEN1_USAGE_PAGE:
data = (u8[4]) { OXP_SET_PROPERTY, enabled, speed, brightness };
- if (drvdata.rgb_effect == OXP_EFFECT_MONO_LIST)
+ if (cfg->rgb_effect == OXP_EFFECT_MONO_LIST)
data[3] = 0x04;
- return oxp_gen_1_property_out(OXP_FID_GEN1_RGB_SET, data, 4);
+ return oxp_gen_1_property_out(cfg, OXP_FID_GEN1_RGB_SET, data, 4);
case GEN2_USAGE_PAGE:
data = (u8[6]) { OXP_SET_PROPERTY, 0x00, 0x02, enabled, speed, brightness };
- if (drvdata.rgb_effect == OXP_EFFECT_MONO_LIST)
+ if (cfg->rgb_effect == OXP_EFFECT_MONO_LIST)
data[5] = 0x04;
- return oxp_gen_2_property_out(OXP_FID_GEN2_STATUS_EVENT, data, 6);
+ return oxp_gen_2_property_out(cfg, OXP_FID_GEN2_STATUS_EVENT, data, 6);
default:
return -ENODEV;
}
}
-static ssize_t oxp_rgb_status_show(void)
+static ssize_t oxp_rgb_status_show(struct oxp_hid_cfg *cfg)
{
- u16 up = get_usage_page(drvdata.hdev);
+ u16 up = get_usage_page(cfg->hdev);
u8 *data;
- guard(mutex)(&drvdata.rgb_mutex);
+ guard(mutex)(&cfg->rgb_mutex);
switch (up) {
case GEN1_USAGE_PAGE:
data = (u8[1]) { OXP_GET_PROPERTY };
- return oxp_gen_1_property_out(OXP_FID_GEN1_RGB_SET, data, 1);
+ return oxp_gen_1_property_out(cfg, OXP_FID_GEN1_RGB_SET, data, 1);
case GEN2_USAGE_PAGE:
data = (u8[3]) { OXP_GET_PROPERTY, 0x00, 0x02 };
- return oxp_gen_2_property_out(OXP_FID_GEN2_STATUS_EVENT, data, 3);
+ return oxp_gen_2_property_out(cfg, OXP_FID_GEN2_STATUS_EVENT, data, 3);
default:
return -ENODEV;
}
}
-static int oxp_rgb_color_set(void)
+static int oxp_rgb_color_set(struct oxp_hid_cfg *cfg)
{
- u8 br = drvdata.led_mc->led_cdev.brightness;
- u16 up = get_usage_page(drvdata.hdev);
+ u8 br = cfg->led_mc->led_cdev.brightness;
+ u16 up = get_usage_page(cfg->hdev);
u8 green, red, blue;
size_t size;
u8 *data;
int i;
- led_mc_calc_color_components(drvdata.led_mc, br);
- red = drvdata.led_mc->subled_info[0].brightness;
- green = drvdata.led_mc->subled_info[1].brightness;
- blue = drvdata.led_mc->subled_info[2].brightness;
+ led_mc_calc_color_components(cfg->led_mc, br);
+ red = cfg->led_mc->subled_info[0].brightness;
+ green = cfg->led_mc->subled_info[1].brightness;
+ blue = cfg->led_mc->subled_info[2].brightness;
switch (up) {
case GEN1_USAGE_PAGE:
@@ -1133,7 +1154,7 @@ static int oxp_rgb_color_set(void)
data[3 * i + 2] = green;
data[3 * i + 3] = blue;
}
- return oxp_gen_1_property_out(OXP_FID_GEN1_RGB_SET, data, size);
+ return oxp_gen_1_property_out(cfg, OXP_FID_GEN1_RGB_SET, data, size);
case GEN2_USAGE_PAGE:
size = 57;
data = (u8[57]) { OXP_EFFECT_MONO_TRUE, 0x00, 0x02 };
@@ -1143,15 +1164,15 @@ static int oxp_rgb_color_set(void)
data[3 * i + 1] = green;
data[3 * i + 2] = blue;
}
- return oxp_gen_2_property_out(OXP_FID_GEN2_STATUS_EVENT, data, size);
+ return oxp_gen_2_property_out(cfg, OXP_FID_GEN2_STATUS_EVENT, data, size);
default:
return -ENODEV;
}
}
-static int oxp_rgb_effect_set(u8 effect)
+static int oxp_rgb_effect_set(struct oxp_hid_cfg *cfg, u8 effect)
{
- u16 up = get_usage_page(drvdata.hdev);
+ u16 up = get_usage_page(cfg->hdev);
u8 *data;
int ret;
@@ -1178,18 +1199,18 @@ static int oxp_rgb_effect_set(u8 effect)
switch (up) {
case GEN1_USAGE_PAGE:
data = (u8[1]) { effect };
- ret = oxp_gen_1_property_out(OXP_FID_GEN1_RGB_SET, data, 1);
+ ret = oxp_gen_1_property_out(cfg, OXP_FID_GEN1_RGB_SET, data, 1);
break;
case GEN2_USAGE_PAGE:
data = (u8[3]) { effect, 0x00, 0x02 };
- ret = oxp_gen_2_property_out(OXP_FID_GEN2_STATUS_EVENT, data, 3);
+ ret = oxp_gen_2_property_out(cfg, OXP_FID_GEN2_STATUS_EVENT, data, 3);
break;
default:
ret = -ENODEV;
}
break;
case OXP_EFFECT_MONO_LIST:
- ret = oxp_rgb_color_set();
+ ret = oxp_rgb_color_set(cfg);
break;
default:
return -EINVAL;
@@ -1198,14 +1219,23 @@ static int oxp_rgb_effect_set(u8 effect)
if (ret)
return ret;
- drvdata.rgb_effect = effect;
+ cfg->rgb_effect = effect;
return 0;
}
+static struct oxp_hid_cfg *oxp_rgb_cfg_from_dev(struct device *dev)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(led_cdev);
+
+ return container_of(mc_cdev, struct oxp_hid_cfg, cdev);
+}
+
static ssize_t enabled_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
+ struct oxp_hid_cfg *cfg = oxp_rgb_cfg_from_dev(dev);
int ret;
u8 val;
@@ -1214,30 +1244,31 @@ static ssize_t enabled_store(struct device *dev, struct device_attribute *attr,
return ret;
val = ret;
- guard(mutex)(&drvdata.rgb_mutex);
+ guard(mutex)(&cfg->rgb_mutex);
- ret = oxp_rgb_status_store(val, drvdata.rgb_speed,
- drvdata.rgb_brightness);
+ ret = oxp_rgb_status_store(cfg, val, cfg->rgb_speed,
+ cfg->rgb_brightness);
if (ret)
return ret;
- drvdata.rgb_en = val;
+ cfg->rgb_en = val;
return count;
}
static ssize_t enabled_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
+ struct oxp_hid_cfg *cfg = oxp_rgb_cfg_from_dev(dev);
int ret;
- ret = oxp_rgb_status_show();
+ ret = oxp_rgb_status_show(cfg);
if (ret)
return ret;
- if (drvdata.rgb_en >= ARRAY_SIZE(oxp_feature_en_text))
+ if (cfg->rgb_en >= ARRAY_SIZE(oxp_feature_en_text))
return -EINVAL;
- return sysfs_emit(buf, "%s\n", oxp_feature_en_text[drvdata.rgb_en]);
+ return sysfs_emit(buf, "%s\n", oxp_feature_en_text[cfg->rgb_en]);
}
static DEVICE_ATTR_RW(enabled);
@@ -1260,6 +1291,7 @@ static DEVICE_ATTR_RO(enabled_index);
static ssize_t effect_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
+ struct oxp_hid_cfg *cfg = oxp_rgb_cfg_from_dev(dev);
u8 old_effect;
int ret;
u8 val;
@@ -1270,20 +1302,20 @@ static ssize_t effect_store(struct device *dev, struct device_attribute *attr,
val = ret;
- guard(mutex)(&drvdata.rgb_mutex);
- old_effect = drvdata.rgb_effect;
- drvdata.rgb_effect = val;
+ guard(mutex)(&cfg->rgb_mutex);
+ old_effect = cfg->rgb_effect;
+ cfg->rgb_effect = val;
- ret = oxp_rgb_status_store(drvdata.rgb_en, drvdata.rgb_speed,
- drvdata.rgb_brightness);
+ ret = oxp_rgb_status_store(cfg, cfg->rgb_en, cfg->rgb_speed,
+ cfg->rgb_brightness);
if (ret) {
- drvdata.rgb_effect = old_effect;
+ cfg->rgb_effect = old_effect;
return ret;
}
- ret = oxp_rgb_effect_set(val);
+ ret = oxp_rgb_effect_set(cfg, val);
if (ret) {
- drvdata.rgb_effect = old_effect;
+ cfg->rgb_effect = old_effect;
return ret;
}
@@ -1293,16 +1325,17 @@ static ssize_t effect_store(struct device *dev, struct device_attribute *attr,
static ssize_t effect_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
+ struct oxp_hid_cfg *cfg = oxp_rgb_cfg_from_dev(dev);
int ret;
- ret = oxp_rgb_status_show();
+ ret = oxp_rgb_status_show(cfg);
if (ret)
return ret;
- if (drvdata.rgb_effect >= ARRAY_SIZE(oxp_rgb_effect_text))
+ if (cfg->rgb_effect >= ARRAY_SIZE(oxp_rgb_effect_text))
return -EINVAL;
- return sysfs_emit(buf, "%s\n", oxp_rgb_effect_text[drvdata.rgb_effect]);
+ return sysfs_emit(buf, "%s\n", oxp_rgb_effect_text[cfg->rgb_effect]);
}
static DEVICE_ATTR_RW(effect);
@@ -1326,6 +1359,7 @@ static DEVICE_ATTR_RO(effect_index);
static ssize_t speed_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
+ struct oxp_hid_cfg *cfg = oxp_rgb_cfg_from_dev(dev);
int ret;
u8 val;
@@ -1336,29 +1370,30 @@ static ssize_t speed_store(struct device *dev, struct device_attribute *attr,
if (val > 9)
return -EINVAL;
- guard(mutex)(&drvdata.rgb_mutex);
+ guard(mutex)(&cfg->rgb_mutex);
- ret = oxp_rgb_status_store(drvdata.rgb_en, val, drvdata.rgb_brightness);
+ ret = oxp_rgb_status_store(cfg, cfg->rgb_en, val, cfg->rgb_brightness);
if (ret)
return ret;
- drvdata.rgb_speed = val;
+ cfg->rgb_speed = val;
return count;
}
static ssize_t speed_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
+ struct oxp_hid_cfg *cfg = oxp_rgb_cfg_from_dev(dev);
int ret;
- ret = oxp_rgb_status_show();
+ ret = oxp_rgb_status_show(cfg);
if (ret)
return ret;
- if (drvdata.rgb_speed > 9)
+ if (cfg->rgb_speed > 9)
return -EINVAL;
- return sysfs_emit(buf, "%hhu\n", drvdata.rgb_speed);
+ return sysfs_emit(buf, "%hhu\n", cfg->rgb_speed);
}
static DEVICE_ATTR_RW(speed);
@@ -1371,42 +1406,47 @@ static DEVICE_ATTR_RO(speed_range);
static void oxp_rgb_queue_fn(struct work_struct *work)
{
- unsigned int max_brightness = drvdata.led_mc->led_cdev.max_brightness;
- unsigned int brightness = drvdata.led_mc->led_cdev.brightness;
+ struct oxp_hid_cfg *cfg = container_of(to_delayed_work(work),
+ struct oxp_hid_cfg, oxp_rgb_queue);
+ unsigned int max_brightness = cfg->led_mc->led_cdev.max_brightness;
+ unsigned int brightness = cfg->led_mc->led_cdev.brightness;
u8 val = 4 * brightness / max_brightness;
int ret;
- if (READ_ONCE(drvdata.removing))
+ if (READ_ONCE(cfg->removing))
return;
- guard(mutex)(&drvdata.rgb_mutex);
+ guard(mutex)(&cfg->rgb_mutex);
- if (drvdata.rgb_brightness != val) {
- ret = oxp_rgb_status_store(drvdata.rgb_en, drvdata.rgb_speed, val);
+ if (cfg->rgb_brightness != val) {
+ ret = oxp_rgb_status_store(cfg, cfg->rgb_en, cfg->rgb_speed, val);
if (ret)
- dev_err(drvdata.led_mc->led_cdev.dev,
+ dev_err(cfg->led_mc->led_cdev.dev,
"Error: Failed to write RGB Status: %i\n", ret);
- drvdata.rgb_brightness = val;
+ cfg->rgb_brightness = val;
}
- if (drvdata.rgb_effect != OXP_EFFECT_MONO_LIST)
+ if (cfg->rgb_effect != OXP_EFFECT_MONO_LIST)
return;
- ret = oxp_rgb_effect_set(drvdata.rgb_effect);
+ ret = oxp_rgb_effect_set(cfg, cfg->rgb_effect);
if (ret)
- dev_err(drvdata.led_mc->led_cdev.dev, "Error: Failed to write RGB color: %i\n",
+ dev_err(cfg->led_mc->led_cdev.dev, "Error: Failed to write RGB color: %i\n",
ret);
}
static void oxp_rgb_brightness_set(struct led_classdev *led_cdev,
enum led_brightness brightness)
{
- if (READ_ONCE(drvdata.removing))
+ 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))
return;
led_cdev->brightness = brightness;
- mod_delayed_work(system_dfl_wq, &drvdata.oxp_rgb_queue, msecs_to_jiffies(50));
+ mod_delayed_work(system_dfl_wq, &cfg->oxp_rgb_queue, msecs_to_jiffies(50));
}
static struct attribute *oxp_rgb_attrs[] = {
@@ -1423,7 +1463,7 @@ static const struct attribute_group oxp_rgb_attr_group = {
.attrs = oxp_rgb_attrs,
};
-static struct mc_subled oxp_rgb_subled_info[] = {
+static const struct mc_subled oxp_rgb_subled_info[] = {
{
.color_index = LED_COLOR_ID_RED,
.intensity = 0x24,
@@ -1444,7 +1484,7 @@ static struct mc_subled oxp_rgb_subled_info[] = {
},
};
-static struct led_classdev_mc oxp_cdev_rgb = {
+static const struct led_classdev_mc oxp_cdev_rgb = {
.led_cdev = {
.name = "oxp:rgb:joystick_rings",
.color = LED_COLOR_ID_RGB,
@@ -1453,7 +1493,6 @@ static struct led_classdev_mc oxp_cdev_rgb = {
.brightness_set = oxp_rgb_brightness_set,
},
.num_colors = ARRAY_SIZE(oxp_rgb_subled_info),
- .subled_info = oxp_rgb_subled_info,
};
struct quirk_entry {
@@ -1506,61 +1545,80 @@ static bool oxp_hybrid_mcu_device(void)
return quirks->hybrid_mcu;
}
-static void oxp_drain_output(void)
+static void oxp_drain_output(struct oxp_hid_cfg *cfg)
{
/* Wait for any in-flight sysfs output before closing the transport. */
- guard(mutex)(&drvdata.cfg_mutex);
+ guard(mutex)(&cfg->cfg_mutex);
}
-static void oxp_quiesce_work(void)
+static void oxp_quiesce_work(struct oxp_hid_cfg *cfg)
{
- WRITE_ONCE(drvdata.removing, true);
- if (drvdata.rgb_work_initialized)
- disable_delayed_work_sync(&drvdata.oxp_rgb_queue);
- if (drvdata.gen2_work_initialized) {
- disable_delayed_work_sync(&drvdata.oxp_btn_queue);
- disable_delayed_work_sync(&drvdata.oxp_mcu_init);
+ WRITE_ONCE(cfg->removing, true);
+ 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();
+ oxp_drain_output(cfg);
+}
+
+static void oxp_cfg_release(void *data)
+{
+ struct oxp_hid_cfg *cfg = data;
+
+ hid_set_drvdata(cfg->hdev, NULL);
}
static int oxp_cfg_probe(struct hid_device *hdev, u16 up)
{
struct oxp_bmap_page_1 *bmap_1;
struct oxp_bmap_page_2 *bmap_2;
+ struct oxp_hid_cfg *cfg;
int ret;
- hid_set_drvdata(hdev, &drvdata);
- mutex_init(&drvdata.cfg_mutex);
- mutex_init(&drvdata.rgb_mutex);
- drvdata.hdev = hdev;
- drvdata.removing = false;
+ cfg = devm_kzalloc(&hdev->dev, sizeof(*cfg), GFP_KERNEL);
+ if (!cfg)
+ return -ENOMEM;
+
+ cfg->hdev = hdev;
+ mutex_init(&cfg->cfg_mutex);
+ mutex_init(&cfg->rgb_mutex);
+
+ /* Clear drvdata after registered callback objects have been released. */
+ hid_set_drvdata(hdev, cfg);
+ ret = devm_add_action_or_reset(&hdev->dev, oxp_cfg_release, cfg);
+ if (ret)
+ return ret;
if (up == GEN2_USAGE_PAGE && oxp_hybrid_mcu_device())
goto skip_rgb;
- drvdata.led_mc = &oxp_cdev_rgb;
+ cfg->cdev = oxp_cdev_rgb;
+ memcpy(cfg->subled_info, oxp_rgb_subled_info, sizeof(cfg->subled_info));
+ cfg->cdev.subled_info = cfg->subled_info;
+ cfg->led_mc = &cfg->cdev;
- INIT_DELAYED_WORK(&drvdata.oxp_rgb_queue, oxp_rgb_queue_fn);
- drvdata.rgb_work_initialized = true;
- ret = devm_led_classdev_multicolor_register(&hdev->dev, &oxp_cdev_rgb);
+ INIT_DELAYED_WORK(&cfg->oxp_rgb_queue, oxp_rgb_queue_fn);
+ cfg->rgb_work_initialized = true;
+ ret = devm_led_classdev_multicolor_register(&hdev->dev, cfg->led_mc);
if (ret) {
dev_err_probe(&hdev->dev, ret,
"Failed to create RGB device\n");
goto err_quiesce;
}
- ret = devm_device_add_group(drvdata.led_mc->led_cdev.dev,
+ ret = devm_device_add_group(cfg->led_mc->led_cdev.dev,
&oxp_rgb_attr_group);
if (ret) {
- dev_err_probe(drvdata.led_mc->led_cdev.dev, ret,
- "Failed to create RGB configuration attributes\n");
+ dev_err_probe(cfg->led_mc->led_cdev.dev, ret,
+ "Failed to create RGB configuration attributes\n");
goto err_quiesce;
}
- ret = oxp_rgb_status_show();
+ ret = oxp_rgb_status_show(cfg);
if (ret)
- dev_warn(drvdata.led_mc->led_cdev.dev,
+ dev_warn(cfg->led_mc->led_cdev.dev,
"Failed to query RGB initial state: %i\n", ret);
/* Below features are only implemented in gen 2 */
@@ -1582,17 +1640,17 @@ skip_rgb:
goto err_quiesce;
}
- drvdata.bmap_1 = bmap_1;
- drvdata.bmap_2 = bmap_2;
- oxp_reset_buttons();
- INIT_DELAYED_WORK(&drvdata.oxp_btn_queue, oxp_btn_queue_fn);
+ cfg->bmap_1 = bmap_1;
+ cfg->bmap_2 = bmap_2;
+ oxp_reset_buttons(cfg);
+ INIT_DELAYED_WORK(&cfg->oxp_btn_queue, oxp_btn_queue_fn);
- drvdata.gamepad_mode = OXP_GP_MODE_XINPUT;
- drvdata.rumble_intensity = 5;
+ cfg->gamepad_mode = OXP_GP_MODE_XINPUT;
+ cfg->rumble_intensity = 5;
- INIT_DELAYED_WORK(&drvdata.oxp_mcu_init, oxp_mcu_init_fn);
- WRITE_ONCE(drvdata.gen2_work_initialized, true);
- mod_delayed_work(system_dfl_wq, &drvdata.oxp_mcu_init, msecs_to_jiffies(50));
+ INIT_DELAYED_WORK(&cfg->oxp_mcu_init, oxp_mcu_init_fn);
+ WRITE_ONCE(cfg->gen2_work_initialized, true);
+ mod_delayed_work(system_dfl_wq, &cfg->oxp_mcu_init, msecs_to_jiffies(50));
ret = devm_device_add_group(&hdev->dev, &oxp_cfg_attrs_group);
if (ret) {
@@ -1604,7 +1662,7 @@ skip_rgb:
return 0;
err_quiesce:
- oxp_quiesce_work();
+ oxp_quiesce_work(cfg);
return ret;
}
@@ -1648,8 +1706,10 @@ static int oxp_hid_probe(struct hid_device *hdev,
static void oxp_hid_remove(struct hid_device *hdev)
{
- if (hid_get_drvdata(hdev))
- oxp_quiesce_work();
+ struct oxp_hid_cfg *cfg = hid_get_drvdata(hdev);
+
+ if (cfg)
+ oxp_quiesce_work(cfg);
hid_hw_close(hdev);
hid_hw_stop(hdev);
}
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 10/15] HID: hid-oxp: handle controller reinitialization across suspend
2026-09-10 3:21 [PATCH 00/15] HID: hid-oxp: fix and extend X2-family controller support Andrei Aldea
` (8 preceding siblings ...)
2026-09-10 3:21 ` [PATCH 09/15] HID: hid-oxp: keep configuration state per HID interface Andrei Aldea
@ 2026-09-10 3:21 ` Andrei Aldea
2026-09-10 3:34 ` sashiko-bot
2026-09-10 3:21 ` [PATCH 11/15] HID: hid-oxp: group declarations and protocol definitions Andrei Aldea
` (4 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Andrei Aldea @ 2026-09-10 3:21 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Derek J. Clark
Cc: linux-input, linux-kernel, Lee Jones, Pavel Machek, linux-leds
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);
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 10/15] HID: hid-oxp: handle controller reinitialization across suspend
2026-09-10 3:21 ` [PATCH 10/15] HID: hid-oxp: handle controller reinitialization across suspend Andrei Aldea
@ 2026-09-10 3:34 ` sashiko-bot
0 siblings, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-09-10 3:34 UTC (permalink / raw)
To: Andrei Aldea; +Cc: dmitry.torokhov, linux-input, linux-leds, lee
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Late monocolor acknowledgments trigger spurious MCU reinitializations.
- [Low] Unnecessary 200ms delay when the transport write fails.
--
commit 0d410b2c1077d136f271fbc3070448d1e53eb0df
Author: Andrei Aldea <andrei1998@gmail.com>
HID: hid-oxp: handle controller reinitialization across suspend
This commit attempts to fix spurious MCU resets caused by confusing a monocolor
acknowledgment with an asynchronous MCU reset notification. It tracks monocolor
writes and consumes the matching acknowledgment. It also drains work queues on
suspend and re-enables them on resume, and queues a fallback reinitialization
after a set interval.
> diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
> index 7b366878fcb3e..e26e6a98294fe 100644
> --- a/drivers/hid/hid-oxp.c
> +++ b/drivers/hid/hid-oxp.c
[ ... ]
> @@ -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) {
[Severity: Medium]
Will this logic misidentify a late acknowledgment as a reset notification?
In mcu_property_out(), rgb_reply_pending is unconditionally cleared after
a strict 200ms timeout. If an acknowledgment from the MCU arrives after
that 200ms window, the solicited check above will fail because
cfg->rgb_reply_pending is already false. The late ACK will then fall
through to this check where data[3] == OXP_EFFECT_MONO_TRUE matches,
mistaking the late reply for an MCU reset and scheduling a spurious
controller reinitialization.
[ ... ]
> @@ -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);
[Severity: Low]
Does this code unnecessarily delay 200ms on transport errors?
When hid_hw_output_report() returns a negative error code (e.g. if the
device is disconnected), this removed the early return but failed to
bypass the sleep, resulting in an unneeded 200ms delay.
> - 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;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260910032115.28669-1-andrei1998@gmail.com?part=10
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 11/15] HID: hid-oxp: group declarations and protocol definitions
2026-09-10 3:21 [PATCH 00/15] HID: hid-oxp: fix and extend X2-family controller support Andrei Aldea
` (9 preceding siblings ...)
2026-09-10 3:21 ` [PATCH 10/15] HID: hid-oxp: handle controller reinitialization across suspend Andrei Aldea
@ 2026-09-10 3:21 ` Andrei Aldea
2026-09-10 3:40 ` sashiko-bot
2026-09-10 3:21 ` [PATCH 12/15] HID: hid-oxp: support three-page button maps on X2 controllers Andrei Aldea
` (3 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Andrei Aldea @ 2026-09-10 3:21 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Derek J. Clark
Cc: linux-input, linux-kernel, Lee Jones, Pavel Machek, linux-leds
Collect protocol definitions and helper macros at the top of the file,
place the quirk structure with the other type declarations, and tidy
configuration field ordering and callback declaration formatting.
Keep this cleanup separate from the preceding behavioral fixes and the
new-controller feature patches.
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, 60 insertions(+), 56 deletions(-)
diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index e26e6a9..8c44f57 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -33,6 +33,30 @@
#define GEN1_USAGE_PAGE 0xff01
#define GEN2_USAGE_PAGE 0xff00
+#define OXP_MAPPING_GAMEPAD 0x01
+#define OXP_MAPPING_KEYBOARD 0x02
+#define OXP_FILL_PAGE_SLOT(page, btn) \
+ { .button_idx = (page)->btn.button_idx, \
+ .mapping_idx = (page)->btn.mapping_idx }
+
+#define OXP_GET_PROPERTY 0xfc
+#define OXP_SET_PROPERTY 0xfd
+#define OXP_EFFECT_MONO_TRUE 0xfe /* actual index for monocolor */
+
+#define OXP_DEVICE_ATTR_RW(_name, _group) \
+ static ssize_t _name##_store(struct device *dev, \
+ struct device_attribute *attr, \
+ const char *buf, size_t count) \
+ { \
+ return _group##_store(dev, attr, buf, count, _name.index); \
+ } \
+ static ssize_t _name##_show(struct device *dev, \
+ struct device_attribute *attr, char *buf) \
+ { \
+ return _group##_show(dev, attr, buf, _name.index); \
+ } \
+ static DEVICE_ATTR_RW(_name)
+
enum oxp_function_index {
OXP_FID_GEN1_RGB_SET = 0x07,
OXP_FID_GEN1_RGB_REPLY = 0x0f,
@@ -42,9 +66,6 @@ enum oxp_function_index {
OXP_FID_GEN2_STATUS_EVENT = 0xb8,
};
-#define OXP_MAPPING_GAMEPAD 0x01
-#define OXP_MAPPING_KEYBOARD 0x02
-
struct oxp_button_data {
u8 mode;
u8 index;
@@ -176,37 +197,38 @@ struct oxp_bmap_page_2 {
/* Hybrid devices expose RGB and controller configuration on separate HIDs. */
struct oxp_hid_cfg {
- struct led_classdev_mc cdev;
- struct mc_subled subled_info[3];
- struct delayed_work oxp_rgb_queue;
+ /* General HID state */
+ struct hid_device *hdev;
+ struct mutex cfg_mutex; /*ensure single synchronous output report*/
+ bool suspended;
+ bool removing;
+
+ /* Gamepad state */
struct delayed_work oxp_btn_queue;
+ struct delayed_work oxp_mcu_init;
struct oxp_bmap_page_1 *bmap_1;
struct oxp_bmap_page_2 *bmap_2;
- struct delayed_work oxp_mcu_init;
+ bool gen2_work_initialized;
+ u8 rumble_intensity;
+ u8 gamepad_mode;
+
+ /* RGB state */
+ struct delayed_work oxp_rgb_queue;
+ struct mc_subled subled_info[3];
struct led_classdev_mc *led_mc;
- struct hid_device *hdev;
- struct mutex cfg_mutex; /*ensure single synchronous output report*/
- struct mutex rgb_mutex; /*serialize complete RGB transactions*/
+ struct led_classdev_mc cdev;
spinlock_t rgb_reply_lock;
+ struct mutex rgb_mutex; /*serialize complete RGB transactions*/
+ bool rgb_work_initialized;
+ bool rgb_reply_pending;
u8 rgb_reply_command;
u8 rgb_reply_zone;
- bool rgb_reply_pending;
u8 rgb_brightness;
- u8 gamepad_mode;
- u8 rumble_intensity;
u8 rgb_effect;
u8 rgb_speed;
u8 rgb_en;
- bool rgb_work_initialized;
- bool gen2_work_initialized;
- bool suspended;
- bool removing;
};
-#define OXP_FILL_PAGE_SLOT(page, btn) \
- { .button_idx = (page)->btn.button_idx, \
- .mapping_idx = (page)->btn.mapping_idx }
-
enum oxp_gamepad_mode_index {
OXP_GP_MODE_XINPUT = 0x00,
OXP_GP_MODE_DEBUG = 0x03,
@@ -251,14 +273,6 @@ enum oxp_rgb_effect_index {
OXP_EFFECT_MONO_LIST, /* placeholder for effect_index_show */
};
-/* These belong to rgb_effect_index, but we want to hide them from
- * rgb_effect_text
- */
-
-#define OXP_GET_PROPERTY 0xfc
-#define OXP_SET_PROPERTY 0xfd
-#define OXP_EFFECT_MONO_TRUE 0xfe /* actual index for monocolor */
-
static const char *const oxp_rgb_effect_text[] = {
[OXP_UNKNOWN] = "unknown",
[OXP_EFFECT_AURORA] = "aurora",
@@ -321,6 +335,10 @@ struct oxp_attr {
u8 index;
};
+struct quirk_entry {
+ bool hybrid_mcu;
+};
+
static u16 get_usage_page(struct hid_device *hdev)
{
return hdev->collection[0].usage >> 16;
@@ -508,8 +526,9 @@ static int oxp_hid_raw_event(struct hid_device *hdev, struct hid_report *report,
return 0;
}
-static int mcu_property_out(struct oxp_hid_cfg *cfg, u8 *header, size_t header_size, u8 *data,
- size_t data_size, u8 *footer, size_t footer_size)
+static int mcu_property_out(struct oxp_hid_cfg *cfg, u8 *header,
+ size_t header_size, u8 *data, size_t data_size,
+ u8 *footer, size_t footer_size)
{
unsigned char *dmabuf __free(kfree) = kzalloc(OXP_PACKET_SIZE, GFP_KERNEL);
bool rgb_write;
@@ -560,7 +579,8 @@ static int mcu_property_out(struct oxp_hid_cfg *cfg, u8 *header, size_t header_s
return ret;
}
-static int oxp_gen_1_property_out(struct oxp_hid_cfg *cfg, enum oxp_function_index fid, u8 *data,
+static int oxp_gen_1_property_out(struct oxp_hid_cfg *cfg,
+ enum oxp_function_index fid, u8 *data,
u8 data_size)
{
u8 header[] = { fid, GEN1_MESSAGE_ID };
@@ -569,7 +589,8 @@ static int oxp_gen_1_property_out(struct oxp_hid_cfg *cfg, enum oxp_function_ind
return mcu_property_out(cfg, header, header_size, data, data_size, NULL, 0);
}
-static int oxp_gen_2_property_out(struct oxp_hid_cfg *cfg, enum oxp_function_index fid, u8 *data,
+static int oxp_gen_2_property_out(struct oxp_hid_cfg *cfg,
+ enum oxp_function_index fid, u8 *data,
u8 data_size)
{
u8 header[] = { fid, GEN2_MESSAGE_ID, 0x01 };
@@ -1030,20 +1051,6 @@ static ssize_t rumble_intensity_range_show(struct device *dev,
}
static DEVICE_ATTR_RO(rumble_intensity_range);
-#define OXP_DEVICE_ATTR_RW(_name, _group) \
- static ssize_t _name##_store(struct device *dev, \
- struct device_attribute *attr, \
- const char *buf, size_t count) \
- { \
- return _group##_store(dev, attr, buf, count, _name.index); \
- } \
- static ssize_t _name##_show(struct device *dev, \
- struct device_attribute *attr, char *buf) \
- { \
- return _group##_show(dev, attr, buf, _name.index); \
- } \
- static DEVICE_ATTR_RW(_name)
-
static struct oxp_attr button_a = { BUTTON_A };
OXP_DEVICE_ATTR_RW(button_a, map_button);
@@ -1130,7 +1137,8 @@ static const struct attribute_group oxp_cfg_attrs_group = {
.attrs = oxp_cfg_attrs,
};
-static int oxp_rgb_status_store(struct oxp_hid_cfg *cfg, u8 enabled, u8 speed, u8 brightness)
+static int oxp_rgb_status_store(struct oxp_hid_cfg *cfg, u8 enabled,
+ u8 speed, u8 brightness)
{
u16 up = get_usage_page(cfg->hdev);
u8 *data;
@@ -1538,10 +1546,6 @@ static const struct led_classdev_mc oxp_cdev_rgb = {
.num_colors = ARRAY_SIZE(oxp_rgb_subled_info),
};
-struct quirk_entry {
- bool hybrid_mcu;
-};
-
static struct quirk_entry quirk_hybrid_mcu = {
.hybrid_mcu = true,
};
@@ -1651,7 +1655,7 @@ static int oxp_cfg_probe(struct hid_device *hdev, u16 up)
ret = devm_led_classdev_multicolor_register(&hdev->dev, cfg->led_mc);
if (ret) {
dev_err_probe(&hdev->dev, ret,
- "Failed to create RGB device\n");
+ "Failed to create RGB device\n");
goto err_quiesce;
}
@@ -1676,14 +1680,14 @@ skip_rgb:
bmap_1 = devm_kzalloc(&hdev->dev, sizeof(struct oxp_bmap_page_1), GFP_KERNEL);
if (!bmap_1) {
ret = dev_err_probe(&hdev->dev, -ENOMEM,
- "Unable to allocate button map page 1\n");
+ "Unable to allocate button map page 1\n");
goto err_quiesce;
}
bmap_2 = devm_kzalloc(&hdev->dev, sizeof(struct oxp_bmap_page_2), GFP_KERNEL);
if (!bmap_2) {
ret = dev_err_probe(&hdev->dev, -ENOMEM,
- "Unable to allocate button map page 2\n");
+ "Unable to allocate button map page 2\n");
goto err_quiesce;
}
@@ -1702,7 +1706,7 @@ skip_rgb:
ret = devm_device_add_group(&hdev->dev, &oxp_cfg_attrs_group);
if (ret) {
dev_err_probe(&hdev->dev, ret,
- "Failed to attach configuration attributes\n");
+ "Failed to attach configuration attributes\n");
goto err_quiesce;
}
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 12/15] HID: hid-oxp: support three-page button maps on X2 controllers
2026-09-10 3:21 [PATCH 00/15] HID: hid-oxp: fix and extend X2-family controller support Andrei Aldea
` (10 preceding siblings ...)
2026-09-10 3:21 ` [PATCH 11/15] HID: hid-oxp: group declarations and protocol definitions Andrei Aldea
@ 2026-09-10 3:21 ` Andrei Aldea
2026-09-10 3:40 ` sashiko-bot
2026-09-10 3:21 ` [PATCH 13/15] HID: hid-oxp: represent RGB LEDs with a common array Andrei Aldea
` (2 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Andrei Aldea @ 2026-09-10 3:21 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Derek J. Clark; +Cc: linux-input, linux-kernel
The ONEXPLAYER 3 and X2 Mini Pro need mapping format 0x02 and a third
page preserving the extra buttons' factory mappings. Use exact DMI
matches and select configuration interface 2; the other HID interfaces
remain available without duplicate configuration or LED registration.
Initialize each page with the selected format. Page three is fixed because
its factory encodings have no entries in the public mapping table.
Legacy devices keep their two-page format 0x20 transaction.
Fixes: e4c850a6e750 ("HID: hid-oxp: Add Button Mapping Interface")
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 | 94 ++++++++++++++++++++++++++++++++++++-------
1 file changed, 79 insertions(+), 15 deletions(-)
diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index 8c44f57..48fa916 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -19,6 +19,7 @@
#include <linux/spinlock.h>
#include <linux/sysfs.h>
#include <linux/types.h>
+#include <linux/usb.h>
#include <linux/workqueue.h>
#include "hid-ids.h"
@@ -35,6 +36,8 @@
#define OXP_MAPPING_GAMEPAD 0x01
#define OXP_MAPPING_KEYBOARD 0x02
+#define OXP_BMAP_FORMAT_DEFAULT 0x20
+#define OXP_BMAP_FORMAT_X2 0x02
#define OXP_FILL_PAGE_SLOT(page, btn) \
{ .button_idx = (page)->btn.button_idx, \
.mapping_idx = (page)->btn.mapping_idx }
@@ -159,9 +162,9 @@ enum oxp_joybutton_index {
BUTTON_DRIGHT,
BUTTON_M1 = 0x22,
BUTTON_M2,
- /* These are unused currently, reserved for future devices */
BUTTON_M3,
BUTTON_M4,
+ /* These are unused currently, reserved for future devices */
BUTTON_M5,
BUTTON_M6,
};
@@ -209,8 +212,10 @@ struct oxp_hid_cfg {
struct oxp_bmap_page_1 *bmap_1;
struct oxp_bmap_page_2 *bmap_2;
bool gen2_work_initialized;
+ bool bmap_page_3;
u8 rumble_intensity;
u8 gamepad_mode;
+ u8 bmap_format;
/* RGB state */
struct delayed_work oxp_rgb_queue;
@@ -337,6 +342,9 @@ struct oxp_attr {
struct quirk_entry {
bool hybrid_mcu;
+ bool bmap_page_3;
+ u8 cfg_interface_num;
+ u8 bmap_format;
};
static u16 get_usage_page(struct hid_device *hdev)
@@ -735,8 +743,17 @@ static void oxp_page_fill_data(char *buf, const struct oxp_button_idx *buttons,
static int oxp_set_buttons(struct oxp_hid_cfg *cfg)
{
- u8 page_1[59] = { 0x02, 0x38, 0x20, 0x01, 0x01 };
- u8 page_2[59] = { 0x02, 0x38, 0x20, 0x02, 0x01 };
+ u8 page_1[59] = { 0x02, 0x38, cfg->bmap_format, 0x01, 0x01 };
+ u8 page_2[59] = { 0x02, 0x38, cfg->bmap_format, 0x02, 0x01 };
+ u8 page_3[59] = {
+ 0x02, 0x38, cfg->bmap_format, 0x03, 0x01,
+ /*
+ * M3/M4 have no mutable sysfs mapping slots. Keep their factory
+ * encodings, which are not entries in oxp_button_table.
+ */
+ BUTTON_M3, OXP_MAPPING_KEYBOARD, 0x02, 0x05, 0x00, 0x00,
+ BUTTON_M4, OXP_MAPPING_GAMEPAD, 0x21, 0x00, 0x00, 0x00,
+ };
u16 up = get_usage_page(cfg->hdev);
int ret;
@@ -774,7 +791,11 @@ static int oxp_set_buttons(struct oxp_hid_cfg *cfg)
if (ret)
return ret;
- return oxp_gen_2_property_out(cfg, OXP_FID_GEN2_KEY_STATE, page_2, ARRAY_SIZE(page_2));
+ ret = oxp_gen_2_property_out(cfg, OXP_FID_GEN2_KEY_STATE, page_2, ARRAY_SIZE(page_2));
+ if (ret || !cfg->bmap_page_3)
+ return ret;
+
+ return oxp_gen_2_property_out(cfg, OXP_FID_GEN2_KEY_STATE, page_3, ARRAY_SIZE(page_3));
}
static void oxp_reset_buttons(struct oxp_hid_cfg *cfg)
@@ -1550,7 +1571,13 @@ static struct quirk_entry quirk_hybrid_mcu = {
.hybrid_mcu = true,
};
-static const struct dmi_system_id oxp_hybrid_mcu_list[] = {
+static struct quirk_entry quirk_x2_bmap = {
+ .bmap_format = OXP_BMAP_FORMAT_X2,
+ .bmap_page_3 = true,
+ .cfg_interface_num = 2,
+};
+
+static const struct dmi_system_id oxp_quirk_list[] = {
{
.ident = "OneXPlayer Apex",
.matches = {
@@ -1575,21 +1602,34 @@ static const struct dmi_system_id oxp_hybrid_mcu_list[] = {
},
.driver_data = &quirk_hybrid_mcu,
},
+ {
+ .ident = "OneXPlayer 3",
+ .matches = {
+ DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ONE-NETBOOK"),
+ DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "ONEXPLAYER 3"),
+ },
+ .driver_data = &quirk_x2_bmap,
+ },
+ {
+ .ident = "OneXPlayer X2 Mini Pro",
+ .matches = {
+ DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ONE-NETBOOK"),
+ DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "ONEXPLAYER X2Mini PRO"),
+ },
+ .driver_data = &quirk_x2_bmap,
+ },
{},
};
-static bool oxp_hybrid_mcu_device(void)
+static const struct quirk_entry *oxp_get_quirks(void)
{
const struct dmi_system_id *dmi_id;
- struct quirk_entry *quirks;
- dmi_id = dmi_first_match(oxp_hybrid_mcu_list);
+ dmi_id = dmi_first_match(oxp_quirk_list);
if (!dmi_id)
- return false;
+ return NULL;
- quirks = dmi_id->driver_data;
-
- return quirks->hybrid_mcu;
+ return dmi_id->driver_data;
}
static void oxp_drain_output(struct oxp_hid_cfg *cfg)
@@ -1613,6 +1653,21 @@ static void oxp_quiesce_work(struct oxp_hid_cfg *cfg)
oxp_drain_output(cfg);
}
+static bool oxp_is_cfg_interface(struct hid_device *hdev,
+ const struct quirk_entry *quirks)
+{
+ struct usb_interface *intf;
+
+ if (!quirks || !quirks->cfg_interface_num)
+ return true;
+ if (hdev->bus != BUS_USB)
+ return false;
+
+ intf = to_usb_interface(hdev->dev.parent);
+ return intf->cur_altsetting->desc.bInterfaceNumber ==
+ quirks->cfg_interface_num;
+}
+
static void oxp_cfg_release(void *data)
{
struct oxp_hid_cfg *cfg = data;
@@ -1620,7 +1675,8 @@ static void oxp_cfg_release(void *data)
hid_set_drvdata(cfg->hdev, NULL);
}
-static int oxp_cfg_probe(struct hid_device *hdev, u16 up)
+static int oxp_cfg_probe(struct hid_device *hdev, u16 up,
+ const struct quirk_entry *quirks)
{
struct oxp_bmap_page_1 *bmap_1;
struct oxp_bmap_page_2 *bmap_2;
@@ -1642,7 +1698,7 @@ static int oxp_cfg_probe(struct hid_device *hdev, u16 up)
if (ret)
return ret;
- if (up == GEN2_USAGE_PAGE && oxp_hybrid_mcu_device())
+ if (up == GEN2_USAGE_PAGE && quirks && quirks->hybrid_mcu)
goto skip_rgb;
cfg->cdev = oxp_cdev_rgb;
@@ -1693,6 +1749,9 @@ skip_rgb:
cfg->bmap_1 = bmap_1;
cfg->bmap_2 = bmap_2;
+ cfg->bmap_format = quirks && quirks->bmap_format ?
+ quirks->bmap_format : OXP_BMAP_FORMAT_DEFAULT;
+ cfg->bmap_page_3 = quirks && quirks->bmap_page_3;
oxp_reset_buttons(cfg);
INIT_DELAYED_WORK(&cfg->oxp_btn_queue, oxp_btn_queue_fn);
@@ -1720,6 +1779,7 @@ err_quiesce:
static int oxp_hid_probe(struct hid_device *hdev,
const struct hid_device_id *id)
{
+ const struct quirk_entry *quirks;
int ret;
u16 up;
@@ -1738,12 +1798,16 @@ static int oxp_hid_probe(struct hid_device *hdev,
}
up = get_usage_page(hdev);
+ quirks = oxp_get_quirks();
dev_dbg(&hdev->dev, "Got usage page %04x\n", up);
switch (up) {
case GEN1_USAGE_PAGE:
case GEN2_USAGE_PAGE:
- ret = oxp_cfg_probe(hdev, up);
+ if (!oxp_is_cfg_interface(hdev, quirks))
+ return 0;
+
+ ret = oxp_cfg_probe(hdev, up, quirks);
if (ret) {
hid_hw_close(hdev);
hid_hw_stop(hdev);
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 12/15] HID: hid-oxp: support three-page button maps on X2 controllers
2026-09-10 3:21 ` [PATCH 12/15] HID: hid-oxp: support three-page button maps on X2 controllers Andrei Aldea
@ 2026-09-10 3:40 ` sashiko-bot
0 siblings, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-09-10 3:40 UTC (permalink / raw)
To: Andrei Aldea; +Cc: linux-input, dmitry.torokhov
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Invalid cast of HID parent device to `usb_interface` without verifying the transport layer.
--
commit 435c693ab184a92fb526c35572b01cbea8f10f4a
Author: Andrei Aldea <andrei1998@gmail.com>
HID: hid-oxp: support three-page button maps on X2 controllers
This commit adds support for a three-page button map format on the ONEXPLAYER
3 and X2 Mini Pro using exact DMI matches. It initializes the pages and limits
duplicate LED registration by restricting initialization to configuration
interface 2.
> diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
> index 8c44f576f3956..48fa9169b84c7 100644
> --- a/drivers/hid/hid-oxp.c
> +++ b/drivers/hid/hid-oxp.c
[ ... ]
> @@ -1613,6 +1653,21 @@ static void oxp_quiesce_work(struct oxp_hid_cfg *cfg)
> oxp_drain_output(cfg);
> }
>
> +static bool oxp_is_cfg_interface(struct hid_device *hdev,
> + const struct quirk_entry *quirks)
> +{
> + struct usb_interface *intf;
> +
> + if (!quirks || !quirks->cfg_interface_num)
> + return true;
> + if (hdev->bus != BUS_USB)
> + return false;
> +
> + intf = to_usb_interface(hdev->dev.parent);
[Severity: High]
Can this cast to a usb_interface lead to an out-of-bounds read or kernel
crash?
Checking that hdev->bus equals BUS_USB is not sufficient to guarantee that
the parent device is actually a USB interface. A user with access to
/dev/uhid can inject a virtual HID device with the bus field spoofed to
BUS_USB.
If that happens, the parent device will be a UHID character device, not a
USB interface. Dereferencing intf->cur_altsetting will then access invalid
memory.
Should this code use the hid_is_usb(hdev) helper to safely validate the USB
transport layer before casting the parent device?
> + return intf->cur_altsetting->desc.bInterfaceNumber ==
> + quirks->cfg_interface_num;
> +}
> +
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260910032115.28669-1-andrei1998@gmail.com?part=12
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 13/15] HID: hid-oxp: represent RGB LEDs with a common array
2026-09-10 3:21 [PATCH 00/15] HID: hid-oxp: fix and extend X2-family controller support Andrei Aldea
` (11 preceding siblings ...)
2026-09-10 3:21 ` [PATCH 12/15] HID: hid-oxp: support three-page button maps on X2 controllers Andrei Aldea
@ 2026-09-10 3:21 ` Andrei Aldea
2026-09-10 3:44 ` sashiko-bot
2026-09-10 3:21 ` [PATCH 14/15] HID: hid-oxp: add Gen3 joystick ring RGB support Andrei Aldea
2026-09-10 3:21 ` [PATCH 15/15] HID: hid-oxp: add X2 auxiliary RGB zones Andrei Aldea
14 siblings, 1 reply; 30+ messages in thread
From: Andrei Aldea @ 2026-09-10 3:21 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Derek J. Clark
Cc: linux-input, linux-kernel, Lee Jones, Pavel Machek, linux-leds
Move the existing joystick-ring class device, color components, delayed
work, and cached settings into a per-LED wrapper owned by each HID
configuration. Use a tagged state pointer so later LED types can share
registration and work management without duplicating the lifecycle.
Track only fully initialized work items and walk that count when
quiescing, suspending, or resuming the configuration. Resolve LED callbacks
through their containing wrapper, preserving the LED core's drvdata.
Keep a single FULL joystick-ring LED and retain the existing Gen1/Gen2
55/57-byte RGB payloads, controls, and defaults. This commit adds no new
hardware protocol or lighting zones.
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 | 433 +++++++++++++++++++++++++++++-------------
1 file changed, 297 insertions(+), 136 deletions(-)
diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index 48fa916..55f8b47 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -198,6 +198,8 @@ struct oxp_bmap_page_2 {
struct oxp_button_idx btn_m2;
} __packed;
+struct oxp_rgb_led;
+
/* Hybrid devices expose RGB and controller configuration on separate HIDs. */
struct oxp_hid_cfg {
/* General HID state */
@@ -218,20 +220,13 @@ struct oxp_hid_cfg {
u8 bmap_format;
/* RGB state */
- struct delayed_work oxp_rgb_queue;
- struct mc_subled subled_info[3];
- struct led_classdev_mc *led_mc;
- struct led_classdev_mc cdev;
+ struct oxp_rgb_led *rgb_leds;
spinlock_t rgb_reply_lock;
struct mutex rgb_mutex; /*serialize complete RGB transactions*/
- bool rgb_work_initialized;
bool rgb_reply_pending;
u8 rgb_reply_command;
u8 rgb_reply_zone;
- u8 rgb_brightness;
- u8 rgb_effect;
- u8 rgb_speed;
- u8 rgb_en;
+ u8 rgb_led_count;
};
enum oxp_gamepad_mode_index {
@@ -336,6 +331,31 @@ struct oxp_gen_2_rgb_report {
u8 effect;
} __packed;
+enum oxp_rgb_type {
+ OXP_RGB_FULL,
+};
+
+struct oxp_rgb_full_state {
+ u8 brightness;
+ u8 enabled;
+ u8 effect;
+ u8 speed;
+};
+
+struct oxp_rgb_led {
+ struct mc_subled subled_info[3];
+ struct led_classdev_mc mc_cdev;
+ struct delayed_work work;
+ struct oxp_hid_cfg *cfg;
+ void *state;
+ u8 type;
+};
+
+struct oxp_rgb_led_desc {
+ const char *name;
+ u8 type;
+};
+
struct oxp_attr {
u8 index;
};
@@ -352,29 +372,55 @@ static u16 get_usage_page(struct hid_device *hdev)
return hdev->collection[0].usage >> 16;
}
+static struct oxp_rgb_led *oxp_rgb_led_by_type(struct oxp_hid_cfg *cfg, u8 type)
+{
+ int i;
+
+ for (i = 0; i < cfg->rgb_led_count; i++)
+ if (cfg->rgb_leds[i].type == type)
+ return &cfg->rgb_leds[i];
+
+ return NULL;
+}
+
+static struct oxp_rgb_full_state *oxp_rgb_full_state(struct oxp_rgb_led *led)
+{
+ if (!led)
+ return NULL;
+
+ switch (led->type) {
+ case OXP_RGB_FULL:
+ return led->state;
+ }
+
+ return NULL;
+}
+
static int oxp_hid_raw_event_gen_1(struct hid_device *hdev,
struct hid_report *report, u8 *data,
int size)
{
struct oxp_hid_cfg *cfg = hid_get_drvdata(hdev);
- struct led_classdev_mc *led_mc = cfg->led_mc;
+ struct oxp_rgb_led *led = oxp_rgb_led_by_type(cfg, OXP_RGB_FULL);
+ struct oxp_rgb_full_state *state = oxp_rgb_full_state(led);
struct oxp_gen_1_rgb_report *rgb_rep;
+ struct led_classdev_mc *led_mc;
- if (size < sizeof(*rgb_rep) || !led_mc)
+ if (size < sizeof(*rgb_rep) || !state)
return 0;
if (data[1] != OXP_FID_GEN1_RGB_REPLY)
return 0;
+ led_mc = &led->mc_cdev;
rgb_rep = (struct oxp_gen_1_rgb_report *)data;
/* Ensure we save monocolor as the list value */
- cfg->rgb_effect = rgb_rep->effect == OXP_EFFECT_MONO_TRUE ?
- OXP_EFFECT_MONO_LIST :
- rgb_rep->effect;
- cfg->rgb_speed = rgb_rep->speed;
- cfg->rgb_en = rgb_rep->enabled == 0 ? OXP_FEAT_DISABLED :
- OXP_FEAT_ENABLED;
- cfg->rgb_brightness = rgb_rep->brightness;
+ state->effect = rgb_rep->effect == OXP_EFFECT_MONO_TRUE ?
+ OXP_EFFECT_MONO_LIST : rgb_rep->effect;
+ state->speed = rgb_rep->speed;
+ state->enabled = rgb_rep->enabled == 0 ? OXP_FEAT_DISABLED :
+ OXP_FEAT_ENABLED;
+ state->brightness = rgb_rep->brightness;
led_mc->led_cdev.brightness = rgb_rep->brightness *
led_mc->led_cdev.max_brightness / 4;
/* If monocolor had less than 100% brightness on the previous boot,
@@ -441,8 +487,10 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
int size)
{
struct oxp_hid_cfg *cfg = hid_get_drvdata(hdev);
- struct led_classdev_mc *led_mc = cfg->led_mc;
+ struct oxp_rgb_led *led = oxp_rgb_led_by_type(cfg, OXP_RGB_FULL);
+ struct oxp_rgb_full_state *state = oxp_rgb_full_state(led);
struct oxp_gen_2_rgb_report *rgb_rep;
+ struct led_classdev_mc *led_mc;
bool solicited = false;
if (size < OXP_STATUS_HEADER_SIZE)
@@ -479,22 +527,22 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
if (data[3] != OXP_GET_PROPERTY)
return 0;
- if (size < sizeof(*rgb_rep) || !led_mc)
+ if (size < sizeof(*rgb_rep) || !state)
return 0;
+ led_mc = &led->mc_cdev;
rgb_rep = (struct oxp_gen_2_rgb_report *)data;
if (rgb_rep->enabled > OXP_FEAT_ENABLED || rgb_rep->speed > 9 ||
rgb_rep->brightness > 4)
return 0;
/* Ensure we save monocolor as the list value */
- cfg->rgb_effect = rgb_rep->effect == OXP_EFFECT_MONO_TRUE ?
- OXP_EFFECT_MONO_LIST :
- rgb_rep->effect;
- cfg->rgb_speed = rgb_rep->speed;
- cfg->rgb_en = rgb_rep->enabled == 0 ? OXP_FEAT_DISABLED :
- OXP_FEAT_ENABLED;
- cfg->rgb_brightness = rgb_rep->brightness;
+ state->effect = rgb_rep->effect == OXP_EFFECT_MONO_TRUE ?
+ OXP_EFFECT_MONO_LIST : rgb_rep->effect;
+ state->speed = rgb_rep->speed;
+ state->enabled = rgb_rep->enabled == 0 ? OXP_FEAT_DISABLED :
+ OXP_FEAT_ENABLED;
+ state->brightness = rgb_rep->brightness;
led_mc->led_cdev.brightness = rgb_rep->brightness *
led_mc->led_cdev.max_brightness / 4;
/* If monocolor had less than 100% brightness on the previous boot,
@@ -1158,9 +1206,11 @@ static const struct attribute_group oxp_cfg_attrs_group = {
.attrs = oxp_cfg_attrs,
};
-static int oxp_rgb_status_store(struct oxp_hid_cfg *cfg, u8 enabled,
- u8 speed, u8 brightness)
+static int oxp_rgb_status_store(struct oxp_rgb_led *led, u8 enabled, u8 speed,
+ u8 brightness)
{
+ struct oxp_rgb_full_state *state = oxp_rgb_full_state(led);
+ struct oxp_hid_cfg *cfg = led->cfg;
u16 up = get_usage_page(cfg->hdev);
u8 *data;
@@ -1170,12 +1220,12 @@ static int oxp_rgb_status_store(struct oxp_hid_cfg *cfg, u8 enabled,
switch (up) {
case GEN1_USAGE_PAGE:
data = (u8[4]) { OXP_SET_PROPERTY, enabled, speed, brightness };
- if (cfg->rgb_effect == OXP_EFFECT_MONO_LIST)
+ if (state->effect == OXP_EFFECT_MONO_LIST)
data[3] = 0x04;
return oxp_gen_1_property_out(cfg, OXP_FID_GEN1_RGB_SET, data, 4);
case GEN2_USAGE_PAGE:
data = (u8[6]) { OXP_SET_PROPERTY, 0x00, 0x02, enabled, speed, brightness };
- if (cfg->rgb_effect == OXP_EFFECT_MONO_LIST)
+ if (state->effect == OXP_EFFECT_MONO_LIST)
data[5] = 0x04;
return oxp_gen_2_property_out(cfg, OXP_FID_GEN2_STATUS_EVENT, data, 6);
default:
@@ -1183,8 +1233,9 @@ static int oxp_rgb_status_store(struct oxp_hid_cfg *cfg, u8 enabled,
}
}
-static ssize_t oxp_rgb_status_show(struct oxp_hid_cfg *cfg)
+static ssize_t oxp_rgb_status_show(struct oxp_rgb_led *led)
{
+ struct oxp_hid_cfg *cfg = led->cfg;
u16 up = get_usage_page(cfg->hdev);
u8 *data;
@@ -1202,19 +1253,21 @@ static ssize_t oxp_rgb_status_show(struct oxp_hid_cfg *cfg)
}
}
-static int oxp_rgb_color_set(struct oxp_hid_cfg *cfg)
+static int oxp_rgb_color_set(struct oxp_rgb_led *led)
{
- u8 br = cfg->led_mc->led_cdev.brightness;
+ struct led_classdev_mc *led_mc = &led->mc_cdev;
+ struct oxp_hid_cfg *cfg = led->cfg;
u16 up = get_usage_page(cfg->hdev);
+ u8 br = led_mc->led_cdev.brightness;
u8 green, red, blue;
size_t size;
u8 *data;
int i;
- led_mc_calc_color_components(cfg->led_mc, br);
- red = cfg->led_mc->subled_info[0].brightness;
- green = cfg->led_mc->subled_info[1].brightness;
- blue = cfg->led_mc->subled_info[2].brightness;
+ led_mc_calc_color_components(led_mc, br);
+ red = led_mc->subled_info[0].brightness;
+ green = led_mc->subled_info[1].brightness;
+ blue = led_mc->subled_info[2].brightness;
switch (up) {
case GEN1_USAGE_PAGE:
@@ -1242,8 +1295,10 @@ static int oxp_rgb_color_set(struct oxp_hid_cfg *cfg)
}
}
-static int oxp_rgb_effect_set(struct oxp_hid_cfg *cfg, u8 effect)
+static int oxp_rgb_effect_set(struct oxp_rgb_led *led, u8 effect)
{
+ struct oxp_rgb_full_state *state = oxp_rgb_full_state(led);
+ struct oxp_hid_cfg *cfg = led->cfg;
u16 up = get_usage_page(cfg->hdev);
u8 *data;
int ret;
@@ -1282,7 +1337,7 @@ static int oxp_rgb_effect_set(struct oxp_hid_cfg *cfg, u8 effect)
}
break;
case OXP_EFFECT_MONO_LIST:
- ret = oxp_rgb_color_set(cfg);
+ ret = oxp_rgb_color_set(led);
break;
default:
return -EINVAL;
@@ -1291,26 +1346,31 @@ static int oxp_rgb_effect_set(struct oxp_hid_cfg *cfg, u8 effect)
if (ret)
return ret;
- cfg->rgb_effect = effect;
+ state->effect = effect;
return 0;
}
-static struct oxp_hid_cfg *oxp_rgb_cfg_from_dev(struct device *dev)
+static struct oxp_rgb_led *oxp_rgb_led_from_dev(struct device *dev)
{
struct led_classdev *led_cdev = dev_get_drvdata(dev);
struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(led_cdev);
- return container_of(mc_cdev, struct oxp_hid_cfg, cdev);
+ return container_of(mc_cdev, struct oxp_rgb_led, mc_cdev);
}
static ssize_t enabled_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
- struct oxp_hid_cfg *cfg = oxp_rgb_cfg_from_dev(dev);
+ struct oxp_rgb_led *led = oxp_rgb_led_from_dev(dev);
+ struct oxp_hid_cfg *cfg = led->cfg;
+ struct oxp_rgb_full_state *state = oxp_rgb_full_state(led);
int ret;
u8 val;
+ if (!state)
+ return -ENODEV;
+
ret = sysfs_match_string(oxp_feature_en_text, buf);
if (ret < 0)
return ret;
@@ -1318,29 +1378,32 @@ static ssize_t enabled_store(struct device *dev, struct device_attribute *attr,
guard(mutex)(&cfg->rgb_mutex);
- ret = oxp_rgb_status_store(cfg, val, cfg->rgb_speed,
- cfg->rgb_brightness);
+ ret = oxp_rgb_status_store(led, val, state->speed, state->brightness);
if (ret)
return ret;
- cfg->rgb_en = val;
+ state->enabled = val;
return count;
}
static ssize_t enabled_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
- struct oxp_hid_cfg *cfg = oxp_rgb_cfg_from_dev(dev);
+ struct oxp_rgb_led *led = oxp_rgb_led_from_dev(dev);
+ struct oxp_rgb_full_state *state = oxp_rgb_full_state(led);
int ret;
- ret = oxp_rgb_status_show(cfg);
+ if (!state)
+ return -ENODEV;
+
+ ret = oxp_rgb_status_show(led);
if (ret)
return ret;
- if (cfg->rgb_en >= ARRAY_SIZE(oxp_feature_en_text))
+ if (state->enabled >= ARRAY_SIZE(oxp_feature_en_text))
return -EINVAL;
- return sysfs_emit(buf, "%s\n", oxp_feature_en_text[cfg->rgb_en]);
+ return sysfs_emit(buf, "%s\n", oxp_feature_en_text[state->enabled]);
}
static DEVICE_ATTR_RW(enabled);
@@ -1363,11 +1426,16 @@ static DEVICE_ATTR_RO(enabled_index);
static ssize_t effect_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
- struct oxp_hid_cfg *cfg = oxp_rgb_cfg_from_dev(dev);
+ struct oxp_rgb_led *led = oxp_rgb_led_from_dev(dev);
+ struct oxp_hid_cfg *cfg = led->cfg;
+ struct oxp_rgb_full_state *state = oxp_rgb_full_state(led);
u8 old_effect;
int ret;
u8 val;
+ if (!state)
+ return -ENODEV;
+
ret = sysfs_match_string(oxp_rgb_effect_text, buf);
if (ret < 0)
return ret;
@@ -1375,19 +1443,19 @@ static ssize_t effect_store(struct device *dev, struct device_attribute *attr,
val = ret;
guard(mutex)(&cfg->rgb_mutex);
- old_effect = cfg->rgb_effect;
- cfg->rgb_effect = val;
+ old_effect = state->effect;
+ state->effect = val;
- ret = oxp_rgb_status_store(cfg, cfg->rgb_en, cfg->rgb_speed,
- cfg->rgb_brightness);
+ ret = oxp_rgb_status_store(led, state->enabled, state->speed,
+ state->brightness);
if (ret) {
- cfg->rgb_effect = old_effect;
+ state->effect = old_effect;
return ret;
}
- ret = oxp_rgb_effect_set(cfg, val);
+ ret = oxp_rgb_effect_set(led, val);
if (ret) {
- cfg->rgb_effect = old_effect;
+ state->effect = old_effect;
return ret;
}
@@ -1397,17 +1465,21 @@ static ssize_t effect_store(struct device *dev, struct device_attribute *attr,
static ssize_t effect_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
- struct oxp_hid_cfg *cfg = oxp_rgb_cfg_from_dev(dev);
+ struct oxp_rgb_led *led = oxp_rgb_led_from_dev(dev);
+ struct oxp_rgb_full_state *state = oxp_rgb_full_state(led);
int ret;
- ret = oxp_rgb_status_show(cfg);
+ if (!state)
+ return -ENODEV;
+
+ ret = oxp_rgb_status_show(led);
if (ret)
return ret;
- if (cfg->rgb_effect >= ARRAY_SIZE(oxp_rgb_effect_text))
+ if (state->effect >= ARRAY_SIZE(oxp_rgb_effect_text))
return -EINVAL;
- return sysfs_emit(buf, "%s\n", oxp_rgb_effect_text[cfg->rgb_effect]);
+ return sysfs_emit(buf, "%s\n", oxp_rgb_effect_text[state->effect]);
}
static DEVICE_ATTR_RW(effect);
@@ -1431,10 +1503,15 @@ static DEVICE_ATTR_RO(effect_index);
static ssize_t speed_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
- struct oxp_hid_cfg *cfg = oxp_rgb_cfg_from_dev(dev);
+ struct oxp_rgb_led *led = oxp_rgb_led_from_dev(dev);
+ struct oxp_hid_cfg *cfg = led->cfg;
+ struct oxp_rgb_full_state *state = oxp_rgb_full_state(led);
int ret;
u8 val;
+ if (!state)
+ return -ENODEV;
+
ret = kstrtou8(buf, 10, &val);
if (ret)
return ret;
@@ -1444,28 +1521,32 @@ static ssize_t speed_store(struct device *dev, struct device_attribute *attr,
guard(mutex)(&cfg->rgb_mutex);
- ret = oxp_rgb_status_store(cfg, cfg->rgb_en, val, cfg->rgb_brightness);
+ ret = oxp_rgb_status_store(led, state->enabled, val, state->brightness);
if (ret)
return ret;
- cfg->rgb_speed = val;
+ state->speed = val;
return count;
}
static ssize_t speed_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
- struct oxp_hid_cfg *cfg = oxp_rgb_cfg_from_dev(dev);
+ struct oxp_rgb_led *led = oxp_rgb_led_from_dev(dev);
+ struct oxp_rgb_full_state *state = oxp_rgb_full_state(led);
int ret;
- ret = oxp_rgb_status_show(cfg);
+ if (!state)
+ return -ENODEV;
+
+ ret = oxp_rgb_status_show(led);
if (ret)
return ret;
- if (cfg->rgb_speed > 9)
+ if (state->speed > 9)
return -EINVAL;
- return sysfs_emit(buf, "%hhu\n", cfg->rgb_speed);
+ return sysfs_emit(buf, "%hhu\n", state->speed);
}
static DEVICE_ATTR_RW(speed);
@@ -1476,49 +1557,64 @@ static ssize_t speed_range_show(struct device *dev,
}
static DEVICE_ATTR_RO(speed_range);
-static void oxp_rgb_queue_fn(struct work_struct *work)
+static void oxp_rgb_full_queue(struct oxp_rgb_led *led,
+ struct oxp_rgb_full_state *state)
{
- struct oxp_hid_cfg *cfg = container_of(to_delayed_work(work),
- struct oxp_hid_cfg, oxp_rgb_queue);
- unsigned int max_brightness = cfg->led_mc->led_cdev.max_brightness;
- unsigned int brightness = cfg->led_mc->led_cdev.brightness;
+ unsigned int max_brightness = led->mc_cdev.led_cdev.max_brightness;
+ unsigned int brightness = led->mc_cdev.led_cdev.brightness;
+ struct oxp_hid_cfg *cfg = led->cfg;
u8 val = 4 * brightness / max_brightness;
int ret;
- if (READ_ONCE(cfg->suspended) || READ_ONCE(cfg->removing))
- return;
-
guard(mutex)(&cfg->rgb_mutex);
- if (cfg->rgb_brightness != val) {
- ret = oxp_rgb_status_store(cfg, cfg->rgb_en, cfg->rgb_speed, val);
+ if (state->brightness != val) {
+ ret = oxp_rgb_status_store(led, state->enabled, state->speed, val);
if (ret)
- dev_err(cfg->led_mc->led_cdev.dev,
+ dev_err(led->mc_cdev.led_cdev.dev,
"Error: Failed to write RGB Status: %i\n", ret);
- cfg->rgb_brightness = val;
+ state->brightness = val;
}
- if (cfg->rgb_effect != OXP_EFFECT_MONO_LIST)
+ if (state->effect != OXP_EFFECT_MONO_LIST)
return;
- ret = oxp_rgb_effect_set(cfg, cfg->rgb_effect);
+ ret = oxp_rgb_effect_set(led, state->effect);
if (ret)
- dev_err(cfg->led_mc->led_cdev.dev, "Error: Failed to write RGB color: %i\n",
- ret);
+ dev_err(led->mc_cdev.led_cdev.dev,
+ "Error: Failed to write RGB color: %i\n", ret);
+}
+
+static void oxp_rgb_queue_fn(struct work_struct *work)
+{
+ struct oxp_rgb_led *led = container_of(to_delayed_work(work),
+ struct oxp_rgb_led, work);
+ struct oxp_hid_cfg *cfg = led->cfg;
+
+ if (READ_ONCE(cfg->suspended) || READ_ONCE(cfg->removing))
+ return;
+
+ switch (led->type) {
+ case OXP_RGB_FULL:
+ oxp_rgb_full_queue(led, oxp_rgb_full_state(led));
+ break;
+ }
}
static void oxp_rgb_brightness_set(struct led_classdev *led_cdev,
enum led_brightness brightness)
{
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);
+ struct oxp_rgb_led *led = container_of(mc_cdev, struct oxp_rgb_led,
+ mc_cdev);
+ struct oxp_hid_cfg *cfg = led->cfg;
if (READ_ONCE(cfg->suspended) || READ_ONCE(cfg->removing))
return;
led_cdev->brightness = brightness;
- mod_delayed_work(system_dfl_wq, &cfg->oxp_rgb_queue, msecs_to_jiffies(50));
+ mod_delayed_work(system_dfl_wq, &led->work, msecs_to_jiffies(50));
}
static struct attribute *oxp_rgb_attrs[] = {
@@ -1535,37 +1631,121 @@ static const struct attribute_group oxp_rgb_attr_group = {
.attrs = oxp_rgb_attrs,
};
-static const struct mc_subled oxp_rgb_subled_info[] = {
+static const struct oxp_rgb_led_desc oxp_rgb_led_descs[] = {
{
+ .name = "oxp:rgb:joystick_rings",
+ .type = OXP_RGB_FULL,
+ },
+};
+
+static int oxp_rgb_led_init(struct oxp_hid_cfg *cfg, struct oxp_rgb_led *led,
+ const struct oxp_rgb_led_desc *desc)
+{
+ struct oxp_rgb_full_state *full_state;
+ struct hid_device *hdev = cfg->hdev;
+ u8 green;
+ u8 blue;
+ u8 red;
+
+ led->cfg = cfg;
+ led->type = desc->type;
+
+ switch (led->type) {
+ case OXP_RGB_FULL:
+ full_state = devm_kzalloc(&hdev->dev, sizeof(*full_state),
+ GFP_KERNEL);
+ if (!full_state)
+ return -ENOMEM;
+ led->state = full_state;
+ led->mc_cdev.led_cdev.brightness = 0x64;
+ red = 0x24;
+ green = 0x22;
+ blue = 0x99;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ led->subled_info[0] = (struct mc_subled) {
.color_index = LED_COLOR_ID_RED,
- .intensity = 0x24,
+ .intensity = red,
.max_intensity = 0xff,
.channel = 0x1,
- },
- {
+ };
+ led->subled_info[1] = (struct mc_subled) {
.color_index = LED_COLOR_ID_GREEN,
- .intensity = 0x22,
+ .intensity = green,
.max_intensity = 0xff,
.channel = 0x2,
- },
- {
+ };
+ led->subled_info[2] = (struct mc_subled) {
.color_index = LED_COLOR_ID_BLUE,
- .intensity = 0x99,
+ .intensity = blue,
.max_intensity = 0xff,
.channel = 0x3,
- },
-};
+ };
+ led->mc_cdev.led_cdev.name = desc->name;
+ led->mc_cdev.led_cdev.color = LED_COLOR_ID_RGB;
+ led->mc_cdev.led_cdev.max_brightness = 0x64;
+ led->mc_cdev.led_cdev.brightness_set = oxp_rgb_brightness_set;
+ led->mc_cdev.num_colors = ARRAY_SIZE(led->subled_info);
+ led->mc_cdev.subled_info = led->subled_info;
+ INIT_DELAYED_WORK(&led->work, oxp_rgb_queue_fn);
-static const struct led_classdev_mc oxp_cdev_rgb = {
- .led_cdev = {
- .name = "oxp:rgb:joystick_rings",
- .color = LED_COLOR_ID_RGB,
- .brightness = 0x64,
- .max_brightness = 0x64,
- .brightness_set = oxp_rgb_brightness_set,
- },
- .num_colors = ARRAY_SIZE(oxp_rgb_subled_info),
-};
+ return 0;
+}
+
+static int oxp_rgb_leds_register(struct oxp_hid_cfg *cfg)
+{
+ int led_count = ARRAY_SIZE(oxp_rgb_led_descs);
+ struct hid_device *hdev = cfg->hdev;
+ struct oxp_rgb_led *led;
+ int ret;
+ int i;
+
+ cfg->rgb_leds = devm_kcalloc(&hdev->dev, led_count,
+ sizeof(*cfg->rgb_leds), GFP_KERNEL);
+ if (!cfg->rgb_leds)
+ return -ENOMEM;
+
+ for (i = 0; i < led_count; i++) {
+ led = &cfg->rgb_leds[i];
+ ret = oxp_rgb_led_init(cfg, led, &oxp_rgb_led_descs[i]);
+ if (ret)
+ return ret;
+ cfg->rgb_led_count++;
+
+ ret = devm_led_classdev_multicolor_register(&hdev->dev,
+ &led->mc_cdev);
+ if (ret)
+ return dev_err_probe(&hdev->dev, ret,
+ "Failed to create RGB device\n");
+
+ ret = devm_device_add_group(led->mc_cdev.led_cdev.dev,
+ &oxp_rgb_attr_group);
+ if (ret)
+ return dev_err_probe(led->mc_cdev.led_cdev.dev, ret,
+ "Failed to create RGB configuration attributes\n");
+ }
+
+ return 0;
+}
+
+static void oxp_rgb_disable_works(struct oxp_hid_cfg *cfg)
+{
+ int i;
+
+ for (i = 0; i < cfg->rgb_led_count; i++)
+ disable_delayed_work_sync(&cfg->rgb_leds[i].work);
+}
+
+static void oxp_rgb_enable_works(struct oxp_hid_cfg *cfg)
+{
+ int i;
+
+ for (i = 0; i < cfg->rgb_led_count; i++)
+ enable_delayed_work(&cfg->rgb_leds[i].work);
+}
static struct quirk_entry quirk_hybrid_mcu = {
.hybrid_mcu = true,
@@ -1644,8 +1824,7 @@ static void oxp_quiesce_work(struct oxp_hid_cfg *cfg)
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);
+ oxp_rgb_disable_works(cfg);
if (cfg->gen2_work_initialized) {
disable_delayed_work_sync(&cfg->oxp_btn_queue);
disable_delayed_work_sync(&cfg->oxp_mcu_init);
@@ -1680,6 +1859,7 @@ static int oxp_cfg_probe(struct hid_device *hdev, u16 up,
{
struct oxp_bmap_page_1 *bmap_1;
struct oxp_bmap_page_2 *bmap_2;
+ struct oxp_rgb_led *rgb_led;
struct oxp_hid_cfg *cfg;
int ret;
@@ -1701,31 +1881,14 @@ static int oxp_cfg_probe(struct hid_device *hdev, u16 up,
if (up == GEN2_USAGE_PAGE && quirks && quirks->hybrid_mcu)
goto skip_rgb;
- cfg->cdev = oxp_cdev_rgb;
- memcpy(cfg->subled_info, oxp_rgb_subled_info, sizeof(cfg->subled_info));
- cfg->cdev.subled_info = cfg->subled_info;
- cfg->led_mc = &cfg->cdev;
-
- INIT_DELAYED_WORK(&cfg->oxp_rgb_queue, oxp_rgb_queue_fn);
- cfg->rgb_work_initialized = true;
- ret = devm_led_classdev_multicolor_register(&hdev->dev, cfg->led_mc);
- if (ret) {
- dev_err_probe(&hdev->dev, ret,
- "Failed to create RGB device\n");
- goto err_quiesce;
- }
-
- ret = devm_device_add_group(cfg->led_mc->led_cdev.dev,
- &oxp_rgb_attr_group);
- if (ret) {
- dev_err_probe(cfg->led_mc->led_cdev.dev, ret,
- "Failed to create RGB configuration attributes\n");
+ ret = oxp_rgb_leds_register(cfg);
+ if (ret)
goto err_quiesce;
- }
- ret = oxp_rgb_status_show(cfg);
+ rgb_led = oxp_rgb_led_by_type(cfg, OXP_RGB_FULL);
+ ret = oxp_rgb_status_show(rgb_led);
if (ret)
- dev_warn(cfg->led_mc->led_cdev.dev,
+ dev_warn(rgb_led->mc_cdev.led_cdev.dev,
"Failed to query RGB initial state: %i\n", ret);
/* Below features are only implemented in gen 2 */
@@ -1841,8 +2004,7 @@ static int __maybe_unused oxp_hid_suspend(struct hid_device *hdev,
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);
+ oxp_rgb_disable_works(cfg);
if (cfg->gen2_work_initialized) {
disable_delayed_work_sync(&cfg->oxp_btn_queue);
disable_delayed_work_sync(&cfg->oxp_mcu_init);
@@ -1860,8 +2022,7 @@ static int __maybe_unused oxp_hid_resume(struct hid_device *hdev)
READ_ONCE(cfg->removing))
return 0;
- if (cfg->rgb_work_initialized)
- enable_delayed_work(&cfg->oxp_rgb_queue);
+ oxp_rgb_enable_works(cfg);
if (cfg->gen2_work_initialized) {
enable_delayed_work(&cfg->oxp_btn_queue);
enable_delayed_work(&cfg->oxp_mcu_init);
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 13/15] HID: hid-oxp: represent RGB LEDs with a common array
2026-09-10 3:21 ` [PATCH 13/15] HID: hid-oxp: represent RGB LEDs with a common array Andrei Aldea
@ 2026-09-10 3:44 ` sashiko-bot
0 siblings, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-09-10 3:44 UTC (permalink / raw)
To: Andrei Aldea; +Cc: linux-leds, lee, linux-input, dmitry.torokhov
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Invalid cast to `usb_interface` without validating the transport via `hid_is_usb()`.
--
commit 4292fdde3cbe4b16ea52c7763677a6325b071bca
Author: Andrei Aldea <andrei1998@gmail.com>
HID: hid-oxp: represent RGB LEDs with a common array
Move the existing joystick-ring class device, color components, delayed
work, and cached settings into a per-LED wrapper owned by each HID
configuration. Use a tagged state pointer so later LED types can share
registration and work management without duplicating the lifecycle.
> diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
> index 48fa9169b84c7..55f8b4773e334 100644
> --- a/drivers/hid/hid-oxp.c
> +++ b/drivers/hid/hid-oxp.c
[Severity: High]
This isn't a bug introduced by this patch (it was added earlier in the
series in patch 12/15), but it remains unfixed in the final state.
Does this code perform an invalid cast if the device is a virtual HID device?
In oxp_is_cfg_interface():
if (hdev->bus != BUS_USB)
return false;
intf = to_usb_interface(hdev->dev.parent);
If a malicious or buggy user-space application (e.g. via uhid) creates a
virtual HID device with BUS_USB, the parent device is the uhid character
device, not a USB interface.
Since the code does not validate the transport via hid_is_usb(), doesn't
this unconditional cast lead to type confusion and an out-of-bounds memory
read?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260910032115.28669-1-andrei1998@gmail.com?part=13
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 14/15] HID: hid-oxp: add Gen3 joystick ring RGB support
2026-09-10 3:21 [PATCH 00/15] HID: hid-oxp: fix and extend X2-family controller support Andrei Aldea
` (12 preceding siblings ...)
2026-09-10 3:21 ` [PATCH 13/15] HID: hid-oxp: represent RGB LEDs with a common array Andrei Aldea
@ 2026-09-10 3:21 ` Andrei Aldea
2026-09-10 3:43 ` sashiko-bot
2026-09-10 3:21 ` [PATCH 15/15] HID: hid-oxp: add X2 auxiliary RGB zones Andrei Aldea
14 siblings, 1 reply; 30+ messages in thread
From: Andrei Aldea @ 2026-09-10 3:21 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Derek J. Clark
Cc: linux-input, linux-kernel, Lee Jones, Pavel Machek, linux-leds
Select the X2/Gen3 RGB protocol early for the ONEXPLAYER 3 and X2 Mini Pro.
These controllers share the Gen2 HID usage page but require a 59-byte
color payload and writes to ring zones 1, 2 and 7. Keep the Gen1 55-byte
and legacy Gen2 57-byte color builders separate and unchanged.
Add dedicated Gen3 status, color, effect and brightness paths. Check its
known command/zone acknowledgments, retain cached color and effect when
status cannot report them, and retry one complete ring pass on failure.
Legacy transport success does not depend on receiving an acknowledgment.
Restore cached X2 RGB once after the final controller-mode change. Generic
ACK discrimination and suspend work management are preceding fixes; this
patch does not add RGB replay to older controllers.
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 | 464 ++++++++++++++++++++++++++++++++++++------
1 file changed, 398 insertions(+), 66 deletions(-)
diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index 55f8b47..9a46602 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -25,8 +25,8 @@
#include "hid-ids.h"
#define OXP_PACKET_SIZE 64
-#define OXP_STATUS_HEADER_SIZE 6
-#define OXP_STATUS_ACK 0x20
+#define OXP_GEN2_EVENT_HEADER_SIZE 6
+#define OXP_GEN2_RGB_ACK 0x20
#define GEN1_MESSAGE_ID 0xff
#define GEN2_MESSAGE_ID 0x3f
@@ -214,19 +214,20 @@ struct oxp_hid_cfg {
struct oxp_bmap_page_1 *bmap_1;
struct oxp_bmap_page_2 *bmap_2;
bool gen2_work_initialized;
- bool bmap_page_3;
u8 rumble_intensity;
+ bool bmap_page_3;
u8 gamepad_mode;
u8 bmap_format;
/* RGB state */
struct oxp_rgb_led *rgb_leds;
- spinlock_t rgb_reply_lock;
+ spinlock_t rgb_reply_lock; /* Protect pending RGB acknowledgment state. */
struct mutex rgb_mutex; /*serialize complete RGB transactions*/
bool rgb_reply_pending;
u8 rgb_reply_command;
u8 rgb_reply_zone;
u8 rgb_led_count;
+ bool x2_rgb;
};
enum oxp_gamepad_mode_index {
@@ -315,6 +316,15 @@ struct oxp_gen_1_rgb_report {
u8 blue;
} __packed;
+struct oxp_gen_2_event_header {
+ u8 report_id;
+ u8 message_id;
+ u8 padding;
+ u8 command;
+ u8 zone;
+ u8 status;
+} __packed;
+
struct oxp_gen_2_rgb_report {
u8 report_id;
u8 header_id;
@@ -331,6 +341,21 @@ struct oxp_gen_2_rgb_report {
u8 effect;
} __packed;
+struct oxp_rgb_color {
+ u8 red;
+ u8 green;
+ u8 blue;
+} __packed;
+
+struct oxp_gen_3_rgb_color_report {
+ u8 effect;
+ u8 zone;
+ u8 mode;
+ struct oxp_rgb_color colors[18];
+ u8 final_red;
+ u8 final_green;
+} __packed;
+
enum oxp_rgb_type {
OXP_RGB_FULL,
};
@@ -361,10 +386,11 @@ struct oxp_attr {
};
struct quirk_entry {
- bool hybrid_mcu;
- bool bmap_page_3;
u8 cfg_interface_num;
+ bool bmap_page_3;
+ bool hybrid_mcu;
u8 bmap_format;
+ bool x2_rgb;
};
static u16 get_usage_page(struct hid_device *hdev)
@@ -443,6 +469,51 @@ static int oxp_gen_2_property_out(struct oxp_hid_cfg *cfg,
static int oxp_set_buttons(struct oxp_hid_cfg *cfg);
static int oxp_rumble_intensity_set(struct oxp_hid_cfg *cfg, u8 intensity);
+static int oxp_rgb_status_store(struct oxp_rgb_led *led, u8 enabled, u8 speed,
+ u8 brightness);
+static int oxp_rgb_effect_set(struct oxp_rgb_led *led, u8 effect);
+
+static void oxp_rgb_restore(struct oxp_hid_cfg *cfg)
+{
+ struct oxp_rgb_full_state *state;
+ struct oxp_rgb_led *led;
+ int ret;
+ int i;
+
+ if (!cfg->x2_rgb)
+ return;
+
+ guard(mutex)(&cfg->rgb_mutex);
+ for (i = 0; i < cfg->rgb_led_count; i++) {
+ led = &cfg->rgb_leds[i];
+ switch (led->type) {
+ case OXP_RGB_FULL:
+ state = led->state;
+ if (state->effect == OXP_UNKNOWN)
+ break;
+
+ ret = oxp_rgb_status_store(led, state->enabled,
+ state->speed,
+ state->brightness);
+ if (ret) {
+ dev_err(&cfg->hdev->dev,
+ "Error: Failed to restore RGB status: %i\n",
+ ret);
+ break;
+ }
+ if (state->enabled == OXP_FEAT_DISABLED)
+ break;
+
+ ret = oxp_rgb_effect_set(led, state->effect);
+ if (ret)
+ dev_err(&cfg->hdev->dev,
+ "Error: Failed to restore RGB effect: %i\n",
+ ret);
+ break;
+ }
+ }
+}
+
static void oxp_mcu_init_fn(struct work_struct *work)
{
struct oxp_hid_cfg *cfg = container_of(to_delayed_work(work),
@@ -459,92 +530,100 @@ static void oxp_mcu_init_fn(struct work_struct *work)
dev_err(&cfg->hdev->dev,
"Error: Failed to set button mapping: %i\n", ret);
- /* Cycle the gamepad mode */
- ret = oxp_gen_2_property_out(cfg, OXP_FID_GEN2_TOGGLE_MODE, gp_mode_data, 3);
+ /* Cycle the gamepad mode before restoring lighting. */
+ ret = oxp_gen_2_property_out(cfg, OXP_FID_GEN2_TOGGLE_MODE,
+ gp_mode_data, sizeof(gp_mode_data));
if (ret)
dev_err(&cfg->hdev->dev,
"Error: Failed to set gamepad mode: %i\n", ret);
- /* Remainder only applies for xinput mode */
+ if (cfg->gamepad_mode != OXP_GP_MODE_DEBUG) {
+ gp_mode_data[0] = OXP_GP_MODE_XINPUT;
+ ret = oxp_gen_2_property_out(cfg, OXP_FID_GEN2_TOGGLE_MODE,
+ gp_mode_data, sizeof(gp_mode_data));
+ if (ret)
+ dev_err(&cfg->hdev->dev,
+ "Error: Failed to set gamepad mode: %i\n", ret);
+ }
+
+ /* The final mode change must precede the single RGB restore. */
+ oxp_rgb_restore(cfg);
if (cfg->gamepad_mode == OXP_GP_MODE_DEBUG)
return;
- gp_mode_data[0] = OXP_GP_MODE_XINPUT;
- ret = oxp_gen_2_property_out(cfg, OXP_FID_GEN2_TOGGLE_MODE, gp_mode_data, 3);
- if (ret)
- dev_err(&cfg->hdev->dev,
- "Error: Failed to set gamepad mode: %i\n", ret);
-
- /* Set vibration level */
ret = oxp_rumble_intensity_set(cfg, cfg->rumble_intensity);
if (ret)
dev_err(&cfg->hdev->dev,
"Error: Failed to set rumble intensity: %i\n", ret);
}
-static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
- struct hid_report *report, u8 *data,
- int size)
+static bool oxp_gen_2_rgb_event(struct oxp_hid_cfg *cfg,
+ const struct oxp_gen_2_event_header *header)
{
- struct oxp_hid_cfg *cfg = hid_get_drvdata(hdev);
- struct oxp_rgb_led *led = oxp_rgb_led_by_type(cfg, OXP_RGB_FULL);
- struct oxp_rgb_full_state *state = oxp_rgb_full_state(led);
- struct oxp_gen_2_rgb_report *rgb_rep;
- struct led_classdev_mc *led_mc;
bool solicited = false;
- if (size < OXP_STATUS_HEADER_SIZE)
- return 0;
-
- 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) {
+ if (header->status == OXP_GEN2_RGB_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) {
+ header->command == cfg->rgb_reply_command &&
+ header->zone == cfg->rgb_reply_zone) {
cfg->rgb_reply_pending = false;
solicited = true;
}
}
if (solicited)
- return 0;
+ return true;
}
- /* 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));
- return 0;
- }
+ /* A color-write acknowledgment must not be treated as an MCU reset. */
+ if (header->command != OXP_EFFECT_MONO_TRUE)
+ return false;
+
+ /* An unsolicited event is sent after the MCU resets on resume. */
+ 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));
+
+ return true;
+}
+
+static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
+ struct hid_report *report, u8 *data,
+ int size)
+{
+ struct oxp_hid_cfg *cfg = hid_get_drvdata(hdev);
+ struct oxp_rgb_led *led = oxp_rgb_led_by_type(cfg, OXP_RGB_FULL);
+ struct oxp_rgb_full_state *state = oxp_rgb_full_state(led);
+ struct oxp_gen_2_event_header *header = (void *)data;
+ struct oxp_gen_2_rgb_report *rgb_rep;
- if (data[3] != OXP_GET_PROPERTY)
+ if (size < OXP_GEN2_EVENT_HEADER_SIZE)
+ return 0;
+ if (header->report_id != OXP_FID_GEN2_STATUS_EVENT)
+ return 0;
+ if (oxp_gen_2_rgb_event(cfg, header))
+ return 0;
+ if (header->command != OXP_GET_PROPERTY)
return 0;
if (size < sizeof(*rgb_rep) || !state)
return 0;
- led_mc = &led->mc_cdev;
rgb_rep = (struct oxp_gen_2_rgb_report *)data;
if (rgb_rep->enabled > OXP_FEAT_ENABLED || rgb_rep->speed > 9 ||
rgb_rep->brightness > 4)
return 0;
- /* Ensure we save monocolor as the list value */
+ /* Ensure we save monocolor as the list value. */
state->effect = rgb_rep->effect == OXP_EFFECT_MONO_TRUE ?
OXP_EFFECT_MONO_LIST : rgb_rep->effect;
state->speed = rgb_rep->speed;
state->enabled = rgb_rep->enabled == 0 ? OXP_FEAT_DISABLED :
- OXP_FEAT_ENABLED;
+ OXP_FEAT_ENABLED;
state->brightness = rgb_rep->brightness;
- led_mc->led_cdev.brightness = rgb_rep->brightness *
- led_mc->led_cdev.max_brightness / 4;
+ led->mc_cdev.led_cdev.brightness = rgb_rep->brightness *
+ led->mc_cdev.led_cdev.max_brightness;
+ led->mc_cdev.led_cdev.brightness /= 4;
/* If monocolor had less than 100% brightness on the previous boot,
* there will be no reliable way to determine the real intensity.
* Since intensity scaling is used with a hardware brightness set at max,
@@ -552,9 +631,52 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
* prevent successive boots from lowering the brightness further.
* Brightness will be "wrong" but the effect will remain the same visually.
*/
- led_mc->subled_info[0].intensity = rgb_rep->red;
- led_mc->subled_info[1].intensity = rgb_rep->green;
- led_mc->subled_info[2].intensity = rgb_rep->blue;
+ led->mc_cdev.subled_info[0].intensity = rgb_rep->red;
+ led->mc_cdev.subled_info[1].intensity = rgb_rep->green;
+ led->mc_cdev.subled_info[2].intensity = rgb_rep->blue;
+
+ return 0;
+}
+
+static int oxp_hid_raw_event_gen_3(struct oxp_hid_cfg *cfg,
+ struct hid_report *report, u8 *data,
+ int size)
+{
+ struct oxp_rgb_led *led = oxp_rgb_led_by_type(cfg, OXP_RGB_FULL);
+ struct oxp_rgb_full_state *state = oxp_rgb_full_state(led);
+ struct oxp_gen_2_event_header *header = (void *)data;
+ struct oxp_gen_2_rgb_report *rgb_rep;
+
+ if (size < OXP_GEN2_EVENT_HEADER_SIZE)
+ return 0;
+ if (header->report_id != OXP_FID_GEN2_STATUS_EVENT)
+ return 0;
+ if (oxp_gen_2_rgb_event(cfg, header))
+ return 0;
+ if (header->command != OXP_GET_PROPERTY)
+ return 0;
+ if (size < sizeof(*rgb_rep) || !state)
+ return 0;
+
+ rgb_rep = (struct oxp_gen_2_rgb_report *)data;
+ if (rgb_rep->enabled > OXP_FEAT_ENABLED || rgb_rep->speed > 9 ||
+ rgb_rep->brightness > 4)
+ return 0;
+
+ /* Gen3 status replies do not report the current effect or RGB color. */
+ state->speed = rgb_rep->speed;
+ state->enabled = rgb_rep->enabled == 0 ? OXP_FEAT_DISABLED :
+ OXP_FEAT_ENABLED;
+ /*
+ * Monocolor uses scaled channels at maximum hardware brightness.
+ * Retain the requested brightness rather than replacing it with 100%.
+ */
+ if (state->effect != OXP_EFFECT_MONO_LIST) {
+ state->brightness = rgb_rep->brightness;
+ led->mc_cdev.led_cdev.brightness = rgb_rep->brightness *
+ led->mc_cdev.led_cdev.max_brightness;
+ led->mc_cdev.led_cdev.brightness /= 4;
+ }
return 0;
}
@@ -574,6 +696,9 @@ static int oxp_hid_raw_event(struct hid_device *hdev, struct hid_report *report,
case GEN1_USAGE_PAGE:
return oxp_hid_raw_event_gen_1(hdev, report, data, size);
case GEN2_USAGE_PAGE:
+ /* X2/Gen3 controllers share the Gen2 HID usage page. */
+ if (cfg->x2_rgb)
+ return oxp_hid_raw_event_gen_3(cfg, report, data, size);
return oxp_hid_raw_event_gen_2(hdev, report, data, size);
default:
break;
@@ -584,9 +709,11 @@ static int oxp_hid_raw_event(struct hid_device *hdev, struct hid_report *report,
static int mcu_property_out(struct oxp_hid_cfg *cfg, u8 *header,
size_t header_size, u8 *data, size_t data_size,
- u8 *footer, size_t footer_size)
+ u8 *footer, size_t footer_size, bool expect_rgb_ack)
{
unsigned char *dmabuf __free(kfree) = kzalloc(OXP_PACKET_SIZE, GFP_KERNEL);
+ u8 rgb_command = 0;
+ u8 rgb_zone = 0;
bool rgb_write;
int ret;
@@ -602,13 +729,21 @@ static int mcu_property_out(struct oxp_hid_cfg *cfg, u8 *header,
if (READ_ONCE(cfg->suspended))
return -EHOSTDOWN;
+ /*
+ * Track legacy monocolor replies to distinguish them from MCU resets.
+ * Only Gen3's known acknowledgment protocol makes a missing reply an
+ * error; retain transport-only success semantics for older controllers.
+ */
rgb_write = header_size && data_size > 1 &&
header[0] == OXP_FID_GEN2_STATUS_EVENT &&
- data[0] == OXP_EFFECT_MONO_TRUE;
+ data[0] != OXP_GET_PROPERTY &&
+ (expect_rgb_ack || data[0] == OXP_EFFECT_MONO_TRUE);
if (rgb_write) {
+ rgb_command = data[0];
+ rgb_zone = data[1];
scoped_guard(spinlock_irqsave, &cfg->rgb_reply_lock) {
- cfg->rgb_reply_command = data[0];
- cfg->rgb_reply_zone = data[1];
+ cfg->rgb_reply_command = rgb_command;
+ cfg->rgb_reply_zone = rgb_zone;
cfg->rgb_reply_pending = true;
}
}
@@ -628,7 +763,13 @@ static int mcu_property_out(struct oxp_hid_cfg *cfg, u8 *header,
if (rgb_write) {
scoped_guard(spinlock_irqsave, &cfg->rgb_reply_lock) {
- cfg->rgb_reply_pending = false;
+ if (cfg->rgb_reply_pending &&
+ cfg->rgb_reply_command == rgb_command &&
+ cfg->rgb_reply_zone == rgb_zone) {
+ if (!ret && expect_rgb_ack)
+ ret = -ETIMEDOUT;
+ cfg->rgb_reply_pending = false;
+ }
}
}
@@ -642,7 +783,8 @@ static int oxp_gen_1_property_out(struct oxp_hid_cfg *cfg,
u8 header[] = { fid, GEN1_MESSAGE_ID };
size_t header_size = ARRAY_SIZE(header);
- return mcu_property_out(cfg, header, header_size, data, data_size, NULL, 0);
+ return mcu_property_out(cfg, header, header_size, data, data_size,
+ NULL, 0, false);
}
static int oxp_gen_2_property_out(struct oxp_hid_cfg *cfg,
@@ -655,7 +797,65 @@ static int oxp_gen_2_property_out(struct oxp_hid_cfg *cfg,
size_t footer_size = ARRAY_SIZE(footer);
return mcu_property_out(cfg, header, header_size, data, data_size, footer,
- footer_size);
+ footer_size, false);
+}
+
+static const u8 oxp_x2_rgb_zones[] = { 0x01, 0x02, 0x07 };
+
+static int oxp_gen_3_property_out(struct oxp_hid_cfg *cfg, u8 *data,
+ u8 data_size)
+{
+ u8 header[] = { OXP_FID_GEN2_STATUS_EVENT, GEN2_MESSAGE_ID, 0x01 };
+ u8 footer[] = { GEN2_MESSAGE_ID, OXP_FID_GEN2_STATUS_EVENT };
+ size_t header_size = ARRAY_SIZE(header);
+ size_t footer_size = ARRAY_SIZE(footer);
+
+ return mcu_property_out(cfg, header, header_size, data, data_size,
+ footer, footer_size, true);
+}
+
+static int oxp_gen_3_rgb_property_out(struct oxp_hid_cfg *cfg, u8 *data,
+ u8 data_size)
+{
+ int first_err;
+ int attempt;
+ int ret;
+ int i;
+
+ /*
+ * Each zone is addressed independently. Complete the pass even if one
+ * write fails so the remaining ring zones still receive the setting.
+ * Retry the full pass once to keep their requested settings identical.
+ */
+ for (attempt = 0; attempt < 2; attempt++) {
+ first_err = 0;
+ for (i = 0; i < ARRAY_SIZE(oxp_x2_rgb_zones); i++) {
+ data[1] = oxp_x2_rgb_zones[i];
+ ret = oxp_gen_3_property_out(cfg, data, data_size);
+ if (ret && !first_err)
+ first_err = ret;
+ }
+ if (!first_err)
+ return 0;
+ }
+
+ return first_err;
+}
+
+static void oxp_gen_3_rgb_fill_color(struct oxp_gen_3_rgb_color_report *report,
+ u8 command, u8 zone, u8 red, u8 green,
+ u8 blue)
+{
+ int i;
+
+ memset(report, 0, sizeof(*report));
+ report->effect = command;
+ report->zone = zone;
+ report->mode = 0x02;
+ for (i = 0; i < ARRAY_SIZE(report->colors); i++)
+ report->colors[i] = (struct oxp_rgb_color) { red, green, blue };
+ report->final_red = red;
+ report->final_green = green;
}
static ssize_t gamepad_mode_store(struct device *dev,
@@ -1206,6 +1406,20 @@ static const struct attribute_group oxp_cfg_attrs_group = {
.attrs = oxp_cfg_attrs,
};
+static int oxp_gen_3_rgb_status_store(struct oxp_rgb_led *led, u8 enabled,
+ u8 speed, u8 brightness)
+{
+ struct oxp_rgb_full_state *state = oxp_rgb_full_state(led);
+ u8 data[6] = { OXP_SET_PROPERTY, 0x00, 0x02, enabled, speed, brightness };
+
+ if (!state)
+ return -ENODEV;
+ if (state->effect == OXP_EFFECT_MONO_LIST)
+ data[5] = 0x04;
+
+ return oxp_gen_3_rgb_property_out(led->cfg, data, sizeof(data));
+}
+
static int oxp_rgb_status_store(struct oxp_rgb_led *led, u8 enabled, u8 speed,
u8 brightness)
{
@@ -1214,6 +1428,11 @@ static int oxp_rgb_status_store(struct oxp_rgb_led *led, u8 enabled, u8 speed,
u16 up = get_usage_page(cfg->hdev);
u8 *data;
+ if (!state)
+ return -ENODEV;
+ if (cfg->x2_rgb)
+ return oxp_gen_3_rgb_status_store(led, enabled, speed, brightness);
+
/* Always default to max brightness and use intensity scaling when in
* monocolor mode.
*/
@@ -1233,13 +1452,25 @@ static int oxp_rgb_status_store(struct oxp_rgb_led *led, u8 enabled, u8 speed,
}
}
+static int oxp_gen_3_rgb_status_show(struct oxp_hid_cfg *cfg)
+{
+ u8 data[3] = { OXP_GET_PROPERTY, 0x07, 0x02 };
+
+ return oxp_gen_3_property_out(cfg, data, sizeof(data));
+}
+
static ssize_t oxp_rgb_status_show(struct oxp_rgb_led *led)
{
struct oxp_hid_cfg *cfg = led->cfg;
u16 up = get_usage_page(cfg->hdev);
u8 *data;
+ if (!oxp_rgb_full_state(led))
+ return -ENODEV;
+
guard(mutex)(&cfg->rgb_mutex);
+ if (cfg->x2_rgb)
+ return oxp_gen_3_rgb_status_show(cfg);
switch (up) {
case GEN1_USAGE_PAGE:
@@ -1253,6 +1484,22 @@ static ssize_t oxp_rgb_status_show(struct oxp_rgb_led *led)
}
}
+static int oxp_gen_3_rgb_color_set(struct oxp_rgb_led *led)
+{
+ struct led_classdev_mc *mc_cdev = &led->mc_cdev;
+ struct oxp_gen_3_rgb_color_report color_report;
+ u8 brightness = mc_cdev->led_cdev.brightness;
+
+ led_mc_calc_color_components(mc_cdev, brightness);
+ oxp_gen_3_rgb_fill_color(&color_report, OXP_EFFECT_MONO_TRUE, 0x00,
+ mc_cdev->subled_info[0].brightness,
+ mc_cdev->subled_info[1].brightness,
+ mc_cdev->subled_info[2].brightness);
+
+ return oxp_gen_3_rgb_property_out(led->cfg, (u8 *)&color_report,
+ sizeof(color_report));
+}
+
static int oxp_rgb_color_set(struct oxp_rgb_led *led)
{
struct led_classdev_mc *led_mc = &led->mc_cdev;
@@ -1264,6 +1511,9 @@ static int oxp_rgb_color_set(struct oxp_rgb_led *led)
u8 *data;
int i;
+ if (cfg->x2_rgb)
+ return oxp_gen_3_rgb_color_set(led);
+
led_mc_calc_color_components(led_mc, br);
red = led_mc->subled_info[0].brightness;
green = led_mc->subled_info[1].brightness;
@@ -1281,6 +1531,7 @@ static int oxp_rgb_color_set(struct oxp_rgb_led *led)
}
return oxp_gen_1_property_out(cfg, OXP_FID_GEN1_RGB_SET, data, size);
case GEN2_USAGE_PAGE:
+ /* Preserve the legacy payload and its two zero bytes before the footer. */
size = 57;
data = (u8[57]) { OXP_EFFECT_MONO_TRUE, 0x00, 0x02 };
@@ -1295,6 +1546,29 @@ static int oxp_rgb_color_set(struct oxp_rgb_led *led)
}
}
+static int oxp_gen_3_rgb_effect_set(struct oxp_rgb_led *led, u8 effect)
+{
+ struct oxp_rgb_full_state *state = oxp_rgb_full_state(led);
+ u8 data[3] = { effect, 0x00, 0x02 };
+ int ret;
+
+ if (!state)
+ return -ENODEV;
+
+ if (effect > OXP_UNKNOWN && effect < OXP_EFFECT_MONO_LIST)
+ ret = oxp_gen_3_rgb_property_out(led->cfg, data, sizeof(data));
+ else if (effect == OXP_EFFECT_MONO_LIST)
+ ret = oxp_gen_3_rgb_color_set(led);
+ else
+ return -EINVAL;
+
+ if (ret)
+ return ret;
+
+ state->effect = effect;
+ return 0;
+}
+
static int oxp_rgb_effect_set(struct oxp_rgb_led *led, u8 effect)
{
struct oxp_rgb_full_state *state = oxp_rgb_full_state(led);
@@ -1303,6 +1577,11 @@ static int oxp_rgb_effect_set(struct oxp_rgb_led *led, u8 effect)
u8 *data;
int ret;
+ if (!state)
+ return -ENODEV;
+ if (cfg->x2_rgb)
+ return oxp_gen_3_rgb_effect_set(led, effect);
+
switch (effect) {
case OXP_EFFECT_AURORA:
case OXP_EFFECT_BIRTHDAY:
@@ -1586,6 +1865,54 @@ static void oxp_rgb_full_queue(struct oxp_rgb_led *led,
"Error: Failed to write RGB color: %i\n", ret);
}
+static void oxp_gen_3_rgb_queue(struct oxp_rgb_led *led,
+ struct oxp_rgb_full_state *state)
+{
+ unsigned int max_brightness = led->mc_cdev.led_cdev.max_brightness;
+ u8 old_effect;
+ unsigned int brightness = led->mc_cdev.led_cdev.brightness;
+ u8 enabled = brightness ? OXP_FEAT_ENABLED : OXP_FEAT_DISABLED;
+ struct oxp_hid_cfg *cfg = led->cfg;
+ u8 val = 4 * brightness / max_brightness;
+ int ret;
+
+ guard(mutex)(&cfg->rgb_mutex);
+ old_effect = state->effect;
+
+ if (!brightness) {
+ ret = oxp_gen_3_rgb_status_store(led, OXP_FEAT_DISABLED, 5, 0);
+ if (ret) {
+ dev_err(led->mc_cdev.led_cdev.dev,
+ "Error: Failed to disable RGB: %i\n", ret);
+ return;
+ }
+
+ state->enabled = OXP_FEAT_DISABLED;
+ state->brightness = 0;
+ state->speed = 5;
+ return;
+ }
+
+ /* Gen3 standard LED writes select solid color and follow brightness/off. */
+ state->effect = OXP_EFFECT_MONO_LIST;
+ ret = oxp_gen_3_rgb_status_store(led, enabled, 5, val);
+ if (ret) {
+ state->effect = old_effect;
+ dev_err(led->mc_cdev.led_cdev.dev,
+ "Error: Failed to write RGB status: %i\n", ret);
+ return;
+ }
+
+ state->enabled = enabled;
+ state->brightness = val;
+ state->speed = 5;
+
+ ret = oxp_gen_3_rgb_effect_set(led, OXP_EFFECT_MONO_LIST);
+ if (ret)
+ dev_err(led->mc_cdev.led_cdev.dev,
+ "Error: Failed to write RGB color: %i\n", ret);
+}
+
static void oxp_rgb_queue_fn(struct work_struct *work)
{
struct oxp_rgb_led *led = container_of(to_delayed_work(work),
@@ -1597,7 +1924,10 @@ static void oxp_rgb_queue_fn(struct work_struct *work)
switch (led->type) {
case OXP_RGB_FULL:
- oxp_rgb_full_queue(led, oxp_rgb_full_state(led));
+ if (cfg->x2_rgb)
+ oxp_gen_3_rgb_queue(led, led->state);
+ else
+ oxp_rgb_full_queue(led, led->state);
break;
}
}
@@ -1751,9 +2081,10 @@ static struct quirk_entry quirk_hybrid_mcu = {
.hybrid_mcu = true,
};
-static struct quirk_entry quirk_x2_bmap = {
+static struct quirk_entry quirk_x2 = {
.bmap_format = OXP_BMAP_FORMAT_X2,
.bmap_page_3 = true,
+ .x2_rgb = true,
.cfg_interface_num = 2,
};
@@ -1788,7 +2119,7 @@ static const struct dmi_system_id oxp_quirk_list[] = {
DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ONE-NETBOOK"),
DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "ONEXPLAYER 3"),
},
- .driver_data = &quirk_x2_bmap,
+ .driver_data = &quirk_x2,
},
{
.ident = "OneXPlayer X2 Mini Pro",
@@ -1796,7 +2127,7 @@ static const struct dmi_system_id oxp_quirk_list[] = {
DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ONE-NETBOOK"),
DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "ONEXPLAYER X2Mini PRO"),
},
- .driver_data = &quirk_x2_bmap,
+ .driver_data = &quirk_x2,
},
{},
};
@@ -1868,6 +2199,7 @@ static int oxp_cfg_probe(struct hid_device *hdev, u16 up,
return -ENOMEM;
cfg->hdev = hdev;
+ cfg->x2_rgb = quirks && quirks->x2_rgb;
mutex_init(&cfg->cfg_mutex);
mutex_init(&cfg->rgb_mutex);
spin_lock_init(&cfg->rgb_reply_lock);
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 14/15] HID: hid-oxp: add Gen3 joystick ring RGB support
2026-09-10 3:21 ` [PATCH 14/15] HID: hid-oxp: add Gen3 joystick ring RGB support Andrei Aldea
@ 2026-09-10 3:43 ` sashiko-bot
0 siblings, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-09-10 3:43 UTC (permalink / raw)
To: Andrei Aldea; +Cc: dmitry.torokhov, linux-leds, linux-input, lee
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Invalid cast to usb_interface when bound to non-USB parent.
--
commit 6855a3cf4aaf330705ad5f598a86f86db5c13fc3
Author: Andrei Aldea <andrei1998@gmail.com>
HID: hid-oxp: add Gen3 joystick ring RGB support
Select the X2/Gen3 RGB protocol early for the ONEXPLAYER 3 and X2 Mini Pro.
These controllers share the Gen2 HID usage page but require a 59-byte
color payload and writes to ring zones 1, 2 and 7. Keep the Gen1 55-byte
and legacy Gen2 57-byte color builders separate and unchanged.
> diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
> index 55f8b4773e334..9a46602238aa3 100644
> --- a/drivers/hid/hid-oxp.c
> +++ b/drivers/hid/hid-oxp.c
[ ... ]
> @@ -1868,6 +2199,7 @@ static int oxp_cfg_probe(struct hid_device *hdev, u16 up,
> return -ENOMEM;
>
> cfg->hdev = hdev;
> + cfg->x2_rgb = quirks && quirks->x2_rgb;
[Severity: High]
This is a pre-existing issue, but does oxp_is_cfg_interface() perform an
unsafe cast of the parent device?
During probe, before oxp_cfg_probe() is called, the driver checks:
if (!oxp_is_cfg_interface(hdev, quirks))
return 0;
Looking at oxp_is_cfg_interface(), it uses:
if (hdev->bus != BUS_USB)
return false;
intf = to_usb_interface(hdev->dev.parent);
return intf->cur_altsetting->desc.bInterfaceNumber ==
quirks->cfg_interface_num;
Could a userspace program create a UHID device spoofing the BUS_USB bus type
and target device IDs? If so, the hdev->bus != BUS_USB check would pass, but
the parent device would be a UHID device rather than a USB interface.
Does unconditionally casting a non-USB parent device to struct
usb_interface lead to an out-of-bounds memory read or kernel panic when
accessing intf->cur_altsetting->desc.bInterfaceNumber? Should this code use
hid_is_usb(hdev) to safely guard the cast?
> mutex_init(&cfg->cfg_mutex);
> mutex_init(&cfg->rgb_mutex);
> spin_lock_init(&cfg->rgb_reply_lock);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260910032115.28669-1-andrei1998@gmail.com?part=14
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 15/15] HID: hid-oxp: add X2 auxiliary RGB zones
2026-09-10 3:21 [PATCH 00/15] HID: hid-oxp: fix and extend X2-family controller support Andrei Aldea
` (13 preceding siblings ...)
2026-09-10 3:21 ` [PATCH 14/15] HID: hid-oxp: add Gen3 joystick ring RGB support Andrei Aldea
@ 2026-09-10 3:21 ` Andrei Aldea
2026-09-10 3:44 ` sashiko-bot
14 siblings, 1 reply; 30+ messages in thread
From: Andrei Aldea @ 2026-09-10 3:21 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Derek J. Clark
Cc: linux-input, linux-kernel, Lee Jones, Pavel Machek, linux-leds,
linux-api
Add separate multicolor LED devices for the X2 guide button and rear logo
using Gen3 zones 5 and 6. Each LED retains independent color, brightness,
and monocolor or breathing effect state.
Use the common LED array and type-tagged state introduced earlier.
Protect auxiliary state snapshots with scoped spinlocks, serialize output
with other RGB operations, and retry a failed command once. Do not alter
an auxiliary zone until userspace has supplied valid state, and restore
valid cached settings after resume.
Expose these zones only on the matched ONEXPLAYER 3 and X2 Mini Pro.
Physical validation of the latter remains outstanding.
Document the auxiliary effects together with the existing joystick-ring
effects and add the previously omitted controller, mapping, rumble, and
ring-lighting ABI. Add the ABI file to the ONEXPLAYER HID maintainer
pattern.
Assisted-by: LLM
Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Signed-off-by: Andrei Aldea <andrei1998@gmail.com>
---
.../ABI/testing/sysfs-driver-hid-oxp | 196 +++++++++++
MAINTAINERS | 1 +
drivers/hid/hid-oxp.c | 305 +++++++++++++++++-
3 files changed, 497 insertions(+), 5 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-oxp
diff --git a/Documentation/ABI/testing/sysfs-driver-hid-oxp b/Documentation/ABI/testing/sysfs-driver-hid-oxp
new file mode 100644
index 0000000..7c97f28
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-hid-oxp
@@ -0,0 +1,196 @@
+What: /sys/class/leds/oxp:rgb:joystick_rings/effect
+What: /sys/class/leds/oxp:rgb:guide_button/effect
+What: /sys/class/leds/oxp:rgb:rear_logo/effect
+Date: April 2026
+Contact: Derek J. Clark <derekjohn.clark@gmail.com>
+Description: Read/write attribute selecting the lighting effect for the LED.
+
+ The joystick_rings LED supports the following effect names:
+
+ ``aurora``, ``birthday_cake``, ``flowing_light``,
+ ``chroma_popping``, ``neon``, ``chroma_breathing``, ``dreamy``,
+ ``warm_sun``, ``cyberpunk``, ``sea_foam``, ``sunset_afterglow``,
+ ``colorful``, ``monster_woke``, ``green_breathing``,
+ ``blue_breathing``, ``yellow_breathing``, ``teal_breathing``,
+ ``purple_breathing``, ``foggy_haze``, and ``monocolor``.
+
+ The guide_button and rear_logo LEDs support ``monocolor`` and
+ ``breathing``. These two LEDs were added in August 2026 and are
+ available on the ONEXPLAYER 3 and ONEXPLAYER X2 Mini Pro.
+
+ The ``monocolor`` effect displays the color selected through
+ the standard multicolor LED ``multi_intensity`` attribute.
+ The auxiliary ``breathing`` effect pulses the selected color.
+ Other joystick ring effects select firmware-defined presets.
+ The standard brightness and multicolor attributes are documented
+ in Documentation/ABI/testing/sysfs-class-led and
+ Documentation/ABI/testing/sysfs-class-led-multicolor.
+
+ Read returns the selected effect name followed by a newline.
+ The value may reflect cached driver state rather than a live
+ hardware query. The joystick_rings LED may report ``unknown``
+ when no known effect has been recorded; this is not a selectable
+ effect. Auxiliary effect writes queue the update asynchronously.
+
+What: /sys/class/leds/oxp:rgb:joystick_rings/effect_index
+What: /sys/class/leds/oxp:rgb:guide_button/effect_index
+What: /sys/class/leds/oxp:rgb:rear_logo/effect_index
+Date: April 2026
+Contact: Derek J. Clark <derekjohn.clark@gmail.com>
+Description: Read-only list of effect names supported by the corresponding
+ LED, separated by spaces and terminated by a newline.
+
+ The supported names are described in the effect entry above.
+ The list is specific to each LED and does not include ``unknown``.
+ The guide_button and rear_logo attributes were added in August
+ 2026 and are available on the ONEXPLAYER 3 and ONEXPLAYER X2
+ Mini Pro.
+
+What: /sys/class/leds/oxp:rgb:joystick_rings/enabled
+Date: April 2026
+Contact: Derek J. Clark <derekjohn.clark@gmail.com>
+Description: Read/write enable state for the joystick ring lighting.
+
+ Write ``true`` to enable lighting or ``false`` to disable it.
+ Only these literal names are accepted, not numeric boolean
+ values. Read requests lighting status and returns the driver's
+ enable state as one of these names followed by a newline.
+
+ This attribute is not exposed for guide_button or rear_logo.
+
+What: /sys/class/leds/oxp:rgb:joystick_rings/enabled_index
+Date: April 2026
+Contact: Derek J. Clark <derekjohn.clark@gmail.com>
+Description: Read-only list of names accepted by enabled. Returns
+ ``false true`` followed by a newline.
+
+What: /sys/class/leds/oxp:rgb:joystick_rings/speed
+Date: April 2026
+Contact: Derek J. Clark <derekjohn.clark@gmail.com>
+Description: Read/write firmware animation-speed setting for the joystick
+ ring lighting. Write a decimal integer from 0 through 9.
+ These are firmware levels, not a duration or frequency.
+
+ Read requests lighting status and returns the driver's speed
+ setting as a decimal integer followed by a newline.
+ This attribute is not exposed for guide_button or rear_logo.
+
+What: /sys/class/leds/oxp:rgb:joystick_rings/speed_range
+Date: April 2026
+Contact: Derek J. Clark <derekjohn.clark@gmail.com>
+Description: Read-only range of values accepted by speed. Returns ``0-9``
+ followed by a newline; both endpoints are inclusive.
+
+What: /sys/bus/hid/devices/.../gamepad_mode
+Date: April 2026
+Contact: Derek J. Clark <derekjohn.clark@gmail.com>
+Description: Read/write controller mode on the HID configuration interface
+ with usage page 0xff00 (shared by Gen2 and X2/Gen3 devices). Write one of the following names:
+
+ * ``xinput``: normal gamepad input.
+ * ``debug``: route controller events to a hidraw interface for
+ input-mapping diagnostics instead of the xpad evdev interface.
+
+ Read returns the driver's cached mode name followed by a newline,
+ not a firmware query. The initial cached mode is ``xinput``.
+ Switching to ``xinput`` also reapplies the cached rumble intensity.
+
+ This attribute and the other controller configuration attributes
+ below are attached directly to the selected HID configuration device's
+ sysfs directory, not an input device or LED directory. They are
+ not exposed on Gen1 configuration interfaces.
+
+What: /sys/bus/hid/devices/.../gamepad_mode_index
+Date: April 2026
+Contact: Derek J. Clark <derekjohn.clark@gmail.com>
+Description: Read-only list of supported controller modes. Returns
+ ``xinput debug`` followed by a newline.
+
+What: /sys/bus/hid/devices/.../button_a
+What: /sys/bus/hid/devices/.../button_b
+What: /sys/bus/hid/devices/.../button_x
+What: /sys/bus/hid/devices/.../button_y
+What: /sys/bus/hid/devices/.../button_lb
+What: /sys/bus/hid/devices/.../button_rb
+What: /sys/bus/hid/devices/.../button_lt
+What: /sys/bus/hid/devices/.../button_rt
+What: /sys/bus/hid/devices/.../button_start
+What: /sys/bus/hid/devices/.../button_select
+What: /sys/bus/hid/devices/.../button_l3
+What: /sys/bus/hid/devices/.../button_r3
+What: /sys/bus/hid/devices/.../button_d_up
+What: /sys/bus/hid/devices/.../button_d_down
+What: /sys/bus/hid/devices/.../button_d_left
+What: /sys/bus/hid/devices/.../button_d_right
+What: /sys/bus/hid/devices/.../button_m1
+What: /sys/bus/hid/devices/.../button_m2
+Date: April 2026
+Contact: Derek J. Clark <derekjohn.clark@gmail.com>
+Description: Read/write output mapping for the named physical controller
+ button on a HID configuration device with usage page 0xff00.
+
+ Write one exact, case-sensitive mapping name from
+ button_mapping_options, such as ``BTN_A`` or ``KEY_F15``.
+ Numeric key codes, multiple names, and arbitrary key names
+ are not accepted.
+
+ Read returns the driver's cached mapping name followed by a
+ newline. Writes update that cache and queue the firmware update
+ asynchronously; successful writes do not confirm that the
+ hardware has already applied the mapping.
+
+What: /sys/bus/hid/devices/.../button_mapping_options
+Date: April 2026
+Contact: Derek J. Clark <derekjohn.clark@gmail.com>
+Description: Read-only list of output mapping names accepted by the
+ controller button attributes, separated by spaces and terminated by a
+ newline. All 18 button attributes accept the same list.
+
+ The gamepad mapping names are:
+
+ ``BTN_A``, ``BTN_B``, ``BTN_X``, ``BTN_Y``, ``BTN_LB``,
+ ``BTN_RB``, ``BTN_LT``, ``BTN_RT``, ``BTN_START``, ``BTN_SELECT``,
+ ``BTN_L3``, ``BTN_R3``, ``DPAD_UP``, ``DPAD_DOWN``,
+ ``DPAD_LEFT``, ``DPAD_RIGHT``, ``JOY_L_UP``, ``JOY_L_UP_RIGHT``,
+ ``JOY_L_RIGHT``, ``JOY_L_DOWN_RIGHT``, ``JOY_L_DOWN``,
+ ``JOY_L_DOWN_LEFT``, ``JOY_L_LEFT``, ``JOY_L_UP_LEFT``,
+ ``JOY_R_UP``, ``JOY_R_UP_RIGHT``, ``JOY_R_RIGHT``,
+ ``JOY_R_DOWN_RIGHT``, ``JOY_R_DOWN``, ``JOY_R_DOWN_LEFT``,
+ ``JOY_R_LEFT``, ``JOY_R_UP_LEFT``, and ``BTN_GUIDE``.
+
+ Keyboard mapping names are ``KEY_F1`` through ``KEY_F24``.
+ There are no ``BTN_M1`` or ``BTN_M2`` output mapping names;
+ button_m1 and button_m2 select from the same options as the
+ other buttons.
+
+What: /sys/bus/hid/devices/.../reset_buttons
+Date: April 2026
+Contact: Derek J. Clark <derekjohn.clark@gmail.com>
+Description: Write-only reset of the controller's button mappings.
+ Write the decimal value ``1`` to restore the driver's default
+ mappings and send them to the controller. Other values are
+ rejected.
+
+ Main controls map to their corresponding gamepad functions;
+ M1 maps to ``KEY_F15`` and M2 to ``KEY_F16``. This resets button
+ mappings only, not lighting, rumble, or other device settings.
+ It is not a factory reset. Cached mappings are reset before
+ the firmware update and are not rolled back if that update fails.
+
+What: /sys/bus/hid/devices/.../rumble_intensity
+Date: April 2026
+Contact: Derek J. Clark <derekjohn.clark@gmail.com>
+Description: Read/write rumble-intensity level on the HID configuration
+ device with usage page 0xff00. Write a decimal integer from 0 through 5. These are
+ firmware levels, not percentages or values in physical units.
+
+ Read returns the driver's cached level as a decimal integer
+ followed by a newline. The initial cached value is 5. A write
+ sends the firmware command before updating the cached level.
+
+What: /sys/bus/hid/devices/.../rumble_intensity_range
+Date: April 2026
+Contact: Derek J. Clark <derekjohn.clark@gmail.com>
+Description: Read-only range of values accepted by the rumble_intensity
+ attribute. Returns ``0-5`` followed by a newline; both endpoints
+ are inclusive.
diff --git a/MAINTAINERS b/MAINTAINERS
index c392ded..fe5a2f6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20381,6 +20381,7 @@ ONEXPLAYER HID DRIVER
M: Derek J. Clark <derekjohn.clark@gmail.com>
L: linux-input@vger.kernel.org
S: Maintained
+F: Documentation/ABI/testing/sysfs-driver-hid-oxp
F: drivers/hid/hid-oxp.c
ONEXPLAYER PLATFORM EC DRIVER
diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index 9a46602..757cef8 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -45,6 +45,7 @@
#define OXP_GET_PROPERTY 0xfc
#define OXP_SET_PROPERTY 0xfd
#define OXP_EFFECT_MONO_TRUE 0xfe /* actual index for monocolor */
+#define OXP_EFFECT_BREATHING_TRUE 0xf0
#define OXP_DEVICE_ATTR_RW(_name, _group) \
static ssize_t _name##_store(struct device *dev, \
@@ -358,6 +359,17 @@ struct oxp_gen_3_rgb_color_report {
enum oxp_rgb_type {
OXP_RGB_FULL,
+ OXP_RGB_AUX,
+};
+
+enum oxp_rgb_aux_effect {
+ OXP_RGB_AUX_EFFECT_MONOCOLOR,
+ OXP_RGB_AUX_EFFECT_BREATHING,
+};
+
+static const char *const oxp_rgb_aux_effect_text[] = {
+ [OXP_RGB_AUX_EFFECT_MONOCOLOR] = "monocolor",
+ [OXP_RGB_AUX_EFFECT_BREATHING] = "breathing",
};
struct oxp_rgb_full_state {
@@ -367,18 +379,30 @@ struct oxp_rgb_full_state {
u8 speed;
};
+struct oxp_rgb_aux_state {
+ enum led_brightness brightness;
+ bool valid;
+ u8 effect;
+ u8 green;
+ u8 blue;
+ u8 red;
+};
+
struct oxp_rgb_led {
struct mc_subled subled_info[3];
struct led_classdev_mc mc_cdev;
struct delayed_work work;
struct oxp_hid_cfg *cfg;
+ spinlock_t state_lock; /* protects auxiliary RGB state */
void *state;
u8 type;
+ u8 zone;
};
struct oxp_rgb_led_desc {
const char *name;
u8 type;
+ u8 zone;
};
struct oxp_attr {
@@ -417,6 +441,23 @@ static struct oxp_rgb_full_state *oxp_rgb_full_state(struct oxp_rgb_led *led)
switch (led->type) {
case OXP_RGB_FULL:
return led->state;
+ case OXP_RGB_AUX:
+ return NULL;
+ }
+
+ return NULL;
+}
+
+static struct oxp_rgb_aux_state *oxp_rgb_aux_state(struct oxp_rgb_led *led)
+{
+ if (!led)
+ return NULL;
+
+ switch (led->type) {
+ case OXP_RGB_FULL:
+ return NULL;
+ case OXP_RGB_AUX:
+ return led->state;
}
return NULL;
@@ -473,6 +514,8 @@ static int oxp_rgb_status_store(struct oxp_rgb_led *led, u8 enabled, u8 speed,
u8 brightness);
static int oxp_rgb_effect_set(struct oxp_rgb_led *led, u8 effect);
+static void oxp_rgb_aux_restore_locked(struct oxp_rgb_led *led);
+
static void oxp_rgb_restore(struct oxp_hid_cfg *cfg)
{
struct oxp_rgb_full_state *state;
@@ -510,6 +553,9 @@ static void oxp_rgb_restore(struct oxp_hid_cfg *cfg)
"Error: Failed to restore RGB effect: %i\n",
ret);
break;
+ case OXP_RGB_AUX:
+ oxp_rgb_aux_restore_locked(led);
+ break;
}
}
}
@@ -842,6 +888,19 @@ static int oxp_gen_3_rgb_property_out(struct oxp_hid_cfg *cfg, u8 *data,
return first_err;
}
+static int oxp_gen_3_rgb_zone_property_out(struct oxp_hid_cfg *cfg,
+ u8 *data, u8 data_size)
+{
+ int ret;
+
+ ret = oxp_gen_3_property_out(cfg, data, data_size);
+ /* Retry once if the write or its acknowledgment failed. */
+ if (ret)
+ ret = oxp_gen_3_property_out(cfg, data, data_size);
+
+ return ret;
+}
+
static void oxp_gen_3_rgb_fill_color(struct oxp_gen_3_rgb_color_report *report,
u8 command, u8 zone, u8 red, u8 green,
u8 blue)
@@ -858,6 +917,85 @@ static void oxp_gen_3_rgb_fill_color(struct oxp_gen_3_rgb_color_report *report,
report->final_green = green;
}
+static bool oxp_rgb_aux_snapshot(struct oxp_rgb_led *led,
+ struct oxp_rgb_aux_state *state)
+{
+ struct oxp_rgb_aux_state *cached = oxp_rgb_aux_state(led);
+
+ if (!cached)
+ return false;
+
+ scoped_guard(spinlock_irqsave, &led->state_lock) {
+ *state = *cached;
+ }
+
+ return state->valid;
+}
+
+static int oxp_rgb_aux_apply(struct oxp_rgb_led *led,
+ const struct oxp_rgb_aux_state *state)
+{
+ struct oxp_hid_cfg *cfg = led->cfg;
+ unsigned int max_brightness = led->mc_cdev.led_cdev.max_brightness;
+ unsigned int brightness = min_t(unsigned int, state->brightness,
+ max_brightness);
+ u8 status_data[6] = { OXP_SET_PROPERTY, led->zone, 0x02,
+ OXP_FEAT_DISABLED, 0x05, 0x04 };
+ struct oxp_gen_3_rgb_color_report color_report;
+ u8 command;
+ u8 green;
+ u8 blue;
+ u8 red;
+ int ret;
+
+ if (brightness)
+ status_data[3] = OXP_FEAT_ENABLED;
+
+ ret = oxp_gen_3_rgb_zone_property_out(cfg, status_data,
+ sizeof(status_data));
+ if (ret || !brightness)
+ return ret;
+
+ /* Scale each RGB channel by brightness, rounding to the nearest integer. */
+ red = ((unsigned int)state->red * brightness + max_brightness / 2) /
+ max_brightness;
+ green = ((unsigned int)state->green * brightness + max_brightness / 2) /
+ max_brightness;
+ blue = ((unsigned int)state->blue * brightness + max_brightness / 2) /
+ max_brightness;
+
+ switch (state->effect) {
+ case OXP_RGB_AUX_EFFECT_MONOCOLOR:
+ command = OXP_EFFECT_MONO_TRUE;
+ break;
+ case OXP_RGB_AUX_EFFECT_BREATHING:
+ command = OXP_EFFECT_BREATHING_TRUE;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ oxp_gen_3_rgb_fill_color(&color_report, command, led->zone, red, green,
+ blue);
+ return oxp_gen_3_rgb_zone_property_out(cfg, (u8 *)&color_report,
+ sizeof(color_report));
+}
+
+static void oxp_rgb_aux_restore_locked(struct oxp_rgb_led *led)
+{
+ struct oxp_hid_cfg *cfg = led->cfg;
+ struct oxp_rgb_aux_state state;
+ int ret;
+
+ if (!oxp_rgb_aux_snapshot(led, &state))
+ return;
+
+ ret = oxp_rgb_aux_apply(led, &state);
+ if (ret)
+ dev_err(&cfg->hdev->dev,
+ "Failed to restore RGB zone %#04x: %i\n", led->zone, ret);
+}
+
static ssize_t gamepad_mode_store(struct device *dev,
struct device_attribute *attr, const char *buf,
size_t count)
@@ -1913,6 +2051,25 @@ static void oxp_gen_3_rgb_queue(struct oxp_rgb_led *led,
"Error: Failed to write RGB color: %i\n", ret);
}
+static void oxp_rgb_aux_queue(struct oxp_rgb_led *led)
+{
+ struct oxp_hid_cfg *cfg = led->cfg;
+ struct oxp_rgb_aux_state state;
+ int ret;
+
+ if (!oxp_rgb_aux_snapshot(led, &state))
+ return;
+
+ guard(mutex)(&cfg->rgb_mutex);
+ if (READ_ONCE(cfg->suspended) || READ_ONCE(cfg->removing))
+ return;
+
+ ret = oxp_rgb_aux_apply(led, &state);
+ if (ret)
+ dev_err(led->mc_cdev.led_cdev.dev,
+ "Failed to write RGB zone %#04x: %i\n", led->zone, ret);
+}
+
static void oxp_rgb_queue_fn(struct work_struct *work)
{
struct oxp_rgb_led *led = container_of(to_delayed_work(work),
@@ -1929,6 +2086,9 @@ static void oxp_rgb_queue_fn(struct work_struct *work)
else
oxp_rgb_full_queue(led, led->state);
break;
+ case OXP_RGB_AUX:
+ oxp_rgb_aux_queue(led);
+ break;
}
}
@@ -1937,16 +2097,112 @@ static void oxp_rgb_brightness_set(struct led_classdev *led_cdev,
{
struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(led_cdev);
struct oxp_rgb_led *led = container_of(mc_cdev, struct oxp_rgb_led,
- mc_cdev);
+ mc_cdev);
struct oxp_hid_cfg *cfg = led->cfg;
+ struct oxp_rgb_aux_state *state;
- if (READ_ONCE(cfg->suspended) || READ_ONCE(cfg->removing))
+ if (READ_ONCE(cfg->removing))
+ return;
+
+ switch (led->type) {
+ case OXP_RGB_FULL:
+ if (READ_ONCE(cfg->suspended))
+ return;
+ led_cdev->brightness = brightness;
+ break;
+ case OXP_RGB_AUX:
+ state = led->state;
+ led_cdev->brightness = brightness;
+ scoped_guard(spinlock_irqsave, &led->state_lock) {
+ state->brightness = brightness;
+ state->red = led->subled_info[0].intensity;
+ state->green = led->subled_info[1].intensity;
+ state->blue = led->subled_info[2].intensity;
+ state->valid = true;
+ }
+ if (READ_ONCE(cfg->suspended))
+ return;
+ break;
+ default:
return;
+ }
- led_cdev->brightness = brightness;
mod_delayed_work(system_dfl_wq, &led->work, msecs_to_jiffies(50));
}
+static ssize_t oxp_rgb_aux_effect_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct oxp_rgb_led *led = oxp_rgb_led_from_dev(dev);
+ struct oxp_hid_cfg *cfg = led->cfg;
+ struct oxp_rgb_aux_state *state = oxp_rgb_aux_state(led);
+ int ret;
+
+ if (!state || READ_ONCE(cfg->removing))
+ return -ENODEV;
+
+ ret = sysfs_match_string(oxp_rgb_aux_effect_text, buf);
+ if (ret < 0)
+ return ret;
+
+ scoped_guard(spinlock_irqsave, &led->state_lock) {
+ state->effect = ret;
+ state->valid = true;
+ }
+
+ if (!READ_ONCE(cfg->suspended) && !READ_ONCE(cfg->removing))
+ mod_delayed_work(system_dfl_wq, &led->work,
+ msecs_to_jiffies(50));
+
+ return count;
+}
+
+static ssize_t oxp_rgb_aux_effect_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct oxp_rgb_led *led = oxp_rgb_led_from_dev(dev);
+ struct oxp_hid_cfg *cfg = led->cfg;
+ struct oxp_rgb_aux_state state;
+
+ if (!oxp_rgb_aux_state(led) || READ_ONCE(cfg->removing))
+ return -ENODEV;
+
+ oxp_rgb_aux_snapshot(led, &state);
+ if (state.effect >= ARRAY_SIZE(oxp_rgb_aux_effect_text))
+ return -EINVAL;
+
+ return sysfs_emit(buf, "%s\n", oxp_rgb_aux_effect_text[state.effect]);
+}
+static DEVICE_ATTR_RW_NAMED(oxp_rgb_aux_effect, "effect");
+
+static ssize_t oxp_rgb_aux_effect_index_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ int count = 0;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(oxp_rgb_aux_effect_text); i++)
+ count += sysfs_emit_at(buf, count, "%s ",
+ oxp_rgb_aux_effect_text[i]);
+ if (count)
+ buf[count - 1] = '\n';
+
+ return count;
+}
+static DEVICE_ATTR_RO_NAMED(oxp_rgb_aux_effect_index, "effect_index");
+
+static struct attribute *oxp_rgb_aux_attrs[] = {
+ &dev_attr_oxp_rgb_aux_effect.attr,
+ &dev_attr_oxp_rgb_aux_effect_index.attr,
+ NULL,
+};
+
+static const struct attribute_group oxp_rgb_aux_attr_group = {
+ .attrs = oxp_rgb_aux_attrs,
+};
+
static struct attribute *oxp_rgb_attrs[] = {
&dev_attr_effect.attr,
&dev_attr_effect_index.attr,
@@ -1966,12 +2222,23 @@ static const struct oxp_rgb_led_desc oxp_rgb_led_descs[] = {
.name = "oxp:rgb:joystick_rings",
.type = OXP_RGB_FULL,
},
+ {
+ .name = "oxp:rgb:guide_button",
+ .type = OXP_RGB_AUX,
+ .zone = 0x05,
+ },
+ {
+ .name = "oxp:rgb:rear_logo",
+ .type = OXP_RGB_AUX,
+ .zone = 0x06,
+ },
};
static int oxp_rgb_led_init(struct oxp_hid_cfg *cfg, struct oxp_rgb_led *led,
const struct oxp_rgb_led_desc *desc)
{
struct oxp_rgb_full_state *full_state;
+ struct oxp_rgb_aux_state *aux_state;
struct hid_device *hdev = cfg->hdev;
u8 green;
u8 blue;
@@ -1979,6 +2246,7 @@ static int oxp_rgb_led_init(struct oxp_hid_cfg *cfg, struct oxp_rgb_led *led,
led->cfg = cfg;
led->type = desc->type;
+ led->zone = desc->zone;
switch (led->type) {
case OXP_RGB_FULL:
@@ -1992,6 +2260,20 @@ static int oxp_rgb_led_init(struct oxp_hid_cfg *cfg, struct oxp_rgb_led *led,
green = 0x22;
blue = 0x99;
break;
+ case OXP_RGB_AUX:
+ aux_state = devm_kzalloc(&hdev->dev, sizeof(*aux_state),
+ GFP_KERNEL);
+ if (!aux_state)
+ return -ENOMEM;
+ aux_state->red = 0xff;
+ aux_state->green = 0xff;
+ aux_state->blue = 0xff;
+ aux_state->effect = OXP_RGB_AUX_EFFECT_MONOCOLOR;
+ led->state = aux_state;
+ red = 0xff;
+ green = 0xff;
+ blue = 0xff;
+ break;
default:
return -EINVAL;
}
@@ -2020,6 +2302,7 @@ static int oxp_rgb_led_init(struct oxp_hid_cfg *cfg, struct oxp_rgb_led *led,
led->mc_cdev.led_cdev.brightness_set = oxp_rgb_brightness_set;
led->mc_cdev.num_colors = ARRAY_SIZE(led->subled_info);
led->mc_cdev.subled_info = led->subled_info;
+ spin_lock_init(&led->state_lock);
INIT_DELAYED_WORK(&led->work, oxp_rgb_queue_fn);
return 0;
@@ -2027,7 +2310,8 @@ static int oxp_rgb_led_init(struct oxp_hid_cfg *cfg, struct oxp_rgb_led *led,
static int oxp_rgb_leds_register(struct oxp_hid_cfg *cfg)
{
- int led_count = ARRAY_SIZE(oxp_rgb_led_descs);
+ int led_count = cfg->x2_rgb ? ARRAY_SIZE(oxp_rgb_led_descs) : 1;
+ const struct attribute_group *attr_group;
struct hid_device *hdev = cfg->hdev;
struct oxp_rgb_led *led;
int ret;
@@ -2051,8 +2335,19 @@ static int oxp_rgb_leds_register(struct oxp_hid_cfg *cfg)
return dev_err_probe(&hdev->dev, ret,
"Failed to create RGB device\n");
+ switch (led->type) {
+ case OXP_RGB_FULL:
+ attr_group = &oxp_rgb_attr_group;
+ break;
+ case OXP_RGB_AUX:
+ attr_group = &oxp_rgb_aux_attr_group;
+ break;
+ default:
+ return -EINVAL;
+ }
+
ret = devm_device_add_group(led->mc_cdev.led_cdev.dev,
- &oxp_rgb_attr_group);
+ attr_group);
if (ret)
return dev_err_probe(led->mc_cdev.led_cdev.dev, ret,
"Failed to create RGB configuration attributes\n");
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 15/15] HID: hid-oxp: add X2 auxiliary RGB zones
2026-09-10 3:21 ` [PATCH 15/15] HID: hid-oxp: add X2 auxiliary RGB zones Andrei Aldea
@ 2026-09-10 3:44 ` sashiko-bot
0 siblings, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-09-10 3:44 UTC (permalink / raw)
To: Andrei Aldea; +Cc: lee, linux-input, dmitry.torokhov, linux-leds
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The driver incorrectly checks `hdev->bus != BUS_USB` before casting `hdev->dev.parent` to a USB interface. This violates HID subsystem guidelines and allows `uhid`-injected devices to cause a kernel crash.
--
commit 2e553029a2661e6b48c17a4d0a1f09df4448e068
Author: Andrei Aldea <andrei1998@gmail.com>
Subject: HID: hid-oxp: add X2 auxiliary RGB zones
This commit adds separate multicolor LED devices for the ONEXPLAYER X2 guide
button and rear logo using Gen3 zones 5 and 6. Each LED retains independent
color, brightness, and effect states. It also documents the new auxiliary
effects and the existing joystick-ring effects in the sysfs ABI.
> diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
> index 9a46602238aa3..757cef88e6c62 100644
> --- a/drivers/hid/hid-oxp.c
> +++ b/drivers/hid/hid-oxp.c
[ ... ]
[Severity: High]
Does this code safely cast to a USB interface? In oxp_is_cfg_interface(),
the check for USB devices uses the bus type:
if (hdev->bus != BUS_USB)
return false;
intf = to_usb_interface(hdev->dev.parent);
If a userspace program injects a HID device with BUS_USB via /dev/uhid, the
bus check passes, but the parent device is not a valid USB interface. This
results in an invalid cast and causes a kernel crash when dereferencing
intf->cur_altsetting.
Should this use hid_is_usb(hdev) instead of checking hdev->bus to prevent
this regression?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260910032115.28669-1-andrei1998@gmail.com?part=15
^ permalink raw reply [flat|nested] 30+ messages in thread