Re: [PATCH] HID: i2c-hid: ensure various commands do not interfere with each other

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 9/9/24 10:37 PM, Dmitry Torokhov wrote:
> i2c-hid uses 2 shared buffers: command and "raw" input buffer for
> sending requests to peripherals and read data from peripherals when
> executing variety of commands. Such commands include reading of HID
> registers, requesting particular power mode, getting and setting
> reports and so on. Because all such requests use the same 2 buffers
> they should not execute simultaneously.
> 
> Fix this by introducing "cmd_lock" mutex and acquire it whenever
> we needs to access ihid->cmdbuf or idid->rawbuf.

Typo: s/idid/ihid/

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 42 +++++++++++++++++++-----------
>  1 file changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 632eaf9e11a6..2f8a9d3f1e86 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -105,6 +105,7 @@ struct i2c_hid {
>  
>  	wait_queue_head_t	wait;		/* For waiting the interrupt */
>  
> +	struct mutex		cmd_lock;	/* protects cmdbuf and rawbuf */
>  	struct mutex		reset_lock;
>  
>  	struct i2chid_ops	*ops;
> @@ -220,6 +221,8 @@ static int i2c_hid_xfer(struct i2c_hid *ihid,
>  static int i2c_hid_read_register(struct i2c_hid *ihid, __le16 reg,
>  				 void *buf, size_t len)
>  {
> +	guard(mutex)(&ihid->cmd_lock);
> +
>  	*(__le16 *)ihid->cmdbuf = reg;
>  
>  	return i2c_hid_xfer(ihid, ihid->cmdbuf, sizeof(__le16), buf, len);
> @@ -252,6 +255,8 @@ static int i2c_hid_get_report(struct i2c_hid *ihid,
>  
>  	i2c_hid_dbg(ihid, "%s\n", __func__);
>  
> +	guard(mutex)(&ihid->cmd_lock);
> +
>  	/* Command register goes first */
>  	*(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
>  	length += sizeof(__le16);
> @@ -342,6 +347,8 @@ static int i2c_hid_set_or_send_report(struct i2c_hid *ihid,
>  	if (!do_set && le16_to_cpu(ihid->hdesc.wMaxOutputLength) == 0)
>  		return -ENOSYS;
>  
> +	guard(mutex)(&ihid->cmd_lock);
> +
>  	if (do_set) {
>  		/* Command register goes first */
>  		*(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
> @@ -384,6 +391,8 @@ static int i2c_hid_set_power_command(struct i2c_hid *ihid, int power_state)
>  {
>  	size_t length;
>  
> +	guard(mutex)(&ihid->cmd_lock);
> +
>  	/* SET_POWER uses command register */
>  	*(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
>  	length = sizeof(__le16);
> @@ -440,25 +449,27 @@ static int i2c_hid_start_hwreset(struct i2c_hid *ihid)
>  	if (ret)
>  		return ret;
>  
> -	/* Prepare reset command. Command register goes first. */
> -	*(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
> -	length += sizeof(__le16);
> -	/* Next is RESET command itself */
> -	length += i2c_hid_encode_command(ihid->cmdbuf + length,
> -					 I2C_HID_OPCODE_RESET, 0, 0);
> +	scoped_guard(mutex, &ihid->cmd_lock) {
> +		/* Prepare reset command. Command register goes first. */
> +		*(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
> +		length += sizeof(__le16);
> +		/* Next is RESET command itself */
> +		length += i2c_hid_encode_command(ihid->cmdbuf + length,
> +						 I2C_HID_OPCODE_RESET, 0, 0);
>  
> -	set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
> +		set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
>  
> -	ret = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0);
> -	if (ret) {
> -		dev_err(&ihid->client->dev,
> -			"failed to reset device: %d\n", ret);
> -		goto err_clear_reset;
> -	}
> +		ret = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0);
> +		if (ret) {
> +			dev_err(&ihid->client->dev,
> +				"failed to reset device: %d\n", ret);
> +			break;
> +		}
>  
> -	return 0;
> +		return 0;
> +	}
>  
> -err_clear_reset:
> +	/* Clean up if sending reset command failed */
>  	clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
>  	i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
>  	return ret;
> @@ -1200,6 +1211,7 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
>  	ihid->is_panel_follower = drm_is_panel_follower(&client->dev);
>  
>  	init_waitqueue_head(&ihid->wait);
> +	mutex_init(&ihid->cmd_lock);
>  	mutex_init(&ihid->reset_lock);
>  	INIT_WORK(&ihid->panel_follower_prepare_work, ihid_core_panel_prepare_work);
>