| Message ID | 20250213133854.100866-3-stuart.a.hayhurst@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [v3] HID: corsair-void: Update power supply values with a unified work handler | expand |
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,
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.
> 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
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.
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);
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(-)