linux-kernel - Re: [PATCH] HID: i2c-hid: Add API to wait for device reset completion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <dc85cf29-7b89-4eb3-99a0-b66572ceca9c@kernel.org>
Date: Fri, 14 Nov 2025 10:13:04 -0600
From: "Mario Limonciello (AMD) (kernel.org)" <superm1@...nel.org>
To: LI Qingwu <Qing-wu.Li@...ca-geosystems.com.cn>, jikos@...nel.org,
 bentiss@...nel.org, dianders@...omium.org, treapking@...omium.org,
 alex.vinarskis@...il.com, dan.carpenter@...aro.org,
 guanwentao@...ontech.com, kl@...wtf, linux-input@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] HID: i2c-hid: Add API to wait for device reset completion



On 11/14/2025 5:24 AM, LI Qingwu wrote:
> Some HID over I2C devices need to signal reset completion to the host
> after firmware updates or device resets. Per the HID over I2C spec,
> devices signal completion by sending an empty input report (0x0000).
> 
> Add i2c_hid_wait_reset_complete() to allow drivers to synchronize
> with device reset operations. The function sets I2C_HID_RESET_PENDING
> and waits for the device's completion signal.
> 
> Returns: 0 on success, -ETIMEDOUT on timeout, -ENODEV if invalid device.
> 
> Upstream-Status: Pending

This tag is probably useful for your downstream tree, but it's not 
useful upstream.  You should strip it when submitting upstream.

> Signed-off-by: LI Qingwu <Qing-wu.Li@...ca-geosystems.com.cn>
> ---
>   drivers/hid/i2c-hid/i2c-hid-core.c | 24 ++++++++++++++++++++++++
>   drivers/hid/i2c-hid/i2c-hid.h      |  1 +
>   2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index d3912e3f2f13a..4feab2327e92d 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -1365,6 +1365,30 @@ const struct dev_pm_ops i2c_hid_core_pm = {
>   };
>   EXPORT_SYMBOL_GPL(i2c_hid_core_pm);
>   
> +int i2c_hid_wait_reset_complete(struct device *dev, unsigned long timeout_ms)
> +{
> +	struct i2c_client *client;
> +	struct i2c_hid *ihid;
> +	long ret;
> +
> +	if (!dev)
> +		return -ENODEV;
> +
> +	client = to_i2c_client(dev);

Check if client is NULL?

> +	ihid = i2c_get_clientdata(client);
> +	if (!ihid)
> +		return -ENODEV;
> +	set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
> +	ret = wait_event_timeout(ihid->wait,
> +				 !test_bit(I2C_HID_RESET_PENDING, &ihid->flags),
> +				 msecs_to_jiffies(timeout_ms));
> +	if (ret == 0) {

Shouldn't need curly braces for a one line statement.

> +		clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
> +	}
> +	return ret ? 0 : -ETIMEDOUT;

Why not just:

return ret;

> +}
> +EXPORT_SYMBOL_GPL(i2c_hid_wait_reset_complete);

Can you please include a second patch in your series demonstrating the 
usage of this in a driver?

> +
>   MODULE_DESCRIPTION("HID over I2C core driver");
>   MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@...il.com>");
>   MODULE_LICENSE("GPL");
> diff --git a/drivers/hid/i2c-hid/i2c-hid.h b/drivers/hid/i2c-hid/i2c-hid.h
> index 2c7b66d5caa0f..1c6d959716858 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.h
> +++ b/drivers/hid/i2c-hid/i2c-hid.h
> @@ -40,6 +40,7 @@ void i2c_hid_core_remove(struct i2c_client *client);
>   
>   void i2c_hid_core_shutdown(struct i2c_client *client);
>   
> +int i2c_hid_wait_reset_complete(struct device *dev, unsigned long timeout_ms);
>   extern const struct dev_pm_ops i2c_hid_core_pm;
>   
>   #endif