HID: i2c-hid: ensure various commands do not interfere with each other

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

Posted by Dmitry Torokhov 2 months, 3 weeks ago

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.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 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);
 
-- 
2.46.0.598.g6f2099f65c-goog


-- 
Dmitry

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

Posted by Jiri Kosina 2 months, 2 weeks ago

On Mon, 9 Sep 2024, 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.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks for the fix, Dmitry. Out of curiosity, did you find it by code 
inspection, or have you actually seen it happening for real, making the 
driver misbehave?

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

Posted by Dmitry Torokhov 2 months, 2 weeks ago

Hi Jiri,

On Wed, Sep 11, 2024 at 03:27:32PM +0200, Jiri Kosina wrote:
> On Mon, 9 Sep 2024, 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.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> Thanks for the fix, Dmitry. Out of curiosity, did you find it by code 
> inspection, or have you actually seen it happening for real, making the 
> driver misbehave?

No, I have not observed this issue in the wild, that is why I di dnot
tag it explicitly for stable. It came about when I was reviewing Goodix
HID SPI driver, noticed that it was using a shared buffer, asked to and
locking, and realized that I2C HID needed the same. And just got around
to sending out the fix...

As far as I can see USB HID driver does not need it - it does not share
URBs but rather allocates new one for each request (via
usb_control_msg()).

Thanks.

-- 
Dmitry

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

Posted by Jiri Kosina 2 months, 2 weeks ago

On Wed, 11 Sep 2024, 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.
> > > 
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > 
> > Thanks for the fix, Dmitry. Out of curiosity, did you find it by code 
> > inspection, or have you actually seen it happening for real, making the 
> > driver misbehave?
> 
> No, I have not observed this issue in the wild, that is why I di dnot
> tag it explicitly for stable. 

Thanks. I was asking whether I should rush it in still for 6.11, or 
whether waiting for 6.12 merge window is sufficient.

So I will send it to Linus for 6.12, but I still think tagging for stable 
should probably be done.

> It came about when I was reviewing Goodix HID SPI driver, noticed that 
> it was using a shared buffer, asked to and locking, and realized that 
> I2C HID needed the same. And just got around to sending out the fix...
> 
> As far as I can see USB HID driver does not need it - it does not share
> URBs but rather allocates new one for each request (via
> usb_control_msg()).

Indeed, USB HID is fine in that respect.

Thanks a lot,

-- 
Jiri Kosina
SUSE Labs

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

Posted by Jiri Kosina 2 months, 2 weeks ago

On Wed, 11 Sep 2024, Jiri Kosina wrote:

> Thanks. I was asking whether I should rush it in still for 6.11, or 
> whether waiting for 6.12 merge window is sufficient.

Anyway, now applied, thanks again.

-- 
Jiri Kosina
SUSE Labs

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

Posted by Hans de Goede 2 months, 2 weeks ago

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);
>