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@xxxxxxxxx>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
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);
>
