Re: [PATCH v5] HID: corsair-void: Add Corsair Void headset family driver

[Jiri Slaby <jirislaby@kernel.org>]

On 09. 10. 24, 1:30, Stuart Hayhurst wrote:
> Introduce a driver for the Corsair Void family of headsets, supporting:
>   - Battery reporting (power_supply)
>   - Sidetone setting support
>   - Physical microphone location reporting
>   - Headset and receiver firmware version reporting
>   - Built-in alert triggering
>   - USB wireless_status
> 
> Tested with a Void Pro Wireless, Void Elite Wireless and a Void Elite Wired
...
> --- /dev/null
> +++ b/drivers/hid/hid-corsair-void.c
> @@ -0,0 +1,829 @@
...
> +static void corsair_void_process_receiver(struct corsair_void_drvdata *drvdata,
> +					  int raw_battery_capacity,
> +					  int raw_connection_status,
> +					  int raw_battery_status)
> +{
> +	struct corsair_void_battery_data *battery_data = &drvdata->battery_data;
> +	struct corsair_void_battery_data orig_battery_data;
> +
> +	/* Save initial battery data, to compare later */
> +	orig_battery_data = *battery_data;
> +
> +	/* Headset not connected, or it's wired */
> +	if (raw_connection_status != CORSAIR_VOID_WIRELESS_CONNECTED)
> +		goto unknown_battery;
> +
> +	/* Battery information unavailable */
> +	if (raw_battery_status == 0)
> +		goto unknown_battery;
> +
> +	/* Battery must be connected then */
> +	battery_data->present = true;
> +	battery_data->capacity_level = POWER_SUPPLY_CAPACITY_LEVEL_NORMAL;
> +
> +	/* Set battery status */
> +	switch (raw_battery_status) {
> +	case CORSAIR_VOID_BATTERY_NORMAL:
> +	case CORSAIR_VOID_BATTERY_LOW:
> +	case CORSAIR_VOID_BATTERY_CRITICAL:
> +		battery_data->status = POWER_SUPPLY_STATUS_DISCHARGING;
> +		if (raw_battery_status == CORSAIR_VOID_BATTERY_LOW)
> +			battery_data->capacity_level = POWER_SUPPLY_CAPACITY_LEVEL_LOW;
> +		else if (raw_battery_status == CORSAIR_VOID_BATTERY_CRITICAL)
> +			battery_data->capacity_level = POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL;
> +
> +		break;
> +	case CORSAIR_VOID_BATTERY_CHARGED:
> +		battery_data->status = POWER_SUPPLY_STATUS_FULL;
> +		break;
> +	case CORSAIR_VOID_BATTERY_CHARGING:
> +		battery_data->status = POWER_SUPPLY_STATUS_CHARGING;
> +		break;
> +	default:
> +		hid_warn(drvdata->hid_dev, "unknown battery status '%d'",
> +			 raw_battery_status);
> +		goto unknown_battery;
> +		break;
> +	}
> +
> +	battery_data->capacity = raw_battery_capacity;
> +	corsair_void_set_wireless_status(drvdata);
> +
> +	goto success;
> +unknown_battery:
> +	corsair_void_set_unknown_batt(drvdata);
> +success:
> +
> +	/* Inform power supply if battery values changed */
> +	if (memcmp(&orig_battery_data, battery_data, sizeof(*battery_data))) {
> +		scoped_guard(mutex, &drvdata->battery_mutex) {

This effectively kills the system. We came here via:
corsair_void_raw_event (hid_driver::raw_event)
   -> corsair_void_process_receiver
     -> scoped_guard(mutex, &drvdata->battery_mutex)

And hid_driver::raw_event can be called from the interrupt context. This 
happened at:
https://bugzilla.suse.com/show_bug.cgi?id=1236843

In particular, from BH (USB URB BH: usb_giveback_urb_bh()), see the 
backtrace at:
https://bugzilla.suse.com/attachment.cgi?id=880175

Perhaps you need a separate _spin_ lock for drvdata->battery and use it 
here for getting and on "= NULL" and "=new_supply" for setting? Or 
schedule another work for power_supply_changed()? Or perhaps you unify 
the add/remove/update work into one?

> +			if (drvdata->battery) {
> +				power_supply_changed(drvdata->battery);
> +			}

...

thanks,
-- 
js
suse labs