* Re: [PATCH v2 2/3] HID: Intel-thc-hid: Intel-quicki2c: Refine recover callback
From: sashiko-bot @ 2026-07-03 8:10 UTC (permalink / raw)
To: Even Xu; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260703075858.2780398-3-even.xu@intel.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Use-After-Free due to premature `cancel_work_sync` during device removal and shutdown.
Pre-existing issues:
- [High] Missing `pci_free_irq_vectors()` in probe error paths and device removal leaks IRQ resources.
- [High] The hard IRQ handler accesses hardware registers unconditionally, violating PM subsystem guidelines.
--
commit e88da9d40562f5d0be755568ae60010a7ba925df
Author: Even Xu <even.xu@intel.com>
HID: Intel-thc-hid: Intel-quicki2c: Refine recover callback
This commit refines the recover flow by offloading it to a workqueue instead
of processing it within the IRQ handler. It simplifies the operation with a
reset API and ensures interrupts are disabled during the recovery process.
> diff --git a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> index 46d3e9a019995..11e0e129b44c3 100644
> --- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> @@ -245,28 +245,28 @@ static irqreturn_t quicki2c_irq_quick_handler(int irq, void *dev_id)
> }
[Severity: High]
This is a pre-existing issue, but does quicki2c_irq_quick_handler() access
hardware registers unconditionally?
If this device shares an INTx interrupt line, a spurious interrupt from
another device could cause quicki2c_irq_quick_handler() to execute. It then
calls thc_interrupt_enable() which writes to memory-mapped registers. If the
device is in a low-power state, this hardware access can trigger a bus fault.
Should pm_runtime_get_if_active() be used to check if the device is active
before touching the hardware registers?
[ ... ]
> @@ -771,6 +771,8 @@ static void quicki2c_remove(struct pci_dev *pdev)
> if (!qcdev)
> return;
>
> + cancel_work_sync(&qcdev->recover_work);
[Severity: High]
Can this sequence lead to a use-after-free?
cancel_work_sync() is called here, but hardware interrupts are not yet
quiesced, and the state is still active. If an interrupt fires immediately
after cancel_work_sync(), quicki2c_irq_thread_handler() can execute and call:
schedule_work(&qcdev->recover_work);
This re-enqueues the work item. After quicki2c_remove() returns, devres
frees the qcdev structure. When the scheduled work later executes, it will
dereference the freed qcdev pointer.
Should the work be cancelled after interrupts are disabled and the device
state is set to QUICKI2C_DISABLED?
> quicki2c_hid_remove(qcdev);
> quicki2c_dma_deinit(qcdev);
>
> pm_runtime_get_noresume(qcdev->dev);
>
> quicki2c_dev_deinit(qcdev);
>
> pci_clear_master(pdev);
> }
[Severity: High]
This is a pre-existing issue, but does quicki2c_remove() leak IRQ resources?
The remove path unbinds the device but doesn't appear to call
pci_free_irq_vectors(). Similarly, if quicki2c_probe() fails after
pci_alloc_irq_vectors(), the error path (disable_pci_device) does not
free the vectors.
Repeated unbind/bind operations or probe deferrals will continually leak
these vectors, which can eventually exhaust system IRQ capacity.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703075858.2780398-1-even.xu@intel.com?part=2
^ permalink raw reply
* Re: [PATCH v2 1/3] HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA
From: sashiko-bot @ 2026-07-03 8:12 UTC (permalink / raw)
To: Even Xu; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260703075858.2780398-2-even.xu@intel.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [High] thc_rxdma_reset() fails to re-initialize the RxDMA PRD tables, leading to hardware reading shrunken buffer sizes on restart.
- [High] thc_rxdma_reset() unsafely clears PRD base addresses for all DMA channels while TX or SW DMA engines may still be running.
- [Medium] thc_rxdma_reset() ignores the return value of thc_interrupt_quiesce(dev, false) on the success path.
Pre-existing issues:
- [High] Incorrect polling condition and execution order in thc_interrupt_quiesce during unquiesce (int_quiesce = false).
--
commit b0beafd1bc7a02f7a2277c5a6d69c17c58724f01
Author: Even Xu <even.xu@intel.com>
HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA
Add a helper function thc_rxdma_reset() to do read DMA reset, it can be
called when fatal DMA error happens.
> diff --git a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
> index 6ee675e0a7384..7ceb8aeeccd37 100644
> --- a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
> +++ b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
> @@ -561,6 +561,57 @@ static int thc_wait_for_dma_pause(struct thc_device *dev, enum thc_dma_channel c
[ ... ]
> +int thc_rxdma_reset(struct thc_device *dev)
> +{
[ ... ]
> + ret = thc_wait_for_dma_pause(dev, THC_RXDMA2);
> + if (ret) {
> + dev_err(dev->dev, "Wait for RxDMA2 pause failed during RxDMA reset\n");
> + goto end;
> + }
> +
> + thc_dma_unconfigure(dev);
[Severity: High]
Could this cause memory corruption or IOMMU faults if TX or SW DMA engines
are still running?
thc_rxdma_reset() explicitly pauses RxDMA1 and RxDMA2 but does not pause
TXDMA or SWDMA. It then calls thc_dma_unconfigure(), which loops over all
DMA channels (MAX_THC_DMA_CHANNEL) and blindly overwrites their PRD base
addresses to 0x0.
If TXDMA was left running (for instance, if a prior thc_dma_write() timed
out without clearing the TXDMA start bit), setting its PRD base address to 0
while it is actively fetching could cause the hardware to fetch descriptors
from physical address 0.
> +
> + ret = thc_dma_configure(dev);
[Severity: High]
Does this leave dirty 'len' descriptors in the PRD memory layout?
thc_dma_configure() restarts the DMA engine pointers to 0, but it does not
re-initialize the PRD memory layout. The hardware updates the 'len' field
of PRD entries to indicate the actual length of received packets.
Restarting the DMA from index 0 without restoring 'len' to the maximum
buffer capacity could cause the hardware to use previously received smaller
packet sizes as the maximum capacity for new transfers, resulting in
truncated packets or DMA errors.
> + if (ret) {
> + dev_err(dev->dev, "Re-config DMA failed during RxDMA reset\n");
> + goto end;
> + }
> +
> + thc_interrupt_quiesce(dev, false);
[Severity: Medium]
Is it safe to ignore the return value of thc_interrupt_quiesce() here?
Discarding this return value on the success path could silently hide a
hardware timeout or regmap read failure, potentially leaving interrupts
disabled while the function reports success.
[Severity: High]
This is a pre-existing issue, but is the polling condition and execution
order in thc_interrupt_quiesce() correct when unquiescing?
When thc_interrupt_quiesce() is called with int_quiesce = false, it polls
the HW_STS bit expecting it to be 1 BEFORE it clears the QUIESCE_EN bit:
drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.c:thc_interrupt_quiesce() {
...
ret = regmap_read_poll_timeout(dev->thc_regmap, THC_M_PRT_CONTROL_OFFSET, ctrl,
ctrl & THC_M_PRT_CONTROL_THC_DEVINT_QUIESCE_HW_STS,
THC_REGMAP_POLLING_INTERVAL_US, THC_QUIESCE_EN_TIMEOUT_US);
...
/* Unquiesce device interrupt - Clear the quiesce bit */
if (!int_quiesce)
regmap_write_bits(dev->thc_regmap, THC_M_PRT_CONTROL_OFFSET,
THC_M_PRT_CONTROL_THC_DEVINT_QUIESCE_EN, 0);
}
Since the device is already quiesced, HW_STS is already 1, causing the poll
to return immediately. It then clears the QUIESCE_EN bit and returns without
ever waiting for HW_STS to become 0 to acknowledge the unquiesce. This could
cause the driver to proceed before interrupts are actually re-enabled.
> +
> +end:
> + mutex_unlock(&dev->thc_bus_lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(thc_rxdma_reset, "INTEL_THC");
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703075858.2780398-1-even.xu@intel.com?part=1
^ permalink raw reply
* Re: [PATCH v9 4/7] input: keyboard: Add driver for ASUS Transformer dock multimedia keys
From: Svyatoslav Ryhel @ 2026-07-03 8:53 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Michał Mirosław, Ion Agorria,
Svyatoslav Ryhel, Sebastian Reichel, Pavel Machek, Lee Jones,
Conor Dooley, Krzysztof Kozlowski, devicetree, linux-kernel,
linux-input, linux-leds, linux-pm
In-Reply-To: <20260625081529.22447-5-clamor95@gmail.com>
чт, 25 черв. 2026 р. о 11:16 Svyatoslav Ryhel <clamor95@gmail.com> пише:
>
> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>
> Add support for multimedia top button row of ASUS Transformer's Mobile
> Dock keyboard. Driver is made that function keys (F1-F12) are used by
> default which suits average Linux use better and with pressing
> ScreenLock + AltGr function keys layout is switched to multimedia keys.
> Only Dock keyboard input events are tracked for AltGr pressing.
>
> Co-developed-by: Ion Agorria <ion@agorria.com>
> Signed-off-by: Ion Agorria <ion@agorria.com>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
> drivers/input/keyboard/Kconfig | 10 +
> drivers/input/keyboard/Makefile | 1 +
> .../input/keyboard/asus-transformer-ec-keys.c | 314 ++++++++++++++++++
> 3 files changed, 325 insertions(+)
> create mode 100644 drivers/input/keyboard/asus-transformer-ec-keys.c
>
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 9d1019ba0245..913cb4900565 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -89,6 +89,16 @@ config KEYBOARD_APPLESPI
> To compile this driver as a module, choose M here: the
> module will be called applespi.
>
> +config KEYBOARD_ASUS_TRANSFORMER_EC
> + tristate "Asus Transformer's Mobile Dock multimedia keys"
> + depends on MFD_ASUS_TRANSFORMER_EC
> + help
> + Say Y here if you want to use multimedia keys present on Asus
> + Transformer's Mobile Dock.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called asus-transformer-ec-keys.
> +
> config KEYBOARD_ATARI
> tristate "Atari keyboard"
> depends on ATARI
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 60bb7baf802f..0d81096887ad 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_KEYBOARD_ADP5585) += adp5585-keys.o
> obj-$(CONFIG_KEYBOARD_ADP5588) += adp5588-keys.o
> obj-$(CONFIG_KEYBOARD_AMIGA) += amikbd.o
> obj-$(CONFIG_KEYBOARD_APPLESPI) += applespi.o
> +obj-$(CONFIG_KEYBOARD_ASUS_TRANSFORMER_EC) += asus-transformer-ec-keys.o
> obj-$(CONFIG_KEYBOARD_ATARI) += atakbd.o
> obj-$(CONFIG_KEYBOARD_ATKBD) += atkbd.o
> obj-$(CONFIG_KEYBOARD_BCM) += bcm-keypad.o
> diff --git a/drivers/input/keyboard/asus-transformer-ec-keys.c b/drivers/input/keyboard/asus-transformer-ec-keys.c
> new file mode 100644
> index 000000000000..53aff3ce7146
> --- /dev/null
> +++ b/drivers/input/keyboard/asus-transformer-ec-keys.c
> @@ -0,0 +1,314 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <linux/array_size.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/mfd/asus-transformer-ec.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define ASUSEC_EXT_KEY_CODES 0x20
> +
> +struct asus_ec_keys_data {
> + struct notifier_block nb;
> + struct asusec_core *ec;
> + struct input_dev *xidev;
> + struct input_handler input_handler;
> + unsigned short keymap[ASUSEC_EXT_KEY_CODES * 2];
> + const char *kbc_phys;
> + bool special_key_pressed;
> + bool special_key_mode;
> +};
> +
> +static void asus_ec_input_event(struct input_handle *handle,
> + unsigned int event_type,
> + unsigned int event_code, int value)
> +{
> + struct asus_ec_keys_data *priv = handle->handler->private;
> +
> + /* Store special key state */
> + if (event_type == EV_KEY && event_code == KEY_RIGHTALT)
> + priv->special_key_pressed = !!value;
> +}
> +
> +static int asus_ec_input_connect(struct input_handler *handler,
> + struct input_dev *dev,
> + const struct input_device_id *id)
> +{
> + struct asus_ec_keys_data *priv = handler->private;
> + struct input_handle *handle;
> + int error;
> +
> + if (!dev->phys || !strstr(dev->phys, priv->kbc_phys))
> + return -ENODEV;
> +
Hello Dmitry!
Would this approach be acceptable? Handler links strictly to asus-ec keyboard.
> + handle = kzalloc_obj(*handle);
> + if (!handle)
> + return -ENOMEM;
> +
> + handle->dev = dev;
> + handle->handler = handler;
> + handle->name = handler->name;
> +
> + error = input_register_handle(handle);
> + if (error)
> + goto err_free_handle;
> +
> + error = input_open_device(handle);
> + if (error)
> + goto err_unregister_handle;
> +
> + return 0;
> +
> + err_unregister_handle:
> + input_unregister_handle(handle);
> + err_free_handle:
> + kfree(handle);
> +
> + return error;
> +}
> +
> +static void asus_ec_input_disconnect(struct input_handle *handle)
> +{
> + input_close_device(handle);
> + input_unregister_handle(handle);
> + kfree(handle);
> +}
> +
> +static const struct input_device_id asus_ec_input_ids[] = {
> + {
> + .flags = INPUT_DEVICE_ID_MATCH_EVBIT,
> + .evbit = { BIT_MASK(EV_KEY) },
> + },
> + { }
> +};
> +
> +static const unsigned short asus_ec_dock_ext_keys[] = {
> + /* Function keys [0x00 - 0x19] */
> + [0x01] = KEY_DELETE,
> + [0x02] = KEY_F1,
> + [0x03] = KEY_F2,
> + [0x04] = KEY_F3,
> + [0x05] = KEY_F4,
> + [0x06] = KEY_F5,
> + [0x07] = KEY_F6,
> + [0x08] = KEY_F7,
> + [0x10] = KEY_F8,
> + [0x11] = KEY_F9,
> + [0x12] = KEY_F10,
> + [0x13] = KEY_F11,
> + [0x14] = KEY_F12,
> + [0x15] = KEY_MUTE,
> + [0x16] = KEY_VOLUMEDOWN,
> + [0x17] = KEY_VOLUMEUP,
> + /* Multimedia keys [0x20 - 0x39] */
> + [0x21] = KEY_SCREENLOCK,
> + [0x22] = KEY_WLAN,
> + [0x23] = KEY_BLUETOOTH,
> + [0x24] = KEY_TOUCHPAD_TOGGLE,
> + [0x25] = KEY_BRIGHTNESSDOWN,
> + [0x26] = KEY_BRIGHTNESSUP,
> + [0x27] = KEY_BRIGHTNESS_AUTO,
> + [0x28] = KEY_PRINT,
> + [0x30] = KEY_WWW,
> + [0x31] = KEY_CONFIG,
> + [0x32] = KEY_PREVIOUSSONG,
> + [0x33] = KEY_PLAYPAUSE,
> + [0x34] = KEY_NEXTSONG,
> + [0x35] = KEY_MUTE,
> + [0x36] = KEY_VOLUMEDOWN,
> + [0x37] = KEY_VOLUMEUP,
> +};
> +
> +static void asus_ec_keys_report_key(struct input_dev *dev, unsigned int code,
> + unsigned int key, bool value)
> +{
> + input_event(dev, EV_MSC, MSC_SCAN, code);
> + input_report_key(dev, key, value);
> + input_sync(dev);
> +}
> +
> +static int asus_ec_keys_process_key(struct input_dev *dev, u8 code)
> +{
> + struct asus_ec_keys_data *priv = dev_get_drvdata(dev->dev.parent);
> + unsigned int key = 0;
> +
> + if (code == 0)
> + return NOTIFY_DONE;
> +
> + /* Flip special key mode state when pressing SCREEN LOCK + R ALT */
> + if (priv->special_key_pressed && code == 1) {
> + priv->special_key_mode = !priv->special_key_mode;
> + return NOTIFY_DONE;
> + }
> +
> + /*
> + * Relocate code to second "page" if pressed state XOR's mode state
> + * This way special key will invert the current mode
> + */
> + if (priv->special_key_mode ^ priv->special_key_pressed)
> + code += ASUSEC_EXT_KEY_CODES;
> +
> + if (code < dev->keycodemax) {
> + unsigned short *map = dev->keycode;
> +
> + key = map[code];
> + }
> +
> + if (!key)
> + key = KEY_UNKNOWN;
> +
> + asus_ec_keys_report_key(dev, code, key, 1);
> + asus_ec_keys_report_key(dev, code, key, 0);
> +
> + return NOTIFY_OK;
> +}
> +
> +static int asus_ec_keys_notify(struct notifier_block *nb,
> + unsigned long action, void *data_)
> +{
> + struct asus_ec_keys_data *priv =
> + container_of(nb, struct asus_ec_keys_data, nb);
> + u8 *data = data_;
> +
> + if (action & ASUSEC_SMI_MASK)
> + return NOTIFY_DONE;
> +
> + if (action & ASUSEC_SCI_MASK)
> + return asus_ec_keys_process_key(priv->xidev, data[2]);
> +
> + return NOTIFY_DONE;
> +}
> +
> +static void asus_ec_keys_setup_keymap(struct asus_ec_keys_data *priv)
> +{
> + struct input_dev *dev = priv->xidev;
> + unsigned int i;
> +
> + BUILD_BUG_ON(ARRAY_SIZE(priv->keymap) < ARRAY_SIZE(asus_ec_dock_ext_keys));
> +
> + dev->keycode = priv->keymap;
> + dev->keycodesize = sizeof(*priv->keymap);
> + dev->keycodemax = ARRAY_SIZE(priv->keymap);
> +
> + input_set_capability(dev, EV_MSC, MSC_SCAN);
> + input_set_capability(dev, EV_KEY, KEY_UNKNOWN);
> +
> + for (i = 0; i < ARRAY_SIZE(asus_ec_dock_ext_keys); i++) {
> + unsigned int code = asus_ec_dock_ext_keys[i];
> +
> + if (!code)
> + continue;
> +
> + __set_bit(code, dev->keybit);
> + priv->keymap[i] = code;
> + }
> +}
> +
> +static int asus_ec_keys_register_handler(struct device *dev,
> + struct asus_ec_keys_data *priv)
> +{
> + struct i2c_client *parent = to_i2c_client(dev->parent);
> + int error;
> +
> + priv->input_handler.event = asus_ec_input_event;
> + priv->input_handler.connect = asus_ec_input_connect;
> + priv->input_handler.disconnect = asus_ec_input_disconnect;
> + priv->input_handler.id_table = asus_ec_input_ids;
> + priv->input_handler.passive_observer = true;
> + priv->input_handler.private = priv;
> + priv->input_handler.name = devm_kasprintf(dev, GFP_KERNEL,
> + "%s-media-handler",
> + priv->ec->name);
> + if (!priv->input_handler.name)
> + return -ENOMEM;
> +
> + priv->kbc_phys = devm_kasprintf(dev, GFP_KERNEL, "i2c-%u-%04x/serio0",
> + i2c_adapter_id(parent->adapter),
> + parent->addr);
> + if (!priv->kbc_phys)
> + return -ENOMEM;
> +
> + error = input_register_handler(&priv->input_handler);
> + if (error)
> + return error;
> +
> + return 0;
> +}
> +
> +static int asus_ec_keys_probe(struct platform_device *pdev)
> +{
> + struct i2c_client *parent = to_i2c_client(pdev->dev.parent);
> + struct asusec_core *ec = dev_get_drvdata(pdev->dev.parent);
> + struct device *dev = &pdev->dev;
> + struct asus_ec_keys_data *priv;
> + int error;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, priv);
> + priv->ec = ec;
> +
> + priv->xidev = devm_input_allocate_device(dev);
> + if (!priv->xidev)
> + return -ENOMEM;
> +
> + priv->xidev->name = devm_kasprintf(dev, GFP_KERNEL, "%s Keyboard Ext",
> + ec->model);
> + priv->xidev->phys = devm_kasprintf(dev, GFP_KERNEL, "i2c-%u-%04x",
> + i2c_adapter_id(parent->adapter),
> + parent->addr);
> +
> + if (!priv->xidev->name || !priv->xidev->phys)
> + return -ENOMEM;
> +
> + asus_ec_keys_setup_keymap(priv);
> +
> + error = input_register_device(priv->xidev);
> + if (error)
> + return dev_err_probe(dev, error,
> + "failed to register extension keys\n");
> +
> + error = asus_ec_keys_register_handler(dev, priv);
> + if (error) {
> + input_unregister_device(priv->xidev);
> + return error;
> + }
> +
> + priv->nb.notifier_call = asus_ec_keys_notify;
> +
> + error = blocking_notifier_chain_register(&ec->notify_list, &priv->nb);
> + if (error) {
> + input_unregister_device(priv->xidev);
> + input_unregister_handler(&priv->input_handler);
> + return error;
> + }
> +
> + return 0;
> +}
> +
> +static void asus_ec_keys_remove(struct platform_device *pdev)
> +{
> + struct asus_ec_keys_data *priv = platform_get_drvdata(pdev);
> + struct asusec_core *ec = priv->ec;
> +
> + blocking_notifier_chain_unregister(&ec->notify_list, &priv->nb);
> + input_unregister_handler(&priv->input_handler);
> + input_unregister_device(priv->xidev);
> +}
> +
> +static struct platform_driver asus_ec_keys_driver = {
> + .driver.name = "asus-transformer-ec-keys",
> + .probe = asus_ec_keys_probe,
> + .remove = asus_ec_keys_remove,
> +};
> +module_platform_driver(asus_ec_keys_driver);
> +
> +MODULE_ALIAS("platform:asus-transformer-ec-keys");
> +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> +MODULE_DESCRIPTION("ASUS Transformer's multimedia keys driver");
> +MODULE_LICENSE("GPL");
> --
> 2.53.0
>
^ permalink raw reply
* Re: [PATCH 09/10] HID: steam: Reject short reads
From: Yousef Alhouseen @ 2026-07-03 11:26 UTC (permalink / raw)
To: vi; +Cc: jikos, bentiss, linux-input, syzbot+75f3f9bff8c510602d36
In-Reply-To: <20260702222145.1863104-9-vi@endrift.com>
Hi Vicki,
I think the length comparison is reversed here.
hid_hw_raw_request() includes the report-ID byte in ret, while
steam_recv_report() strips that byte before copying, so only ret - 1
bytes are present in data. The encoded report body is data[1] + 2
bytes (command, length, and payload). A short report therefore
satisfies:
ret - 1 < data[1] + 2
or equivalently:
ret < data[1] + 3
The current data[1] > ret + 2 test will miss the usual short-read
cases. Also, when ret == 2, only data[0] was copied, so reading
data[1] is itself uninitialized despite the ret >= 2 guard.
Perhaps the check could be structured as:
if (ret > 0 && (ret < 3 || ret < data[1] + 3))
return -EPROTO;
with the diagnostic adjusted depending on whether the two-byte header
was present.
Thanks,
Yousef
On Thu, 2 Jul 2026 15:21:42 -0700, Vicki Pfau <vi@endrift.com> wrote:
> Steam Controller FEATURE reports encode the size of the message in the
> message itself. Previously we were trusting that the size reported matched
> the size we actually read, leading to a potential issue with short reads.
> Instead, we should actually verify the length of the read.
>
> Fixes: c164d6abf384 ("HID: add driver for Valve Steam Controller")
> Reported-by: syzbot+75f3f9bff8c510602d36@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=75f3f9bff8c510602d36
>
> Signed-off-by: Vicki Pfau <vi@endrift.com>
> ---
> drivers/hid/hid-steam.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 593151709cf1..e97431bc2828 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -389,6 +389,12 @@ static int steam_recv_report(struct steam_device *steam,
> hid_err(steam->hdev, "%s: error %d\n", __func__, ret);
> else
> hid_dbg(steam->hdev, "Received report %*ph\n", ret, data);
> +
> + if (ret >= 2 && data[1] > ret + 2) {
> + hid_err(steam->hdev, "%s: expected %u bytes, read %i\n",
> + __func__, data[1] + 2, ret);
> + return -EPROTO;
> + }
> return ret;
> }
>
> --
> 2.54.0
^ permalink raw reply
* [PATCH v2 0/7] arm64: mediatek: Chromebook trackpad supply fixes
From: Chen-Yu Tsai @ 2026-07-03 11:55 UTC (permalink / raw)
To: Matthias Brugger, AngeloGioacchino Del Regno, Benson Leung,
Tzung-Bi Shih, Dmitry Torokhov, Jiri Kosina, Andi Shyti
Cc: Chen-Yu Tsai, linux-mediatek, devicetree, linux-arm-kernel,
chrome-platform, linux-input, linux-i2c, linux-kernel
Hi everyone,
This series fixes the trackpad descriptions on some MediaTek-based
Chromebooks: either the trackpad's supply was set as always-on to
workaround missing delays in the driver, or the delay and supply
are missing from the trackpad's device node.
v1 was just the first patch [1]. It has since grown to cover multiple
drivers and devices.
Patch 1 adds the correct enable delay after enabling the supply regulator
for the Elan trackpad to initialize. Compared to v1, the delay is now
skipped if the regulator was already enabled to avoid impacting boot
time or time before the trackpad is operational.
Patch 2 applies the same logic of skipping the power on delay to the
i2c-hid-of driver.
Patch 3 applies the same logic of skipping the power on delay to the
i2c OF component prober library.
Patch 4 adds a delay between when the device node found is enabled and
when regulator_disable() is called. This gives an asynchronously probing
driver some time to increment the enable count of their regulator
reference, thus keeping the device operational and allowing the driver
to skip the initialization delay.
Patch 5 adds the correct delay for probing trackpads for Hana devices
to the ChromeOS OF component prober.
Patch 6 removes the "always-on" setting from the trackpad supply for
Elm / Hana and adds the correct delay to the second source trackpad.
This corrects the hardware description.
Patch 7 adds the supply and power on delay properties to the Synaptics
trackpad on the Spherion device. Combined with previous driver changes
this should cause no actual functional changes or delays.
Please take a look. There are no build time dependencies between any
of the patches, but the DT changes must go in after all the driver
changes land, especially the first one adding delays to the Elan
trackpad driver. Otherwise one could potentially end up with a
non-functional trackpad on the device.
Thanks
ChenYu
[1] https://lore.kernel.org/all/20241001093815.2481899-1-wenst@chromium.org/
Chen-Yu Tsai (7):
Input: elan_i2c - Wait for initialization after enabling regulator
supply
HID: i2c-hid-of: skip post-power-on delay if already powered on
i2c: of-prober: skip post-power-on delay if already powered on
i2c: of-prober: Defer regulator_disable() on successful probe in
simple helper
platform/chrome: of_hw_prober: Add delay for hana trackpads
arm64: dts: mediatek: mt8173-elm-hana: Unmark trackpad supply as
always-on
arm64: dts: mediatek: mt8192-asurada-spherion: Add Synaptics
trackpad's supply
.../boot/dts/mediatek/mt8173-elm-hana.dtsi | 8 +----
arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 1 -
.../mediatek/mt8192-asurada-spherion-r0.dts | 2 ++
drivers/hid/i2c-hid/i2c-hid-of.c | 8 ++++-
drivers/i2c/i2c-core-of-prober.c | 29 +++++++++++++++----
drivers/input/mouse/elan_i2c_core.c | 13 +++++++++
.../platform/chrome/chromeos_of_hw_prober.c | 4 +--
7 files changed, 48 insertions(+), 17 deletions(-)
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply
* [PATCH v2 1/7] Input: elan_i2c - Wait for initialization after enabling regulator supply
From: Chen-Yu Tsai @ 2026-07-03 11:55 UTC (permalink / raw)
To: Matthias Brugger, AngeloGioacchino Del Regno, Benson Leung,
Tzung-Bi Shih, Dmitry Torokhov, Jiri Kosina, Andi Shyti
Cc: Chen-Yu Tsai, linux-mediatek, devicetree, linux-arm-kernel,
chrome-platform, linux-input, linux-i2c, linux-kernel
In-Reply-To: <20260703115601.1323491-1-wenst@chromium.org>
Elan trackpad controllers require some delay after enabling power to
the controller for the hardware and firmware to initialize:
- 2ms for hardware initialization
- 100ms for firmware initialization
Until then, the hardware will not respond to I2C transfers. This was
observed on the MT8173 Chromebooks after the regulator supply for the
trackpad was changed to "not always on".
Add proper delays after regulator_enable() calls. To avoid impacting
the boot time of existing devices that have the power rails always on,
skip the delay if the regulator supply was already enabled. In this
case the regulator is either always on, was on by default at power up,
or was left on by some other driver, such as the I2C OF component
prober. Either way the controller has had ample time to initialize.
Fixes: 6696777c6506 ("Input: add driver for Elan I2C/SMbus touchpad")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v1:
- Delay only if the regulator was previously disabled / turned off
- Link to v1
https://lore.kernel.org/all/20241001093815.2481899-1-wenst@chromium.org/
---
drivers/input/mouse/elan_i2c_core.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index f93dd545d66b..db48d7ef8357 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -47,6 +47,8 @@
#define ETP_FWIDTH_REDUCE 90
#define ETP_FINGER_WIDTH 15
#define ETP_RETRY_COUNT 3
+/* H/W init 2 ms + F/W init 100 ms w/ round up */
+#define ETP_POWER_ON_DELAY 110
/* quirks to control the device */
#define ETP_QUIRK_QUICK_WAKEUP BIT(0)
@@ -1219,6 +1221,7 @@ static int elan_probe(struct i2c_client *client)
struct device *dev = &client->dev;
struct elan_tp_data *data;
unsigned long irqflags;
+ bool supply_was_enabled;
int error;
if (IS_ENABLED(CONFIG_MOUSE_ELAN_I2C_I2C) &&
@@ -1250,6 +1253,8 @@ static int elan_probe(struct i2c_client *client)
if (IS_ERR(data->vcc))
return dev_err_probe(dev, PTR_ERR(data->vcc), "Failed to get 'vcc' regulator\n");
+ supply_was_enabled = regulator_is_enabled(data->vcc);
+
error = regulator_enable(data->vcc);
if (error) {
dev_err(dev, "Failed to enable regulator: %d\n", error);
@@ -1263,6 +1268,9 @@ static int elan_probe(struct i2c_client *client)
return error;
}
+ if (!supply_was_enabled)
+ msleep(ETP_POWER_ON_DELAY);
+
/* Make sure there is something at this address */
error = i2c_smbus_read_byte(client);
if (error < 0) {
@@ -1406,11 +1414,16 @@ static int elan_resume(struct device *dev)
int error;
if (!device_may_wakeup(dev)) {
+ bool supply_was_enabled = regulator_is_enabled(data->vcc);
+
error = regulator_enable(data->vcc);
if (error) {
dev_err(dev, "error %d enabling regulator\n", error);
goto err;
}
+
+ if (!supply_was_enabled)
+ msleep(ETP_POWER_ON_DELAY);
}
error = elan_set_power(data, true);
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply related
* [PATCH v2 2/7] HID: i2c-hid-of: skip post-power-on delay if already powered on
From: Chen-Yu Tsai @ 2026-07-03 11:55 UTC (permalink / raw)
To: Matthias Brugger, AngeloGioacchino Del Regno, Benson Leung,
Tzung-Bi Shih, Dmitry Torokhov, Jiri Kosina, Andi Shyti
Cc: Chen-Yu Tsai, linux-mediatek, devicetree, linux-arm-kernel,
chrome-platform, linux-input, linux-i2c, linux-kernel
In-Reply-To: <20260703115601.1323491-1-wenst@chromium.org>
On some devices the HID device is powered from an always-on power rail,
or the power rail has been left on by either POR defaults or the
bootloader. By the time the driver probes, the device most certainly
has finished initializing. There is no need for the delay.
In such designs, the system integrators tend to work around the delay
to avoid the boot time penalty by simply omitting it from the device
tree. This is undesired, as the device tree is not fully describing
the hardware.
Instead, check if the regulator supplies are all enabled, and skip
the post-power-on delay if that is the case.
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
drivers/hid/i2c-hid/i2c-hid-of.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
index 59393d71ddb9..70afdfb207ac 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of.c
@@ -29,6 +29,7 @@
#include <linux/of.h>
#include <linux/pm.h>
#include <linux/regulator/consumer.h>
+#include <linux/string_choices.h>
#include "i2c-hid.h"
@@ -46,8 +47,12 @@ static int i2c_hid_of_power_up(struct i2chid_ops *ops)
{
struct i2c_hid_of *ihid_of = container_of(ops, struct i2c_hid_of, ops);
struct device *dev = &ihid_of->client->dev;
+ bool supply_was_enabled = true;
int ret;
+ for (unsigned int i = 0; i < ARRAY_SIZE(ihid_of->supplies); i++)
+ supply_was_enabled &= regulator_is_enabled(ihid_of->supplies[i].consumer);
+
ret = regulator_bulk_enable(ARRAY_SIZE(ihid_of->supplies),
ihid_of->supplies);
if (ret) {
@@ -55,7 +60,8 @@ static int i2c_hid_of_power_up(struct i2chid_ops *ops)
return ret;
}
- if (ihid_of->post_power_delay_ms)
+ dev_dbg(dev, "supply was %s.\n", str_on_off(supply_was_enabled));
+ if (!supply_was_enabled && ihid_of->post_power_delay_ms)
msleep(ihid_of->post_power_delay_ms);
gpiod_set_value_cansleep(ihid_of->reset_gpio, 0);
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply related
* [PATCH v2 3/7] i2c: of-prober: skip post-power-on delay if already powered on
From: Chen-Yu Tsai @ 2026-07-03 11:55 UTC (permalink / raw)
To: Matthias Brugger, AngeloGioacchino Del Regno, Benson Leung,
Tzung-Bi Shih, Dmitry Torokhov, Jiri Kosina, Andi Shyti
Cc: Chen-Yu Tsai, linux-mediatek, devicetree, linux-arm-kernel,
chrome-platform, linux-input, linux-i2c, linux-kernel
In-Reply-To: <20260703115601.1323491-1-wenst@chromium.org>
On some devices the I2C component is powered from an always-on power
rail, or the power rail has been left on by either POR defaults or
the bootloader. By the time the prober probes the device, the device
most certainly has finished initializing and can respond. There is no
need for the delay.
In such designs, the system integrators tend to work around the delay
to avoid the boot time penalty by simply omitting it from the device
tree and the component prober. This is undesired, as the device tree
is not fully describing the hardware.
Instead, check if the regulator supplies are all enabled, and skip
the post-power-on delay if that is the case.
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
drivers/i2c/i2c-core-of-prober.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c
index 6a82b03809d4..f274e260353c 100644
--- a/drivers/i2c/i2c-core-of-prober.c
+++ b/drivers/i2c/i2c-core-of-prober.c
@@ -18,6 +18,7 @@
#include <linux/regulator/consumer.h>
#include <linux/slab.h>
#include <linux/stddef.h>
+#include <linux/string_choices.h>
/*
* Some devices, such as Google Hana Chromebooks, are produced by multiple
@@ -219,19 +220,25 @@ static void i2c_of_probe_simple_put_supply(struct i2c_of_probe_simple_ctx *ctx)
static int i2c_of_probe_simple_enable_regulator(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
{
+ bool supply_was_on;
int ret;
if (!ctx->supply)
return 0;
- dev_dbg(dev, "Enabling regulator supply \"%s\"\n", ctx->opts->supply_name);
+ supply_was_on = regulator_is_enabled(ctx->supply);
+
+ dev_dbg(dev, "Enabling regulator supply \"%s\" (was %s)\n", ctx->opts->supply_name,
+ str_on_off(supply_was_on));
ret = regulator_enable(ctx->supply);
if (ret)
return ret;
- if (ctx->opts->post_power_on_delay_ms)
+ if (!supply_was_on && ctx->opts->post_power_on_delay_ms) {
+ dev_dbg(dev, "Waiting after enabling regulator\n");
msleep(ctx->opts->post_power_on_delay_ms);
+ }
return 0;
}
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply related
* [PATCH v2 4/7] i2c: of-prober: Defer regulator_disable() on successful probe in simple helper
From: Chen-Yu Tsai @ 2026-07-03 11:55 UTC (permalink / raw)
To: Matthias Brugger, AngeloGioacchino Del Regno, Benson Leung,
Tzung-Bi Shih, Dmitry Torokhov, Jiri Kosina, Andi Shyti
Cc: Chen-Yu Tsai, linux-mediatek, devicetree, linux-arm-kernel,
chrome-platform, linux-input, linux-i2c, linux-kernel
In-Reply-To: <20260703115601.1323491-1-wenst@chromium.org>
When a I2C component is found, it's device node is immediately enabled.
This triggers device creation and driver binding. The prober will hold
the regulator enable reference across this part. If the driver probes
synchronously, then it happens within this window. On the other hand,
if the driver probes asynchronously, there is high chance that it
happens after the prober's cleanup function was called, in which case
the regulator would have been disabled when the driver's probe function
is called. This would then require the driver to wait 100 ms for the
hardware to reinitialize, even if the probe function was just a split
second late and the regulator was disabled a few milliseconds ago.
Recently, some of the drivers for the component that are targeted by the
I2C OF component prober gained the ability to skip waiting for hardware
initialization if the regulator was left enabled. This happens when the
PMIC has them on by default, or if the component prober left them on
after probing the component.
Wait a bit of time before dropping the enable refcount on our end so
that the actual driver has the opportunity to catch and increase the
refcount on their end. The 100 ms delay was arbitrarily chosen.
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
drivers/i2c/i2c-core-of-prober.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c
index f274e260353c..17495125467e 100644
--- a/drivers/i2c/i2c-core-of-prober.c
+++ b/drivers/i2c/i2c-core-of-prober.c
@@ -243,11 +243,23 @@ static int i2c_of_probe_simple_enable_regulator(struct device *dev, struct i2c_o
return 0;
}
-static void i2c_of_probe_simple_disable_regulator(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
+static void i2c_of_probe_simple_disable_regulator(struct device *dev,
+ struct i2c_of_probe_simple_ctx *ctx,
+ bool defer_disable)
{
if (!ctx->supply)
return;
+ /*
+ * Wait a bit of time for async drivers to probe and increase the
+ * regulator enable count. This allows the drivers to check and
+ * skip waiting for re-initialization.
+ */
+ if (defer_disable) {
+ dev_dbg(dev, "Deferring regulator disable\n");
+ msleep(100);
+ }
+
dev_dbg(dev, "Disabling regulator supply \"%s\"\n", ctx->opts->supply_name);
regulator_disable(ctx->supply);
@@ -364,7 +376,7 @@ int i2c_of_probe_simple_enable(struct device *dev, struct device_node *bus_node,
return 0;
out_disable_regulator:
- i2c_of_probe_simple_disable_regulator(dev, ctx);
+ i2c_of_probe_simple_disable_regulator(dev, ctx, false);
out_put_gpiod:
i2c_of_probe_simple_put_gpiod(ctx);
out_put_supply:
@@ -409,7 +421,7 @@ void i2c_of_probe_simple_cleanup(struct device *dev, void *data)
i2c_of_probe_simple_disable_gpio(dev, ctx);
i2c_of_probe_simple_put_gpiod(ctx);
- i2c_of_probe_simple_disable_regulator(dev, ctx);
+ i2c_of_probe_simple_disable_regulator(dev, ctx, true);
i2c_of_probe_simple_put_supply(ctx);
}
EXPORT_SYMBOL_NS_GPL(i2c_of_probe_simple_cleanup, "I2C_OF_PROBER");
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply related
* [PATCH v2 5/7] platform/chrome: of_hw_prober: Add delay for hana trackpads
From: Chen-Yu Tsai @ 2026-07-03 11:55 UTC (permalink / raw)
To: Matthias Brugger, AngeloGioacchino Del Regno, Benson Leung,
Tzung-Bi Shih, Dmitry Torokhov, Jiri Kosina, Andi Shyti
Cc: Chen-Yu Tsai, linux-mediatek, devicetree, linux-arm-kernel,
chrome-platform, linux-input, linux-i2c, linux-kernel
In-Reply-To: <20260703115601.1323491-1-wenst@chromium.org>
Up until now, the MT8173 elm/hana device tree has set the dedicated
regulator supplying the trackpad as always-on, simply because the Elan
driver was missing proper delays. As a result the delay for the
Synaptics trackpad was also omitted, as it was not strictly required
under such a model and delayed the availability of the trackpad to the
user.
The Elan driver recently gained proper delays after power-up, with
adaptive skipping of the delay if the regulator was originally
on. The I2C HID driver and I2C OF component prober library gained
similar adaptive delay skipping. The device tree will be fixed to have
the regulator not be always on, and proper post-power-on delay time
added to the I2C HID device.
Also add the post-power-on delay to the ChromeOS OF component prober,
so that if the regulator is off at the time of probing, the prober knows
to wait for the hardware to initialize.
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
drivers/platform/chrome/chromeos_of_hw_prober.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/platform/chrome/chromeos_of_hw_prober.c b/drivers/platform/chrome/chromeos_of_hw_prober.c
index 8562a0e89dc6..54d8941617e2 100644
--- a/drivers/platform/chrome/chromeos_of_hw_prober.c
+++ b/drivers/platform/chrome/chromeos_of_hw_prober.c
@@ -70,10 +70,8 @@ static const struct chromeos_i2c_probe_data chromeos_i2c_probe_hana_trackpad = {
/*
* ELAN trackpad needs 2 ms for H/W init and 100 ms for F/W init.
* Synaptics trackpad needs 100 ms.
- * However, the regulator is set to "always-on", presumably to
- * avoid this delay. The ELAN driver is also missing delays.
*/
- .post_power_on_delay_ms = 0,
+ .post_power_on_delay_ms = 110,
},
};
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply related
* [PATCH v2 6/7] arm64: dts: mediatek: mt8173-elm-hana: Unmark trackpad supply as always-on
From: Chen-Yu Tsai @ 2026-07-03 11:55 UTC (permalink / raw)
To: Matthias Brugger, AngeloGioacchino Del Regno, Benson Leung,
Tzung-Bi Shih, Dmitry Torokhov, Jiri Kosina, Andi Shyti
Cc: Chen-Yu Tsai, linux-mediatek, devicetree, linux-arm-kernel,
chrome-platform, linux-input, linux-i2c, linux-kernel
In-Reply-To: <20260703115601.1323491-1-wenst@chromium.org>
Up until now, the MT8173 elm/hana device tree has set the dedicated
regulator supplying the trackpad as always-on, simply because the Elan
driver was missing proper delays. As a result the delay for the
Synaptics trackpad was also omitted, as it was not strictly required
under such a model and delayed the availability of the trackpad to the
user.
The Elan driver recently gained proper delays after power up, with
opportunistic skipping of the delay when the regulator was originally
on. The I2C HID driver gained similar opportunistic delay skipping.
So has the I2C OF component prober library.
Now fix the device tree to have the regulator not be always on, and
let the I2C HID device have the correct post-power-on delay time.
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi | 8 +-------
arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 1 -
2 files changed, 1 insertion(+), 8 deletions(-)
diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi
index 1004eb8ea52c..b9e311fcd9a0 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi
@@ -62,13 +62,7 @@ trackpad2: trackpad@2c {
pinctrl-0 = <&trackpad_irq>;
reg = <0x2c>;
hid-descr-addr = <0x0020>;
- /*
- * The trackpad needs a post-power-on delay of 100ms,
- * but at time of writing, the power supply for it on
- * this board is always on. The delay is therefore not
- * added to avoid impacting the readiness of the
- * trackpad.
- */
+ post-power-on-delay-ms = <100>;
vdd-supply = <&mt6397_vgp6_reg>;
wakeup-source;
status = "fail-needs-probe";
diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
index a0573bc359fb..6b9f47f515c7 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
@@ -1093,7 +1093,6 @@ mt6397_vgp6_reg: ldo_vgp6 {
regulator-min-microvolt = <3300000>;
regulator-max-microvolt = <3300000>;
regulator-enable-ramp-delay = <218>;
- regulator-always-on;
};
mt6397_vibr_reg: ldo_vibr {
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply related
* [PATCH v2 7/7] arm64: dts: mediatek: mt8192-asurada-spherion: Add Synaptics trackpad's supply
From: Chen-Yu Tsai @ 2026-07-03 11:56 UTC (permalink / raw)
To: Matthias Brugger, AngeloGioacchino Del Regno, Benson Leung,
Tzung-Bi Shih, Dmitry Torokhov, Jiri Kosina, Andi Shyti
Cc: Chen-Yu Tsai, linux-mediatek, devicetree, linux-arm-kernel,
chrome-platform, linux-input, linux-i2c, linux-kernel,
stable+noautosel
In-Reply-To: <20260703115601.1323491-1-wenst@chromium.org>
The Synaptics trackpad, like the Elan trackpad option, is fed from the
system 3.3V power rail. Add it to the trackpad device node.
Also add the correct post-power-on delay, even though in practice it is
not required. The Synaptics trackpad requires 100ms after power-on (or
deasserting the reset, whichever comes later) to fully initialize. The
power is always on and the reset pin is not routed out, so the
implementation could try skipping the delay.
Cc: <stable+noautosel@kernel.org> # Without driver changes only lengthens probe time
Fixes: 925ebc0cd55c ("arm64: dts: mt8192-asurada-spherion: Add Synaptics trackpad support")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
I think this shouldn't be backported, as backporting it without the
driver enhancements just delays the trackpad probing with no real
gains.
---
arch/arm64/boot/dts/mediatek/mt8192-asurada-spherion-r0.dts | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada-spherion-r0.dts b/arch/arm64/boot/dts/mediatek/mt8192-asurada-spherion-r0.dts
index 163960f58db5..147a8e9a3a71 100644
--- a/arch/arm64/boot/dts/mediatek/mt8192-asurada-spherion-r0.dts
+++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada-spherion-r0.dts
@@ -90,6 +90,8 @@ trackpad@2c {
hid-descr-addr = <0x20>;
interrupts-extended = <&pio 15 IRQ_TYPE_LEVEL_LOW>;
wakeup-source;
+ vdd-supply = <&pp3300_u>;
+ post-power-on-delay-ms = <100>;
status = "fail-needs-probe";
};
};
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply related
* Re: [PATCH v2 3/7] i2c: of-prober: skip post-power-on delay if already powered on
From: sashiko-bot @ 2026-07-03 12:05 UTC (permalink / raw)
To: Chen-Yu Tsai; +Cc: conor+dt, dmitry.torokhov, robh, linux-input, devicetree
In-Reply-To: <20260703115601.1323491-4-wenst@chromium.org>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] The integer return value of `regulator_is_enabled()` (which can be a negative error code) is implicitly cast to or bitwise-ANDed with `bool` variables, leading to incorrect evaluations that inadvertently skip required hardware power-on delays.
- [Medium] Shared regulator logic flaw and TOCTOU race allow skipping required power-on delays.
Pre-existing issues:
- [High] Optional regulator supplies missing from the device tree incorrectly cause the prober to fail, contradicting the intended optional behavior.
--
commit 7072efff03a9502c14f9b2eb5a3863213e3992ef
Author: Chen-Yu Tsai <wenst@chromium.org>
i2c: of-prober: skip post-power-on delay if already powered on
This commit modifies the I2C OF prober to check if regulator supplies are
already enabled before applying a post-power-on delay. If the supply is
already active, it skips the delay to avoid unnecessary boot time penalties.
> diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c
> index 6a82b03809d4f..f274e260353cd 100644
> --- a/drivers/i2c/i2c-core-of-prober.c
> +++ b/drivers/i2c/i2c-core-of-prober.c
[ ... ]
> @@ -219,19 +220,25 @@ static void i2c_of_probe_simple_put_supply(struct i2c_of_probe_simple_ctx *ctx)
>
> static int i2c_of_probe_simple_enable_regulator(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
> {
> + bool supply_was_on;
> int ret;
>
> if (!ctx->supply)
> return 0;
>
> - dev_dbg(dev, "Enabling regulator supply \"%s\"\n", ctx->opts->supply_name);
> + supply_was_on = regulator_is_enabled(ctx->supply);
[Severity: High]
Could this introduce a regression by skipping required hardware power-on
delays if regulator_is_enabled() fails?
The integer return value of regulator_is_enabled() can be a negative error
code, but it is implicitly cast to the bool variable supply_was_on without
error checking. Any non-zero integer, including negative values, becomes true.
If regulator_is_enabled() returns an error, the code will consider the supply
as already on, causing the device to skip the mandatory post-power-on delay
and fail to probe because it has not finished initializing.
I noticed similar patterns in other drivers:
- drivers/hid/i2c-hid/i2c-hid-of.c:i2c_hid_of_power_up()
- drivers/input/mouse/elan_i2c_core.c:elan_probe()
- drivers/input/mouse/elan_i2c_core.c:elan_resume()
[Severity: Medium]
Is there a potential regression here regarding a shared regulator TOCTOU race
condition?
If two devices share a regulator and probe concurrently, Driver A might enable
the regulator. When Driver B checks regulator_is_enabled(), it returns true.
Driver B will then skip its required power-on delay and attempt to communicate
with the device.
If Driver A enabled the regulator less than the required delay time ago,
Driver B will access its device before the hardware has initialized, leading
to probe failure or I2C communication errors.
> +
> + dev_dbg(dev, "Enabling regulator supply \"%s\" (was %s)\n", ctx->opts->supply_name,
> + str_on_off(supply_was_on));
>
> ret = regulator_enable(ctx->supply);
> if (ret)
> return ret;
>
> - if (ctx->opts->post_power_on_delay_ms)
> + if (!supply_was_on && ctx->opts->post_power_on_delay_ms) {
> + dev_dbg(dev, "Waiting after enabling regulator\n");
> msleep(ctx->opts->post_power_on_delay_ms);
> + }
>
> return 0;
> }
[Severity: High]
This is a pre-existing issue, but I noticed optional regulator supplies missing
from the device tree incorrectly cause the prober to fail, contradicting the
intended optional behavior.
drivers/i2c/i2c-core-of-prober.c:i2c_of_probe_simple_get_supply() {
...
supply = of_regulator_get_optional(dev, node, supply_name);
if (IS_ERR(supply)) {
return dev_err_probe(dev, PTR_ERR(supply),
"Failed to get regulator supply \"%s\" from %pOF\n",
supply_name, node);
}
...
}
of_regulator_get_optional() returns -ENODEV when the supply is not in the
device tree. The function checks IS_ERR(supply) and propagates the error,
failing the probe unconditionally on hardware platforms where the device tree
omits the optional regulator.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703115601.1323491-1-wenst@chromium.org?part=3
^ permalink raw reply
* Re: [PATCH v2 1/7] Input: elan_i2c - Wait for initialization after enabling regulator supply
From: sashiko-bot @ 2026-07-03 12:06 UTC (permalink / raw)
To: Chen-Yu Tsai; +Cc: conor+dt, devicetree, linux-input, robh, dmitry.torokhov
In-Reply-To: <20260703115601.1323491-2-wenst@chromium.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The return value of `regulator_is_enabled()` is implicitly cast to `bool`, causing negative error codes to evaluate as true and incorrectly bypassing the required power-on delay.
- [High] Skipping the power-on delay for shared regulators is susceptible to unsafe power domain assumptions and TOCTOU races, leading to bypassed hardware initialization.
--
commit 62ef124436c7ca613cd0f8d922094f42cdd2a0b8
Author: Chen-Yu Tsai <wenst@chromium.org>
Input: elan_i2c - Wait for initialization after enabling regulator supply
This commit adds proper delays after enabling the regulator supply for Elan
trackpad controllers, which require time for hardware and firmware to
initialize. It attempts to avoid boot time impact by skipping the delay if
the regulator was already enabled.
> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index fee1796da3d08..110c662cef96f 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
[ ... ]
> @@ -1187,6 +1189,7 @@ static int elan_probe(struct i2c_client *client)
> struct device *dev = &client->dev;
> struct elan_tp_data *data;
> unsigned long irqflags;
> + bool supply_was_enabled;
> int error;
>
> if (IS_ENABLED(CONFIG_MOUSE_ELAN_I2C_I2C) &&
> @@ -1218,6 +1221,8 @@ static int elan_probe(struct i2c_client *client)
> if (IS_ERR(data->vcc))
> return dev_err_probe(dev, PTR_ERR(data->vcc), "Failed to get 'vcc' regulator\n");
>
> + supply_was_enabled = regulator_is_enabled(data->vcc);
[Severity: High]
In elan_probe(), regulator_is_enabled() returns an int, which can include
negative error codes like -EPROBE_DEFER or -EREMOTEIO. Will assigning this to
the bool supply_was_enabled cause negative error codes to evaluate as true?
If this happens, it looks like it would incorrectly bypass the power-on delay
and lead to a permanent -ENXIO probe failure later rather than allowing a
proper deferral.
> +
> error = regulator_enable(data->vcc);
> if (error) {
> dev_err(dev, "Failed to enable regulator: %d\n", error);
> @@ -1231,6 +1236,9 @@ static int elan_probe(struct i2c_client *client)
> return error;
> }
>
> + if (!supply_was_enabled)
> + msleep(ETP_POWER_ON_DELAY);
[Severity: High]
Is it safe to assume that a regulator that is already enabled has been powered
on for a sufficient amount of time?
If the trackpad shares a regulator with another device (like a touchscreen)
that just finished its own probe milliseconds earlier, the regulator would
already be enabled, but the trackpad hardware wouldn't be fully initialized.
Could this cause elan_probe() to skip the required hardware initialization
delay and fail the subsequent i2c_smbus_read_byte() check?
[ ... ]
> @@ -1374,11 +1382,16 @@ static int elan_resume(struct device *dev)
> int error;
>
> if (!device_may_wakeup(dev)) {
> + bool supply_was_enabled = regulator_is_enabled(data->vcc);
[Severity: High]
In elan_resume(), could the same boolean cast issue happen if
regulator_is_enabled() encounters a transient failure during system resume,
causing the resume delay to be skipped?
> +
> error = regulator_enable(data->vcc);
> if (error) {
> dev_err(dev, "error %d enabling regulator\n", error);
> goto err;
> }
> +
> + if (!supply_was_enabled)
> + msleep(ETP_POWER_ON_DELAY);
[Severity: High]
Could the same shared-regulator timing race occur during the system resume
path, skipping the initialization delay if another device on the same shared
power domain resumed just before the trackpad?
> }
>
> error = elan_set_power(data, true);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703115601.1323491-1-wenst@chromium.org?part=1
^ permalink raw reply
* Re: [PATCH v2 2/7] HID: i2c-hid-of: skip post-power-on delay if already powered on
From: sashiko-bot @ 2026-07-03 12:07 UTC (permalink / raw)
To: Chen-Yu Tsai; +Cc: devicetree, robh, conor+dt, linux-input, dmitry.torokhov
In-Reply-To: <20260703115601.1323491-3-wenst@chromium.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Bitwise AND assignment (`&=`) between a `bool` and the `int` return value of `regulator_is_enabled()` causes pseudo-random evaluation based on the least significant bit (LSB) of the integer, potentially skipping mandatory delays.
- [High] Skipping the post-power-on delay based solely on the current state of `regulator_is_enabled()` introduces a race condition for devices sharing a power rail.
--
commit 14324865d2e3b5ccaf90d6ff025e907befb17313
Author: Chen-Yu Tsai <wenst@chromium.org>
HID: i2c-hid-of: skip post-power-on delay if already powered on
This patch checks if the regulator supplies for an i2c-hid device are
already enabled during probe. If all supplies are already on, it skips
the post-power-on delay to save boot time.
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
> --- a/drivers/hid/i2c-hid/i2c-hid-of.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of.c
> @@ -46,8 +47,12 @@ static int i2c_hid_of_power_up(struct i2chid_ops *ops)
> {
> struct i2c_hid_of *ihid_of = container_of(ops, struct i2c_hid_of, ops);
> struct device *dev = &ihid_of->client->dev;
> + bool supply_was_enabled = true;
> int ret;
>
> + for (unsigned int i = 0; i < ARRAY_SIZE(ihid_of->supplies); i++)
> + supply_was_enabled &= regulator_is_enabled(ihid_of->supplies[i].consumer);
[Severity: High]
Could this bitwise operation cause a regression by inadvertently evaluating
the least significant bit of the return value?
The function regulator_is_enabled() returns an int (positive state, 0, or
a negative error). In C, performing a bitwise AND (&=) between a bool
(promoted to integer 1) and an int isolates the least significant bit.
For example, if regulator_is_enabled() returns a negative error code like
-ENODEV (-19), the least significant bit is 1, causing supply_was_enabled
to evaluate to true despite the error. If it returns an even positive
state like 2, the least significant bit is 0, making it false.
[ ... ]
> if (ret) {
> dev_warn(dev, "Failed to enable supplies: %d\n", ret);
> return ret;
> }
>
> - if (ihid_of->post_power_delay_ms)
> + dev_dbg(dev, "supply was %s.\n", str_on_off(supply_was_enabled));
> + if (!supply_was_enabled && ihid_of->post_power_delay_ms)
> msleep(ihid_of->post_power_delay_ms);
[Severity: High]
Is it safe to skip this delay based solely on whether the regulator is
currently enabled?
If two devices (e.g. a touchscreen and a trackpad) share the same
non-always-on regulator and probe asynchronously:
Device 1 starts probing, calls regulator_bulk_enable(), and begins
waiting its post-power delay.
Device 2 concurrently probes in i2c_hid_of_power_up(), sees
regulator_is_enabled() is true, and skips its post_power_delay_ms here.
Device 2 would then immediately issue I2C transactions to a newly powered
device that might not have finished initializing, potentially resulting
in NACKs and a probe regression.
Does the regulator framework provide a way to ensure the rail has been on
for the required duration, rather than just currently enabled?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703115601.1323491-1-wenst@chromium.org?part=2
^ permalink raw reply
* Re: [PATCH v2 4/7] i2c: of-prober: Defer regulator_disable() on successful probe in simple helper
From: sashiko-bot @ 2026-07-03 12:08 UTC (permalink / raw)
To: Chen-Yu Tsai; +Cc: dmitry.torokhov, devicetree, linux-input, conor+dt, robh
In-Reply-To: <20260703115601.1323491-5-wenst@chromium.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The prober unconditionally sleeps for 100ms during the cleanup path even when no matching I2C component is found, unnecessarily delaying system boot.
--
commit 772a8f5b00b1078ef6740a0746df36699760f2f2
Author: Chen-Yu Tsai <wenst@chromium.org>
i2c: of-prober: Defer regulator_disable() on successful probe in simple helper
This patch defers regulator disabling in the I2C OF component prober by adding
a 100ms delay. The delay allows asynchronously probing drivers to catch and
increase the regulator enable count, avoiding hardware re-initialization.
> diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c
> index f274e260353cd..17495125467ed 100644
> --- a/drivers/i2c/i2c-core-of-prober.c
> +++ b/drivers/i2c/i2c-core-of-prober.c
> @@ -243,11 +243,23 @@ static int i2c_of_probe_simple_enable_regulator(struct device *dev, struct i2c_o
> return 0;
> }
>
> -static void i2c_of_probe_simple_disable_regulator(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
> +static void i2c_of_probe_simple_disable_regulator(struct device *dev,
> + struct i2c_of_probe_simple_ctx *ctx,
> + bool defer_disable)
> {
> if (!ctx->supply)
> return;
>
> + /*
> + * Wait a bit of time for async drivers to probe and increase the
> + * regulator enable count. This allows the drivers to check and
> + * skip waiting for re-initialization.
> + */
> + if (defer_disable) {
> + dev_dbg(dev, "Deferring regulator disable\n");
> + msleep(100);
> + }
> +
[ ... ]
> @@ -409,7 +421,7 @@ void i2c_of_probe_simple_cleanup(struct device *dev, void *data)
> i2c_of_probe_simple_disable_gpio(dev, ctx);
> i2c_of_probe_simple_put_gpiod(ctx);
>
> - i2c_of_probe_simple_disable_regulator(dev, ctx);
> + i2c_of_probe_simple_disable_regulator(dev, ctx, true);
[Severity: Medium]
Does this unconditionally sleep for 100ms even when no matching I2C component
is found?
If i2c_of_probe_component() iterates over potential components but finds none,
it calls ops->cleanup(dev, ctx), which invokes i2c_of_probe_simple_cleanup().
Because defer_disable is hardcoded to true here,
i2c_of_probe_simple_disable_regulator() will execute the 100ms sleep.
Could this result in an unnecessary 100ms boot delay on systems missing the
probed component, since no async driver will ever probe in this failure path?
> i2c_of_probe_simple_put_supply(ctx);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703115601.1323491-1-wenst@chromium.org?part=4
^ permalink raw reply
* Re: [PATCH v2 5/7] platform/chrome: of_hw_prober: Add delay for hana trackpads
From: sashiko-bot @ 2026-07-03 12:10 UTC (permalink / raw)
To: Chen-Yu Tsai; +Cc: devicetree, linux-input, conor+dt, dmitry.torokhov, robh
In-Reply-To: <20260703115601.1323491-6-wenst@chromium.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The result of regulator_is_enabled() is assigned to or bitwise-ANDed with a boolean variable, causing negative error codes to incorrectly evaluate to true and skip required hardware initialization delays.
--
commit 1aa1abec8abc658e89bd1583fb36c5b23bda7cf8
Author: Chen-Yu Tsai <wenst@chromium.org>
platform/chrome: of_hw_prober: Add delay for hana trackpads
Up until now, the MT8173 elm/hana device tree has set the dedicated
regulator supplying the trackpad as always-on, simply because the Elan
driver was missing proper delays. As a result the delay for the
Synaptics trackpad was also omitted, as it was not strictly required
under such a model and delayed the availability of the trackpad to the
user.
> The Elan driver recently gained proper delays after power-up, with
> adaptive skipping of the delay if the regulator was originally
> on. The I2C HID driver and I2C OF component prober library gained
> similar adaptive delay skipping. The device tree will be fixed to have
> the regulator not be always on, and proper post-power-on delay time
> added to the I2C HID device.
[Severity: High]
Does the adaptive skipping logic mentioned here safely handle negative error
codes from regulator_is_enabled()?
Looking at the implementations of this adaptive skipping,
regulator_is_enabled() can return a negative error code (e.g., -EIO, -EBUSY)
on failure, but this result is being directly assigned to or bitwise-ANDed
with a boolean.
For example, in drivers/hid/i2c-hid/i2c-hid-of.c:i2c_hid_of_power_up():
for (unsigned int i = 0; i < ARRAY_SIZE(ihid_of->supplies); i++)
supply_was_enabled &= regulator_is_enabled(ihid_of->supplies[i].consumer);
Since supply_was_enabled is promoted to int for the bitwise AND, does this
outcome depend on the parity of the error code? It looks like odd errors
(like -EIO) will yield true (skipping the delay), while even errors (like
-EBUSY) will yield false.
Similarly, in drivers/i2c/i2c-core-of-prober.c:
i2c_of_probe_simple_enable_regulator():
supply_was_on = regulator_is_enabled(ctx->supply);
And in drivers/input/mouse/elan_i2c_core.c:elan_probe() and elan_resume():
supply_was_enabled = regulator_is_enabled(data->vcc);
Since any non-zero integer evaluates to true in C, assigning a negative error
code directly to a bool means all errors will incorrectly signal that the
regulator was already on.
Could this cause a regression where a transient I2C or hardware error when
querying the regulator state makes the driver incorrectly skip required
hardware initialization delays and fail the device probe?
> Also add the post-power-on delay to the ChromeOS OF component prober,
> so that if the regulator is off at the time of probing, the prober knows
> to wait for the hardware to initialize.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703115601.1323491-1-wenst@chromium.org?part=5
^ permalink raw reply
* Re: [PATCH v2 2/6] iio: hid-sensors: align function parenthesis for readability
From: Andy Shevchenko @ 2026-07-03 12:52 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Sanjay Chitroda via B4 Relay, sanjayembeddedse, David Lechner,
Nuno Sá, Andy Shevchenko, Jiri Kosina, Srinivas Pandruvada,
linux-iio, linux-kernel, linux-input
In-Reply-To: <20260702182015.303db93c@jic23-huawei>
On Thu, Jul 02, 2026 at 06:20:15PM +0100, Jonathan Cameron wrote:
> On Thu, 02 Jul 2026 21:47:59 +0530
> Sanjay Chitroda via B4 Relay <devnull+sanjayembeddedse.gmail.com@kernel.org> wrote:
>
> > Adjust alignment of parentheses across HID sensor IIO drivers to
> > improve readability and maintain consistency with kernel coding style.
> >
> > While updating the formatting, group related arguments consistently in
> > multi-line function signatures where appropriate.
> >
> > No functional change intended.
>
> Whilst I appreciate this code isn't quite in line with standards
> and usually like that stuff to be fixed up, in this particular case
> this is a massive amount of churn. That churn will make backporting
> fixes etc messier, so I'd like input on whether others consider this
> one worthwhile. Jiri, Srinivas, Andy etc. What do you think?
I am fine as long as Srinivas is. I understand pros and cons of this, but from
time to time we have patches à la this one that messes up with backporting but
were accepted as a good part of some bigger series.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 1/2] HID: logitech-hidpp: add HID++ 2.0 reprogrammable button support
From: Bastien Nocera @ 2026-07-03 12:53 UTC (permalink / raw)
To: Elliot Douglas; +Cc: linux-input, lains, jikos, bentiss, linux-kernel
In-Reply-To: <CAGt6S1pqCNPeV-Hy6w-8eynKhKwzz5X4fefkJfM0gYGgQzGAwA@mail.gmail.com>
On Wed, 2026-07-01 at 15:32 -0700, Elliot Douglas wrote:
> Just wanted to poke on this thread again, Benjamin or Bastien, what
> is needed to
> push this forward or should I send the v2 at this point?
You're definitely better off sending a new version when there's no
feedback incoming, it acts as a gentle reminder and avoids another back
and forth if there's no further comments (apart from the ones in the
original review).
>
> Thanks,
> Elliot
>
> On Wed, Jun 17, 2026 at 6:16 PM Elliot Douglas
> <edouglas7358@gmail.com> wrote:
> >
> > Thanks, that makes sense.
> >
> > For Solaar, this is not continuously forced. The kernel only
> > programs
> > temporary diversion when the device connects. Solaar can still
> > issue HID++
> > commands through hidraw, so if Solaar changes reporting for the
> > same controls
> > afterwards, the last writer wins.
> >
> > If Solaar takes over those controls for custom actions, the kernel
> > would stop
> > receiving the diverted button notifications for normal evdev
> > reporting until
> > the kernel diverts the controls again, for example after reconnect.
> > While the
> > controls remain diverted, hidraw clients should still receive the
> > raw HID++
> > reports.
> >
> > I have addressed the inline comments locally for v2:
> > - replaced the profile/count wrapper with NULL-terminated mapping
> > arrays
> > - cached the selected mapping pointer in struct hidpp_device
> >
> > I'll wait for Benjamin's input to send Patch v2.
> >
> >
> > On Wed, Jun 17, 2026 at 3:28 AM Bastien Nocera <hadess@hadess.net>
> > wrote:
> > >
> > > On Sat, 2026-06-13 at 10:51 -0700, Elliot Douglas wrote:
> > > > Some Logitech HID++ 2.0 mice can report diverted reprogrammable
> > > > controls through HID++ feature 0x1b04, SpecialKeysMseButtons /
> > > > REPROG_CONTROLS_V4, instead of the normal HID mouse report.
> > > >
> > > > Add a quirk-gated event path for those controls. The handler
> > > > temporarily
> > > > diverts verified per-product controls, parses
> > > > divertedButtonsEvent as the
> > > > current pressed-control list, and reports the corresponding
> > > > evdev key state
> > > > for every mapped control.
> > > >
> > > > Keep the control mappings in per-product profiles so adding
> > > > support for
> > > > another mouse does not change the evdev capabilities advertised
> > > > by
> > > > already-supported devices.
> > >
> > > How does this forced setting work/clash with the programmable
> > > buttons
> > > in Solaar?
> > >
> > > I've added some inline comments below.
> > >
> > > >
> > > > Documentation for feature 0x1b04 describes divertedButtonsEvent
> > > > as a list
> > > > of currently pressed diverted buttons, which is the event
> > > > format handled
> > > > here.
> > > >
> > > > Link:
> > > > https://lekensteyn.nl/files/logitech/x1b04_specialkeysmsebuttons.html
> > > >
> > > > Signed-off-by: Elliot Douglas <edouglas7358@gmail.com>
> > > > ---
> > > > drivers/hid/hid-logitech-hidpp.c | 215
> > > > +++++++++++++++++++++++++++++++
> > > > 1 file changed, 215 insertions(+)
> > > >
> > > > diff --git a/drivers/hid/hid-logitech-hidpp.c
> > > > b/drivers/hid/hid-logitech-hidpp.c
> > > > index 70ba1a5e40d8..24c9cfaa4f37 100644
> > > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > > @@ -76,6 +76,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
> > > > #define HIDPP_QUIRK_HI_RES_SCROLL_1P0 BIT(28)
> > > > #define HIDPP_QUIRK_WIRELESS_STATUS BIT(29)
> > > > #define HIDPP_QUIRK_RESET_HI_RES_SCROLL BIT(30)
> > > > +#define HIDPP_QUIRK_HIDPP_REPROG_CONTROLS_BTNS BIT(31)
> > > >
> > > > /* These are just aliases for now */
> > > > #define HIDPP_QUIRK_KBD_SCROLL_WHEEL HIDPP_QUIRK_HIDPP_WHEELS
> > > > @@ -205,6 +206,7 @@ struct hidpp_device {
> > > > struct hidpp_scroll_counter vertical_wheel_counter;
> > > >
> > > > u8 wireless_feature_index;
> > > > + u8 reprog_controls_feature_index;
> > > >
> > > > int hires_wheel_multiplier;
> > > > u8 hires_wheel_feature_index;
> > > > @@ -3601,6 +3603,209 @@ static int
> > > > hidpp10_extra_mouse_buttons_raw_event(struct hidpp_device
> > > > *hidpp,
> > > > return 1;
> > > > }
> > > >
> > > > +/* -----------------------------------------------------------
> > > > --------------- */
> > > > +/* HID++2.0 reprogrammable
> > > > controls */
> > > > +/* -----------------------------------------------------------
> > > > --------------- */
> > > > +
> > > > +#define HIDPP_PAGE_REPROG_CONTROLS_V4
> > > > 0x1b04
> > > > +
> > > > +#define HIDPP_REPROG_CONTROLS_GET_COUNT
> > > > 0x00
> > > > +#define HIDPP_REPROG_CONTROLS_GET_CID_INFO 0x10
> > > > +#define HIDPP_REPROG_CONTROLS_SET_CONTROL_REPORTING 0x30
> > > > +
> > > > +#define HIDPP_REPROG_CONTROLS_FLAG_MOUSE BIT(0)
> > > > +#define HIDPP_REPROG_CONTROLS_FLAG_DIVERT BIT(5)
> > > > +
> > > > +#define HIDPP_REPROG_CONTROLS_TEMPORARY_DIVERTED BIT(0)
> > > > +#define HIDPP_REPROG_CONTROLS_CHANGE_TEMPORARY_DIVERT
> > > > BIT(1)
> > > > +
> > > > +#define HIDPP_REPROG_CONTROLS_EVENT_DIVERTED 0x00
> > > > +
> > > > +struct hidpp_reprog_control_mapping {
> > > > + u16 control;
> > > > + u16 code;
> > > > +};
> > > > +
> > > > +struct hidpp_reprog_controls_profile {
> > > > + const struct hidpp_reprog_control_mapping *mappings;
> > >
> > > probably needs a __counted_by(), or maybe as it's static, it
> > > might be
> > > better to not require an intermediate struct, and return a NULL-
> > > terminated array instead.
> > >
> > > > + unsigned int mapping_count;
> > > > +};
> > > > +
> > > > +static const struct hidpp_reprog_controls_profile *
> > > > +hidpp20_reprog_controls_get_profile(struct hidpp_device
> > > > *hidpp)
> > > > +{
> > > > + return NULL;
> > > > +}
> > > > +
> > > > +static int hidpp20_reprog_controls_get_count(struct
> > > > hidpp_device *hidpp)
> > > > +{
> > > > + struct hidpp_report response;
> > > > + u8 feature_index = hidpp->reprog_controls_feature_index;
> > > > + u8 cmd = HIDPP_REPROG_CONTROLS_GET_COUNT;
> > > > + int ret;
> > > > +
> > > > + ret = hidpp_send_fap_command_sync(hidpp, feature_index,
> > > > cmd, NULL, 0,
> > > > + &response);
> > > > + if (ret > 0)
> > > > + return -EPROTO;
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + return response.fap.params[0];
> > > > +}
> > > > +
> > > > +static int hidpp20_reprog_controls_get_cid_info(struct
> > > > hidpp_device *hidpp,
> > > > + u8 index, u16
> > > > *control,
> > > > + u8 *flags)
> > > > +{
> > > > + struct hidpp_report response;
> > > > + u8 feature_index = hidpp->reprog_controls_feature_index;
> > > > + u8 cmd = HIDPP_REPROG_CONTROLS_GET_CID_INFO;
> > > > + int ret;
> > > > +
> > > > + ret = hidpp_send_fap_command_sync(hidpp, feature_index,
> > > > cmd, &index,
> > > > + sizeof(index),
> > > > &response);
> > > > + if (ret > 0)
> > > > + return -EPROTO;
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + *control = get_unaligned_be16(&response.fap.params[0]);
> > > > + *flags = response.fap.params[4];
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static bool hidpp20_reprog_controls_find_control(struct
> > > > hidpp_device *hidpp,
> > > > + u16 control)
> > > > +{
> > > > + int count, ret;
> > > > + u16 cid;
> > > > + u8 flags;
> > > > + int i;
> > > > +
> > > > + count = hidpp20_reprog_controls_get_count(hidpp);
> > > > + if (count < 0)
> > > > + return false;
> > > > +
> > > > + for (i = 0; i < count; i++) {
> > > > + ret = hidpp20_reprog_controls_get_cid_info(hidpp,
> > > > i, &cid,
> > > > +
> > > > &flags);
> > > > + if (ret)
> > > > + return false;
> > > > +
> > > > + if (cid == control)
> > > > + return (flags &
> > > > HIDPP_REPROG_CONTROLS_FLAG_MOUSE) &&
> > > > + (flags &
> > > > HIDPP_REPROG_CONTROLS_FLAG_DIVERT);
> > > > + }
> > > > +
> > > > + return false;
> > > > +}
> > > > +
> > > > +static int
> > > > hidpp20_reprog_controls_set_control_reporting(struct
> > > > hidpp_device *hidpp,
> > > > + u16
> > > > control, u8 flags)
> > > > +{
> > > > + struct hidpp_report response;
> > > > + u8 params[5];
> > > > +
> > > > + put_unaligned_be16(control, ¶ms[0]);
> > > > + params[2] = flags;
> > > > + put_unaligned_be16(control, ¶ms[3]);
> > > > +
> > > > + return hidpp_send_fap_command_sync(hidpp,
> > > > + hidpp-
> > > > >reprog_controls_feature_index,
> > > > +
> > > > HIDPP_REPROG_CONTROLS_SET_CONTROL_REPORTING,
> > > > + params,
> > > > sizeof(params), &response);
> > > > +}
> > > > +
> > > > +static void hidpp20_reprog_controls_connect(struct
> > > > hidpp_device *hidpp)
> > > > +{
> > > > + const struct hidpp_reprog_controls_profile *profile;
> > > > + u8 flags = HIDPP_REPROG_CONTROLS_TEMPORARY_DIVERTED |
> > > > + HIDPP_REPROG_CONTROLS_CHANGE_TEMPORARY_DIVERT;
> > > > + unsigned int i;
> > > > +
> > > > + if (!(hidpp->quirks &
> > > > HIDPP_QUIRK_HIDPP_REPROG_CONTROLS_BTNS))
> > > > + return;
> > > > +
> > > > + profile = hidpp20_reprog_controls_get_profile(hidpp);
> > >
> > > Could the profile be cached in the hidpp_device struct?
> > >
> > > > + if (!profile)
> > > > + return;
> > > > +
> > > > + if (hidpp_root_get_feature(hidpp,
> > > > HIDPP_PAGE_REPROG_CONTROLS_V4,
> > > > + &hidpp-
> > > > >reprog_controls_feature_index))
> > > > + return;
> > > > +
> > > > + for (i = 0; i < profile->mapping_count; i++) {
> > > > + u16 control = profile->mappings[i].control;
> > > > +
> > > > + if (!hidpp20_reprog_controls_find_control(hidpp,
> > > > control))
> > > > + continue;
> > > > +
> > > > +
> > > > hidpp20_reprog_controls_set_control_reporting(hidpp, control,
> > > > flags);
> > > > + }
> > > > +}
> > > > +
> > > > +static int hidpp20_reprog_controls_raw_event(struct
> > > > hidpp_device *hidpp,
> > > > + u8 *data, int size)
> > > > +{
> > > > + const struct hidpp_reprog_controls_profile *profile;
> > > > + const struct hidpp_reprog_control_mapping *mapping;
> > > > + struct hidpp_report *report = (struct hidpp_report
> > > > *)data;
> > > > + u16 controls[4];
> > > > + bool pressed;
> > > > + unsigned int i, j;
> > > > +
> > > > + if (!(hidpp->quirks &
> > > > HIDPP_QUIRK_HIDPP_REPROG_CONTROLS_BTNS) ||
> > > > + !hidpp->input ||
> > > > + hidpp->reprog_controls_feature_index == 0xff)
> > > > + return 0;
> > > > +
> > > > + profile = hidpp20_reprog_controls_get_profile(hidpp);
> > > > + if (!profile)
> > > > + return 0;
> > > > +
> > > > + if (size < HIDPP_REPORT_LONG_LENGTH ||
> > > > + report->fap.feature_index != hidpp-
> > > > >reprog_controls_feature_index ||
> > > > + report->fap.funcindex_clientid !=
> > > > HIDPP_REPROG_CONTROLS_EVENT_DIVERTED)
> > > > + return 0;
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(controls); i++)
> > > > + controls[i] = get_unaligned_be16(&report-
> > > > >fap.params[i * 2]);
> > > > +
> > > > + for (i = 0; i < profile->mapping_count; i++) {
> > > > + mapping = &profile->mappings[i];
> > > > + pressed = false;
> > > > +
> > > > + for (j = 0; j < ARRAY_SIZE(controls); j++) {
> > > > + if (controls[j] == mapping->control) {
> > > > + pressed = true;
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + input_report_key(hidpp->input, mapping->code,
> > > > pressed);
> > > > + }
> > > > +
> > > > + input_sync(hidpp->input);
> > > > +
> > > > + return 1;
> > > > +}
> > > > +
> > > > +static void hidpp20_reprog_controls_populate_input(struct
> > > > hidpp_device *hidpp,
> > > > + struct
> > > > input_dev *input_dev)
> > > > +{
> > > > + const struct hidpp_reprog_controls_profile *profile;
> > > > + unsigned int i;
> > > > +
> > > > + profile = hidpp20_reprog_controls_get_profile(hidpp);
> > > > + if (!profile)
> > > > + return;
> > > > +
> > > > + for (i = 0; i < profile->mapping_count; i++)
> > > > + input_set_capability(input_dev, EV_KEY, profile-
> > > > >mappings[i].code);
> > > > +}
> > > > +
> > > > static void hidpp10_extra_mouse_buttons_populate_input(
> > > > struct hidpp_device *hidpp, struct
> > > > input_dev *input_dev)
> > > > {
> > > > @@ -3859,6 +4064,9 @@ static void hidpp_populate_input(struct
> > > > hidpp_device *hidpp,
> > > >
> > > > if (hidpp->quirks & HIDPP_QUIRK_HIDPP_EXTRA_MOUSE_BTNS)
> > > > hidpp10_extra_mouse_buttons_populate_input(hidpp,
> > > > input);
> > > > +
> > > > + if (hidpp->quirks &
> > > > HIDPP_QUIRK_HIDPP_REPROG_CONTROLS_BTNS)
> > > > + hidpp20_reprog_controls_populate_input(hidpp,
> > > > input);
> > > > }
> > > >
> > > > static int hidpp_input_configured(struct hid_device *hdev,
> > > > @@ -3971,6 +4179,10 @@ static int hidpp_raw_hidpp_event(struct
> > > > hidpp_device *hidpp, u8 *data,
> > > > return ret;
> > > > }
> > > >
> > > > + ret = hidpp20_reprog_controls_raw_event(hidpp, data,
> > > > size);
> > > > + if (ret != 0)
> > > > + return ret;
> > > > +
> > > > if (hidpp->quirks &
> > > > HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS) {
> > > > ret = hidpp10_consumer_keys_raw_event(hidpp,
> > > > data, size);
> > > > if (ret != 0)
> > > > @@ -4264,6 +4476,8 @@ static void hidpp_connect_event(struct
> > > > work_struct *work)
> > > > return;
> > > > }
> > > >
> > > > + hidpp20_reprog_controls_connect(hidpp);
> > > > +
> > > > if (hidpp->quirks &
> > > > HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS) {
> > > > ret = hidpp10_consumer_keys_connect(hidpp);
> > > > if (ret)
> > > > @@ -4436,6 +4650,7 @@ static int hidpp_probe(struct hid_device
> > > > *hdev, const struct hid_device_id *id)
> > > > hidpp->hid_dev = hdev;
> > > > hidpp->name = hdev->name;
> > > > hidpp->quirks = id->driver_data;
> > > > + hidpp->reprog_controls_feature_index = 0xff;
> > > > hid_set_drvdata(hdev, hidpp);
> > > >
> > > > ret = hid_parse(hdev);
^ permalink raw reply
* Re: [PATCH v4 0/5] Add OneXPlayer Configuration HID Driver
From: Günther Noack @ 2026-07-03 14:25 UTC (permalink / raw)
To: Derek J. Clark
Cc: Jiri Kosina, Benjamin Tissoires, Pierre-Loup A . Griffais,
Lee Jones, Lambert Fan, Zhouwang Huang, linux-input, linux-doc,
linux-kernel
In-Reply-To: <20260419042624.625746-1-derekjohn.clark@gmail.com>
Hello Derek!
On Sat, Apr 18, 2026 at 09:26:19PM -0700, Derek J. Clark wrote:
> Adds an HID driver for OneXPlayer HID configuration devices. There are
> currently 2 generations of OneXPlayer HID protocol. The first (OneXPlayer
> F1 series) only provides an RGB control interface over HID. The Second
> (X1 mini series, G1 series, AOKZOE A1X) also includes a hardware level
> button mapping interface, vibration intensity settings, and the ability
> to switch output between xinput and a debug mode that can be used to debug
> the button mapping. Some devices (G1 Series, APEX) use a hybrid of Gen1
> RGB control and Gen 2 controller settings. To ensure there is no conflicts
> when the driver is loaded, we skip creating the RGB interface for Gen 2
> devices if there is a DMI match.
>
> I'll also add a note that Gen 1 devices also have an interface for
> setting the key map and debug mode, but that is done entirely over a
> serial TTY device so it is not able to be added to this driver. There
> are also some "Gen 0" devices (OneXPlayer 2 Series) also use it, but
> the TTY interface also handles the RGB control so no support is
> provided by this driver for those interfaces.
>
> Signed-off-by: Derel J. Clark <derekjohn.clark@gmail.com>
Sorry I am late to this review, but here are two issues I discovered
when looking at the code:
(1) The functions oxp_hid_raw_event_gen_1() and
oxp_hid_raw_event_gen_2() are both forgetting to do bounds checks
against the "size" argument.
For real devices, which send a real report descriptor, these buffers
will be large enough, but a device that sends a faked report
descriptor can provoke an out-of-bounds-read here by underspecifying
the size for these reports.
(2) oxp_hid_probe() and other functions are populating drvdata, and
drvdata is a static variable. If you plug in two of these devices
at the same time, they will step on each other's toes, and this
leads to all kinds of memory corruption problems when they do.
I believe the right way to go about this is to allocate a separate
piece of memory for each device that you are plugging in. Other
device drivers do this uing devm_kzalloc().
Disclaimer:
I found these through code inspection and curiosity but have not tried
to reproduce the crashes.
Per Linux's official threat model[1], these are not considered security
vulnerabilities. An attacker who impersonates a USB device and gains
illegitimate access to the USB port might be able to provoke these bugs
though, and I wouldn't be surprised if (2) also just leads to system
crashes when using two of these devices at the same time.
—Günther
[1] https://docs.kernel.org/process/threat-model.html
^ permalink raw reply
* [PATCH] Add a quirk for a Cirque I2C device.
From: Vadim Klishko @ 2026-07-03 14:47 UTC (permalink / raw)
To: jikos; +Cc: linux-input, linux-kernel, Vadim Klishko
Cirque touchpads with PID D0C1 generate an error when probed
by the I2C HID driver, resulting in no hidraw device created.
Signed-off-by: Vadim Klishko <vadim@cirque.com>
---
drivers/hid/hid-ids.h | 1 +
drivers/hid/i2c-hid/i2c-hid-core.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 1059922baaac..496589277bd5 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -334,6 +334,7 @@
#define I2C_VENDOR_ID_CIRQUE 0x0488
#define I2C_PRODUCT_ID_CIRQUE_1063 0x1063
+#define I2C_PRODUCT_ID_CIRQUE_D0C1 0xD0C1
#define USB_VENDOR_ID_CJTOUCH 0x24b8
#define USB_DEVICE_ID_CJTOUCH_MULTI_TOUCH_0020 0x0020
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 3adb16366e93..63004bcbfa71 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -136,6 +136,8 @@ static const struct i2c_hid_quirks {
I2C_HID_QUIRK_BAD_INPUT_SIZE },
{ I2C_VENDOR_ID_CIRQUE, I2C_PRODUCT_ID_CIRQUE_1063,
I2C_HID_QUIRK_NO_SLEEP_ON_SUSPEND },
+ { I2C_VENDOR_ID_CIRQUE, I2C_PRODUCT_ID_CIRQUE_D0C1,
+ I2C_HID_QUIRK_NO_IRQ_AFTER_RESET },
/*
* Without additional power on command, at least some QTEC devices send garbage
*/
--
2.34.1
^ permalink raw reply related
* [PATCH 0/3] hid: fix missing hid_is_usb() checks in three drivers
From: Jann Horn @ 2026-07-03 15:16 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Ilpo Järvinen, Mario Limonciello, Luke D. Jones, Miao Li,
linux-input, linux-kernel, Jann Horn, stable
This fixes missing hid_is_usb() checks before to_usb_interface() in
three HID drivers.
I've split it into three patches so that they can have separate "Fixes"
tags, hopefully they are easier to stable-backport this way.
Signed-off-by: Jann Horn <jannh@google.com>
---
Jann Horn (3):
HID: asus: fix missing hid_is_usb() check
HID: huawei: fix missing hid_is_usb() check
HID: rapoo: fix missing hid_is_usb() check
drivers/hid/hid-asus.c | 2 +-
drivers/hid/hid-huawei.c | 5 +++--
drivers/hid/hid-rapoo.c | 2 +-
3 files changed, 5 insertions(+), 4 deletions(-)
---
base-commit: 51512e22efe813d8223de27f6fd02a8a48ea2323
change-id: 20260703-hid-usbcheck-9163e6cf6015
Best regards,
--
Jann Horn <jannh@google.com>
^ permalink raw reply
* [PATCH 1/3] HID: asus: fix missing hid_is_usb() check
From: Jann Horn @ 2026-07-03 15:16 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Ilpo Järvinen, Mario Limonciello, Luke D. Jones, Miao Li,
linux-input, linux-kernel, Jann Horn, stable
In-Reply-To: <20260703-hid-usbcheck-v1-0-e80259ff625d@google.com>
to_usb_interface() can only be used on a hid_device whose parent is really
USB; uhid can create devices that identify as being on BUS_USB, but don't
actually have a USB parent.
Fix the use of to_usb_interface() without a hid_is_usb() check.
I have verified that it is currently possible to trigger a kernel splat due
to this bug in an ASAN build, and that this commit fixes the issue.
Fixes: 00e005c952f7 ("hid-asus: check ROG Ally MCU version and warn")
Cc: stable@vger.kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
---
drivers/hid/hid-asus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 3f5e96900b67..befa990b3210 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -753,7 +753,7 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
return ret;
}
- if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
+ if ((drvdata->quirks & QUIRK_ROG_ALLY_XPAD) && hid_is_usb(hdev)) {
intf = to_usb_interface(hdev->dev.parent);
udev = interface_to_usbdev(intf);
validate_mcu_fw_version(hdev,
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply related
* [PATCH 2/3] HID: huawei: fix missing hid_is_usb() check
From: Jann Horn @ 2026-07-03 15:16 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Ilpo Järvinen, Mario Limonciello, Luke D. Jones, Miao Li,
linux-input, linux-kernel, Jann Horn, stable
In-Reply-To: <20260703-hid-usbcheck-v1-0-e80259ff625d@google.com>
to_usb_interface() can only be used on a hid_device whose parent is really
USB; uhid can create devices that identify as being on BUS_USB, but don't
actually have a USB parent.
Fix the use of to_usb_interface() without a hid_is_usb() check.
I have verified that it is currently possible to trigger a kernel splat due
to this bug in an ASAN build, and that this commit fixes the issue.
Fixes: e93faaca84b7 ("HID: huawei: fix CD30 keyboard report descriptor issue")
Cc: stable@vger.kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
---
drivers/hid/hid-huawei.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-huawei.c b/drivers/hid/hid-huawei.c
index 6a616bf21b38..ee3fc6f68475 100644
--- a/drivers/hid/hid-huawei.c
+++ b/drivers/hid/hid-huawei.c
@@ -44,11 +44,12 @@ static const __u8 huawei_cd30_kbd_rdesc_fixed[] = {
static const __u8 *huawei_report_fixup(struct hid_device *hdev, __u8 *rdesc,
unsigned int *rsize)
{
- struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
+ struct usb_interface *intf = hid_is_usb(hdev) ?
+ to_usb_interface(hdev->dev.parent) : NULL;
switch (hdev->product) {
case USB_DEVICE_ID_HUAWEI_CD30KBD:
- if (intf->cur_altsetting->desc.bInterfaceNumber == 1) {
+ if (!intf || intf->cur_altsetting->desc.bInterfaceNumber == 1) {
if (*rsize != sizeof(huawei_cd30_kbd_rdesc_fixed) ||
memcmp(huawei_cd30_kbd_rdesc_fixed, rdesc,
sizeof(huawei_cd30_kbd_rdesc_fixed)) != 0) {
--
2.55.0.rc0.799.gd6f94ed593-goog
^ permalink raw reply related
* [PATCH 3/3] HID: rapoo: fix missing hid_is_usb() check
From: Jann Horn @ 2026-07-03 15:16 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Ilpo Järvinen, Mario Limonciello, Luke D. Jones, Miao Li,
linux-input, linux-kernel, Jann Horn, stable
In-Reply-To: <20260703-hid-usbcheck-v1-0-e80259ff625d@google.com>
to_usb_interface() can only be used on a hid_device whose parent is really
USB; uhid can create devices that identify as being on BUS_USB, but don't
actually have a USB parent.
Fix the use of to_usb_interface() without a hid_is_usb() check.
I have verified that it is currently possible to trigger a kernel splat due
to this bug in an ASAN build, and that this commit fixes the issue.
Fixes: 00e005c952f7 ("hid-asus: check ROG Ally MCU version and warn")Fixes: b3b1c68fb726 ("HID: rapoo: Add support for side buttons on RAPOO 0x2015 mouse")
Cc: stable@vger.kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
---
drivers/hid/hid-rapoo.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-rapoo.c b/drivers/hid/hid-rapoo.c
index 4c81f3086de4..5c9c396fabf7 100644
--- a/drivers/hid/hid-rapoo.c
+++ b/drivers/hid/hid-rapoo.c
@@ -36,7 +36,7 @@ static int rapoo_probe(struct hid_device *hdev, const struct hid_device_id *id)
return ret;
}
- if (hdev->bus == BUS_USB) {
+ if (hid_is_usb(hdev)) {
struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
if (intf->cur_altsetting->desc.bInterfaceNumber != 1)
--
2.55.0.rc0.799.gd6f94ed593-goog
^ 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