[PATCH v3 1/6] HID: logitech-hidpp: Simplify array length check

[PATCH v3 1/6] HID: logitech-hidpp: Simplify array length check

[Bastien Nocera]

Use the compiler to force a 100-length array, rather than check the
length after the fact.

Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
New in v2, following a review comment in the 1f20 enablement patch.
No changes in v3.

 drivers/hid/hid-logitech-hidpp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index ff1fcebf2ec7..f55b2233dbea 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1356,7 +1356,7 @@ static int hidpp20_map_battery_capacity(struct hid_device *hid_dev, int voltage)
 	 * there are a few devices that use different battery technology.
 	 */
 
-	static const int voltages[] = {
+	static const int voltages[100] = {
 		4186, 4156, 4143, 4133, 4122, 4113, 4103, 4094, 4086, 4075,
 		4067, 4059, 4051, 4043, 4035, 4027, 4019, 4011, 4003, 3997,
 		3989, 3983, 3976, 3969, 3961, 3955, 3949, 3942, 3935, 3929,
@@ -1371,8 +1371,6 @@ static int hidpp20_map_battery_capacity(struct hid_device *hid_dev, int voltage)
 
 	int i;
 
-	BUILD_BUG_ON(ARRAY_SIZE(voltages) != 100);
-
 	if (unlikely(voltage < 3500 || voltage >= 5000))
 		hid_warn_once(hid_dev,
 			      "%s: possibly using the wrong voltage curve\n",
-- 
2.39.2

[PATCH v3 2/6] HID: logitech-hidpp: Add support for ADC measurement feature

[Bastien Nocera]

This is used in a number of Logitech headsets to report the voltage
of the battery.

Tested on a Logitech G935.

Signed-off-by: Bastien Nocera <hadess@hadess.net>
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=216483
---
Fixed array length checking in v2.
No changes in v3

 drivers/hid/hid-logitech-hidpp.c | 172 ++++++++++++++++++++++++++++++-
 1 file changed, 170 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index f55b2233dbea..5a95f01a6129 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -94,6 +94,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_CAPABILITY_HIDPP20_HI_RES_WHEEL	BIT(7)
 #define HIDPP_CAPABILITY_HIDPP20_HI_RES_SCROLL	BIT(8)
 #define HIDPP_CAPABILITY_HIDPP10_FAST_SCROLL	BIT(9)
+#define HIDPP_CAPABILITY_ADC_MEASUREMENT	BIT(10)
 
 #define lg_map_key_clear(c)  hid_map_usage_clear(hi, usage, bit, max, EV_KEY, (c))
 
@@ -145,6 +146,7 @@ struct hidpp_battery {
 	u8 feature_index;
 	u8 solar_feature_index;
 	u8 voltage_feature_index;
+	u8 adc_measurement_feature_index;
 	struct power_supply_desc desc;
 	struct power_supply *ps;
 	char name[64];
@@ -1742,6 +1744,162 @@ static int hidpp_set_wireless_feature_index(struct hidpp_device *hidpp)
 	return ret;
 }
 
+/* -------------------------------------------------------------------------- */
+/* 0x1f20: ADC measurement                                                    */
+/* -------------------------------------------------------------------------- */
+
+#define HIDPP_PAGE_ADC_MEASUREMENT 0x1f20
+
+#define CMD_ADC_MEASUREMENT_GET_ADC_MEASUREMENT 0x00
+
+#define EVENT_ADC_MEASUREMENT_STATUS_BROADCAST 0x00
+
+static int hidpp20_map_adc_measurement_1f20_capacity(struct hid_device *hid_dev, int voltage)
+{
+	/* NB: This voltage curve doesn't necessarily map perfectly to all
+	 * devices that implement the ADC_MEASUREMENT feature. This is because
+	 * there are a few devices that use different battery technology.
+	 *
+	 * Adapted from:
+	 * https://github.com/Sapd/HeadsetControl/blob/acd972be0468e039b93aae81221f20a54d2d60f7/src/devices/logitech_g633_g933_935.c#L44-L52
+	 */
+	static const int voltages[100] = {
+		4030, 4024, 4018, 4011, 4003, 3994, 3985, 3975, 3963, 3951,
+		3937, 3922, 3907, 3893, 3880, 3868, 3857, 3846, 3837, 3828,
+		3820, 3812, 3805, 3798, 3791, 3785, 3779, 3773, 3768, 3762,
+		3757, 3752, 3747, 3742, 3738, 3733, 3729, 3724, 3720, 3716,
+		3712, 3708, 3704, 3700, 3696, 3692, 3688, 3685, 3681, 3677,
+		3674, 3670, 3667, 3663, 3660, 3657, 3653, 3650, 3646, 3643,
+		3640, 3637, 3633, 3630, 3627, 3624, 3620, 3617, 3614, 3611,
+		3608, 3604, 3601, 3598, 3595, 3592, 3589, 3585, 3582, 3579,
+		3576, 3573, 3569, 3566, 3563, 3560, 3556, 3553, 3550, 3546,
+		3543, 3539, 3536, 3532, 3529, 3525, 3499, 3466, 3433, 3399,
+	};
+
+	int i;
+
+	if (voltage == 0)
+		return 0;
+
+	if (unlikely(voltage < 3400 || voltage >= 5000))
+		hid_warn_once(hid_dev,
+			      "%s: possibly using the wrong voltage curve\n",
+			      __func__);
+
+	for (i = 0; i < ARRAY_SIZE(voltages); i++) {
+		if (voltage >= voltages[i])
+			return ARRAY_SIZE(voltages) - i;
+	}
+
+	return 0;
+}
+
+static int hidpp20_map_adc_measurement_1f20(u8 data[3], int *voltage)
+{
+	int status;
+	u8 flags;
+
+	flags = data[2];
+
+	switch (flags) {
+	case 0x01:
+		status = POWER_SUPPLY_STATUS_DISCHARGING;
+		break;
+	case 0x03:
+		status = POWER_SUPPLY_STATUS_CHARGING;
+		break;
+	case 0x07:
+		status = POWER_SUPPLY_STATUS_FULL;
+		break;
+	case 0x0F:
+	default:
+		status = POWER_SUPPLY_STATUS_UNKNOWN;
+		break;
+	}
+
+	*voltage = get_unaligned_be16(data);
+
+	dbg_hid("Parsed 1f20 data as flag 0x%02x voltage %dmV\n",
+		flags, *voltage);
+
+	return status;
+}
+
+/* Return value is whether the device is online */
+static bool hidpp20_get_adc_measurement_1f20(struct hidpp_device *hidpp,
+						 u8 feature_index,
+						 int *status, int *voltage)
+{
+	struct hidpp_report response;
+	int ret;
+	u8 *params = (u8 *)response.fap.params;
+
+	*status = POWER_SUPPLY_STATUS_UNKNOWN;
+	*voltage = 0;
+	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
+					  CMD_ADC_MEASUREMENT_GET_ADC_MEASUREMENT,
+					  NULL, 0, &response);
+
+	if (ret > 0) {
+		hid_dbg(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
+			__func__, ret);
+		return false;
+	}
+
+	*status = hidpp20_map_adc_measurement_1f20(params, voltage);
+	return true;
+}
+
+static int hidpp20_query_adc_measurement_info_1f20(struct hidpp_device *hidpp)
+{
+	u8 feature_type;
+
+	if (hidpp->battery.adc_measurement_feature_index == 0xff) {
+		int ret;
+
+		ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_ADC_MEASUREMENT,
+					     &hidpp->battery.adc_measurement_feature_index,
+					     &feature_type);
+		if (ret)
+			return ret;
+
+		hidpp->capabilities |= HIDPP_CAPABILITY_ADC_MEASUREMENT;
+	}
+
+	hidpp->battery.online = hidpp20_get_adc_measurement_1f20(hidpp,
+								 hidpp->battery.adc_measurement_feature_index,
+								 &hidpp->battery.status,
+								 &hidpp->battery.voltage);
+	hidpp->battery.capacity = hidpp20_map_adc_measurement_1f20_capacity(hidpp->hid_dev,
+									    hidpp->battery.voltage);
+
+	return 0;
+}
+
+static int hidpp20_adc_measurement_event_1f20(struct hidpp_device *hidpp,
+					    u8 *data, int size)
+{
+	struct hidpp_report *report = (struct hidpp_report *)data;
+	int status, voltage;
+
+	if (report->fap.feature_index != hidpp->battery.adc_measurement_feature_index ||
+		report->fap.funcindex_clientid != EVENT_ADC_MEASUREMENT_STATUS_BROADCAST)
+		return 0;
+
+	status = hidpp20_map_adc_measurement_1f20(report->fap.params, &voltage);
+
+	hidpp->battery.online = status != POWER_SUPPLY_STATUS_UNKNOWN;
+
+	if (voltage != hidpp->battery.voltage || status != hidpp->battery.status) {
+		hidpp->battery.status = status;
+		hidpp->battery.voltage = voltage;
+		hidpp->battery.capacity = hidpp20_map_adc_measurement_1f20_capacity(hidpp->hid_dev, voltage);
+		if (hidpp->battery.ps)
+			power_supply_changed(hidpp->battery.ps);
+	}
+	return 0;
+}
+
 /* -------------------------------------------------------------------------- */
 /* 0x2120: Hi-resolution scrolling                                            */
 /* -------------------------------------------------------------------------- */
@@ -3660,6 +3818,9 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
 		ret = hidpp20_battery_voltage_event(hidpp, data, size);
 		if (ret != 0)
 			return ret;
+		ret = hidpp20_adc_measurement_event_1f20(hidpp, data, size);
+		if (ret != 0)
+			return ret;
 	}
 
 	if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP10_BATTERY) {
@@ -3783,6 +3944,7 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 	hidpp->battery.feature_index = 0xff;
 	hidpp->battery.solar_feature_index = 0xff;
 	hidpp->battery.voltage_feature_index = 0xff;
+	hidpp->battery.adc_measurement_feature_index = 0xff;
 
 	if (hidpp->protocol_major >= 2) {
 		if (hidpp->quirks & HIDPP_QUIRK_CLASS_K750)
@@ -3796,6 +3958,8 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 				ret = hidpp20_query_battery_info_1004(hidpp);
 			if (ret)
 				ret = hidpp20_query_battery_voltage_info(hidpp);
+			if (ret)
+				ret = hidpp20_query_adc_measurement_info_1f20(hidpp);
 		}
 
 		if (ret)
@@ -3825,7 +3989,8 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 
 	if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_MILEAGE ||
 	    hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_PERCENTAGE ||
-	    hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE)
+	    hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE ||
+	    hidpp->capabilities & HIDPP_CAPABILITY_ADC_MEASUREMENT)
 		battery_props[num_battery_props++] =
 				POWER_SUPPLY_PROP_CAPACITY;
 
@@ -3833,7 +3998,8 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 		battery_props[num_battery_props++] =
 				POWER_SUPPLY_PROP_CAPACITY_LEVEL;
 
-	if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE)
+	if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE ||
+	    hidpp->capabilities & HIDPP_CAPABILITY_ADC_MEASUREMENT)
 		battery_props[num_battery_props++] =
 			POWER_SUPPLY_PROP_VOLTAGE_NOW;
 
@@ -4006,6 +4172,8 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 			hidpp20_query_battery_voltage_info(hidpp);
 		else if (hidpp->capabilities & HIDPP_CAPABILITY_UNIFIED_BATTERY)
 			hidpp20_query_battery_info_1004(hidpp);
+		else if (hidpp->capabilities & HIDPP_CAPABILITY_ADC_MEASUREMENT)
+			hidpp20_query_adc_measurement_info_1f20(hidpp);
 		else
 			hidpp20_query_battery_info_1000(hidpp);
 	}
-- 
2.39.2

[PATCH v3 3/6] HID: logitech-hidpp: Add Logitech G935 headset

[Bastien Nocera]

Add the Logitech G935 headset that uses the HID++ protocol to the
list of supported devices.

Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
No changes since v1

 drivers/hid/hid-logitech-hidpp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 5a95f01a6129..4708819a6d79 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4556,6 +4556,9 @@ static const struct hid_device_id hidpp_devices[] = {
 	{ /* Logitech G Pro Gaming Mouse over USB */
 	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC088) },
 
+	{ /* G935 Gaming Headset */
+	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0x0a87) },
+
 	{ /* MX5000 keyboard over Bluetooth */
 	  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb305),
 	  .driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS },
-- 
2.39.2

[PATCH v3 4/6] USB: core: Add wireless_status sysfs attribute

[Bastien Nocera]

Add a wireless_status sysfs attribute to USB devices to keep track of
whether a USB device that's comprised of a receiver dongle and an emitter
device over a, most of the time proprietary, wireless link has its emitter
connected or disconnected.

This will be used by user-space OS components to determine whether the
battery-powered part of the device is wirelessly connected or not,
allowing, for example:
- upower to hide the battery for devices where the device is turned off
  but the receiver plugged in, rather than showing 0%, or other values
  that could be confusing to users
- Pipewire to hide a headset from the list of possible inputs or outputs
  or route audio appropriately if the headset is suddenly turned off, or
  turned on
- libinput to determine whether a keyboard or mouse is present when its
  receiver is plugged in.

This is done at the USB interface level as:
- the interface on which the wireless status is detected is sometimes
  not the same as where it could be consumed (eg. the audio interface
  on a headset dongle will still appear even if the headset is turned
  off), and we cannot have synchronisation of status across subsystems.
- this behaviour is not specific to HID devices, even if the protocols
  used to determine whether or not the remote device is connected can
  be HID.

This is not an attribute that is meant to replace protocol specific
APIs, such as the ones available for WWAN, WLAN/Wi-Fi, or Bluetooth
or any other sort of networking, but solely for wireless devices with
an ad-hoc “lose it and your device is e-waste” receiver dongle.

The USB interface will only be exporting the wireless_status sysfs
attribute if it gets set through the API exported in the next commit.

Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
Updated commit message and documentation in v2 so that the commit
doesn't need to reference older discussions.

Trimmed the width of the sysfs docs in v3.

 Documentation/ABI/testing/sysfs-bus-usb | 17 +++++++++
 drivers/usb/core/sysfs.c                | 50 +++++++++++++++++++++++++
 drivers/usb/core/usb.h                  |  1 +
 include/linux/usb.h                     |  9 +++++
 4 files changed, 77 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
index 545c2dd97ed0..cb172db41b34 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -166,6 +166,23 @@ Description:
 		The file will be present for all speeds of USB devices, and will
 		always read "no" for USB 1.1 and USB 2.0 devices.
 
+What:		/sys/bus/usb/devices/<INTERFACE>/wireless_status
+Date:		February 2023
+Contact:	Bastien Nocera <hadess@hadess.net>
+Description:
+		Some USB devices use a USB receiver dongle to communicate
+		wirelessly with their device using proprietary protocols. This
+		attribute allows user-space to know whether the device is
+		connected to its receiver dongle, and, for example, consider
+		the device to be absent when choosing whether to show the
+		device's battery, show a headset in a list of outputs, or show
+		an on-screen keyboard if the only wireless keyboard is
+		turned off.
+		This attribute is not to be used to replace protocol specific
+		statuses available in WWAN, WLAN/Wi-Fi, Bluetooth, etc.
+		If the device does not use a receiver dongle with a wireless
+		device, then this attribute will not exist.
+
 What:		/sys/bus/usb/devices/.../<hub_interface>/port<X>
 Date:		August 2012
 Contact:	Lan Tianyu <tianyu.lan@intel.com>
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index 8217032dfb85..da3c0f0dd633 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -1232,9 +1232,59 @@ static const struct attribute_group intf_assoc_attr_grp = {
 	.is_visible =	intf_assoc_attrs_are_visible,
 };
 
+static ssize_t wireless_status_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct usb_interface *intf;
+
+	intf = to_usb_interface(dev);
+	if (intf->wireless_status == USB_WIRELESS_STATUS_DISCONNECTED)
+		return sysfs_emit(buf, "%s\n", "disconnected");
+	return sysfs_emit(buf, "%s\n", "connected");
+}
+static DEVICE_ATTR_RO(wireless_status);
+
+static struct attribute *intf_wireless_status_attrs[] = {
+	&dev_attr_wireless_status.attr,
+	NULL
+};
+
+static umode_t intf_wireless_status_attr_is_visible(struct kobject *kobj,
+		struct attribute *a, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct usb_interface *intf = to_usb_interface(dev);
+
+	if (a != &dev_attr_wireless_status.attr ||
+	    intf->wireless_status != USB_WIRELESS_STATUS_NA)
+		return a->mode;
+	return 0;
+}
+
+static const struct attribute_group intf_wireless_status_attr_grp = {
+	.attrs =	intf_wireless_status_attrs,
+	.is_visible =	intf_wireless_status_attr_is_visible,
+};
+
+int usb_update_wireless_status_attr(struct usb_interface *intf)
+{
+	struct device *dev = &intf->dev;
+	int ret;
+
+	ret = sysfs_update_group(&dev->kobj, &intf_wireless_status_attr_grp);
+	if (ret < 0)
+		return ret;
+
+	sysfs_notify(&dev->kobj, NULL, "wireless_status");
+	kobject_uevent(&dev->kobj, KOBJ_CHANGE);
+
+	return 0;
+}
+
 const struct attribute_group *usb_interface_groups[] = {
 	&intf_attr_grp,
 	&intf_assoc_attr_grp,
+	&intf_wireless_status_attr_grp,
 	NULL
 };
 
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 0eac7d4285d1..3f14e15f07f6 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -15,6 +15,7 @@ extern int usb_create_sysfs_dev_files(struct usb_device *dev);
 extern void usb_remove_sysfs_dev_files(struct usb_device *dev);
 extern void usb_create_sysfs_intf_files(struct usb_interface *intf);
 extern void usb_remove_sysfs_intf_files(struct usb_interface *intf);
+extern int usb_update_wireless_status_attr(struct usb_interface *intf);
 extern int usb_create_ep_devs(struct device *parent,
 				struct usb_host_endpoint *endpoint,
 				struct usb_device *udev);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 86d1c8e79566..46fc85aba0df 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -170,6 +170,12 @@ usb_find_last_int_out_endpoint(struct usb_host_interface *alt,
 	return usb_find_common_endpoints_reverse(alt, NULL, NULL, NULL, int_out);
 }
 
+enum usb_wireless_status {
+	USB_WIRELESS_STATUS_NA = 0,
+	USB_WIRELESS_STATUS_DISCONNECTED,
+	USB_WIRELESS_STATUS_CONNECTED,
+};
+
 /**
  * struct usb_interface - what usb device drivers talk to
  * @altsetting: array of interface structures, one for each alternate
@@ -203,6 +209,8 @@ usb_find_last_int_out_endpoint(struct usb_host_interface *alt,
  * @reset_ws: Used for scheduling resets from atomic context.
  * @resetting_device: USB core reset the device, so use alt setting 0 as
  *	current; needs bandwidth alloc after reset.
+ * @wireless_status: if the USB device uses a receiver/emitter combo, whether
+ *	the emitter is connected.
  *
  * USB device drivers attach to interfaces on a physical device.  Each
  * interface encapsulates a single high level function, such as feeding
@@ -253,6 +261,7 @@ struct usb_interface {
 	unsigned needs_binding:1;	/* needs delayed unbind/rebind */
 	unsigned resetting_device:1;	/* true: bandwidth alloc after reset */
 	unsigned authorized:1;		/* used for interface authorization */
+	enum usb_wireless_status wireless_status;
 
 	struct device dev;		/* interface specific device info */
 	struct device *usb_dev;
-- 
2.39.2

Re: [PATCH v3 4/6] USB: core: Add wireless_status sysfs attribute

[Alan Stern]

On Thu, Mar 02, 2023 at 11:55:53AM +0100, Bastien Nocera wrote:
> Add a wireless_status sysfs attribute to USB devices to keep track of
> whether a USB device that's comprised of a receiver dongle and an emitter
> device over a, most of the time proprietary, wireless link has its emitter
> connected or disconnected.
> 
> This will be used by user-space OS components to determine whether the
> battery-powered part of the device is wirelessly connected or not,
> allowing, for example:
> - upower to hide the battery for devices where the device is turned off
>   but the receiver plugged in, rather than showing 0%, or other values
>   that could be confusing to users
> - Pipewire to hide a headset from the list of possible inputs or outputs
>   or route audio appropriately if the headset is suddenly turned off, or
>   turned on
> - libinput to determine whether a keyboard or mouse is present when its
>   receiver is plugged in.
> 
> This is done at the USB interface level as:
> - the interface on which the wireless status is detected is sometimes
>   not the same as where it could be consumed (eg. the audio interface
>   on a headset dongle will still appear even if the headset is turned
>   off), and we cannot have synchronisation of status across subsystems.
> - this behaviour is not specific to HID devices, even if the protocols
>   used to determine whether or not the remote device is connected can
>   be HID.
> 
> This is not an attribute that is meant to replace protocol specific
> APIs, such as the ones available for WWAN, WLAN/Wi-Fi, or Bluetooth
> or any other sort of networking, but solely for wireless devices with
> an ad-hoc “lose it and your device is e-waste” receiver dongle.
> 
> The USB interface will only be exporting the wireless_status sysfs
> attribute if it gets set through the API exported in the next commit.
> 
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---

For parts 4 and 5:

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH v3 4/6] USB: core: Add wireless_status sysfs attribute

[Greg Kroah-Hartman]

On Thu, Mar 02, 2023 at 11:55:53AM +0100, Bastien Nocera wrote:
> Add a wireless_status sysfs attribute to USB devices to keep track of
> whether a USB device that's comprised of a receiver dongle and an emitter
> device over a, most of the time proprietary, wireless link has its emitter
> connected or disconnected.
> 
> This will be used by user-space OS components to determine whether the
> battery-powered part of the device is wirelessly connected or not,
> allowing, for example:
> - upower to hide the battery for devices where the device is turned off
>   but the receiver plugged in, rather than showing 0%, or other values
>   that could be confusing to users
> - Pipewire to hide a headset from the list of possible inputs or outputs
>   or route audio appropriately if the headset is suddenly turned off, or
>   turned on
> - libinput to determine whether a keyboard or mouse is present when its
>   receiver is plugged in.
> 
> This is done at the USB interface level as:
> - the interface on which the wireless status is detected is sometimes
>   not the same as where it could be consumed (eg. the audio interface
>   on a headset dongle will still appear even if the headset is turned
>   off), and we cannot have synchronisation of status across subsystems.
> - this behaviour is not specific to HID devices, even if the protocols
>   used to determine whether or not the remote device is connected can
>   be HID.
> 
> This is not an attribute that is meant to replace protocol specific
> APIs, such as the ones available for WWAN, WLAN/Wi-Fi, or Bluetooth
> or any other sort of networking, but solely for wireless devices with
> an ad-hoc “lose it and your device is e-waste” receiver dongle.
> 
> The USB interface will only be exporting the wireless_status sysfs
> attribute if it gets set through the API exported in the next commit.
> 
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
> Updated commit message and documentation in v2 so that the commit
> doesn't need to reference older discussions.
> 
> Trimmed the width of the sysfs docs in v3.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH v3 5/6] USB: core: Add API to change the wireless_status

[Bastien Nocera]

This adds the API that allows device specific drivers to tell user-space
about whether the wireless device is connected to its receiver dongle.

See "USB: core: Add wireless_status sysfs attribute" for a detailed
explanation of what this attribute should be used for.

Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
Fixed locking/use-after-free in v2, thanks to Alan Stern

Fixed ordering of locking/put in v3, thanks to Alan Stern again

 drivers/usb/core/message.c | 40 ++++++++++++++++++++++++++++++++++++++
 include/linux/usb.h        |  5 +++++
 2 files changed, 45 insertions(+)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 127fac1af676..7930dca84616 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1908,6 +1908,45 @@ static void __usb_queue_reset_device(struct work_struct *ws)
 	usb_put_intf(iface);	/* Undo _get_ in usb_queue_reset_device() */
 }
 
+/*
+ * Internal function to set the wireless_status sysfs attribute
+ * See usb_set_wireless_status() for more details
+ */
+static void __usb_wireless_status_intf(struct work_struct *ws)
+{
+	struct usb_interface *iface =
+		container_of(ws, struct usb_interface, wireless_status_work);
+
+	device_lock(iface->dev.parent);
+	if (iface->sysfs_files_created)
+		usb_update_wireless_status_attr(iface);
+	device_unlock(iface->dev.parent);
+	usb_put_intf(iface);	/* Undo _get_ in usb_set_wireless_status() */
+}
+
+/**
+ * usb_set_wireless_status - sets the wireless_status struct member
+ * @dev: the device to modify
+ * @status: the new wireless status
+ *
+ * Set the wireless_status struct member to the new value, and emit
+ * sysfs changes as necessary.
+ *
+ * Returns: 0 on success, -EALREADY if already set.
+ */
+int usb_set_wireless_status(struct usb_interface *iface,
+		enum usb_wireless_status status)
+{
+	if (iface->wireless_status == status)
+		return -EALREADY;
+
+	usb_get_intf(iface);
+	iface->wireless_status = status;
+	schedule_work(&iface->wireless_status_work);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_set_wireless_status);
 
 /*
  * usb_set_configuration - Makes a particular device setting be current
@@ -2100,6 +2139,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 		intf->dev.type = &usb_if_device_type;
 		intf->dev.groups = usb_interface_groups;
 		INIT_WORK(&intf->reset_ws, __usb_queue_reset_device);
+		INIT_WORK(&intf->wireless_status_work, __usb_wireless_status_intf);
 		intf->minor = -1;
 		device_initialize(&intf->dev);
 		pm_runtime_no_callbacks(&intf->dev);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 46fc85aba0df..a48eeec62a66 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -262,6 +262,7 @@ struct usb_interface {
 	unsigned resetting_device:1;	/* true: bandwidth alloc after reset */
 	unsigned authorized:1;		/* used for interface authorization */
 	enum usb_wireless_status wireless_status;
+	struct work_struct wireless_status_work;
 
 	struct device dev;		/* interface specific device info */
 	struct device *usb_dev;
@@ -896,6 +897,10 @@ static inline int usb_interface_claimed(struct usb_interface *iface)
 
 extern void usb_driver_release_interface(struct usb_driver *driver,
 			struct usb_interface *iface);
+
+int usb_set_wireless_status(struct usb_interface *iface,
+			enum usb_wireless_status status);
+
 const struct usb_device_id *usb_match_id(struct usb_interface *interface,
 					 const struct usb_device_id *id);
 extern int usb_match_one_id(struct usb_interface *interface,
-- 
2.39.2

Re: [PATCH v3 5/6] USB: core: Add API to change the wireless_status

[Greg Kroah-Hartman]

On Thu, Mar 02, 2023 at 11:55:54AM +0100, Bastien Nocera wrote:
> This adds the API that allows device specific drivers to tell user-space
> about whether the wireless device is connected to its receiver dongle.
> 
> See "USB: core: Add wireless_status sysfs attribute" for a detailed
> explanation of what this attribute should be used for.
> 
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
> Fixed locking/use-after-free in v2, thanks to Alan Stern
> 
> Fixed ordering of locking/put in v3, thanks to Alan Stern again
> 
>  drivers/usb/core/message.c | 40 ++++++++++++++++++++++++++++++++++++++
>  include/linux/usb.h        |  5 +++++
>  2 files changed, 45 insertions(+)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH v3 6/6] HID: logitech-hidpp: Set wireless_status for G935 receiver

[Bastien Nocera]

Set the USB interface "wireless_status" for the G935 receiver when
receiving battery notifications.

This will allow sound daemons such as Pipewire or PulseAudio to know
whether or not the headset is turned on and connected.

Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
No changes since v1

 drivers/hid/hid-logitech-hidpp.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 4708819a6d79..c7d81b4241ad 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -74,6 +74,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_QUIRK_HIDPP_EXTRA_MOUSE_BTNS	BIT(27)
 #define HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS	BIT(28)
 #define HIDPP_QUIRK_HI_RES_SCROLL_1P0		BIT(29)
+#define HIDPP_QUIRK_WIRELESS_STATUS		BIT(30)
 
 /* These are just aliases for now */
 #define HIDPP_QUIRK_KBD_SCROLL_WHEEL HIDPP_QUIRK_HIDPP_WHEELS
@@ -472,6 +473,26 @@ static void hidpp_prefix_name(char **name, int name_length)
 	*name = new_name;
 }
 
+/*
+ * Updates the USB wireless_status based on whether the headset
+ * is turned on and reachable.
+ */
+static void hidpp_update_usb_wireless_status(struct hidpp_device *hidpp)
+{
+	struct hid_device *hdev = hidpp->hid_dev;
+	struct usb_interface *intf;
+
+	if (!(hidpp->quirks & HIDPP_QUIRK_WIRELESS_STATUS))
+		return;
+	if (!hid_is_usb(hdev))
+		return;
+
+	intf = to_usb_interface(hdev->dev.parent);
+	usb_set_wireless_status(intf, hidpp->battery.online ?
+				USB_WIRELESS_STATUS_CONNECTED :
+				USB_WIRELESS_STATUS_DISCONNECTED);
+}
+
 /**
  * hidpp_scroll_counter_handle_scroll() - Send high- and low-resolution scroll
  *                                        events given a high-resolution wheel
@@ -1872,6 +1893,7 @@ static int hidpp20_query_adc_measurement_info_1f20(struct hidpp_device *hidpp)
 								 &hidpp->battery.voltage);
 	hidpp->battery.capacity = hidpp20_map_adc_measurement_1f20_capacity(hidpp->hid_dev,
 									    hidpp->battery.voltage);
+	hidpp_update_usb_wireless_status(hidpp);
 
 	return 0;
 }
@@ -1896,6 +1918,7 @@ static int hidpp20_adc_measurement_event_1f20(struct hidpp_device *hidpp,
 		hidpp->battery.capacity = hidpp20_map_adc_measurement_1f20_capacity(hidpp->hid_dev, voltage);
 		if (hidpp->battery.ps)
 			power_supply_changed(hidpp->battery.ps);
+		hidpp_update_usb_wireless_status(hidpp);
 	}
 	return 0;
 }
@@ -4557,7 +4580,8 @@ static const struct hid_device_id hidpp_devices[] = {
 	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC088) },
 
 	{ /* G935 Gaming Headset */
-	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0x0a87) },
+	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0x0a87),
+		.driver_data = HIDPP_QUIRK_WIRELESS_STATUS },
 
 	{ /* MX5000 keyboard over Bluetooth */
 	  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb305),
-- 
2.39.2

Re: [PATCH v3 1/6] HID: logitech-hidpp: Simplify array length check

[Greg Kroah-Hartman]

On Thu, Mar 02, 2023 at 11:55:50AM +0100, Bastien Nocera wrote:
> Use the compiler to force a 100-length array, rather than check the
> length after the fact.
> 
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
> New in v2, following a review comment in the 1f20 enablement patch.
> No changes in v3.
> 
>  drivers/hid/hid-logitech-hidpp.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

These all look good to me, and probably should go through the HID tree,
so I've acked the USB-specific ones to allow that.

thanks,

greg k-h

Re: [PATCH v3 1/6] HID: logitech-hidpp: Simplify array length check

[Benjamin Tissoires]

On Thu, 02 Mar 2023 11:55:50 +0100, Bastien Nocera wrote:
> Use the compiler to force a 100-length array, rather than check the
> length after the fact.
> 
> 

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git (for-6.4/logitech-hidpp), thanks!

[1/6] HID: logitech-hidpp: Simplify array length check
      https://git.kernel.org/hid/hid/c/e0138763be2d
[2/6] HID: logitech-hidpp: Add support for ADC measurement feature
      https://git.kernel.org/hid/hid/c/c361982a13c9
[3/6] HID: logitech-hidpp: Add Logitech G935 headset
      https://git.kernel.org/hid/hid/c/4a1529f44e32
[4/6] USB: core: Add wireless_status sysfs attribute
      https://git.kernel.org/hid/hid/c/f98e0640c5c6
[5/6] USB: core: Add API to change the wireless_status
      https://git.kernel.org/hid/hid/c/0a4db185f078
[6/6] HID: logitech-hidpp: Set wireless_status for G935 receiver
      https://git.kernel.org/hid/hid/c/d9d5623f37c0

Cheers,
-- 
Benjamin Tissoires <benjamin.tissoires@redhat.com>