| Message ID | 20240318105902.34789-1-namcao@linutronix.de (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: i2c-hid: remove I2C_HID_READ_PENDING flag to prevent lock-up | expand |
On Mon, 18 Mar 2024, Nam Cao wrote: > The flag I2C_HID_READ_PENDING is used to serialize I2C operations. > However, this is not necessary, because I2C core already has its own > locking for that. > > More importantly, this flag can cause a lock-up: if the flag is set in > i2c_hid_xfer() and an interrupt happens, the interrupt handler > (i2c_hid_irq) will check this flag and return immediately without doing > anything, then the interrupt handler will be invoked again in an > infinite loop. > > Since interrupt handler is an RT task, it takes over the CPU and the > flag-clearing task never gets scheduled, thus we have a lock-up. > > Delete this unnecessary flag. Hmm, right, good catch, I can't figure out what extra semantic I2C_HID_READ_PENDING would be adding (rather than deadlock :) ). Why RT throttling didn't happen and let the system in a completely locked up state is something I don't understand, but that's separate. I have now queued this in hid.git#for-6.9/upstream-fixes Thanks,
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 2df1ab3c31cc..1c86c97688e9 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -64,7 +64,6 @@ /* flags */ #define I2C_HID_STARTED 0 #define I2C_HID_RESET_PENDING 1 -#define I2C_HID_READ_PENDING 2 #define I2C_HID_PWR_ON 0x00 #define I2C_HID_PWR_SLEEP 0x01 @@ -190,15 +189,10 @@ static int i2c_hid_xfer(struct i2c_hid *ihid, msgs[n].len = recv_len; msgs[n].buf = recv_buf; n++; - - set_bit(I2C_HID_READ_PENDING, &ihid->flags); } ret = i2c_transfer(client->adapter, msgs, n); - if (recv_len) - clear_bit(I2C_HID_READ_PENDING, &ihid->flags); - if (ret != n) return ret < 0 ? ret : -EIO; @@ -556,9 +550,6 @@ static irqreturn_t i2c_hid_irq(int irq, void *dev_id) { struct i2c_hid *ihid = dev_id; - if (test_bit(I2C_HID_READ_PENDING, &ihid->flags)) - return IRQ_HANDLED; - i2c_hid_get_input(ihid); return IRQ_HANDLED;
The flag I2C_HID_READ_PENDING is used to serialize I2C operations. However, this is not necessary, because I2C core already has its own locking for that. More importantly, this flag can cause a lock-up: if the flag is set in i2c_hid_xfer() and an interrupt happens, the interrupt handler (i2c_hid_irq) will check this flag and return immediately without doing anything, then the interrupt handler will be invoked again in an infinite loop. Since interrupt handler is an RT task, it takes over the CPU and the flag-clearing task never gets scheduled, thus we have a lock-up. Delete this unnecessary flag. Reported-and-tested-by: Eva Kurchatova <nyandarknessgirl@gmail.com> Closes: https://lore.kernel.org/r/CA+eeCSPUDpUg76ZO8dszSbAGn+UHjcyv8F1J-CUPVARAzEtW9w@mail.gmail.com Fixes: 4a200c3b9a40 ("HID: i2c-hid: introduce HID over i2c specification implementation") Cc: <stable@vger.kernel.org> Signed-off-by: Nam Cao <namcao@linutronix.de> --- drivers/hid/i2c-hid/i2c-hid-core.c | 9 --------- 1 file changed, 9 deletions(-)