HID: corsair-void: Update power supply values with a unified work handler

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
Fixes: 6ea2a6fd3872 ("HID: corsair-void: Add Corsair Void headset family driver")
Cc: stable@vger.kernel.org
Signed-off-by: Stuart Hayhurst <stuart.a.hayhurst@gmail.com>
Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
Signed-off-by: Jiri Kosina <jkosina@suse.com>

diff --git a/drivers/hid/hid-corsair-void.c b/drivers/hid/hid-corsair-void.c
index 56e858066c3c3113c5c61c35dac4380481a0cbf4..afbd67aa9719200afb91de115336fb75e979a16c 100644 (file)
--- a/drivers/hid/hid-corsair-void.c
+++ b/drivers/hid/hid-corsair-void.c
@@ -71,11 +71,9 @@
 
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
-#include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/hid.h>
 #include <linux/module.h>
-#include <linux/mutex.h>
 #include <linux/power_supply.h>
 #include <linux/usb.h>
 #include <linux/workqueue.h>
@@ -120,6 +118,12 @@ enum {
        CORSAIR_VOID_BATTERY_CHARGING   = 5,
 };
 
+enum {
+       CORSAIR_VOID_ADD_BATTERY        = 0,
+       CORSAIR_VOID_REMOVE_BATTERY     = 1,
+       CORSAIR_VOID_UPDATE_BATTERY     = 2,
+};
+
 static enum power_supply_property corsair_void_battery_props[] = {
        POWER_SUPPLY_PROP_STATUS,
        POWER_SUPPLY_PROP_PRESENT,
@@ -155,12 +159,12 @@ struct corsair_void_drvdata {
 
        struct power_supply *battery;
        struct power_supply_desc battery_desc;
-       struct mutex battery_mutex;
 
        struct delayed_work delayed_status_work;
        struct delayed_work delayed_firmware_work;
-       struct work_struct battery_remove_work;
-       struct work_struct battery_add_work;
+
+       unsigned long battery_work_flags;
+       struct work_struct battery_work;
 };
 
 /*
@@ -260,11 +264,9 @@ success:
 
        /* Inform power supply if battery values changed */
        if (memcmp(&orig_battery_data, battery_data, sizeof(*battery_data))) {
-               scoped_guard(mutex, &drvdata->battery_mutex) {
-                       if (drvdata->battery) {
-                               power_supply_changed(drvdata->battery);
-                       }
-               }
+               set_bit(CORSAIR_VOID_UPDATE_BATTERY,
+                       &drvdata->battery_work_flags);
+               schedule_work(&drvdata->battery_work);
        }
 }
 
@@ -536,29 +538,11 @@ static void corsair_void_firmware_work_handler(struct work_struct *work)
 
 }
 
-static void corsair_void_battery_remove_work_handler(struct work_struct *work)
-{
-       struct corsair_void_drvdata *drvdata;
-
-       drvdata = container_of(work, struct corsair_void_drvdata,
-                              battery_remove_work);
-       scoped_guard(mutex, &drvdata->battery_mutex) {
-               if (drvdata->battery) {
-                       power_supply_unregister(drvdata->battery);
-                       drvdata->battery = NULL;
-               }
-       }
-}
-
-static void corsair_void_battery_add_work_handler(struct work_struct *work)
+static void corsair_void_add_battery(struct corsair_void_drvdata *drvdata)
 {
-       struct corsair_void_drvdata *drvdata;
        struct power_supply_config psy_cfg = {};
        struct power_supply *new_supply;
 
-       drvdata = container_of(work, struct corsair_void_drvdata,
-                              battery_add_work);
-       guard(mutex)(&drvdata->battery_mutex);
        if (drvdata->battery)
                return;
 
@@ -583,16 +567,42 @@ 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);
+
+       if (add_battery && !remove_battery) {
+               corsair_void_add_battery(drvdata);
+       } else if (remove_battery && !add_battery && drvdata->battery) {
+               power_supply_unregister(drvdata->battery);
+               drvdata->battery = NULL;
+       }
+
+       if (update_battery && drvdata->battery)
+               power_supply_changed(drvdata->battery);
+
+}
+
 static void corsair_void_headset_connected(struct corsair_void_drvdata *drvdata)
 {
-       schedule_work(&drvdata->battery_add_work);
+       set_bit(CORSAIR_VOID_ADD_BATTERY, &drvdata->battery_work_flags);
+       schedule_work(&drvdata->battery_work);
        schedule_delayed_work(&drvdata->delayed_firmware_work,
                              msecs_to_jiffies(100));
 }
 
 static void corsair_void_headset_disconnected(struct corsair_void_drvdata *drvdata)
 {
-       schedule_work(&drvdata->battery_remove_work);
+       set_bit(CORSAIR_VOID_REMOVE_BATTERY, &drvdata->battery_work_flags);
+       schedule_work(&drvdata->battery_work);
 
        corsair_void_set_unknown_wireless_data(drvdata);
        corsair_void_set_unknown_batt(drvdata);
@@ -678,13 +688,7 @@ static int corsair_void_probe(struct hid_device *hid_dev,
        drvdata->battery_desc.get_property = corsair_void_battery_get_property;
 
        drvdata->battery = NULL;
-       INIT_WORK(&drvdata->battery_remove_work,
-                 corsair_void_battery_remove_work_handler);
-       INIT_WORK(&drvdata->battery_add_work,
-                 corsair_void_battery_add_work_handler);
-       ret = devm_mutex_init(drvdata->dev, &drvdata->battery_mutex);
-       if (ret)
-               return ret;
+       INIT_WORK(&drvdata->battery_work, corsair_void_battery_work_handler);
 
        ret = sysfs_create_group(&hid_dev->dev.kobj, &corsair_void_attr_group);
        if (ret)
@@ -721,8 +725,7 @@ static void corsair_void_remove(struct hid_device *hid_dev)
        struct corsair_void_drvdata *drvdata = hid_get_drvdata(hid_dev);
 
        hid_hw_stop(hid_dev);
-       cancel_work_sync(&drvdata->battery_remove_work);
-       cancel_work_sync(&drvdata->battery_add_work);
+       cancel_work_sync(&drvdata->battery_work);
        if (drvdata->battery)
                power_supply_unregister(drvdata->battery);