* [PATCH] Input: ims-pcu - only expose sysfs attributes on control interface
From: Dmitry Torokhov @ 2026-05-30 5:40 UTC (permalink / raw)
To: linux-input; +Cc: linux-kernel
When the driver was converted to use the driver core to instantiate device
attributes (via .dev_groups in the usb_driver structure), the attributes
started appearing on all interfaces bound to the driver. Since the ims-pcu
driver manually claims the secondary data interface during probe, the
driver core automatically creates the sysfs attributes for that interface
as well.
However, the driver only supports these attributes on the primary control
interface. Data interfaces lack the necessary descriptors and internal
state to handle these requests, and accessing them can lead to unexpected
behavior or crashes.
Fix this by updating the is_visible() callbacks for both the main and OFN
attribute groups to verify that the interface being accessed is indeed the
control interface.
Fixes: 204d18a7a0c6 ("Input: ims-pcu - use driver core to instantiate device attributes")
Cc: stable@vger.kernel.org
Reported-by: Sashiko bot <sashiko-bot@kernel.org>
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/misc/ims-pcu.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 13367ec66936..ddfb88b7c8f5 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -1283,6 +1283,9 @@ static umode_t ims_pcu_is_attr_visible(struct kobject *kobj,
struct ims_pcu *pcu = usb_get_intfdata(intf);
umode_t mode = attr->mode;
+ if (intf != pcu->ctrl_intf)
+ return 0;
+
if (pcu->bootloader_mode) {
if (attr != &dev_attr_update_firmware_status.attr &&
attr != &dev_attr_update_firmware.attr &&
@@ -1534,6 +1537,9 @@ static umode_t ims_pcu_ofn_is_attr_visible(struct kobject *kobj,
struct ims_pcu *pcu = usb_get_intfdata(intf);
umode_t mode = attr->mode;
+ if (intf != pcu->ctrl_intf)
+ return 0;
+
/*
* PCU-B devices, both GEN_1 and GEN_2 do not have OFN sensor.
*/
--
2.54.0.823.g6e5bcc1fc9-goog
--
Dmitry
^ permalink raw reply related
* Re: [PATCH 4/4] dt-bindings: input: remove obsolete matrix-keymap.txt
From: sashiko-bot @ 2026-05-30 5:23 UTC (permalink / raw)
To: Akash Sukhavasi; +Cc: dmitry.torokhov, robh, linux-input, devicetree, conor+dt
In-Reply-To: <20260529052246.4934-5-akash.sukhavasi@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The commit message inaccurately claims that 'Eight files still reference matrix-keymap.txt', when only 6 files actually do.
--
commit eb5de2cbf16470ef87ea36fcd4695758a1c30a9f
Author: Akash Sukhavasi <akash.sukhavasi@gmail.com>
dt-bindings: input: remove obsolete matrix-keymap.txt
This commit removes the obsolete matrix-keymap.txt stub and updates all
remaining references across the input and mfd binding documentation to
point directly to the matrix-keymap.yaml schema.
> Eight files still reference matrix-keymap.txt, forcing readers
> through an extra hop to reach the .yaml. The stub has not been
> touched since June 2020. Update all references across input and mfd
> binding documentation to point directly to matrix-keymap.yaml and
> remove the stub.
[Severity: Low]
This isn't a bug, but the commit message states that eight files still
reference the text file. Looking at the patch, there are eight total
references updated, but they are spread across only six files since
tc3589x.txt contains three of those references.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260529052246.4934-1-akash.sukhavasi@gmail.com?part=4
^ permalink raw reply
* Re: [PATCH 01/10] [v2] [net-next] net: dsa: b53: hide legacy gpiolib usage on non-mips
From: sashiko-bot @ 2026-05-30 0:45 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260520183815.2510387-2-arnd@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Probe errors from devm_gpio_request_one() are silently ignored, failing to propagate critical errors such as -EPROBE_DEFER.
--
commit 46ca85c6cb9da93cad093c8eb342afc0ea94be33
Author: Arnd Bergmann <arnd@arndb.de>
net: dsa: b53: hide legacy gpiolib usage on non-mips
This commit moves legacy gpiolib usage inside an #ifdef block and transitions
the common parts of the driver to use modern GPIO descriptors. This allows
building the driver even when gpio_set_value is left out of the kernel.
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 2c4131ed7e30b..d838511ae6744 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
[ ... ]
> @@ -3173,12 +3172,6 @@ static int b53_switch_init(struct b53_device *dev)
> return -ENOMEM;
>
> dev->reset_gpio = b53_switch_get_reset_gpio(dev);
> - if (dev->reset_gpio >= 0) {
> - ret = devm_gpio_request_one(dev->dev, dev->reset_gpio,
> - GPIOF_OUT_INIT_HIGH, "robo_reset");
> - if (ret)
> - return ret;
> - }
>
> return 0;
> }
[Severity: High]
Does this silently ignore probe errors like -EPROBE_DEFER?
Previously, if devm_gpio_request_one failed, the error was returned to the
caller. Now, b53_switch_get_reset_gpio handles the request and returns an
ERR_PTR on failure, but that error pointer is simply stored in dev->reset_gpio
while b53_switch_init unconditionally returns 0.
Later in the code, b53_switch_reset_gpio relies on IS_ERR(dev->reset_gpio)
to gracefully return, which would silently skip the reset sequence entirely
if the GPIO controller simply isn't ready yet (-EPROBE_DEFER) or if there
was a memory allocation failure (-ENOMEM).
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260520183815.2510387-1-arnd@kernel.org?part=1
^ permalink raw reply
* Re: [PATCH 02/10] [v3] input: gpio-keys: make legacy gpiolib optional
From: sashiko-bot @ 2026-05-30 0:45 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260520183815.2510387-3-arnd@kernel.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Modifying the legacy GPIO validation to check `!button->irq` breaks legacy devices that legitimately provide both a GPIO and an IRQ.
- [Medium] Conditionally removing the `gpio` field from `struct gpio_keys_button` causes build failures in drivers that assign to this field when `CONFIG_GPIOLIB_LEGACY` is disabled.
--
commit cb399daff5ceabcf54d5ea611fd309f5c0600ab4
Author: Arnd Bergmann <arnd@arndb.de>
input: gpio-keys: make legacy gpiolib optional
This commit makes the legacy gpiolib interface optional for gpio-keys and
gpio-keys-polled by adding ifdef blocks. It also modifies the gpio-keys
driver to ignore the gpio number if an IRQ is passed to keep Rohm PMIC
drivers working with and without CONFIG_GPIOLIB_LEGACY.
Link: https://lore.kernel.org/all/b3c94552-c104-42e3-be15-7e8362e8039e@gmail.com/
Link: https://lore.kernel.org/all/afJXG4_rtaj3l2Dk@google.com/
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
[ ... ]
> @@ -528,7 +528,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
> */
> bdata->gpiod = NULL;
> }
> - } else if (gpio_is_valid(button->gpio)) {
> +#ifdef CONFIG_GPIOLIB_LEGACY
> + } else if (!button->irq && gpio_is_valid(button->gpio)) {
[Severity: High]
Does this change break legacy devices that legitimately provide both a valid
GPIO and a valid IRQ?
If a legacy platform provides both a valid GPIO and an IRQ, this condition
now evaluates to false.
By skipping the GPIO request entirely, the driver falls back to treating the
device as a pure IRQ button. This forces the driver to lose hardware
debouncing and rely on a simulated release timer instead of checking the
physical GPIO line.
[ ... ]
> diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
> --- a/include/linux/gpio_keys.h
> +++ b/include/linux/gpio_keys.h
[ ... ]
> @@ -25,7 +25,9 @@ struct device;
> */
> struct gpio_keys_button {
> unsigned int code;
> +#ifdef CONFIG_GPIOLIB_LEGACY
> int gpio;
> +#endif
> int active_low;
[Severity: Medium]
Will this cause build failures for drivers that assign to this field when
CONFIG_GPIOLIB_LEGACY is disabled?
Other modern drivers dynamically create the platform device and pass legacy
GPIO numbers by directly assigning to this field:
drivers/input/misc/soc_button_array.c:soc_button_device_create() {
...
gpio_keys[n_buttons].gpio = gpio;
...
}
When CONFIG_GPIOLIB_LEGACY is disabled, this results in a compilation failure
since struct gpio_keys_button no longer has the gpio member.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260520183815.2510387-1-arnd@kernel.org?part=2
^ permalink raw reply
* Re: [PATCH 10/10] gpiolib: turn off legacy interface by default
From: sashiko-bot @ 2026-05-30 0:45 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260520183815.2510387-11-arnd@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Turning off `CONFIG_GPIOLIB_LEGACY` by default will break the build for multiple architectures and platforms that still rely on the legacy GPIO interface but do not select `CONFIG_GPIOLIB_LEGACY`.
--
commit a920fbf61a1d127c3a98b41cb32f7260acd5657b
Author: Arnd Bergmann <arnd@arndb.de>
gpiolib: turn off legacy interface by default
This commit removes the default enablement of GPIOLIB_LEGACY, requiring
platforms to explicitly select it if they still use the legacy interface.
It also adds a prompt to enable it manually for compile testing.
> All users of the legacy interface now select CONFIG_GPIOLIB_LEGACY,
> so it can be turned off by default and only get built on platforms
> that still have one unconverted driver.
[Severity: Low]
Is this statement accurate? There appear to be platforms like ARM PXA
(arch/arm/mach-pxa/spitz_pm.c), ARM Orion5x, and m68k Coldfire that still
contain direct calls to legacy GPIO functions like gpio_request() but do
not select GPIOLIB_LEGACY in their Kconfig files.
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 020e51e30317a..543de15a12f63 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -4,7 +4,14 @@
> #
>
> config GPIOLIB_LEGACY
> - def_bool y
[Severity: Low]
Will removing this default cause build regressions for the unconverted
platforms mentioned above?
It seems the legacy function declarations in include/linux/gpio.h will be
hidden, leading to implicit declaration errors when compiling those
architectures.
> + bool "Legacy GPIO interfaces" if COMPILE_TEST
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260520183815.2510387-1-arnd@kernel.org?part=10
^ permalink raw reply
* Re: [PATCH 08/10] [v6 net-next] p54spi: convert to devicetree
From: sashiko-bot @ 2026-05-30 0:45 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260520183815.2510387-9-arnd@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] SPI transfers are using stack-allocated buffers for `tx_buf` and `rx_buf`, violating the SPI subsystem's DMA requirements.
--
commit 8ba2b2da7516b867a52706d993b9ce2d49c99105
Author: Arnd Bergmann <arnd@arndb.de>
p54spi: convert to devicetree
The Prism54 SPI driver hardcodes GPIO numbers and expects users to
pass them as module parameters, apparently a relic from its life as a
staging driver. This works because there is only one user, the Nokia
N8x0 tablet.
Convert this to the gpio descriptor interface and DT based probing
to improve this and simplify the code at the same time.
> diff --git a/drivers/net/wireless/intersil/p54/p54spi.c b/drivers/net/wireless/intersil/p54/p54spi.c
> index 9d66dcae54e02..d18be25450285 100644
> --- a/drivers/net/wireless/intersil/p54/p54spi.c
> +++ b/drivers/net/wireless/intersil/p54/p54spi.c
[ ... ]
> @@ -30,19 +31,6 @@
> MODULE_FIRMWARE("3826.arm");
> MODULE_FIRMWARE("3826.eeprom");
>
> -/* gpios should be handled in board files and provided via platform data,
> - * but because it's currently impossible for p54spi to have a header file
> - * in include/linux, let's use module parameters for now
> - */
> -
> -static int p54spi_gpio_power = 97;
> -module_param(p54spi_gpio_power, int, 0444);
> -MODULE_PARM_DESC(p54spi_gpio_power, "gpio number for power line");
> -
> -static int p54spi_gpio_irq = 87;
> -module_param(p54spi_gpio_irq, int, 0444);
> -MODULE_PARM_DESC(p54spi_gpio_irq, "gpio number for irq line");
> -
> static void p54spi_spi_read(struct p54s_priv *priv, u8 address,
> void *buf, size_t len)
> {
[Severity: High]
This isn't a bug introduced by this patch, but does this code use
stack-allocated buffers for SPI transfers?
It appears that p54spi_spi_read() and other similar functions pass stack
variables (e.g., addr in p54spi_spi_read(), rx_head in p54spi_rx(), and
val in p54spi_read32()) directly into spi_sync() as tx_buf and rx_buf.
If the underlying SPI controller uses DMA, the SPI core maps these buffers.
Using stack memory for DMA is unsafe because, with options like
CONFIG_VMAP_STACK enabled, the stack may not be physically contiguous.
Even without it, stack DMA leads to cache-line sharing and incoherency,
which can result in severe memory corruption or kernel panics.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260520183815.2510387-1-arnd@kernel.org?part=8
^ permalink raw reply
* Re: [PATCH 05/10] [v2] mips: select legacy gpiolib interfaces where used
From: sashiko-bot @ 2026-05-30 0:45 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260520183815.2510387-6-arnd@kernel.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Duplicate `select GPIOLIB` in `config MIPS_ALCHEMY`.
- [Low] Unnecessary `select GPIOLIB_LEGACY` for `config MIKROTIK_RB532`.
--
commit 0726793f1a39f1a0b2ac5d24243bcbe5e8999b93
Author: Arnd Bergmann <arnd@arndb.de>
mips: select legacy gpiolib interfaces where used
This commit updates several old mips machines that have not been converted
away from the old-style gpiolib interfaces to select CONFIG_GPIOLIB_LEGACY.
This ensures the legacy code continues to work where needed but can be
omitted otherwise.
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 4364f3dba688f..b91e62d69a5db 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -206,6 +206,8 @@ config MIPS_ALCHEMY
> select CSRC_R4K
> select IRQ_MIPS_CPU
> select DMA_NONCOHERENT # Au1000,1500,1100 aren't, rest is
> + select GPIOLIB
> + select GPIOLIB_LEGACY
[Severity: Low]
Is this select GPIOLIB a duplicate? It appears select GPIOLIB is already
present further down in the MIPS_ALCHEMY block.
> select MIPS_FIXUP_BIGPHYS_ADDR if PCI
> select SYS_HAS_CPU_MIPS32_R1
> select SYS_SUPPORTS_32BIT_KERNEL
[ ... ]
> @@ -999,6 +1003,7 @@ config MIKROTIK_RB532
> select SWAP_IO_SPACE
> select BOOT_RAW
> select GPIOLIB
> + select GPIOLIB_LEGACY
[Severity: Low]
Does this select GPIOLIB_LEGACY introduce an unnecessary dependency for
MIKROTIK_RB532? A look at the code indicates that MIKROTIK_RB532 and its
associated drivers (like pata_rb532_cf and rb532_button) use the modern
descriptor-based gpiod API and do not seem to call the legacy GPIO functions.
Could this dependency be omitted so we don't compile legacy interfaces
unnecessarily?
> select MIPS_L1_CACHE_SHIFT_4
> help
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260520183815.2510387-1-arnd@kernel.org?part=5
^ permalink raw reply
* Re: [PATCH] HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume
From: sashiko-bot @ 2026-05-29 23:12 UTC (permalink / raw)
To: d3z-the-dev; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260529222210.1532876-1-d3z.the.dev@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Unconditional hardware reset during system resume destroys pending wake events and breaks Wake-on-Touch.
Pre-existing issues:
- [High] Using wait_event_interruptible_timeout in a system resume callback can prematurely abort device resume.
- [High] Concurrent execution of `reset_tic()` from the PM resume callback and the IRQ thread without proper locking.
--
commit e9088cb0484bc3fbc7b5dd49968293dc14d55d36
Author: d3z-the-dev <d3z.the.dev@gmail.com>
HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume
This commit modifies quickspi_resume() to perform a full reset of the touch
IC during system resume. It mirrors the hibernation restore path to fix an
issue where the touch IC loses power during s2idle suspend on certain platforms.
Link: https://github.com/linux-surface/linux-surface/issues/1799
> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> index f669235f18839..d59278a3e5c10 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
[ ... ]
> @@ -780,24 +780,54 @@ static int quickspi_resume(struct device *device)
> thc_interrupt_config(qsdev->thc_hw);
>
> thc_interrupt_enable(qsdev->thc_hw, true);
[Severity: High]
This is a pre-existing issue, but does enabling interrupts right before calling
reset_tic() introduce a race condition?
If the touch controller generates an error interrupt during the reset sequence,
the OS schedules the IRQ thread, which calls try_recover() and executes
reset_tic() again. Since both the PM thread and the IRQ thread execute
reset_tic() concurrently without a mutex:
PM thread
quickspi_resume()
reset_tic()
IRQ thread
quickspi_irq_thread_handler()
try_recover()
reset_tic()
Can this corrupt the device state and cause both threads to wait on the same
reset_ack_wq?
>
> - ret = thc_dma_configure(qsdev->thc_hw);
> + /* The TIC may lose power across system suspend, reset it to recover */
> + ret = reset_tic(qsdev);
[Severity: High]
This isn't a bug introduced by this patch, but since reset_tic() is now
used in the primary system resume path, is it safe for it to use
wait_event_interruptible_timeout() internally?
System resume callbacks are often executed by a user-space process which
might have a pending signal from the task freeze phase. If
wait_event_interruptible_timeout() aborts immediately and returns
-ERESTARTSYS, won't reset_tic() fail with -ETIMEDOUT and completely abort
the device resume?
> if (ret)
> return ret;
>
> - ret = thc_interrupt_quiesce(qsdev->thc_hw, false);
> + ret = thc_dma_configure(qsdev->thc_hw);
> if (ret)
> return ret;
>
> - if (!device_may_wakeup(qsdev->dev))
> - return quickspi_set_power(qsdev, HIDSPI_ON);
[Severity: High]
Does unconditionally calling reset_tic() here break Wake-on-Touch for
platforms that don't lose power?
Previously, this function had a check to avoid disturbing a wake-enabled
device:
- if (!device_may_wakeup(qsdev->dev))
- return quickspi_set_power(qsdev, HIDSPI_ON);
Will a hard reset wipe the internal buffers of the touch controller before
the OS can fetch the wake-triggering touch event, resulting in permanently
lost events?
> + thc_ltr_config(qsdev->thc_hw,
> + qsdev->active_ltr_val,
> + qsdev->low_power_ltr_val);
> +
> + thc_change_ltr_mode(qsdev->thc_hw, THC_LTR_MODE_ACTIVE);
> +
> + qsdev->state = QUICKSPI_ENABLED;
>
> return 0;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260529222210.1532876-1-d3z.the.dev@gmail.com?part=1
^ permalink raw reply
* [PATCH] HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume
From: d3z-the-dev @ 2026-05-29 22:22 UTC (permalink / raw)
To: Even Xu, Xinpeng Sun, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, Abhishek Tamboli, Sakari Ailus,
d3z-the-dev
On the Surface Pro 10 (Meteor Lake) the touchscreen stops working after a
suspend/resume cycle and only recovers after a reboot. The driver logs
"GET_DEVICE_INFO: recv failed: -11" on resume.
The touch IC loses power during system suspend (s2idle) on this platform,
the same way it does across hibernation. quickspi_resume() only restores
the THC port, interrupts and DMA and sends a HIDSPI_ON command, assuming
the touch IC kept its power and state. When it has actually lost power the
HIDSPI_ON command is never acknowledged and the descriptor read fails,
leaving the touchscreen dead until the module is reloaded.
quickspi_restore() already handles this for hibernation by running
reset_tic() and reconfiguring the THC SPI/LTR settings. Make
quickspi_resume() do the same: quiesce interrupts, re-select the THC port,
reconfigure the SPI input/output addresses and read/write parameters, run
reset_tic() to re-enumerate the device and restore the LTR configuration.
Tested on a Surface Pro 10 across multiple s2idle suspend/resume cycles.
Link: https://github.com/linux-surface/linux-surface/issues/1799
Signed-off-by: d3z-the-dev <d3z.the.dev@gmail.com>
---
.../intel-quickspi/pci-quickspi.c | 38 +++++++++++++++++--
1 file changed, 34 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
index f669235f1883..d59278a3e5c1 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
@@ -780,24 +780,54 @@ static int quickspi_resume(struct device *device)
if (!qsdev)
return -ENODEV;
+ ret = thc_interrupt_quiesce(qsdev->thc_hw, true);
+ if (ret)
+ return ret;
+
ret = thc_port_select(qsdev->thc_hw, THC_PORT_TYPE_SPI);
if (ret)
return ret;
+ thc_spi_input_output_address_config(qsdev->thc_hw,
+ qsdev->input_report_hdr_addr,
+ qsdev->input_report_bdy_addr,
+ qsdev->output_report_addr);
+
+ ret = thc_spi_read_config(qsdev->thc_hw, qsdev->spi_freq_val,
+ qsdev->spi_read_io_mode,
+ qsdev->spi_read_opcode,
+ qsdev->spi_packet_size);
+ if (ret)
+ return ret;
+
+ ret = thc_spi_write_config(qsdev->thc_hw, qsdev->spi_freq_val,
+ qsdev->spi_write_io_mode,
+ qsdev->spi_write_opcode,
+ qsdev->spi_packet_size,
+ qsdev->performance_limit);
+ if (ret)
+ return ret;
+
thc_interrupt_config(qsdev->thc_hw);
thc_interrupt_enable(qsdev->thc_hw, true);
- ret = thc_dma_configure(qsdev->thc_hw);
+ /* The TIC may lose power across system suspend, reset it to recover */
+ ret = reset_tic(qsdev);
if (ret)
return ret;
- ret = thc_interrupt_quiesce(qsdev->thc_hw, false);
+ ret = thc_dma_configure(qsdev->thc_hw);
if (ret)
return ret;
- if (!device_may_wakeup(qsdev->dev))
- return quickspi_set_power(qsdev, HIDSPI_ON);
+ thc_ltr_config(qsdev->thc_hw,
+ qsdev->active_ltr_val,
+ qsdev->low_power_ltr_val);
+
+ thc_change_ltr_mode(qsdev->thc_hw, THC_LTR_MODE_ACTIVE);
+
+ qsdev->state = QUICKSPI_ENABLED;
return 0;
}
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v2] HID: wacom: fix NULL pointer dereference in wacom_intuos_pad()
From: Dmitry Torokhov @ 2026-05-29 21:44 UTC (permalink / raw)
To: Jinmo Yang
Cc: Ping Cheng, Jason Gerecke, Jiri Kosina, Benjamin Tissoires,
linux-input, linux-kernel
In-Reply-To: <20260523150619.615565-1-jinmo44.yang@gmail.com>
On Sun, May 24, 2026 at 12:06:19AM +0900, Jinmo Yang wrote:
> wacom_intuos_pad() dereferences wacom->pad_input without a NULL check.
> When a Wacom tablet is created via UHID with parameters that route
> through wacom_bpt_irq() -> wacom_intuos_irq() -> wacom_intuos_pad(),
> but probe did not allocate pad_input, the call to
> wacom_report_numbered_buttons() passes a NULL input_dev, causing a
> general protection fault in input_get_drvdata().
>
> Add a NULL check for pad_input at the top of wacom_intuos_pad() to
> bail out early when the pad input device was not set up.
>
> The bug was found by syzkaller on linux-next 7.1.0-rc4 (next-20260522):
>
> Oops: general protection fault, probably for non-canonical address 0xdffffc0000000052
> KASAN: null-ptr-deref in range [0x0000000000000290-0x0000000000000297]
> RIP: 0010:input_get_drvdata include/linux/input.h:390 [inline]
> RIP: 0010:wacom_report_numbered_buttons+0x37/0x210 drivers/hid/wacom_wac.c:4210
> Call Trace:
> wacom_intuos_pad drivers/hid/wacom_wac.c:643 [inline]
> wacom_intuos_irq+0x29a/0x32b0 drivers/hid/wacom_wac.c:1042
> wacom_bpt_irq drivers/hid/wacom_wac.c:3275 [inline]
> wacom_wac_irq+0x12ed/0xaef0 drivers/hid/wacom_wac.c:3545
> wacom_raw_event+0x677/0xa90 drivers/hid/wacom_sys.c:184
> __hid_input_report.constprop.0+0x39a/0x4d0 drivers/hid/hid-core.c:2161
> uhid_dev_input2 drivers/hid/uhid.c:618 [inline]
> uhid_char_write+0xa8a/0xfa0 drivers/hid/uhid.c:776
>
> Also confirmed on a Pixel 9 Pro (Android 16, kernel 6.1.124) where
> it causes an immediate kernel panic and reboot via /dev/uhid without
> requiring root privileges:
>
> KP: Oops: Fatal exception: comm:wacom_27qhdt
> Reboot reason: 0xbaba - Kernel PANIC
>
> Reproducer (unprivileged):
> open("/dev/uhid", O_RDWR)
> write(fd, UHID_CREATE2{vendor=0x056a, product=0x0020})
> write(fd, UHID_INPUT2{report_id=0x0c, size=10})
>
> Fixes: c7f0522a1ad1 ("HID: wacom: Slim down wacom_intuos_pad processing")
> Signed-off-by: Jinmo Yang <jinmo44.yang@gmail.com>
> ---
> drivers/hid/wacom_wac.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index da1f0ea85..251ddda3e 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -515,7 +515,6 @@ static int wacom_intuos_pad(struct wacom_wac *wacom)
> struct wacom_features *features = &wacom->features;
> unsigned char *data = wacom->data;
> struct input_dev *input = wacom->pad_input;
> - int i;
> int buttons = 0, nbuttons = features->numbered_buttons;
> int keys = 0, nkeys = 0;
> int ring1 = 0, ring2 = 0;
> @@ -523,6 +522,10 @@ static int wacom_intuos_pad(struct wacom_wac *wacom)
> bool prox = false;
> bool wrench = false, keyboard = false, mute_touch = false, menu = false,
> info = false;
> + int i;
> +
> + if (!input)
> + return 0;
>
> /* pad packets. Works as a second tool and is always in prox */
> if (!(data[0] == WACOM_REPORT_INTUOSPAD || data[0] == WACOM_REPORT_INTUOS5PAD ||
I think there are many more places in the driver where it used
wacom->pad_input without verifying that it exists.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] HID: wacom: fix slab-out-of-bounds write in wacom_wac_queue_insert
From: Dmitry Torokhov @ 2026-05-29 21:34 UTC (permalink / raw)
To: Jinmo Yang; +Cc: linux-input, jikos, benjamin.tissoires, stable
In-Reply-To: <20260528175945.2987781-1-jinmo44.yang@gmail.com>
On Fri, May 29, 2026 at 02:59:45AM +0900, Jinmo Yang wrote:
> wacom_wac_queue_insert() calls kfifo_skip() in a loop when the kfifo
> doesn't have enough space for the incoming report. If the kfifo is
> empty, kfifo_skip() reads stale data left in the kmalloc'd buffer
> via __kfifo_peek_n() and interprets it as a record length, advancing
> fifo->out by that garbage value. This corrupts the internal kfifo
> state, causing kfifo_unused() to return a value much larger than the
> actual buffer size, which bypasses __kfifo_in_r()'s guard:
>
> if (len + recsize > kfifo_unused(fifo))
> return 0;
>
> kfifo_copy_in() then performs an out-of-bounds memcpy, writing up to
> 3842 bytes past the 256-byte buffer.
>
> Add a !kfifo_is_empty() condition to the while loop so kfifo_skip()
> is never called on an empty fifo, and check the return value of
> kfifo_in() to reject reports that are too large for the fifo.
>
> Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Fixes: 5e013ad20689 ("HID: wacom: Remove static WACOM_PKGLEN_MAX limit")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jinmo Yang <jinmo44.yang@gmail.com>
> ---
> Changes in v2:
> - Instead of a size check at the top, add !kfifo_is_empty() to the
> while loop condition to prevent kfifo_skip() on an empty fifo
> (Suggested by Dmitry Torokhov)
> - Check kfifo_in() return value to reject oversized reports instead
> of a separate guard
>
> drivers/hid/wacom_sys.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index a32320b35..489ca68f1 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -54,7 +54,7 @@ static void wacom_wac_queue_insert(struct hid_device *hdev,
> {
> bool warned = false;
>
> - while (kfifo_avail(fifo) < size) {
> + while (kfifo_avail(fifo) < size && !kfifo_is_empty(fifo)) {
> if (!warned)
> hid_warn(hdev, "%s: kfifo has filled, starting to drop events\n", __func__);
> warned = true;
> @@ -62,7 +62,9 @@ static void wacom_wac_queue_insert(struct hid_device *hdev,
> kfifo_skip(fifo);
> }
>
> - kfifo_in(fifo, raw_data, size);
> + if (!kfifo_in(fifo, raw_data, size))
> + hid_warn_ratelimited(hdev, "%s: report is too large (%d)\n",
> + __func__, size);
> }
>
> static void wacom_wac_queue_flush(struct hid_device *hdev,
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] HID: wacom: fix slab-out-of-bounds write in wacom_wac_queue_insert
From: Dmitry Torokhov @ 2026-05-29 21:33 UTC (permalink / raw)
To: sashiko-reviews; +Cc: Jinmo Yang, linux-input
In-Reply-To: <20260528184231.1F0581F000E9@smtp.kernel.org>
On Thu, May 28, 2026 at 06:42:29PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] wacom_wac_queue_flush() allocates memory with GFP_KERNEL while executing in an atomic context, leading to a 'scheduling while atomic' panic.
> --
>
> commit 70e9615b93de485ba1816f95e27e25b054df4b6a
> Author: Jinmo Yang <jinmo44.yang@gmail.com>
>
> HID: wacom: fix slab-out-of-bounds write in wacom_wac_queue_insert
>
> This commit fixes a slab-out-of-bounds write in wacom_wac_queue_insert().
> It prevents kfifo_skip() from operating on an empty fifo and validates
> the return value of kfifo_in().
>
> > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> > index a32320b351e3e..489ca68f1560f 100644
> > --- a/drivers/hid/wacom_sys.c
> > +++ b/drivers/hid/wacom_sys.c
>
> [ ... ]
>
> > @@ -62,7 +62,9 @@ static void wacom_wac_queue_insert(struct hid_device *hdev,
> > kfifo_skip(fifo);
> > }
> >
> > - kfifo_in(fifo, raw_data, size);
> > + if (!kfifo_in(fifo, raw_data, size))
> > + hid_warn_ratelimited(hdev, "%s: report is too large (%d)\n",
> > + __func__, size);
> > }
> >
> > static void wacom_wac_queue_flush(struct hid_device *hdev,
> > struct kfifo_rec_ptr_2 *fifo)
> > {
> > while (!kfifo_is_empty(fifo)) {
> > int size = kfifo_peek_len(fifo);
> > u8 *buf;
> > unsigned int count;
> > int err;
> >
> > buf = kzalloc(size, GFP_KERNEL);
>
> [Severity: High]
> This is a pre-existing issue, but is it safe to use GFP_KERNEL here?
>
> Looking at the call chain, wacom_wac_queue_flush() is invoked via
> wacom_wac_pen_serial_enforce() from the driver's .raw_event callback,
> wacom_raw_event().
>
> For USB HID devices, this callback is typically triggered by hid_irq_in(),
> which is an URB completion handler that runs in an atomic context.
>
> Since using GFP_KERNEL allows the allocator to sleep, could this lead to a
> scheduling while atomic panic? Would it be better to use GFP_ATOMIC instead?
>
This looks like valid analysis. Would you be willing to send a
[completely separate] patch?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] HID: i2c-hid-of: Fall back to ACPI _DSM when hid-descr-addr is missing
From: Dmitry Torokhov @ 2026-05-29 19:36 UTC (permalink / raw)
To: Hans de Goede
Cc: 谢致邦 (XIE Zhibang), linux-input, bentiss,
dianders, jikos, linux-kernel, superm1
In-Reply-To: <7c2c86d8-be10-4e8e-8e96-d22788ec488f@kernel.org>
On Fri, May 29, 2026 at 05:00:37PM +0200, Hans de Goede wrote:
> Hi,
>
> On 29-May-26 14:16, 谢致邦 (XIE Zhibang) wrote:
> > Before commit b33752c30023 ("HID: i2c-hid: Reorganize so ACPI and OF are
> > separate modules"), the unified i2c-hid driver handled both PNP0C50 ACPI
> > devices and hid-over-i2c OF devices. After the split, devices with _HID
> > "PRP0001" and _DSD compatible "hid-over-i2c" are only probed by
> > i2c_hid_of, which requires "hid-descr-addr" in the _DSD. Some devices,
> > for example the Lenovo KaiTian N60d and Inspur CP300L3, provide the HID
> > descriptor address only through the _DSM method. Fall back to the _DSM
> > call when the property is absent.
> >
> > Fixes: b33752c30023 ("HID: i2c-hid: Reorganize so ACPI and OF are separate modules")
> > Signed-off-by: 谢致邦 (XIE Zhibang) <Yeking@Red54.com>
>
> Thank you for the new patch, this is an interesting approach and better
> then the modalias magic from the previous version.
>
> Note I'm not the i2c-hid maintainer, with that said I think this should
> be acceptable. But currently it duplicates the _DSM handling code and
> that should be fixed.
>
> I think this should be changed to a series of 3 patches:
>
> 1. Move the i2c_hid_acpi_blacklist handling out of
> i2c_hid_acpi_get_descriptor() into i2c_hid_acpi_probe()
> to above the devm_kzalloc() call.
>
> 2. Move i2c_hid_acpi_get_descriptor() to a generic
> int i2c_hid_acpi_get_descriptor(struct device *dev)
> helper in drivers/hid/i2c-hid/i2c-hid-core.c .
> Wrapped in #ifdef CONFIG_ACPI and with a static inline
> stub in drivers/hid/i2c-hid/i2c-hid.h when CONFIG_ACPI
> is not set, e.g. in drivers/hid/i2c-hid/i2c-hid.h add:
>
> #ifdef CONFIG_ACPI
> int i2c_hid_acpi_get_descriptor_address(struct device *dev);
> #else
> static inline int i2c_hid_acpi_get_descriptor_address(struct device *dev)
> {
> return -ENODEV;
> }
> #endif
>
> 3. Modify i2c-hid-of.c to try i2c_hid_acpi_get_descriptor_address() as
> fallback for the missing "hid-descr-addr" property. Please also add
> a comment in the code explaining that this fallback is about ACPI I2C-hid
> devices which use a "PRP0001" ACPI _HID with an "hid-over-i2c" compatible.
I think we should also stick a big fat warning if _DSM succeeds in that
code branch: hopefully manufacturers will notice and fix the firmware on
new devices.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] HID: hid-goodix-spi: validate report size to prevent stack buffer overflow
From: Dmitry Torokhov @ 2026-05-29 19:31 UTC (permalink / raw)
To: Tianchu Chen; +Cc: jikos, bentiss, linux-input, linux-kernel, stable
In-Reply-To: <f7e444a3facbe5fb2627167ab205771476e46bc8@linux.dev>
On Fri, May 29, 2026 at 01:42:47PM +0000, Tianchu Chen wrote:
> From: Tianchu Chen <flynnnchen@tencent.com>
>
> goodix_hid_set_raw_report() builds a protocol frame in a 128-byte stack
> buffer (tmp_buf), writing an 11-12 byte header followed by the
> caller-supplied report data. The HID core caps report size at
> HID_MAX_BUFFER_SIZE (16384) by default, while the driver does not set
> hid_ll_driver.max_buffer_size and performs no bounds checking before
> copying the payload:
>
> memcpy(tmp_buf + tx_len, buf, len);
>
> A hidraw SET_REPORT ioctl with a report larger than ~116 bytes
> overflows the stack buffer.
>
> Add a size check after constructing the header, rejecting reports that
> would exceed the buffer capacity.
>
> Discovered by Atuin - Automated Vulnerability Discovery Engine.
>
> Fixes: 75e16c8ce283 ("HID: hid-goodix: Add Goodix HID-over-SPI driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tianchu Chen <flynnnchen@tencent.com>
> ---
> drivers/hid/hid-goodix-spi.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/hid/hid-goodix-spi.c b/drivers/hid/hid-goodix-spi.c
> index 80c0288a3..288cb827e 100644
> --- a/drivers/hid/hid-goodix-spi.c
> +++ b/drivers/hid/hid-goodix-spi.c
> @@ -520,6 +520,9 @@ static int goodix_hid_set_raw_report(struct hid_device *hid,
> memcpy(tmp_buf + tx_len, args, args_len);
> tx_len += args_len;
>
> + if (tx_len + len > sizeof(tmp_buf))
> + return -EINVAL;
> +
We can also consider returning -E2BIG here.
> memcpy(tmp_buf + tx_len, buf, len);
> tx_len += len;
>
In any case:
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
I think we can ignore Sashiko's ramblings on this patch, it needs some
instructions detailing order of operations/timing of callbacks in HID
subsystem.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v10 1/4] HID: hid-msi: Add MSI Claw configuration driver
From: Dmitry Torokhov @ 2026-05-29 18:29 UTC (permalink / raw)
To: Derek John Clark
Cc: Jiri Kosina, Benjamin Tissoires, Pierre-Loup A . Griffais,
Denis Benato, Zhouwang Huang, linux-input, linux-doc,
linux-kernel
In-Reply-To: <CAFqHKT=zRMW4gu09xz2WAukjXB0i9d-z-SfkxU67yJkCA0DZvQ@mail.gmail.com>
On Thu, May 28, 2026 at 11:34:18PM -0700, Derek John Clark wrote:
> On Wed, May 27, 2026 at 10:32 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Hi Derek,
> >
> > On Wed, May 27, 2026 at 10:21:19PM +0000, Derek J. Clark wrote:
> > > Adds configuration HID driver for the MSI Claw series of handheld PC's.
> > > In this initial patch add the initial driver outline and attributes for
> > > changing the gamepad mode, M-key behavior, and add a WO reset function.
> > >
> > > Sending the SWITCH_MODE and RESET commands causes a USB disconnect in
> > > the device. The completion will therefore never get hit and would trigger
> > > an -EIO. To avoid showing the user an error for every write to these
> > > attrs a bypass for the completion handling is introduced when timeout ==
> > > 0.
> > >
> > > The initial version of this patch was written by Denis Benato, which
> > > contained the initial reverse-engineering and implementation for the
> > > gamepad mode switching. This work was later expanded by Zhouwang Huang
> > > to include more gamepad modes. Finally, I refactored the drivers data
> > > in/out flow and overall format to conform to kernel driver best
> > > practices and style guides. Claude was used as an initial reviewer of
> > > this patch.
> >
> > I wonder why do you need to roll asynchronous probing and asynchronous
> > resume by hand? This I think complicates the driver greatly and forces
> > you to use a ton of works, spinlocks, and checks.
> >
> > Thanks.
> >
>
> Hi Dmitry,
>
> I suppose being asked this means my cover letter and commit
> descriptions need some additional context. The MCU in these Claw
> devices is quite temperamental. There are a few specific issues that
> cause the need for multiple work queues, a serialization mutex, and
> subsequently spinlocks to prevent stale data reads.
>
> 1.) The MCU will halt function if it receives any output reports
> before ~500MS after probe or resume. This can either manifest as the
> device never responding to a command, or it can cause the entire
> system to become unstable and reboot. This creates the need for
> cfg_setup to query the MCU and then add the gamepad attrs, led_mc
> device, and rgb attrs. As a side effect, because a system could
> technically be suspended during that 500ms delay, there exists the
> need to re-queue the work if it was never triggered, hence the resume
> queue.
> 2.) The MCU will not always respond in order if two or more output
> reports are sent within a few ms of each other. Since many of the
> commands use a generic "ACK", or share an "ACK" type but don't provide
> specific context about what sub-function called them, we could
> potentially have cross talk where data is saved in the wrong attribute
> or errors propagate because of a missed message. To get around this
> serialization issue we hold a mutex through a completion triggered in
> raw_event and, for most events, save a state machine on what command
> is expected and what sub-command was the initiator. (I.E. profile
> events handle the M1, M2, RGB, Left rumble, and right rumble). Since
> the state machine is accessed on both sides, we need spinlocks
> guarding the reads. This essentially serializes the data and makes it
> predictable. Using this pattern I haven't had any issues reading from
> or writing to the MCU.
> 3.) Some commands will never return their "ACK" while a completion is
> held, so we have a workaround to basically ignore them and hope the
> command worked. This is only needed for SYNC_TO_ROM, from which we
> don't need to set anything on its "ACK", and switching the gamepad
> mode, which causes USB disconnect/reconnect and the driver fully
> reloads, so we'll never be able to read it anyway.
> 4.) The RGB work queue is used to free the userspace write while the
> completion is held. I found that use without it could stall userspace
> quite significantly if it has multiple writes back to back. I
> experienced this using Steam's customization menu, which sends a
> single write for every increment of its color and brightness sliders.
> when traversing the full length of the slider it is possible to have
> effects changing for nearly a minute after stopping. With the queue,
> only the most recent write is eventually sent to the MCU. This issue
> also affects the Go 2 driver as well, though not to the same extent,
> but for which I'll be adding a similar de-bounce queue soon. Go S is
> also technically affected by this bug, but that returns quickly enough
> that it isn't really feasible to trigger the bug with much frequency.
> I'll still fix that one as well though.
>
> TBH I'm not "happy" with the complexity of the driver, but I don't see
> a reasonable alternative. If you have any specific suggestions that I
> could try that might simplify it, I'd be more than happy to give it a
> shot. That being said, I'm not very optimistic about it. Development
> on this device has been like wrestling a bear.
Thank you for this detailed explanation. I would like to concentrate on
the #1 first. What happens in the driver is you are essentially rolling
asynchronous probing and asynchronous suspend/resume in the driver
itself, and end up fighting with the kernel and the driver core
specifically.
As far as suspend/resume goes: HID subsystem already enables
asynchronous resume handling (checkout the call to
device_enable_async_suspend() in
drivers/hid/hid-core.c::hid_allocate_device()). Therefore I think you
just need to stick the necessary delay in your resume method() and call
it a day.
For the probing I would look into annotating the driver as
PROBE_PREFER_ASYNCHRONOUS and relying on that. Again, if you stick the
required delay in probe then sysfs attributes will not be created too
early, same for the rest of concerns with the device being exposed to
userspace before it is ready to handle requests.
If there are issues with HID subsystem honoring
PROBE_PREFER_ASYNCHRONOUS I would look into fixing the subsystem rather
than try to work around it in the driver.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] HID: i2c-hid-of: Fall back to ACPI _DSM when hid-descr-addr is missing
From: Hans de Goede @ 2026-05-29 15:00 UTC (permalink / raw)
To: 谢致邦 (XIE Zhibang), linux-input
Cc: bentiss, dianders, jikos, linux-kernel, superm1
In-Reply-To: <tencent_ECC71F91403F5AC69E1E6B963FA667ABF607@qq.com>
Hi,
On 29-May-26 14:16, 谢致邦 (XIE Zhibang) wrote:
> Before commit b33752c30023 ("HID: i2c-hid: Reorganize so ACPI and OF are
> separate modules"), the unified i2c-hid driver handled both PNP0C50 ACPI
> devices and hid-over-i2c OF devices. After the split, devices with _HID
> "PRP0001" and _DSD compatible "hid-over-i2c" are only probed by
> i2c_hid_of, which requires "hid-descr-addr" in the _DSD. Some devices,
> for example the Lenovo KaiTian N60d and Inspur CP300L3, provide the HID
> descriptor address only through the _DSM method. Fall back to the _DSM
> call when the property is absent.
>
> Fixes: b33752c30023 ("HID: i2c-hid: Reorganize so ACPI and OF are separate modules")
> Signed-off-by: 谢致邦 (XIE Zhibang) <Yeking@Red54.com>
Thank you for the new patch, this is an interesting approach and better
then the modalias magic from the previous version.
Note I'm not the i2c-hid maintainer, with that said I think this should
be acceptable. But currently it duplicates the _DSM handling code and
that should be fixed.
I think this should be changed to a series of 3 patches:
1. Move the i2c_hid_acpi_blacklist handling out of
i2c_hid_acpi_get_descriptor() into i2c_hid_acpi_probe()
to above the devm_kzalloc() call.
2. Move i2c_hid_acpi_get_descriptor() to a generic
int i2c_hid_acpi_get_descriptor(struct device *dev)
helper in drivers/hid/i2c-hid/i2c-hid-core.c .
Wrapped in #ifdef CONFIG_ACPI and with a static inline
stub in drivers/hid/i2c-hid/i2c-hid.h when CONFIG_ACPI
is not set, e.g. in drivers/hid/i2c-hid/i2c-hid.h add:
#ifdef CONFIG_ACPI
int i2c_hid_acpi_get_descriptor_address(struct device *dev);
#else
static inline int i2c_hid_acpi_get_descriptor_address(struct device *dev)
{
return -ENODEV;
}
#endif
3. Modify i2c-hid-of.c to try i2c_hid_acpi_get_descriptor_address() as
fallback for the missing "hid-descr-addr" property. Please also add
a comment in the code explaining that this fallback is about ACPI I2C-hid
devices which use a "PRP0001" ACPI _HID with an "hid-over-i2c" compatible.
Regards,
Hans
> ---
> drivers/hid/i2c-hid/i2c-hid-of.c | 44 ++++++++++++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
> index 57379b77e977..62c089a6455a 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-of.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of.c
> @@ -19,6 +19,7 @@
> * more details.
> */
>
> +#include <linux/acpi.h>
> #include <linux/delay.h>
> #include <linux/device.h>
> #include <linux/gpio/consumer.h>
> @@ -74,6 +75,39 @@ static void i2c_hid_of_power_down(struct i2chid_ops *ops)
> ihid_of->supplies);
> }
>
> +#ifdef CONFIG_ACPI
> +/* HID I²C Device: 3cdff6f7-4267-4555-ad05-b30a3d8938de */
> +static guid_t i2c_hid_of_acpi_guid =
> + GUID_INIT(0x3CDFF6F7, 0x4267, 0x4555,
> + 0xAD, 0x05, 0xB3, 0x0A, 0x3D, 0x89, 0x38, 0xDE);
> +
> +/*
> + * Some devices, for example the Lenovo KaiTian N60d and Inspur CP300L3,
> + * declare their I2C HID touchpad with _HID "PRP0001" and _DSD compatible
> + * "hid-over-i2c" but lack the "hid-descr-addr" property. Use the _DSM
> + * method to obtain the HID descriptor address.
> + */
> +static int i2c_hid_of_acpi_get_descriptor(struct device *dev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> + union acpi_object *obj;
> + u16 addr;
> +
> + if (!adev)
> + return -ENODEV;
> +
> + obj = acpi_evaluate_dsm_typed(acpi_device_handle(adev),
> + &i2c_hid_of_acpi_guid, 1, 1, NULL,
> + ACPI_TYPE_INTEGER);
> + if (!obj)
> + return -ENODEV;
> +
> + addr = obj->integer.value;
> + ACPI_FREE(obj);
> + return addr;
> +}
> +#endif
> +
> static int i2c_hid_of_probe(struct i2c_client *client)
> {
> struct device *dev = &client->dev;
> @@ -92,6 +126,16 @@ static int i2c_hid_of_probe(struct i2c_client *client)
> ihid_of->ops.power_down = i2c_hid_of_power_down;
>
> ret = device_property_read_u32(dev, "hid-descr-addr", &val);
> +#ifdef CONFIG_ACPI
> + if (ret) {
> + int dsm_ret = i2c_hid_of_acpi_get_descriptor(dev);
> +
> + if (dsm_ret >= 0) {
> + val = dsm_ret;
> + ret = 0;
> + }
> + }
> +#endif
> if (ret) {
> dev_err(dev, "HID register address not provided\n");
> return -ENODEV;
^ permalink raw reply
* Re: [PATCH] HID: hid-goodix-spi: validate report size to prevent stack buffer overflow
From: sashiko-bot @ 2026-05-29 14:27 UTC (permalink / raw)
To: Tianchu Chen; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <f7e444a3facbe5fb2627167ab205771476e46bc8@linux.dev>
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues:
- [Medium] The patch artificially caps SET_REPORT payload size to a 128-byte stack buffer, potentially breaking legitimate large HID reports.
Pre-existing issues:
- [High] Concurrent reallocation of `ts->event_buf` in `goodix_hid_start` and access in `goodix_hid_irq` leads to a Use-After-Free.
- [Medium] Uninitialized kernel memory is transmitted over the SPI bus during read operations.
- [Medium] The driver performs SPI reads and writes without verifying the hardware suspend state.
- [Medium] In `goodix_spi_resume()`, the IRQ is enabled before the device is powered on.
- [Medium] Unsafe assignment of `devm_krealloc` directly to the buffer pointer causes a device-lifetime memory leak.
--
commit a3440b67c988b5226da753835060ad3d93591c74
Author: Tianchu Chen <flynnnchen@tencent.com>
Subject: HID: hid-goodix-spi: validate report size to prevent stack buffer overflow
This commit adds a bounds check in goodix_hid_set_raw_report() to ensure that
the constructed protocol frame does not exceed the size of the 128-byte stack
buffer tmp_buf. If a caller supplies a report that exceeds this limit, the
function now returns -EINVAL to prevent a stack buffer overflow.
> diff --git a/drivers/hid/hid-goodix-spi.c b/drivers/hid/hid-goodix-spi.c
> index 80c0288a3a38b..288cb827e9d6d 100644
> --- a/drivers/hid/hid-goodix-spi.c
> +++ b/drivers/hid/hid-goodix-spi.c
> @@ -520,6 +520,9 @@ static int goodix_hid_set_raw_report(struct hid_device *hid,
> memcpy(tmp_buf + tx_len, args, args_len);
> tx_len += args_len;
>
> + if (tx_len + len > sizeof(tmp_buf))
> + return -EINVAL;
[Severity: Medium]
Does returning `-EINVAL` here artificially cap the SET_REPORT payload size?
If the device's HID report descriptor defines larger output or feature reports
(e.g., for firmware updates or configuration), would this fix block them?
Could we dynamically allocate a buffer of adequate size or use the already
available 2KB `ts->xfer_buf` instead of relying on a hardcoded stack limit?
> +
> memcpy(tmp_buf + tx_len, buf, len);
> tx_len += len;
>
[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk with
`ts->event_buf` between `goodix_hid_irq()` and `goodix_hid_start()`?
If the HID driver is unbound and rebound, `goodix_hid_start()` will execute
`devm_krealloc()`, freeing the old `ts->event_buf`:
drivers/hid/hid-goodix-spi.c:goodix_hid_start() {
...
ts->event_buf = devm_krealloc(ts->dev, ts->event_buf,
report_size, GFP_KERNEL);
...
}
Meanwhile, if `goodix_hid_close()` clears the `GOODIX_HID_STARTED` flag but
does not synchronize with the threaded IRQ, a preempted IRQ handler can pass
the flag check and then use the now-freed `ts->event_buf`:
drivers/hid/hid-goodix-spi.c:goodix_hid_irq() {
...
if (!test_bit(GOODIX_HID_STARTED, &ts->flags))
return IRQ_HANDLED;
...
event = goodix_get_event_report(ts, ts->hid_report_addr, ts->event_buf,
GOODIX_HID_ACK_HEADER_SIZE +
GOODIX_HID_COOR_PKG_LEN);
...
}
[Severity: Medium]
This is a pre-existing issue, but does assigning the result of
`devm_krealloc()` directly to `ts->event_buf` cause a memory leak if it fails?
drivers/hid/hid-goodix-spi.c:goodix_hid_start() {
...
ts->event_buf = devm_krealloc(ts->dev, ts->event_buf,
report_size, GFP_KERNEL);
...
}
If `devm_krealloc()` returns `NULL`, the original memory chunk remains in the
`devres` list, but the pointer in `ts->event_buf` is overwritten. Subsequent
re-binds would allocate a new devres chunk instead of reusing the old one,
accumulating leaked chunks until the SPI device is unbound.
[Severity: Medium]
This is a pre-existing issue, but does this transmit uninitialized kernel
memory over the SPI MOSI line during read operations?
In `goodix_get_event_report()`, `ts->event_buf` is used as the transaction
buffer, and it is allocated with `devm_kmalloc()` (which doesn't zero memory).
The first 5 bytes are initialized, but the dummy bytes and read payload
section are left uninitialized:
drivers/hid/hid-goodix-spi.c:goodix_get_event_report() {
...
data[0] = GOODIX_SPI_READ_FLAG;
put_unaligned_be32(addr, data + GOODIX_SPI_TRANS_PREFIX_LEN);
spi_message_init(&spi_msg);
memset(&xfers, 0, sizeof(xfers));
xfers.tx_buf = data;
xfers.rx_buf = data;
xfers.len = GOODIX_SPI_READ_PREFIX_LEN + len;
...
}
Because `xfers.tx_buf` is provided for the entire transfer length, is the
uninitialized memory sent to the hardware during the dummy and read phases,
resulting in an information leak?
[Severity: Medium]
This is a pre-existing issue, but does `goodix_hid_raw_request()` perform SPI
operations without verifying the hardware suspend state?
drivers/hid/hid-goodix-spi.c:goodix_hid_raw_request() {
...
guard(mutex)(&ts->hid_request_lock);
switch (reqtype) {
case HID_REQ_GET_REPORT:
error = goodix_hid_get_raw_report(hid, reportnum, buf,
len, rtype);
break;
case HID_REQ_SET_REPORT:
if (buf[0] == reportnum)
error = goodix_hid_set_raw_report(hid, reportnum,
buf, len, rtype);
break;
...
}
If a request is issued while the device is in the `GOODIX_SPI_POWER_SLEEP`
state, the driver will directly perform SPI read/write operations on the
powered-down hardware. Could this cause timeouts or undefined hardware states?
[Severity: Medium]
This is a pre-existing issue, but should the IRQ be enabled before the device
is powered on during resume?
drivers/hid/hid-goodix-spi.c:goodix_spi_resume() {
...
enable_irq(ts->spi->irq);
return goodix_spi_set_power(ts, GOODIX_SPI_POWER_ON);
}
If a spurious interrupt or power-on glitch asserts the IRQ line while the
device is still in the sleep state, the interrupt handler will execute and
attempt to perform SPI reads on the waking hardware. Would it be safer to
bring the device to an active power state before unmasking its interrupts?
--
Sashiko AI review · https://sashiko.dev/#/patchset/f7e444a3facbe5fb2627167ab205771476e46bc8@linux.dev?part=1
^ permalink raw reply
* [PATCH] HID: hid-goodix-spi: validate report size to prevent stack buffer overflow
From: Tianchu Chen @ 2026-05-29 13:42 UTC (permalink / raw)
To: jikos, bentiss; +Cc: linux-input, linux-kernel, stable
From: Tianchu Chen <flynnnchen@tencent.com>
goodix_hid_set_raw_report() builds a protocol frame in a 128-byte stack
buffer (tmp_buf), writing an 11-12 byte header followed by the
caller-supplied report data. The HID core caps report size at
HID_MAX_BUFFER_SIZE (16384) by default, while the driver does not set
hid_ll_driver.max_buffer_size and performs no bounds checking before
copying the payload:
memcpy(tmp_buf + tx_len, buf, len);
A hidraw SET_REPORT ioctl with a report larger than ~116 bytes
overflows the stack buffer.
Add a size check after constructing the header, rejecting reports that
would exceed the buffer capacity.
Discovered by Atuin - Automated Vulnerability Discovery Engine.
Fixes: 75e16c8ce283 ("HID: hid-goodix: Add Goodix HID-over-SPI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Tianchu Chen <flynnnchen@tencent.com>
---
drivers/hid/hid-goodix-spi.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/hid/hid-goodix-spi.c b/drivers/hid/hid-goodix-spi.c
index 80c0288a3..288cb827e 100644
--- a/drivers/hid/hid-goodix-spi.c
+++ b/drivers/hid/hid-goodix-spi.c
@@ -520,6 +520,9 @@ static int goodix_hid_set_raw_report(struct hid_device *hid,
memcpy(tmp_buf + tx_len, args, args_len);
tx_len += args_len;
+ if (tx_len + len > sizeof(tmp_buf))
+ return -EINVAL;
+
memcpy(tmp_buf + tx_len, buf, len);
tx_len += len;
--
2.51.0
^ permalink raw reply related
* [PATCH] HID: i2c-hid-of: Fall back to ACPI _DSM when hid-descr-addr is missing
From: 谢致邦 (XIE Zhibang) @ 2026-05-29 12:16 UTC (permalink / raw)
To: linux-input, hansg
Cc: Yeking, bentiss, dianders, jikos, linux-kernel, superm1
In-Reply-To: <9e5232a7-5b8b-4025-8a31-108eb164861e@kernel.org>
Before commit b33752c30023 ("HID: i2c-hid: Reorganize so ACPI and OF are
separate modules"), the unified i2c-hid driver handled both PNP0C50 ACPI
devices and hid-over-i2c OF devices. After the split, devices with _HID
"PRP0001" and _DSD compatible "hid-over-i2c" are only probed by
i2c_hid_of, which requires "hid-descr-addr" in the _DSD. Some devices,
for example the Lenovo KaiTian N60d and Inspur CP300L3, provide the HID
descriptor address only through the _DSM method. Fall back to the _DSM
call when the property is absent.
Fixes: b33752c30023 ("HID: i2c-hid: Reorganize so ACPI and OF are separate modules")
Signed-off-by: 谢致邦 (XIE Zhibang) <Yeking@Red54.com>
---
drivers/hid/i2c-hid/i2c-hid-of.c | 44 ++++++++++++++++++++++++++++++++
1 file changed, 44 insertions(+)
diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
index 57379b77e977..62c089a6455a 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of.c
@@ -19,6 +19,7 @@
* more details.
*/
+#include <linux/acpi.h>
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/gpio/consumer.h>
@@ -74,6 +75,39 @@ static void i2c_hid_of_power_down(struct i2chid_ops *ops)
ihid_of->supplies);
}
+#ifdef CONFIG_ACPI
+/* HID I²C Device: 3cdff6f7-4267-4555-ad05-b30a3d8938de */
+static guid_t i2c_hid_of_acpi_guid =
+ GUID_INIT(0x3CDFF6F7, 0x4267, 0x4555,
+ 0xAD, 0x05, 0xB3, 0x0A, 0x3D, 0x89, 0x38, 0xDE);
+
+/*
+ * Some devices, for example the Lenovo KaiTian N60d and Inspur CP300L3,
+ * declare their I2C HID touchpad with _HID "PRP0001" and _DSD compatible
+ * "hid-over-i2c" but lack the "hid-descr-addr" property. Use the _DSM
+ * method to obtain the HID descriptor address.
+ */
+static int i2c_hid_of_acpi_get_descriptor(struct device *dev)
+{
+ struct acpi_device *adev = ACPI_COMPANION(dev);
+ union acpi_object *obj;
+ u16 addr;
+
+ if (!adev)
+ return -ENODEV;
+
+ obj = acpi_evaluate_dsm_typed(acpi_device_handle(adev),
+ &i2c_hid_of_acpi_guid, 1, 1, NULL,
+ ACPI_TYPE_INTEGER);
+ if (!obj)
+ return -ENODEV;
+
+ addr = obj->integer.value;
+ ACPI_FREE(obj);
+ return addr;
+}
+#endif
+
static int i2c_hid_of_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
@@ -92,6 +126,16 @@ static int i2c_hid_of_probe(struct i2c_client *client)
ihid_of->ops.power_down = i2c_hid_of_power_down;
ret = device_property_read_u32(dev, "hid-descr-addr", &val);
+#ifdef CONFIG_ACPI
+ if (ret) {
+ int dsm_ret = i2c_hid_of_acpi_get_descriptor(dev);
+
+ if (dsm_ret >= 0) {
+ val = dsm_ret;
+ ret = 0;
+ }
+ }
+#endif
if (ret) {
dev_err(dev, "HID register address not provided\n");
return -ENODEV;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v11 4/4] HID: hid-msi: Add Rumble Intensity Attributes
From: sashiko-bot @ 2026-05-29 9:53 UTC (permalink / raw)
To: Derek J. Clark; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260529072111.7565-5-derekjohn.clark@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Unwaited ACKs from SYNC_TO_ROM commands can prematurely complete the waits of subsequent unrelated commands.
--
commit 11fcab6387a146514df018948771932cc7f57ed2
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add Rumble Intensity Attributes
This commit adds intensity adjustment attributes for the left and right
rumble motors on MSI Claw devices.
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 96710d80543d4..f5c54cd62f3bb 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> @@ -930,6 +962,162 @@ static ssize_t button_mapping_options_show(struct device *dev,
[ ... ]
> +static ssize_t rumble_intensity_left_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
[ ... ]
> + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
> + (u8 *)&report, sizeof(report), 25);
> + if (ret)
> + return ret;
> +
> + /* MCU will not send ACK until the USB transaction completes. ACK is sent
> + * immediately after and will hit the stale state machine, before the next
> + * command re-arms the state machine. Timeout 0 ensures no deadlock waiting
> + * for ACK that ill never come.
> + */
> + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 0);
[Severity: High]
This is a pre-existing issue, but this patch propagates it to the new rumble
attributes.
If a SYNC_TO_ROM command is sent with a timeout of 0, the driver does not
wait for the hardware's ACK, but the hardware still sends a generic ACK
packet for it.
If userspace rapidly writes to multiple sysfs attributes (for example,
rumble_intensity_left then rumble_intensity_right), the second write will
send a WRITE_PROFILE_DATA command and wait for its ACK. Can the stray ACK
from the first SYNC_TO_ROM command arrive during this wait window?
Since the generic ACK doesn't indicate which command it belongs to, would
this cause the driver to falsely assume the WRITE_PROFILE_DATA command has
completed and prematurely wake the waiter? This could cause the driver to
send the next command while the hardware is still processing the previous
one, leading to dropped commands and device state desynchronization.
> + if (ret)
> + return ret;
> +
> + return count;
> +}
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260529072111.7565-1-derekjohn.clark@gmail.com?part=4
^ permalink raw reply
* Re: [PATCH v11 3/4] HID: hid-msi: Add RGB control interface
From: sashiko-bot @ 2026-05-29 9:13 UTC (permalink / raw)
To: Derek J. Clark; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260529072111.7565-4-derekjohn.clark@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] A race condition in `claw_remove()` allows the `rgb_queue` delayed work to be re-armed after it has been cancelled, leading to a Use-After-Free of the `drvdata` structure.
- [Medium] A hardcoded LED class device name prevents supporting multiple connected devices.
--
commit 151b50cdc17e18af52c3d79eb6a71e3b5e4583d8
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add RGB control interface
Adds RGB control interface for MSI Claw devices using a unique frame-based
RGB protocol. Mirrors Windows effects using a delayed work queue to apply
state to the device.
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> @@ -954,6 +1572,7 @@ static int claw_probe(struct hid_device *hdev, u8 ep)
[ ... ]
> if (!drvdata->bmap_support)
> dev_dbg(&hdev->dev, "M-Key mapping is not supported. Update firmware to enable.\n");
>
> + drvdata->led_mc.led_cdev.name = "msi_claw:rgb:joystick_rings";
[Severity: Medium]
Does hardcoding the LED class device name prevent multiple MSI Claw devices
from being connected at the same time?
If two devices are connected, it seems the second one will fail to register
its LED class device due to a sysfs name collision. Would it be safer to
include a unique identifier like dev_name(&hdev->dev) in the string?
[ ... ]
> @@ -1030,6 +1662,7 @@ static void claw_remove(struct hid_device *hdev)
[ ... ]
> if (gp_registered)
> device_remove_group(&hdev->dev, &claw_gamepad_attr_group);
>
> + cancel_delayed_work_sync(&drvdata->rgb_queue);
> + if (rgb_registered) {
> + device_remove_group(drvdata->led_mc.led_cdev.dev, &claw_rgb_attr_group);
> + led_classdev_multicolor_unregister(&drvdata->led_mc);
> + }
> +
> hid_hw_close(hdev);
> }
[Severity: High]
Can a race condition here lead to a use-after-free of drvdata?
In sysfs store functions like effect_store(), rgb_registered is checked
under the registration_lock, but mod_delayed_work() is called after dropping
the lock.
If a sysfs thread is preempted right after dropping the lock, claw_remove()
could set rgb_registered to false, execute cancel_delayed_work_sync(), and
block on device_remove_group() waiting for the sysfs operation to finish.
Once the sysfs thread resumes, it would call mod_delayed_work(), effectively
re-arming the workqueue after it was supposed to be cancelled. When
claw_remove() completes, drvdata is freed, and the delayed work timer firing
later would result in a use-after-free.
Would moving cancel_delayed_work_sync() after the device_remove_group() and
led_classdev_multicolor_unregister() calls prevent this race?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260529072111.7565-1-derekjohn.clark@gmail.com?part=3
^ permalink raw reply
* Re: [PATCH v11 2/4] HID: hid-msi: Add M-key mapping attributes
From: sashiko-bot @ 2026-05-29 8:40 UTC (permalink / raw)
To: Derek J. Clark; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260529072111.7565-3-derekjohn.clark@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Timeout of READ_PROFILE commands leaves profile_pending dirty and falsely aborts subsequent requests due to unhandled stale ACKs.
- [High] Abandoning the ACK for CLAW_COMMAND_TYPE_SYNC_TO_ROM leaves a stale ACK in the USB input queue, falsely completing subsequent commands.
- [High] Unsafe spinlock acquisition without disabling interrupts in HID raw_event callback causes deadlocks.
--
commit ae4697cc229899bfbbf1b0ea18f1f026bc1b15e7
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add M-key mapping attributes
This commit adds attributes to allow remapping the M-keys with up to 5 values
when the device is in macro mode. When mapped, these events can fire from
the xpad device or the evdev interfaces provided by the input core.
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 3a176f44f2c43..f099d6cb7ea8d 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> @@ -137,6 +321,39 @@ static int claw_gamepad_mode_event(struct claw_drvdata *drvdata,
> return 0;
> }
>
> +static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_report *cmd_rep)
> +{
> + enum claw_profile_ack_pending profile;
> + struct claw_mkey_report *mkeys;
> + u8 *codes, key;
> + int i;
> +
> + scoped_guard(spinlock, &drvdata->profile_lock)
^^^^^^^^
[Severity: High]
Could this cause a deadlock?
HID raw_event callbacks can be invoked from process context with IRQs
enabled (e.g., when input is simulated via the uhid character device).
If an actual hardware interrupt (such as a USB URB completion) occurs on
the same CPU while the lock is held in process context, the interrupt handler
will invoke raw_event and attempt to reacquire profile_lock, resulting
in a deadlock.
Should this use spinlock_irqsave instead?
> + profile = drvdata->profile_pending;
> +
> + switch (profile) {
> + case CLAW_M1_PENDING:
[ ... ]
> + dev_dbg(&drvdata->hdev->dev,
> + "Got profile event without changes pending from command: %x\n",
> + cmd_rep->cmd);
> + return -EINVAL;
> + }
> + scoped_guard(spinlock, &drvdata->profile_lock)
^^^^^^^^
[Severity: High]
Same question here, could this deadlock if interrupted by a hardware
interrupt on the same CPU?
> + drvdata->profile_pending = CLAW_NO_PENDING;
> +
> + return 0;
> +}
[ ... ]
> +static int claw_mkey_store(struct device *dev, const char *buf, u8 mkey)
> +{
[ ... ]
> +set_buttons:
> + scoped_guard(mutex, &drvdata->rom_mutex) {
> + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
> + (u8 *)&report, sizeof(report), 25);
> + if (ret)
> + return ret;
> + /* MCU will not send ACK until the USB transaction completes. ACK is sent
> + * immediately after and will hit the stale state machine, before the next
> + * command re-arms the state machine. Timeout 0 ensures no deadlock waiting
> + * for ACK that ill never come.
> + */
> + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 0);
[Severity: High]
If the ACK for CLAW_COMMAND_TYPE_SYNC_TO_ROM is abandoned, could it leave a
stale ACK in the USB input queue?
If userspace rapidly writes to the sysfs attributes (e.g., configuring
button_m1 then button_m2), the second write would acquire rom_mutex, send
WRITE_PROFILE_DATA, and set waiting_for_ack = true.
When the late ACK from the preceding SYNC_TO_ROM command arrives,
claw_raw_event() might process it as a generic CLAW_COMMAND_TYPE_ACK and
prematurely complete the wait for the new WRITE_PROFILE_DATA command.
The driver might then immediately send the next SYNC_TO_ROM while the MCU
may still be processing the profile data, potentially leading to dropped
configurations or MCU instability.
> + }
> +
> + return ret;
> +}
[ ... ]
> +static int claw_mkey_show(struct device *dev, char *buf, enum claw_key_index m_key)
> +{
> + struct hid_device *hdev = to_hid_device(dev);
[ ... ]
> + guard(mutex)(&drvdata->profile_mutex);
> + scoped_guard(spinlock_irqsave, &drvdata->profile_lock)
> + drvdata->profile_pending = (m_key == CLAW_KEY_M1) ? CLAW_M1_PENDING
> + : CLAW_M2_PENDING;
> +
> + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_PROFILE,
> + (u8 *)&report, sizeof(report), 25);
> + if (ret)
> + return ret;
[Severity: High]
If claw_hw_output_report() times out, could this leave profile_pending
dirty?
If it returns early without resetting drvdata->profile_pending to
CLAW_NO_PENDING, and another process subsequently invokes claw_mkey_show()
for a different key, it would overwrite profile_pending and begin waiting
for a new ACK.
If the late ACK from the initial timed-out request arrives during this
window, claw_profile_event() processes it, detects an address mismatch
(since it evaluates against the new profile_pending state), and returns
-EINVAL.
claw_raw_event() would then blindly propagate this error to cmd_status and
call complete(), falsely aborting the valid, ongoing read.
> +
> + for (i = 0; i < CLAW_KEYS_MAX; i++) {
> + name = mkey_mapping_code_to_name(codes[i]);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260529072111.7565-1-derekjohn.clark@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH v11 1/4] HID: hid-msi: Add MSI Claw configuration driver
From: sashiko-bot @ 2026-05-29 7:59 UTC (permalink / raw)
To: Derek J. Clark; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260529072111.7565-2-derekjohn.clark@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Mixing `goto`-based error handling with scope-based cleanup (`__free`/`guard`).
- [High] TOCTOU race condition causing lost device configuration updates.
--
commit 5c893bbc0296560ff4fc46c1a17db05f45daeaf6
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add MSI Claw configuration driver
This commit adds a configuration HID driver for the MSI Claw series of
handheld PCs. It introduces sysfs attributes to change the gamepad mode,
M-key behavior, and perform a device reset.
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> +static int claw_hw_output_report(struct hid_device *hdev, u8 index, u8 *data,
> + size_t len, unsigned int timeout)
> +{
> + unsigned char *dmabuf __free(kfree) = NULL;
> + u8 header[] = { CLAW_OUTPUT_REPORT_ID, 0, 0, 0x3c, index };
[ ... ]
> + guard(mutex)(&drvdata->cfg_mutex);
> + if (timeout) {
[ ... ]
> + ret = hid_hw_output_report(hdev, dmabuf, CLAW_PACKET_SIZE);
> + if (ret < 0)
> + goto err_clear_state;
[Severity: High]
Is it intended to mix goto-based error handling with scope-based cleanup
in this function?
The cleanup subsystem guidelines mention that functions should either
convert all resources that need cleanup to scope-based cleanup, or convert
none of them. Using guard and __free alongside a goto label can create
confusing ownership and lock lifetime semantics.
Could this be converted entirely to scope-based cleanup?
> +
> + ret = ret == CLAW_PACKET_SIZE ? 0 : -EIO;
> + if (ret)
> + goto err_clear_state;
[ ... ]
> +err_clear_state:
> + if (timeout) {
> + guard(spinlock_irqsave)(&drvdata->cmd_lock);
> + drvdata->waiting_cmd = CLAW_COMMAND_TYPE_NONE;
> + drvdata->waiting_for_ack = false;
> + }
> +
> + return ret;
> +}
[ ... ]
> +static ssize_t gamepad_mode_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
[ ... ]
> + data[0] = ret;
> + scoped_guard(spinlock_irqsave, &drvdata->mode_lock)
> + data[1] = drvdata->mkeys_function;
> +
> + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data, ARRAY_SIZE(data), 0);
[Severity: High]
Does this sequence introduce a time-of-check to time-of-use race condition?
The current state of mkeys_function is read while holding mode_lock, but the
lock is released before claw_hw_output_report() acquires cfg_mutex. If two
threads concurrently write to gamepad_mode and mkeys_function, both could
read the other's state before either acquires cfg_mutex.
This could result in the second thread sending a command that uses stale
data, silently reverting the first thread's update when the device
acknowledges the command.
Should the read of the current state and the device update be atomic under
a single lock?
> + if (ret)
> + return ret;
> +
> + return count;
> +}
[ ... ]
> +static ssize_t mkeys_function_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
[ ... ]
> + scoped_guard(spinlock_irqsave, &drvdata->mode_lock)
> + data[0] = drvdata->gamepad_mode;
> + data[1] = ret;
> +
> + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data, ARRAY_SIZE(data), 0);
[Severity: High]
A similar sequence happens here. Can concurrent writes to these attributes
lead to lost device configuration updates due to the locks being acquired and
released separately?
> + if (ret)
> + return ret;
> +
> + return count;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260529072111.7565-1-derekjohn.clark@gmail.com?part=1
^ permalink raw reply
* [PATCH v11 4/4] HID: hid-msi: Add Rumble Intensity Attributes
From: Derek J. Clark @ 2026-05-29 7:21 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Pierre-Loup A . Griffais, Denis Benato, Zhouwang Huang,
Derek J . Clark, linux-input, linux-doc, linux-kernel
In-Reply-To: <20260529072111.7565-1-derekjohn.clark@gmail.com>
Adds intensity adjustment for the left and right rumble motors.
Claude was used during the reverse-engineering data gathering for this
feature done by Zhouwang Huang. As the code had already been affected,
I used Claude to create the initial framing for the feature, then did
manual cleanup of the _show and _store functions afterwards to fix bugs
and keep the coding style consistent. Claude was also used as an initial
reviewer of this patch.
Assisted-by: Claude:claude-sonnet-4-6
Co-developed-by: Zhouwang Huang <honjow311@gmail.com>
Signed-off-by: Zhouwang Huang <honjow311@gmail.com>
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
v11:
- Restore dropped changes from v10.
v7:
- Match on write address for rumble reports to prevent late ACK
from causing synchronization errors.
- Use spinlock for read/write profile_pending.
- Use smp_[store_release|load_acquire] pattern for checking
gamepad_registered to avoid possible races during teardown.
- Use struct for rumble reports.
v6:
- Make all timeouts 25ms to ensure at least 2 jiffies in a 100Hz
config.
- Add spinlock_irqsave for read/write access on rumble_intensity
variables.
- Gate all attribute show/store functions with gamepad_registered.
v5:
- Remove mkey related changes.
v2:
- Use pending_profile and sync to rom mutexes.
---
drivers/hid/hid-msi.c | 202 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 201 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
index 96710d80543d4..f5c54cd62f3bb 100644
--- a/drivers/hid/hid-msi.c
+++ b/drivers/hid/hid-msi.c
@@ -79,6 +79,8 @@ enum claw_profile_ack_pending {
CLAW_M1_PENDING,
CLAW_M2_PENDING,
CLAW_RGB_PENDING,
+ CLAW_RUMBLE_LEFT_PENDING,
+ CLAW_RUMBLE_RIGHT_PENDING,
};
enum claw_key_index {
@@ -266,6 +268,11 @@ static const u16 button_mapping_addr_new[] = {
static const u16 rgb_addr_old = 0x01fa;
static const u16 rgb_addr_new = 0x024a;
+static const u16 rumble_addr[] = {
+ 0x0022, /* left */
+ 0x0023, /* right */
+};
+
struct claw_command_report {
u8 report_id;
u8 padding[2];
@@ -308,6 +315,12 @@ struct claw_rgb_report {
struct rgb_frame zone_data;
} __packed;
+struct claw_rumble_report {
+ struct claw_profile_report;
+ u8 padding;
+ u8 intensity;
+} __packed;
+
struct claw_drvdata {
/* MCU General Variables */
enum claw_profile_ack_pending profile_pending;
@@ -332,8 +345,12 @@ struct claw_drvdata {
enum claw_gamepad_mode_index gamepad_mode;
u8 m1_codes[CLAW_KEYS_MAX];
u8 m2_codes[CLAW_KEYS_MAX];
- spinlock_t mode_lock; /* Lock for mode data read/write */
+ u8 rumble_intensity_right;
+ u8 rumble_intensity_left;
const u16 *bmap_addr;
+ spinlock_t rumble_lock; /* lock for rumble_intensity read/write */
+ spinlock_t mode_lock; /* Lock for mode data read/write */
+ bool rumble_support;
bool gp_registered;
bool bmap_support;
@@ -382,6 +399,7 @@ static int claw_gamepad_mode_event(struct claw_drvdata *drvdata,
static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_report *cmd_rep)
{
enum claw_profile_ack_pending profile;
+ struct claw_rumble_report *rumble;
struct claw_mkey_report *mkeys;
struct claw_rgb_report *frame;
u16 rgb_addr, read_addr;
@@ -431,6 +449,20 @@ static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_
}
break;
+ case CLAW_RUMBLE_LEFT_PENDING:
+ rumble = (struct claw_rumble_report *)cmd_rep->data;
+ if (be16_to_cpu(rumble->read_addr) != rumble_addr[0])
+ return -EINVAL;
+ scoped_guard(spinlock, &drvdata->rumble_lock)
+ drvdata->rumble_intensity_left = rumble->intensity;
+ break;
+ case CLAW_RUMBLE_RIGHT_PENDING:
+ rumble = (struct claw_rumble_report *)cmd_rep->data;
+ if (be16_to_cpu(rumble->read_addr) != rumble_addr[1])
+ return -EINVAL;
+ scoped_guard(spinlock, &drvdata->rumble_lock)
+ drvdata->rumble_intensity_right = rumble->intensity;
+ break;
default:
dev_dbg(&drvdata->hdev->dev,
"Got profile event without changes pending from command: %x\n",
@@ -930,6 +962,162 @@ static ssize_t button_mapping_options_show(struct device *dev,
}
static DEVICE_ATTR_RO(button_mapping_options);
+static ssize_t rumble_intensity_left_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct claw_rumble_report report = { {0x01, cpu_to_be16(rumble_addr[0])}, 0x01 };
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ u8 val;
+ int ret;
+
+ scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ if (!smp_load_acquire(&drvdata->gp_registered))
+ return -ENODEV;
+ }
+
+ ret = kstrtou8(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ if (val > 100)
+ return -EINVAL;
+
+ report.intensity = val;
+
+ guard(mutex)(&drvdata->rom_mutex);
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
+ (u8 *)&report, sizeof(report), 25);
+ if (ret)
+ return ret;
+
+ /* MCU will not send ACK until the USB transaction completes. ACK is sent
+ * immediately after and will hit the stale state machine, before the next
+ * command re-arms the state machine. Timeout 0 ensures no deadlock waiting
+ * for ACK that ill never come.
+ */
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 0);
+ if (ret)
+ return ret;
+
+ return count;
+}
+
+static ssize_t rumble_intensity_left_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct claw_rumble_report report = { {0x01, cpu_to_be16(rumble_addr[0])}, 0x01 };
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ int ret;
+ u8 val;
+
+ scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ if (!smp_load_acquire(&drvdata->gp_registered))
+ return -ENODEV;
+ }
+
+ guard(mutex)(&drvdata->profile_mutex);
+ scoped_guard(spinlock_irqsave, &drvdata->profile_lock)
+ drvdata->profile_pending = CLAW_RUMBLE_LEFT_PENDING;
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_PROFILE,
+ (u8 *)&report, sizeof(report), 25);
+ if (ret)
+ return ret;
+
+ scoped_guard(spinlock_irqsave, &drvdata->rumble_lock)
+ val = drvdata->rumble_intensity_left;
+
+ return sysfs_emit(buf, "%u\n", val);
+}
+static DEVICE_ATTR_RW(rumble_intensity_left);
+
+static ssize_t rumble_intensity_right_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct claw_rumble_report report = { {0x01, cpu_to_be16(rumble_addr[1])}, 0x01 };
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ u8 val;
+ int ret;
+
+ scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ if (!smp_load_acquire(&drvdata->gp_registered))
+ return -ENODEV;
+ }
+
+ ret = kstrtou8(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ if (val > 100)
+ return -EINVAL;
+
+ report.intensity = val;
+
+ guard(mutex)(&drvdata->rom_mutex);
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
+ (u8 *)&report, sizeof(report), 25);
+ if (ret)
+ return ret;
+
+ /* MCU will not send ACK until the USB transaction completes. ACK is sent
+ * immediately after and will hit the stale state machine, before the next
+ * command re-arms the state machine. Timeout 0 ensures no deadlock waiting
+ * for ACK that ill never come.
+ */
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 0);
+ if (ret)
+ return ret;
+
+ return count;
+}
+
+static ssize_t rumble_intensity_right_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct claw_rumble_report report = { {0x01, cpu_to_be16(rumble_addr[1])}, 0x01 };
+ struct hid_device *hdev = to_hid_device(dev);
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ int ret;
+ u8 val;
+
+ scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ if (!smp_load_acquire(&drvdata->gp_registered))
+ return -ENODEV;
+ }
+
+ guard(mutex)(&drvdata->profile_mutex);
+ scoped_guard(spinlock_irqsave, &drvdata->profile_lock)
+ drvdata->profile_pending = CLAW_RUMBLE_RIGHT_PENDING;
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_PROFILE,
+ (u8 *)&report, sizeof(report), 25);
+ if (ret)
+ return ret;
+
+ scoped_guard(spinlock_irqsave, &drvdata->rumble_lock)
+ val = drvdata->rumble_intensity_right;
+
+ return sysfs_emit(buf, "%u\n", val);
+}
+static DEVICE_ATTR_RW(rumble_intensity_right);
+
+static ssize_t rumble_intensity_range_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ return sysfs_emit(buf, "0-100\n");
+}
+static DEVICE_ATTR_RO(rumble_intensity_range);
+
static umode_t claw_gamepad_attr_is_visible(struct kobject *kobj, struct attribute *attr,
int n)
{
@@ -950,6 +1138,12 @@ static umode_t claw_gamepad_attr_is_visible(struct kobject *kobj, struct attribu
attr == &dev_attr_reset.attr)
return attr->mode;
+ /* Hide rumble attrs if not supported */
+ if (attr == &dev_attr_rumble_intensity_left.attr ||
+ attr == &dev_attr_rumble_intensity_right.attr ||
+ attr == &dev_attr_rumble_intensity_range.attr)
+ return drvdata->rumble_support ? attr->mode : 0;
+
/* Hide button mapping attrs if it isn't supported */
return drvdata->bmap_support ? attr->mode : 0;
}
@@ -963,6 +1157,9 @@ static struct attribute *claw_gamepad_attrs[] = {
&dev_attr_mkeys_function.attr,
&dev_attr_mkeys_function_index.attr,
&dev_attr_reset.attr,
+ &dev_attr_rumble_intensity_left.attr,
+ &dev_attr_rumble_intensity_right.attr,
+ &dev_attr_rumble_intensity_range.attr,
NULL,
};
@@ -1542,6 +1739,7 @@ static void claw_features_supported(struct claw_drvdata *drvdata)
drvdata->bmap_support = true;
if (minor >= 0x66) {
drvdata->bmap_addr = button_mapping_addr_new;
+ drvdata->rumble_support = true;
drvdata->rgb_addr = rgb_addr_new;
} else {
drvdata->bmap_addr = button_mapping_addr_old;
@@ -1553,6 +1751,7 @@ static void claw_features_supported(struct claw_drvdata *drvdata)
if ((major == 0x02 && minor >= 0x17) || major >= 0x03) {
drvdata->bmap_support = true;
drvdata->bmap_addr = button_mapping_addr_new;
+ drvdata->rumble_support = true;
drvdata->rgb_addr = rgb_addr_new;
return;
}
@@ -1602,6 +1801,7 @@ static int claw_probe(struct hid_device *hdev, u8 ep)
spin_lock_init(&drvdata->mode_lock);
spin_lock_init(&drvdata->profile_lock);
spin_lock_init(&drvdata->frame_lock);
+ spin_lock_init(&drvdata->rumble_lock);
init_completion(&drvdata->send_cmd_complete);
INIT_DELAYED_WORK(&drvdata->cfg_resume, &cfg_resume_fn);
INIT_DELAYED_WORK(&drvdata->cfg_setup, &cfg_setup_fn);
--
2.53.0
^ permalink raw reply related
* [PATCH v11 3/4] HID: hid-msi: Add RGB control interface
From: Derek J. Clark @ 2026-05-29 7:21 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Pierre-Loup A . Griffais, Denis Benato, Zhouwang Huang,
Derek J . Clark, linux-input, linux-doc, linux-kernel
In-Reply-To: <20260529072111.7565-1-derekjohn.clark@gmail.com>
Adds RGB control interface for MSI Claw devices. The MSI Claw uses a
fairly unique RGB interface. It has 9 total zones (4 per joystick ring
and 1 for the ABXY buttons), and supports up to 8 sequential frames of
RGB zone data. Each frame is written to a specific area of MCU memory by
the profile command, the value of which changes based on the firmware of
the device. Unlike other devices (such as the Legion Go or the OneXPlayer
devices), there are no hard coded effects built into the MCU. Instead,
the basic effects are provided as a series of frame data. I have
mirrored the effects available in Windows in this driver, while keeping
the effect names consistent with the Lenovo drivers for the effects that
are similar.
Initial reverse-engineering and implementation of this feature was done
by Zhouwang Huang. I refactored the overall format to conform to kernel
driver best practices and style guides. Claude was used as an initial
reviewer of this patch.
Assisted-by: Claude:claude-sonnet-4-6
Co-developed-by: Zhouwang Huang <honjow311@gmail.com>
Signed-off-by: Zhouwang Huang <honjow311@gmail.com>
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
v9:
- Limit guard usage in cfg_setup_fn to avoid holding a lock during
registration and add group events.
- Don't use devm_ functions in cfg_setup_fn. Manually setup/teardown
devices and groups to prevent use after free.
- Ensure rgb_queue work is canceled during suspend.
- Check for drvdata in rgb_queue_fn to avoid use after free during
teardown.
v8:
- Ensure led_classdev is unregistered if adding attribute group fails.
- Reorder remove actions to ensure no use-after free or rearming cleared
flags.
v7:
- Use smp_[store_release|load_acquire] pattern for checking
rgb_registered to avoid possible races during teardown.
- Add gating to cfg_setup_fn, allowing either gamepad settings or rgb
settings to populate if the other fails for any reason.
- Use spinlock when writing profile_pending.
v6:
- Make all timeouts 25ms to ensure at least 2 jiffies in a 100Hz
config.
- Gate all attribute show/store functions with rgb_registered,
enabling use of devm_device_add_group.
v5:
- Move adding the RGB device into cfg_setup to prevent led core
attributes from being written to prior to setup completing.
- Ensure frame_lock is properly init.
- Change variable names in RGB functions from frame and zone to f and
z respectively to fit all scoped_guard actions in 100 columns.
v4:
- Fix frame_calc validity check to use >=.
- USe spinlock instead of mutex in raw_event and related attribute
_store function.
- Ensure delayed work is canceled in suspend & canceled before sysfs
attribute removal.
v3:
- Add mutex for read/write of rgb frame data.
- Remove setting rgb_frame_count when reading rgb profiles as it always
returns garbage data.
- Ensure rgb_speed is getting drvdata from a valid lookup (not hdev).
v2:
- Use pending_profile mutex
---
drivers/hid/hid-msi.c | 682 ++++++++++++++++++++++++++++++++++++++++--
1 file changed, 663 insertions(+), 19 deletions(-)
diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
index f099d6cb7ea8d..96710d80543d4 100644
--- a/drivers/hid/hid-msi.c
+++ b/drivers/hid/hid-msi.c
@@ -21,6 +21,7 @@
#include <linux/device.h>
#include <linux/hid.h>
#include <linux/kobject.h>
+#include <linux/led-class-multicolor.h>
#include <linux/leds.h>
#include <linux/module.h>
#include <linux/mutex.h>
@@ -44,6 +45,10 @@
#define CLAW_KEYS_MAX 5
+#define CLAW_RGB_ZONES 9
+#define CLAW_RGB_MAX_FRAMES 8
+#define CLAW_RGB_FRAME_OFFSET 0x24
+
enum claw_command_index {
CLAW_COMMAND_TYPE_NONE = 0x00,
CLAW_COMMAND_TYPE_READ_PROFILE = 0x04,
@@ -73,6 +78,7 @@ enum claw_profile_ack_pending {
CLAW_NO_PENDING,
CLAW_M1_PENDING,
CLAW_M2_PENDING,
+ CLAW_RGB_PENDING,
};
enum claw_key_index {
@@ -231,6 +237,22 @@ static const struct {
{ 0xff, "DISABLED" },
};
+enum claw_rgb_effect_index {
+ CLAW_RGB_EFFECT_MONOCOLOR,
+ CLAW_RGB_EFFECT_BREATHE,
+ CLAW_RGB_EFFECT_CHROMA,
+ CLAW_RGB_EFFECT_RAINBOW,
+ CLAW_RGB_EFFECT_FROSTFIRE,
+};
+
+static const char * const claw_rgb_effect_text[] = {
+ [CLAW_RGB_EFFECT_MONOCOLOR] = "monocolor",
+ [CLAW_RGB_EFFECT_BREATHE] = "breathe",
+ [CLAW_RGB_EFFECT_CHROMA] = "chroma",
+ [CLAW_RGB_EFFECT_RAINBOW] = "rainbow",
+ [CLAW_RGB_EFFECT_FROSTFIRE] = "frostfire",
+};
+
static const u16 button_mapping_addr_old[] = {
0x007a, /* M1 */
0x011f, /* M2 */
@@ -241,6 +263,9 @@ static const u16 button_mapping_addr_new[] = {
0x0164, /* M2 */
};
+static const u16 rgb_addr_old = 0x01fa;
+static const u16 rgb_addr_new = 0x024a;
+
struct claw_command_report {
u8 report_id;
u8 padding[2];
@@ -262,6 +287,27 @@ struct claw_mkey_report {
u8 codes[5];
} __packed;
+struct rgb_zone {
+ u8 red;
+ u8 green;
+ u8 blue;
+};
+
+struct rgb_frame {
+ struct rgb_zone zone[CLAW_RGB_ZONES];
+};
+
+struct claw_rgb_report {
+ struct claw_profile_report;
+ u8 frame_bytes;
+ u8 padding;
+ u8 frame_count;
+ u8 state; /* Always 0x09 */
+ u8 speed;
+ u8 brightness;
+ struct rgb_frame zone_data;
+} __packed;
+
struct claw_drvdata {
/* MCU General Variables */
enum claw_profile_ack_pending profile_pending;
@@ -286,10 +332,22 @@ struct claw_drvdata {
enum claw_gamepad_mode_index gamepad_mode;
u8 m1_codes[CLAW_KEYS_MAX];
u8 m2_codes[CLAW_KEYS_MAX];
- const u16 *bmap_addr;
spinlock_t mode_lock; /* Lock for mode data read/write */
+ const u16 *bmap_addr;
bool gp_registered;
bool bmap_support;
+
+ /* RGB Variables */
+ struct rgb_frame rgb_frames[CLAW_RGB_MAX_FRAMES];
+ enum claw_rgb_effect_index rgb_effect;
+ struct led_classdev_mc led_mc;
+ struct delayed_work rgb_queue;
+ spinlock_t frame_lock; /* lock for rgb_frames read/write */
+ bool rgb_registered;
+ u8 rgb_frame_count;
+ bool rgb_enabled;
+ u8 rgb_speed;
+ u16 rgb_addr;
};
static int get_endpoint_address(struct hid_device *hdev)
@@ -325,8 +383,11 @@ static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_
{
enum claw_profile_ack_pending profile;
struct claw_mkey_report *mkeys;
- u8 *codes, key;
- int i;
+ struct claw_rgb_report *frame;
+ u16 rgb_addr, read_addr;
+ u8 *codes, key, f_idx;
+ u16 frame_calc;
+ int i, ret = 0;
scoped_guard(spinlock, &drvdata->profile_lock)
profile = drvdata->profile_pending;
@@ -342,6 +403,34 @@ static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_
for (i = 0; i < CLAW_KEYS_MAX; i++)
codes[i] = (mkeys->codes[i]);
break;
+ case CLAW_RGB_PENDING:
+ frame = (struct claw_rgb_report *)cmd_rep->data;
+ rgb_addr = drvdata->rgb_addr;
+ read_addr = be16_to_cpu(frame->read_addr);
+ frame_calc = (read_addr - rgb_addr) / CLAW_RGB_FRAME_OFFSET;
+ if (frame_calc >= CLAW_RGB_MAX_FRAMES) {
+ dev_err(&drvdata->hdev->dev, "Got unsupported frame index: %x\n",
+ frame_calc);
+ return -EINVAL;
+ }
+ f_idx = frame_calc;
+
+ scoped_guard(spinlock, &drvdata->frame_lock) {
+ memcpy(&drvdata->rgb_frames[f_idx], &frame->zone_data,
+ sizeof(struct rgb_frame));
+
+ /* Only use frame 0 for remaining variable assignment */
+ if (f_idx != 0)
+ break;
+
+ drvdata->rgb_speed = frame->speed;
+ drvdata->led_mc.led_cdev.brightness = frame->brightness;
+ drvdata->led_mc.subled_info[0].intensity = frame->zone_data.zone[0].red;
+ drvdata->led_mc.subled_info[1].intensity = frame->zone_data.zone[0].green;
+ drvdata->led_mc.subled_info[2].intensity = frame->zone_data.zone[0].blue;
+ }
+
+ break;
default:
dev_dbg(&drvdata->hdev->dev,
"Got profile event without changes pending from command: %x\n",
@@ -351,7 +440,7 @@ static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_
scoped_guard(spinlock, &drvdata->profile_lock)
drvdata->profile_pending = CLAW_NO_PENDING;
- return 0;
+ return ret;
}
static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *report,
@@ -707,7 +796,7 @@ static int claw_mkey_store(struct device *dev, const char *buf, u8 mkey)
int ret, key_count, i;
scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
- /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
if (!smp_load_acquire(&drvdata->gp_registered))
return -ENODEV;
}
@@ -757,7 +846,7 @@ static int claw_mkey_show(struct device *dev, char *buf, enum claw_key_index m_k
u8 *codes;
scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
- /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
if (!smp_load_acquire(&drvdata->gp_registered))
return -ENODEV;
}
@@ -882,32 +971,553 @@ static const struct attribute_group claw_gamepad_attr_group = {
.is_visible = claw_gamepad_attr_is_visible,
};
+/* Read RGB config from device */
+static int claw_read_rgb_config(struct hid_device *hdev)
+{
+ u8 data[4] = { 0x01, 0x00, 0x00, CLAW_RGB_FRAME_OFFSET };
+ struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
+ u16 read_addr = drvdata->rgb_addr;
+ size_t len = ARRAY_SIZE(data);
+ int ret, i;
+
+ if (!drvdata->rgb_addr)
+ return -ENODEV;
+
+ /* Loop through all 8 pages of RGB data */
+ guard(mutex)(&drvdata->profile_mutex);
+ for (i = 0; i < CLAW_RGB_MAX_FRAMES; i++) {
+ scoped_guard(spinlock_irqsave, &drvdata->profile_lock)
+ drvdata->profile_pending = CLAW_RGB_PENDING;
+ data[1] = (read_addr >> 8) & 0xff;
+ data[2] = read_addr & 0x00ff;
+ ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_PROFILE, data, len, 25);
+ if (ret)
+ return ret;
+
+ read_addr += CLAW_RGB_FRAME_OFFSET;
+ }
+
+ return 0;
+}
+
+/* Send RGB configuration to device */
+static int claw_write_rgb_state(struct claw_drvdata *drvdata)
+{
+ struct claw_rgb_report report = { {0x01, 0}, CLAW_RGB_FRAME_OFFSET, 0x00,
+ drvdata->rgb_frame_count, 0x09, drvdata->rgb_speed,
+ drvdata->led_mc.led_cdev.brightness };
+ u16 write_addr = drvdata->rgb_addr;
+ int f, ret;
+
+ if (!drvdata->rgb_addr)
+ return -ENODEV;
+
+ if (!drvdata->rgb_frame_count)
+ return -EINVAL;
+
+ guard(mutex)(&drvdata->rom_mutex);
+ /* Loop through (up to) 8 pages of RGB data */
+ for (f = 0; f < drvdata->rgb_frame_count; f++) {
+ scoped_guard(spinlock_irqsave, &drvdata->frame_lock)
+ report.zone_data = drvdata->rgb_frames[f];
+
+ /* Set the MCU address to write the frame data to */
+ report.read_addr = cpu_to_be16(write_addr);
+
+ /* Serialize the rgb_report and write it to MCU */
+ ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
+ (u8 *)&report, sizeof(report), 25);
+ if (ret)
+ return ret;
+
+ /* Increment the write addr by the offset for the next frame */
+ write_addr += CLAW_RGB_FRAME_OFFSET;
+ }
+
+ /* MCU will not send ACK until the USB transaction completes. ACK is sent
+ * immediately after and will hit the stale state machine, before the next
+ * command re-arms the state machine. Timeout 0 ensures no deadlock waiting
+ * for ACK that ill never come.
+ */
+ ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 0);
+
+ return ret;
+}
+
+/* Fill all zones with the same color */
+static void claw_frame_fill_solid(struct rgb_frame *frame, struct rgb_zone zone)
+{
+ int z;
+
+ for (z = 0; z < CLAW_RGB_ZONES; z++)
+ frame->zone[z] = zone;
+}
+
+/* Apply solid effect (1 frame, no color) */
+static int claw_apply_disabled(struct claw_drvdata *drvdata)
+{
+ struct rgb_zone off = { 0x00, 0x00, 0x00};
+
+ scoped_guard(spinlock_irqsave, &drvdata->frame_lock) {
+ drvdata->rgb_frame_count = 1;
+ claw_frame_fill_solid(&drvdata->rgb_frames[0], off);
+ }
+
+ return claw_write_rgb_state(drvdata);
+}
+
+/* Apply solid effect (1 frame, all zones same color) */
+static int claw_apply_monocolor(struct claw_drvdata *drvdata)
+{
+ struct mc_subled *subleds = drvdata->led_mc.subled_info;
+ struct rgb_zone zone = { subleds[0].intensity, subleds[1].intensity,
+ subleds[2].intensity };
+
+ scoped_guard(spinlock_irqsave, &drvdata->frame_lock) {
+ drvdata->rgb_frame_count = 1;
+ claw_frame_fill_solid(&drvdata->rgb_frames[0], zone);
+ }
+
+ return claw_write_rgb_state(drvdata);
+}
+
+/* Apply breathe effect (2 frames: color -> off) */
+static int claw_apply_breathe(struct claw_drvdata *drvdata)
+{
+ struct mc_subled *subleds = drvdata->led_mc.subled_info;
+ struct rgb_zone zone = { subleds[0].intensity, subleds[1].intensity,
+ subleds[2].intensity };
+ static const struct rgb_zone off = { 0, 0, 0 };
+
+ scoped_guard(spinlock_irqsave, &drvdata->frame_lock) {
+ drvdata->rgb_frame_count = 2;
+ claw_frame_fill_solid(&drvdata->rgb_frames[0], zone);
+ claw_frame_fill_solid(&drvdata->rgb_frames[1], off);
+ }
+
+ return claw_write_rgb_state(drvdata);
+}
+
+/* Apply chroma effect (6 frames: rainbow cycle, all zones sync) */
+static int claw_apply_chroma(struct claw_drvdata *drvdata)
+{
+ static const struct rgb_zone colors[] = {
+ {255, 0, 0}, /* red */
+ {255, 255, 0}, /* yellow */
+ { 0, 255, 0}, /* green */
+ { 0, 255, 255}, /* cyan */
+ { 0, 0, 255}, /* blue */
+ {255, 0, 255}, /* magenta */
+ };
+ u8 frame_count = ARRAY_SIZE(colors);
+ int f;
+
+ scoped_guard(spinlock_irqsave, &drvdata->frame_lock) {
+ drvdata->rgb_frame_count = frame_count;
+
+ for (f = 0; f < frame_count; f++)
+ claw_frame_fill_solid(&drvdata->rgb_frames[f], colors[f]);
+ }
+
+ return claw_write_rgb_state(drvdata);
+}
+
+/* Apply rainbow effect (4 frames: rotating colors around joysticks) */
+static int claw_apply_rainbow(struct claw_drvdata *drvdata)
+{
+ static const struct rgb_zone colors[] = {
+ {255, 0, 0}, /* red */
+ { 0, 255, 0}, /* green */
+ { 0, 255, 255}, /* cyan */
+ { 0, 0, 255}, /* blue */
+ };
+ u8 frame_count = ARRAY_SIZE(colors);
+ int f, z;
+
+ scoped_guard(spinlock_irqsave, &drvdata->frame_lock) {
+ drvdata->rgb_frame_count = frame_count;
+
+ for (f = 0; f < frame_count; f++) {
+ for (z = 0; z < 4; z++) {
+ drvdata->rgb_frames[f].zone[z] = colors[(z + f) % 4];
+ drvdata->rgb_frames[f].zone[z + 4] = colors[(z + f) % 4];
+ }
+ drvdata->rgb_frames[f].zone[8] = colors[f];
+ }
+ }
+
+ return claw_write_rgb_state(drvdata);
+}
+
+/*
+ * Apply frostfire effect (4 frames: fire vs ice rotating)
+ * Right joystick: fire red -> dark -> ice blue -> dark (clockwise)
+ * Left joystick: ice blue -> dark -> fire red -> dark (counter-clockwise)
+ * ABXY: fire red -> dark -> ice blue -> dark
+ */
+static int claw_apply_frostfire(struct claw_drvdata *drvdata)
+{
+ static const struct rgb_zone colors[] = {
+ {255, 0, 0}, /* fire red */
+ { 0, 0, 0}, /* dark */
+ { 0, 0, 255}, /* ice blue */
+ { 0, 0, 0}, /* dark */
+ };
+ u8 frame_count = ARRAY_SIZE(colors);
+ int f, z;
+
+ scoped_guard(spinlock_irqsave, &drvdata->frame_lock) {
+ drvdata->rgb_frame_count = frame_count;
+
+ for (f = 0; f < frame_count; f++) {
+ for (z = 0; z < 4; z++) {
+ drvdata->rgb_frames[f].zone[z] = colors[(z + f) % 4];
+ drvdata->rgb_frames[f].zone[z + 4] = colors[(z - f + 6) % 4];
+ }
+ drvdata->rgb_frames[f].zone[8] = colors[f];
+ }
+ }
+
+ return claw_write_rgb_state(drvdata);
+}
+
+/* Apply current state to device */
+static int claw_apply_rgb_state(struct claw_drvdata *drvdata)
+{
+ if (!drvdata->rgb_enabled)
+ return claw_apply_disabled(drvdata);
+
+ switch (drvdata->rgb_effect) {
+ case CLAW_RGB_EFFECT_MONOCOLOR:
+ return claw_apply_monocolor(drvdata);
+ case CLAW_RGB_EFFECT_BREATHE:
+ return claw_apply_breathe(drvdata);
+ case CLAW_RGB_EFFECT_CHROMA:
+ return claw_apply_chroma(drvdata);
+ case CLAW_RGB_EFFECT_RAINBOW:
+ return claw_apply_rainbow(drvdata);
+ case CLAW_RGB_EFFECT_FROSTFIRE:
+ return claw_apply_frostfire(drvdata);
+ default:
+ dev_err(drvdata->led_mc.led_cdev.dev,
+ "No supported rgb_effect selected\n");
+ return -EINVAL;
+ }
+}
+
+static void claw_rgb_queue_fn(struct work_struct *work)
+{
+ struct delayed_work *dwork = container_of(work, struct delayed_work, work);
+ struct claw_drvdata *drvdata = container_of(dwork, struct claw_drvdata, rgb_queue);
+ int ret;
+
+ if (!drvdata)
+ return;
+
+ scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ if (!smp_load_acquire(&drvdata->rgb_registered))
+ return;
+ }
+
+ ret = claw_apply_rgb_state(drvdata);
+ if (ret)
+ dev_err(drvdata->led_mc.led_cdev.dev,
+ "Failed to apply RGB state: %d\n", ret);
+}
+
+static ssize_t effect_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
+ struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
+ int ret;
+
+ scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ if (!smp_load_acquire(&drvdata->rgb_registered))
+ return -ENODEV;
+ }
+
+ ret = sysfs_match_string(claw_rgb_effect_text, buf);
+ if (ret < 0)
+ return ret;
+
+ drvdata->rgb_effect = ret;
+ mod_delayed_work(system_wq, &drvdata->rgb_queue, msecs_to_jiffies(50));
+
+ return count;
+}
+
+static ssize_t effect_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
+ struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
+
+ scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ if (!smp_load_acquire(&drvdata->rgb_registered))
+ return -ENODEV;
+ }
+
+ if (drvdata->rgb_effect >= ARRAY_SIZE(claw_rgb_effect_text))
+ return -EINVAL;
+
+ return sysfs_emit(buf, "%s\n", claw_rgb_effect_text[drvdata->rgb_effect]);
+}
+
+static DEVICE_ATTR_RW(effect);
+
+static ssize_t effect_index_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int i, count = 0;
+
+ for (i = 0; i < ARRAY_SIZE(claw_rgb_effect_text); i++)
+ count += sysfs_emit_at(buf, count, "%s ", claw_rgb_effect_text[i]);
+
+ if (count)
+ buf[count - 1] = '\n';
+
+ return count;
+}
+static DEVICE_ATTR_RO(effect_index);
+
+static ssize_t enabled_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
+ struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
+ bool val;
+ int ret;
+
+ scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ if (!smp_load_acquire(&drvdata->rgb_registered))
+ return -ENODEV;
+ }
+
+ ret = kstrtobool(buf, &val);
+ if (ret)
+ return ret;
+
+ drvdata->rgb_enabled = val;
+ mod_delayed_work(system_wq, &drvdata->rgb_queue, msecs_to_jiffies(50));
+
+ return count;
+}
+
+static ssize_t enabled_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
+ struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
+
+ scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ if (!smp_load_acquire(&drvdata->rgb_registered))
+ return -ENODEV;
+ }
+
+ return sysfs_emit(buf, "%s\n", drvdata->rgb_enabled ? "true" : "false");
+}
+static DEVICE_ATTR_RW(enabled);
+
+static ssize_t enabled_index_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return sysfs_emit(buf, "true false\n");
+}
+static DEVICE_ATTR_RO(enabled_index);
+
+static ssize_t speed_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
+ struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
+ unsigned int val, speed;
+ int ret;
+
+ scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ if (!smp_load_acquire(&drvdata->rgb_registered))
+ return -ENODEV;
+ }
+
+ ret = kstrtouint(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ if (val > 20)
+ return -EINVAL;
+
+ /* 0 is fastest, invert value for intuitive userspace speed */
+ speed = 20 - val;
+
+ drvdata->rgb_speed = speed;
+ mod_delayed_work(system_wq, &drvdata->rgb_queue, msecs_to_jiffies(50));
+
+ return count;
+}
+
+static ssize_t speed_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
+ struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
+ u8 speed = 20 - drvdata->rgb_speed;
+
+ scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ if (!smp_load_acquire(&drvdata->rgb_registered))
+ return -ENODEV;
+ }
+
+ return sysfs_emit(buf, "%u\n", speed);
+}
+static DEVICE_ATTR_RW(speed);
+
+static ssize_t speed_range_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return sysfs_emit(buf, "0-20\n");
+}
+static DEVICE_ATTR_RO(speed_range);
+
+static void claw_led_brightness_set(struct led_classdev *led_cdev,
+ enum led_brightness _brightness)
+{
+ struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
+ struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
+
+ scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ if (!smp_load_acquire(&drvdata->rgb_registered))
+ return;
+ }
+
+ mod_delayed_work(system_wq, &drvdata->rgb_queue, msecs_to_jiffies(50));
+}
+
+static struct attribute *claw_rgb_attrs[] = {
+ &dev_attr_effect.attr,
+ &dev_attr_effect_index.attr,
+ &dev_attr_enabled.attr,
+ &dev_attr_enabled_index.attr,
+ &dev_attr_speed.attr,
+ &dev_attr_speed_range.attr,
+ NULL,
+};
+
+static const struct attribute_group claw_rgb_attr_group = {
+ .attrs = claw_rgb_attrs,
+};
+
+static struct mc_subled claw_rgb_subled_info[] = {
+ {
+ .color_index = LED_COLOR_ID_RED,
+ .channel = 0x1,
+ },
+ {
+ .color_index = LED_COLOR_ID_GREEN,
+ .channel = 0x2,
+ },
+ {
+ .color_index = LED_COLOR_ID_BLUE,
+ .channel = 0x3,
+ },
+};
+
static void cfg_setup_fn(struct work_struct *work)
{
struct delayed_work *dwork = container_of(work, struct delayed_work, work);
struct claw_drvdata *drvdata = container_of(dwork, struct claw_drvdata, cfg_setup);
+ bool gamepad_ready = false, rgb_ready = false, gp_registered, rgb_registered;
int ret;
ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_READ_GAMEPAD_MODE,
NULL, 0, 25);
if (ret) {
dev_err(&drvdata->hdev->dev,
- "Failed to setup device, can't read gamepad mode: %d\n", ret);
- return;
+ "Failed to read gamepad mode: %d\n", ret);
+ goto prep_rgb;
}
+ gamepad_ready = true;
- /* Add sysfs attributes after we get the device state */
- ret = device_add_group(&drvdata->hdev->dev, &claw_gamepad_attr_group);
+prep_rgb:
+ ret = claw_read_rgb_config(drvdata->hdev);
if (ret) {
dev_err(&drvdata->hdev->dev,
- "Failed to setup device, can't create gamepad attrs: %d\n", ret);
- return;
+ "Failed to read RGB config: %d\n", ret);
+ goto try_gamepad;
}
+ rgb_ready = true;
+
+ /* Add sysfs attributes after we get the device state */
+try_gamepad:
scoped_guard(spinlock_irqsave, &drvdata->registration_lock)
- /* Pairs with smp_load_acquire in attribute show/store functions */
- smp_store_release(&drvdata->gp_registered, true);
+ /* Pairs with smp_store_release from below */
+ gp_registered = smp_load_acquire(&drvdata->gp_registered);
+
+ if (!gp_registered && gamepad_ready) {
+ ret = device_add_group(&drvdata->hdev->dev, &claw_gamepad_attr_group);
+ if (ret) {
+ dev_err(&drvdata->hdev->dev,
+ "Failed to create gamepad attrs: %d\n", ret);
+ goto try_rgb;
+ }
- kobject_uevent(&drvdata->hdev->dev.kobj, KOBJ_CHANGE);
+ scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
+ /* Pairs with smp_load_acquire in attribute show/store functions */
+ smp_store_release(&drvdata->gp_registered, true);
+ gp_registered = true;
+ }
+ }
+
+try_rgb:
+ /* Add and enable RGB interface once we have the device state */
+ scoped_guard(spinlock_irqsave, &drvdata->registration_lock)
+ /* Pairs with smp_store_release from below */
+ rgb_registered = smp_load_acquire(&drvdata->rgb_registered);
+
+ if (!rgb_registered && rgb_ready) {
+ ret = led_classdev_multicolor_register(&drvdata->hdev->dev,
+ &drvdata->led_mc);
+ if (ret) {
+ dev_err(&drvdata->hdev->dev,
+ "Failed to create led device: %d\n", ret);
+ goto update_kobjects;
+ }
+
+ ret = device_add_group(drvdata->led_mc.led_cdev.dev, &claw_rgb_attr_group);
+ if (ret) {
+ dev_err(&drvdata->hdev->dev,
+ "Failed to create RGB attrs: %d\n", ret);
+ led_classdev_multicolor_unregister(&drvdata->led_mc);
+ goto update_kobjects;
+ }
+
+ scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
+ /* Pairs with smp_load_acquire in attribute show/store functions */
+ smp_store_release(&drvdata->rgb_registered, true);
+ rgb_registered = true;
+ }
+ }
+
+update_kobjects:
+ if (gp_registered)
+ kobject_uevent(&drvdata->hdev->dev.kobj, KOBJ_CHANGE);
+ if (rgb_registered)
+ kobject_uevent(&drvdata->led_mc.led_cdev.dev->kobj, KOBJ_CHANGE);
}
static void cfg_resume_fn(struct work_struct *work)
@@ -916,8 +1526,10 @@ static void cfg_resume_fn(struct work_struct *work)
struct claw_drvdata *drvdata = container_of(dwork, struct claw_drvdata, cfg_resume);
guard(spinlock_irqsave)(&drvdata->registration_lock);
- /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
- if (!smp_load_acquire(&drvdata->gp_registered))
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ if (!smp_load_acquire(&drvdata->gp_registered) ||
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ !smp_load_acquire(&drvdata->rgb_registered))
schedule_delayed_work(&drvdata->cfg_setup, msecs_to_jiffies(500));
}
@@ -928,18 +1540,24 @@ static void claw_features_supported(struct claw_drvdata *drvdata)
if (major == 0x01) {
drvdata->bmap_support = true;
- if (minor >= 0x66)
+ if (minor >= 0x66) {
drvdata->bmap_addr = button_mapping_addr_new;
- else
+ drvdata->rgb_addr = rgb_addr_new;
+ } else {
drvdata->bmap_addr = button_mapping_addr_old;
+ drvdata->rgb_addr = rgb_addr_old;
+ }
return;
}
if ((major == 0x02 && minor >= 0x17) || major >= 0x03) {
drvdata->bmap_support = true;
drvdata->bmap_addr = button_mapping_addr_new;
+ drvdata->rgb_addr = rgb_addr_new;
return;
}
+
+ drvdata->rgb_addr = rgb_addr_old;
}
static int claw_probe(struct hid_device *hdev, u8 ep)
@@ -954,6 +1572,7 @@ static int claw_probe(struct hid_device *hdev, u8 ep)
return -ENOMEM;
drvdata->gamepad_mode = CLAW_GAMEPAD_MODE_XINPUT;
+ drvdata->rgb_enabled = true;
drvdata->hdev = hdev;
drvdata->ep = ep;
@@ -964,6 +1583,17 @@ static int claw_probe(struct hid_device *hdev, u8 ep)
if (!drvdata->bmap_support)
dev_dbg(&hdev->dev, "M-Key mapping is not supported. Update firmware to enable.\n");
+ drvdata->led_mc.led_cdev.name = "msi_claw:rgb:joystick_rings";
+ drvdata->led_mc.led_cdev.brightness = 0x50;
+ drvdata->led_mc.led_cdev.max_brightness = 0x64;
+ drvdata->led_mc.led_cdev.color = LED_COLOR_ID_RGB;
+ drvdata->led_mc.led_cdev.brightness_set = claw_led_brightness_set;
+ drvdata->led_mc.num_colors = 3;
+ drvdata->led_mc.subled_info = devm_kmemdup(&hdev->dev, claw_rgb_subled_info,
+ sizeof(claw_rgb_subled_info), GFP_KERNEL);
+ if (!drvdata->led_mc.subled_info)
+ return -ENOMEM;
+
mutex_init(&drvdata->cfg_mutex);
mutex_init(&drvdata->profile_mutex);
mutex_init(&drvdata->rom_mutex);
@@ -971,9 +1601,11 @@ static int claw_probe(struct hid_device *hdev, u8 ep)
spin_lock_init(&drvdata->cmd_lock);
spin_lock_init(&drvdata->mode_lock);
spin_lock_init(&drvdata->profile_lock);
+ spin_lock_init(&drvdata->frame_lock);
init_completion(&drvdata->send_cmd_complete);
INIT_DELAYED_WORK(&drvdata->cfg_resume, &cfg_resume_fn);
INIT_DELAYED_WORK(&drvdata->cfg_setup, &cfg_setup_fn);
+ INIT_DELAYED_WORK(&drvdata->rgb_queue, &claw_rgb_queue_fn);
/* For control interface: open the HID transport for sending commands. */
ret = hid_hw_open(hdev);
@@ -1030,6 +1662,7 @@ static void claw_remove(struct hid_device *hdev)
{
struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
bool gp_registered;
+ bool rgb_registered;
if (!drvdata)
return;
@@ -1042,11 +1675,21 @@ static void claw_remove(struct hid_device *hdev)
gp_registered = smp_load_acquire(&drvdata->gp_registered);
/* Pairs with smp_load_acquire in attribute show/store functions */
smp_store_release(&drvdata->gp_registered, false);
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ rgb_registered = smp_load_acquire(&drvdata->rgb_registered);
+ /* Pairs with smp_load_acquire in attribute show/store functions */
+ smp_store_release(&drvdata->rgb_registered, false);
}
if (gp_registered)
device_remove_group(&hdev->dev, &claw_gamepad_attr_group);
+ cancel_delayed_work_sync(&drvdata->rgb_queue);
+ if (rgb_registered) {
+ device_remove_group(drvdata->led_mc.led_cdev.dev, &claw_rgb_attr_group);
+ led_classdev_multicolor_unregister(&drvdata->led_mc);
+ }
+
hid_hw_close(hdev);
}
@@ -1106,6 +1749,7 @@ static int claw_suspend(struct hid_device *hdev)
cancel_delayed_work_sync(&drvdata->cfg_resume);
cancel_delayed_work_sync(&drvdata->cfg_setup);
+ cancel_delayed_work_sync(&drvdata->rgb_queue);
return 0;
}
--
2.53.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox