[PATCH v2] HID: corsair-void: Update power supply values with a unified work handler

[PATCH v2] HID: corsair-void: Update power supply values with a unified work handler

[Stuart Hayhurst]

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>
---

v1 -> v2:
 - Actually remove the mutex

---
 drivers/hid/hid-corsair-void.c | 87 ++++++++++++++++++----------------
 1 file changed, 47 insertions(+), 40 deletions(-)

diff --git a/drivers/hid/hid-corsair-void.c b/drivers/hid/hid-corsair-void.c
index 56e858066c3c..2f70db8a2370 100644
--- 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>
@@ -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)
+
 enum {
 	CORSAIR_VOID_WIRELESS,
 	CORSAIR_VOID_WIRED,
@@ -155,12 +157,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 +262,9 @@ static void corsair_void_process_receiver(struct corsair_void_drvdata *drvdata,
 
 	/* 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 +536,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 +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 */
+	if (add_battery && !remove_battery) {
+		corsair_void_add_battery(drvdata);
+	} else if (remove_battery && !add_battery) {
+		if (drvdata->battery) {
+			power_supply_unregister(drvdata->battery);
+			drvdata->battery = NULL;
+		}
+	}
+
+	/* Communicate that battery values changed */
+	if (update_battery) {
+		if (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 +692,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 +729,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);
 
-- 
2.47.2

Re: [PATCH v2] HID: corsair-void: Update power supply values with a unified work handler

[Jiri Slaby]

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@vger.kernel.org
> Signed-off-by: Stuart Hayhurst <stuart.a.hayhurst@gmail.com>
> ---
> --- 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

Re: [PATCH v2] HID: corsair-void: Update power supply values with a unified work handler

[Stuart]

> No \n ^^ here.

Thanks, corrected that

> I would do an enum, but it's a matter of taste/preference.

I suppose that makes more sense since they're related, applied

> What is to skip a battery? Anyway, the comments here seem to be
> superfluous as the code is obvious™.

It was supposed to indicate doing nothing if there was a request to remove a
battery and another to add the battery, what would you suggest?

> Perhaps '&& drvdata->battery' instead of the nested 'if'?

Sure, what about corsair_void_add_battery()? It's got an equivalent
condition inside it, should I leave it there or move it to the
'add_battery && !remove_battery' for consistency?

Thanks for the review,

Stuart