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

[PATCH v3] 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>
---

v2 -> v3:
 - Use an enum instead of a define for battery flag values
 - Use an integer instead of BIT() for the bit index
 - Drop unhelpful comments
 - Simplify corsair_void_battery_work_handler logic
 - Remove extra newline in commit message
v1 -> v2:
 - Actually remove the mutex

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

diff --git a/drivers/hid/hid-corsair-void.c b/drivers/hid/hid-corsair-void.c
index 56e858066c3c..afbd67aa9719 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>
@@ -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 @@ 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 +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);
 
-- 
2.47.2

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

[Jiri Slaby]

On 13. 02. 25, 14:38, 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
> 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>

> ---
> 
> v2 -> v3:
>   - Use an enum instead of a define for battery flag values
>   - Use an integer instead of BIT() for the bit index

Good catch :).

>   - Drop unhelpful comments
>   - Simplify corsair_void_battery_work_handler logic
>   - Remove extra newline in commit message
> v1 -> v2:
>   - Actually remove the mutex
> 
> ---
>   drivers/hid/hid-corsair-void.c | 83 ++++++++++++++++++----------------
>   1 file changed, 43 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/hid/hid-corsair-void.c b/drivers/hid/hid-corsair-void.c
> index 56e858066c3c..afbd67aa9719 100644
> --- a/drivers/hid/hid-corsair-void.c
> +++ b/drivers/hid/hid-corsair-void.c
...
> @@ -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;
> +	}

Now I think, what is actually expected to happen if both add_battery and 
remove_battery is set? Do nothing as the code does?

> +	if (update_battery && drvdata->battery)
> +		power_supply_changed(drvdata->battery);
> +
> +}

thanks,
-- 
js
suse labs

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

[Jiri Slaby]

On 13. 02. 25, 14:38, 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
> 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>
> ---
> 
> v2 -> v3:
>   - Use an enum instead of a define for battery flag values
>   - Use an integer instead of BIT() for the bit index
>   - Drop unhelpful comments
>   - Simplify corsair_void_battery_work_handler logic
>   - Remove extra newline in commit message
> v1 -> v2:
>   - Actually remove the mutex
> 
> ---
>   drivers/hid/hid-corsair-void.c | 83 ++++++++++++++++++----------------
>   1 file changed, 43 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/hid/hid-corsair-void.c b/drivers/hid/hid-corsair-void.c
> index 56e858066c3c..afbd67aa9719 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>
> @@ -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,

BTW numbering these explicitly is superfluous.

-- 
js
suse labs

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

[Stuart]

> Now I think, what is actually expected to happen if both add_battery and
> remove_battery is set? Do nothing as the code does?

It means that either the headset connected and then disconnected again, or
it disconnected and reconnected again. Either way, the battery should be left
in its current state.

Of course it could connect, disconnect and connect again to end up in
that state,
but if the driver is 3 events (a physical action) behind, we're already done for

Stuart

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

[Jiri Kosina]

On Thu, 13 Feb 2025, 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
> 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>

Thanks a lot to both you and Jiri for working on having this fixed 
properly. Now applied.

-- 
Jiri Kosina
SUSE Labs