diff --git a/CHANGELOG.md b/CHANGELOG.md index f7b7944..0cc104a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,22 @@ the contract is "it works on RS50 and G Pro as listed here". ## Unreleased +**A kernel panic on the G923 Xbox edition during long AC and AC EVO +sessions, fixed.** The driver's answer matcher took a held send mutex to +mean a command was waiting for its reply, and matched every incoming HID++ +report against the last reply buffer, which lives on the stack of whoever +sent the last command. Four senders in this driver (the OLED frame worker +and its handback, the rev-light level sender and the G923 rev-light +worker, which sleeps while it holds the mutex) hold that mutex without a +reply buffer of their own, so a report arriving during their hold was +compared against a stack frame that was long gone; once the process that +owned it had exited (session restarts spawn short-lived helpers that write +sysfs) the read faulted inside the USB interrupt and the kernel panicked, +leaving nothing in the journal. Caught on screen by a reporter after weeks +of hunting ([#128](../../issues/128), [#90](../../issues/90)). The reply +pointer is now cleared before the mutex is released, and the matcher only +runs with a live one. + **DiRT Rally 2.0's setup text carries the `device_defines.xml` line itself.** It pointed to the wiki for the exact line, and that wiki page had been emptied by a bad edit (restored), so an RS50 owner used the wheel's diff --git a/mainline/hid-logitech-hidpp.c b/mainline/hid-logitech-hidpp.c index fdd7bc7..0b5c8d9 100644 --- a/mainline/hid-logitech-hidpp.c +++ b/mainline/hid-logitech-hidpp.c @@ -705,7 +705,7 @@ static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp, __must_hold(&hidpp->send_mutex); - hidpp->send_receive_buf = response; + WRITE_ONCE(hidpp->send_receive_buf, response); hidpp->answer_available = false; /* @@ -879,6 +879,16 @@ static int hidpp_send_message_sync_timeout(struct hidpp_device *hidpp, } while (--max_retries); hidpp_note_sync_result(hidpp, ret); + /* + * The response lives on this caller's stack. Drop the pointer + * before the mutex goes, or the next holder that is not a sync + * sender (the OLED and rev-light workers lock it to serialise + * their writes) leaves the raw-event path matching incoming + * reports against a frame that is gone, and once that stack is + * unmapped the match faults in interrupt context and the kernel + * panics (#128, the freeze #90 chased). + */ + WRITE_ONCE(hidpp->send_receive_buf, NULL); mutex_unlock(&hidpp->send_mutex); return ret; @@ -18487,17 +18497,19 @@ static int hidpp_input_configured(struct hid_device *hdev, static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data, int size) { - struct hidpp_report *question = hidpp->send_receive_buf; - struct hidpp_report *answer = hidpp->send_receive_buf; + struct hidpp_report *question = READ_ONCE(hidpp->send_receive_buf); + struct hidpp_report *answer = question; struct hidpp_report *report = (struct hidpp_report *)data; int ret; int last_online; /* - * If the mutex is locked then we have a pending answer from a - * previously sent command. + * A pending answer needs a sync sender waiting on it, and the + * mutex alone does not say there is one: the OLED and rev-light + * workers hold it too, with no buffer to answer into. Only a + * live buffer is a question (#128). */ - if (unlikely(mutex_is_locked(&hidpp->send_mutex))) { + if (unlikely(question && mutex_is_locked(&hidpp->send_mutex))) { /* * Check for a correct hidpp20 answer or the corresponding * error