| Message ID | Zt9clAu04BinzIcd@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | HID: i2c-hid: ensure various commands do not interfere with each other | expand |
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); >
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,
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.
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,
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.
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);
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(-)