On 11. 02. 25, 23:46, Stuart Hayhurst wrote:
corsair_void_process_receiver can be called from an interrupt context,
locking battery_mutex in it was causing a kernel panic.
Fix it by moving the critical section into its own work, sharing this
work with battery_add_work and battery_remove_work to remove the need
for any locking
Closes: https://bugzilla.suse.com/show_bug.cgi?id=1236843
No \n ^^ here.
Fixes: 6ea2a6fd3872 ("HID: corsair-void: Add Corsair Void headset family driver")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Stuart Hayhurst <stuart.a.hayhurst@xxxxxxxxx>
---
--- a/drivers/hid/hid-corsair-void.c
+++ b/drivers/hid/hid-corsair-void.c
...
@@ -107,6 +105,10 @@
#define CORSAIR_VOID_SIDETONE_MAX_WIRELESS 55
#define CORSAIR_VOID_SIDETONE_MAX_WIRED 4096
+#define CORSAIR_VOID_ADD_BATTERY BIT(0)
+#define CORSAIR_VOID_REMOVE_BATTERY BIT(1)
+#define CORSAIR_VOID_UPDATE_BATTERY BIT(2)
I would do an enum, but it's a matter of taste/preference.
@@ -583,16 +565,48 @@ static void corsair_void_battery_add_work_handler(struct work_struct *work)
drvdata->battery = new_supply;
}
+static void corsair_void_battery_work_handler(struct work_struct *work)
+{
+ struct corsair_void_drvdata *drvdata = container_of(work,
+ struct corsair_void_drvdata, battery_work);
+
+ bool add_battery = test_and_clear_bit(CORSAIR_VOID_ADD_BATTERY,
+ &drvdata->battery_work_flags);
+ bool remove_battery = test_and_clear_bit(CORSAIR_VOID_REMOVE_BATTERY,
+ &drvdata->battery_work_flags);
+ bool update_battery = test_and_clear_bit(CORSAIR_VOID_UPDATE_BATTERY,
+ &drvdata->battery_work_flags);
+
+ /* Add, remove or skip battery */
What is to skip a battery? Anyway, the comments here seem to be
superfluous as the code is obvious™.
+ if (add_battery && !remove_battery) {
+ corsair_void_add_battery(drvdata);
+ } else if (remove_battery && !add_battery) {
+ if (drvdata->battery) {
Perhaps '&& drvdata->battery' instead of the nested 'if'?
+ power_supply_unregister(drvdata->battery);
+ drvdata->battery = NULL;
+ }
+ }
+
+ /* Communicate that battery values changed */
+ if (update_battery) {
+ if (drvdata->battery)
Ditto.
+ power_supply_changed(drvdata->battery);
+ }
+
+}
Overall, LGTM.
thanks,
--
js
suse labs
