[PATCH 1/5] HID: logitech-hidpp: Add support for ADC measurement feature

[PATCH 1/5] HID: logitech-hidpp: Add support for ADC measurement feature

[Bastien Nocera]

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

The voltage to capacity conversion is based on the C implementation
in HeadsetControl.

Signed-off-by: Bastien Nocera <hadess@hadess.net>
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=216483
---
 drivers/hid/hid-logitech-hidpp.c | 174 ++++++++++++++++++++++++++++++-
 1 file changed, 172 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index ff1fcebf2ec7..f6365cdf2e21 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];
@@ -1744,6 +1746,164 @@ 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[] = {
+		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;
+
+	BUILD_BUG_ON(ARRAY_SIZE(voltages) != 100);
+
+	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, flags;
+
+	flags = (int) 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("%s: Parsed 1f20 data as flag 0x%02x voltage %dmV\n",
+		__func__, 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                                            */
 /* -------------------------------------------------------------------------- */
@@ -3662,6 +3822,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) {
@@ -3785,6 +3948,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)
@@ -3798,6 +3962,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)
@@ -3827,7 +3993,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;
 
@@ -3835,7 +4002,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;
 
@@ -4008,6 +4176,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 2/5] 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>
---
 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 f6365cdf2e21..166cfe92192e 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4560,6 +4560,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 3/5] 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 uses a receiver/emitter combo has its
emitter connected or disconnected.

By default, the USB device will declare not to use a receiver/emitter.

Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
 Documentation/ABI/testing/sysfs-bus-usb | 12 ++++++
 drivers/usb/core/sysfs.c                | 50 +++++++++++++++++++++++++
 drivers/usb/core/usb.h                  |  1 +
 include/linux/usb.h                     | 10 +++++
 4 files changed, 73 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
index 545c2dd97ed0..0bd22ece05cd 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -166,6 +166,18 @@ 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:		January 2023
+Contact:	Bastien Nocera <hadess@hadess.net>
+Description:
+		Some USB devices use a small USB receiver coupled with a larger
+		wireless device, usually communicating using proprietary
+		wireless protocols. This attribute will read either "connected"
+		or "disconnected" depending on whether the emitter is turned on,
+		in range and connected, on the interface which is used to detect
+		this state. If the device does not use a receiver/emitter combo,
+		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..517ae4b4e333 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;
@@ -887,6 +896,7 @@ static inline int usb_interface_claimed(struct usb_interface *iface)
 
 extern void usb_driver_release_interface(struct usb_driver *driver,
 			struct usb_interface *iface);
+
 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

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

[Bastien Nocera]

Allow device specific drivers to change the wireless status of a device.
This will allow user-space to know whether the device is available,
whether or not specific USB interfaces can detect it.

This can be used by wireless headsets with USB receivers to propagate to
user-space whether or not the headset is turned on, so as to consider it
as unavailable, and not switch to it just because the receiver is
plugged in.

Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
 drivers/usb/core/message.c | 13 +++++++++++++
 drivers/usb/core/usb.c     | 24 ++++++++++++++++++++++++
 include/linux/usb.h        |  4 ++++
 3 files changed, 41 insertions(+)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 127fac1af676..d5c7749d515e 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1908,6 +1908,18 @@ 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);
+
+	usb_update_wireless_status_attr(iface);
+	usb_put_intf(iface);	/* Undo _get_ in usb_set_wireless_status() */
+}
 
 /*
  * usb_set_configuration - Makes a particular device setting be current
@@ -2100,6 +2112,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/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 11b15d7b357a..5f42c5b9d209 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -871,6 +871,30 @@ int usb_get_current_frame_number(struct usb_device *dev)
 }
 EXPORT_SYMBOL_GPL(usb_get_current_frame_number);
 
+/**
+ * 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_get_extra_descriptor() finds a descriptor of specific type in the
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 517ae4b4e333..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;
@@ -897,6 +898,9 @@ 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

[PATCH 5/5] 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>
---
 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 166cfe92192e..7809d13bb84e 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
@@ -1876,6 +1897,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;
 }
@@ -1900,6 +1922,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;
 }
@@ -4561,7 +4584,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 3/5] USB: core: Add wireless_status sysfs attribute

[Greg Kroah-Hartman]

On Thu, Feb 23, 2023 at 02:24:50PM +0100, Bastien Nocera wrote:
> Add a wireless_status sysfs attribute to USB devices to keep track of
> whether a USB device that uses a receiver/emitter combo has its
> emitter connected or disconnected.

That's going to be very vague, and is starting to get very
interface-specific as an attibute here.

Why can't it just be an input device attribute?  Why is "wireless"
suddenly a special case for USB devices (we thought we got rid of the
old wireless usb code...)

> By default, the USB device will declare not to use a receiver/emitter.

I do not understand this statement, what do you mean by this?

> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
>  Documentation/ABI/testing/sysfs-bus-usb | 12 ++++++
>  drivers/usb/core/sysfs.c                | 50 +++++++++++++++++++++++++
>  drivers/usb/core/usb.h                  |  1 +
>  include/linux/usb.h                     | 10 +++++
>  4 files changed, 73 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
> index 545c2dd97ed0..0bd22ece05cd 100644
> --- a/Documentation/ABI/testing/sysfs-bus-usb
> +++ b/Documentation/ABI/testing/sysfs-bus-usb
> @@ -166,6 +166,18 @@ 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:		January 2023

January was last month :(

> +Contact:	Bastien Nocera <hadess@hadess.net>
> +Description:
> +		Some USB devices use a small USB receiver coupled with a larger
> +		wireless device, usually communicating using proprietary
> +		wireless protocols. This attribute will read either "connected"
> +		or "disconnected" depending on whether the emitter is turned on,
> +		in range and connected, on the interface which is used to detect
> +		this state. If the device does not use a receiver/emitter combo,
> +		then this attribute will not exist.

So would you declare a wireless network device such a thing?

See, it gets tricky, I do not think this should be a generic USB
attribute at all.


> +
>  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);

That could be very noisy, why does that deserve a KOBJ_CHANGE event?

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Thu, Feb 23, 2023 at 02:24:51PM +0100, Bastien Nocera wrote:
> Allow device specific drivers to change the wireless status of a device.
> This will allow user-space to know whether the device is available,
> whether or not specific USB interfaces can detect it.
> 
> This can be used by wireless headsets with USB receivers to propagate to
> user-space whether or not the headset is turned on, so as to consider it
> as unavailable, and not switch to it just because the receiver is
> plugged in.
> 
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
>  drivers/usb/core/message.c | 13 +++++++++++++
>  drivers/usb/core/usb.c     | 24 ++++++++++++++++++++++++
>  include/linux/usb.h        |  4 ++++
>  3 files changed, 41 insertions(+)
> 
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 127fac1af676..d5c7749d515e 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1908,6 +1908,18 @@ 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);
> +
> +	usb_update_wireless_status_attr(iface);
> +	usb_put_intf(iface);	/* Undo _get_ in usb_set_wireless_status() */
> +}

Why is this in a workqueue?  Why can't it just happen when asked?

I do not see any justification for that in your changelog or code :(

thanks,

greg k-h

Re: [PATCH 1/5] HID: logitech-hidpp: Add support for ADC measurement feature

[Greg Kroah-Hartman]

On Thu, Feb 23, 2023 at 02:24:48PM +0100, Bastien Nocera wrote:
> This is used in a number of headsets to report the voltage of the
> battery.
> 
> The voltage to capacity conversion is based on the C implementation
> in HeadsetControl.

What is "HeadsetControl"?

> 
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=216483
> ---
>  drivers/hid/hid-logitech-hidpp.c | 174 ++++++++++++++++++++++++++++++-
>  1 file changed, 172 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index ff1fcebf2ec7..f6365cdf2e21 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];
> @@ -1744,6 +1746,164 @@ 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
> +	 */
> +

No need for a blank line.

> +	static const int voltages[] = {
> +		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;
> +
> +	BUILD_BUG_ON(ARRAY_SIZE(voltages) != 100);

Why is 100 magic?  If you want it to be 100, say so up in the declaraion
so that the code will enforce that.

> +
> +	if (voltage == 0)
> +		return 0;
> +
> +	if (unlikely(voltage < 3400 || voltage >= 5000))

Why unlikely?  That should only ever be used if you can measure the
performance impact, otherwise please remove it.

> +		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, flags;
> +
> +	flags = (int) data[2];

Why is this now an int?

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

You are only checking it for a range 1-f, u8 is just fine, right?

> +		status = POWER_SUPPLY_STATUS_UNKNOWN;
> +		break;
> +	}
> +
> +	*voltage = get_unaligned_be16(data);
> +
> +	dbg_hid("%s: Parsed 1f20 data as flag 0x%02x voltage %dmV\n",
> +		__func__, flags, *voltage);

I doubt you need the __func__ line, right?  dynamic debug provides that
for you automatically if you ask for it.

thanks,

greg k-h

Re: [PATCH 1/5] HID: logitech-hidpp: Add support for ADC measurement feature

[Bastien Nocera]

On Thu, 2023-02-23 at 14:56 +0100, Greg Kroah-Hartman wrote:
> On Thu, Feb 23, 2023 at 02:24:48PM +0100, Bastien Nocera wrote:
> > This is used in a number of headsets to report the voltage of the
> > battery.
> > 
> > The voltage to capacity conversion is based on the C implementation
> > in HeadsetControl.
> 
> What is "HeadsetControl"?

It the software referenced in the comment after which I don't need a
blank line, just below this comment ;)

> 
> > 
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=216483
> > ---
> >  drivers/hid/hid-logitech-hidpp.c | 174
> > ++++++++++++++++++++++++++++++-
> >  1 file changed, 172 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-
> > logitech-hidpp.c
> > index ff1fcebf2ec7..f6365cdf2e21 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];
> > @@ -1744,6 +1746,164 @@ 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
> > +        */
> > +

This is the software ^^^^^^^^^^

> No need for a blank line.

Sure.

> > +       static const int voltages[] = {
> > +               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;
> > +
> > +       BUILD_BUG_ON(ARRAY_SIZE(voltages) != 100);
> 
> Why is 100 magic?  If you want it to be 100, say so up in the
> declaraion
> so that the code will enforce that.

It's the same pattern used in hidpp20_map_battery_capacity() in the
same file. I'll send a patch to fix that too.

> 
> > +
> > +       if (voltage == 0)
> > +               return 0;
> > +
> > +       if (unlikely(voltage < 3400 || voltage >= 5000))
> 
> Why unlikely?  That should only ever be used if you can measure the
> performance impact, otherwise please remove it.
> 
> > +               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, flags;
> > +
> > +       flags = (int) data[2];
> 
> Why is this now an int?

Sure.

> > +
> > +       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:
> 
> You are only checking it for a range 1-f, u8 is just fine, right?

Sure.

> 
> > +               status = POWER_SUPPLY_STATUS_UNKNOWN;
> > +               break;
> > +       }
> > +
> > +       *voltage = get_unaligned_be16(data);
> > +
> > +       dbg_hid("%s: Parsed 1f20 data as flag 0x%02x voltage
> > %dmV\n",
> > +               __func__, flags, *voltage);
> 
> I doubt you need the __func__ line, right?  dynamic debug provides
> that
> for you automatically if you ask for it.

Removed.

Re: [PATCH 3/5] USB: core: Add wireless_status sysfs attribute

[Bastien Nocera]

On Thu, 2023-02-23 at 14:51 +0100, Greg Kroah-Hartman wrote:
> On Thu, Feb 23, 2023 at 02:24:50PM +0100, Bastien Nocera wrote:
> > Add a wireless_status sysfs attribute to USB devices to keep track
> > of
> > whether a USB device that uses a receiver/emitter combo has its
> > emitter connected or disconnected.
> 
> That's going to be very vague, and is starting to get very
> interface-specific as an attibute here.
> 
> Why can't it just be an input device attribute?  Why is "wireless"
> suddenly a special case for USB devices (we thought we got rid of the
> old wireless usb code...)

We already had that discussion in December:
https://patchwork.kernel.org/project/linux-usb/patch/d9f8b9413c10fcf067658979d16a4f5c7abe69e7.camel@hadess.net/

Do you want me to reference this discussion somewhere?

It's not an input device attribute because, in the rest of this
patchset, the fact that it uses HID is an implementation detail.

> > By default, the USB device will declare not to use a
> > receiver/emitter.
> 
> I do not understand this statement, what do you mean by this?

That the default is for the wireless_status sysfs attribute not to be
present, which means that the device isn't a receiver/emitter. I can
remove that if it's confusing.

> 
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-usb | 12 ++++++
> >  drivers/usb/core/sysfs.c                | 50
> > +++++++++++++++++++++++++
> >  drivers/usb/core/usb.h                  |  1 +
> >  include/linux/usb.h                     | 10 +++++
> >  4 files changed, 73 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-usb
> > b/Documentation/ABI/testing/sysfs-bus-usb
> > index 545c2dd97ed0..0bd22ece05cd 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-usb
> > +++ b/Documentation/ABI/testing/sysfs-bus-usb
> > @@ -166,6 +166,18 @@ 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:          January 2023
> 
> January was last month :(

Sure.

> > +Contact:       Bastien Nocera <hadess@hadess.net>
> > +Description:
> > +               Some USB devices use a small USB receiver coupled
> > with a larger
> > +               wireless device, usually communicating using
> > proprietary
> > +               wireless protocols. This attribute will read either
> > "connected"
> > +               or "disconnected" depending on whether the emitter
> > is turned on,
> > +               in range and connected, on the interface which is
> > used to detect
> > +               this state. If the device does not use a
> > receiver/emitter combo,
> > +               then this attribute will not exist.
> 
> So would you declare a wireless network device such a thing?

No, because the emitter and receiver together aren't considered a
single device.

> See, it gets tricky, I do not think this should be a generic USB
> attribute at all.

See above, it was already discussed.

> > +
> >  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);
> 
> That could be very noisy, why does that deserve a KOBJ_CHANGE event?

Because otherwise user-space can't know that the property either
appeared or changed. This change only happens when the device is
powered on or off while the receiver is plugged in, so this shouldn't
happen at any excessive rate.

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

[Bastien Nocera]

On Thu, 2023-02-23 at 14:52 +0100, Greg Kroah-Hartman wrote:
> On Thu, Feb 23, 2023 at 02:24:51PM +0100, Bastien Nocera wrote:
> > Allow device specific drivers to change the wireless status of a
> > device.
> > This will allow user-space to know whether the device is available,
> > whether or not specific USB interfaces can detect it.
> > 
> > This can be used by wireless headsets with USB receivers to
> > propagate to
> > user-space whether or not the headset is turned on, so as to
> > consider it
> > as unavailable, and not switch to it just because the receiver is
> > plugged in.
> > 
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > ---
> >  drivers/usb/core/message.c | 13 +++++++++++++
> >  drivers/usb/core/usb.c     | 24 ++++++++++++++++++++++++
> >  include/linux/usb.h        |  4 ++++
> >  3 files changed, 41 insertions(+)
> > 
> > diff --git a/drivers/usb/core/message.c
> > b/drivers/usb/core/message.c
> > index 127fac1af676..d5c7749d515e 100644
> > --- a/drivers/usb/core/message.c
> > +++ b/drivers/usb/core/message.c
> > @@ -1908,6 +1908,18 @@ 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);
> > +
> > +       usb_update_wireless_status_attr(iface);
> > +       usb_put_intf(iface);    /* Undo _get_ in
> > usb_set_wireless_status() */
> > +}
> 
> Why is this in a workqueue?  Why can't it just happen when asked?
> 
> I do not see any justification for that in your changelog or code :(

Because, in many cases, updates would be triggered from USB events,
which happen in a softirq. Is that explanation good enough for the
commit message, or do you prefer it in the code?

Looks something like this if you don't use a workqueue:
Call Trace:
 <IRQ>
 dump_stack_lvl+0x5b/0x77
 mark_lock.cold+0x48/0xe1
 ? lock_is_held_type+0xe8/0x140
 ? lock_is_held_type+0xe8/0x140
 __lock_acquire+0x800/0x1ef0
 ? update_load_avg+0x762/0x890
 lock_acquire+0xce/0x2d0
 ? __kmem_cache_alloc_node+0x31/0x3b0
 ? lock_is_held_type+0xe8/0x140
 ? find_held_lock+0x32/0x90
 fs_reclaim_acquire+0xa5/0xe0
 ? __kmem_cache_alloc_node+0x31/0x3b0
 __kmem_cache_alloc_node+0x31/0x3b0
 ? usb_set_wireless_status+0x29/0x120
 kmalloc_trace+0x26/0x60
 usb_set_wireless_status+0x29/0x120
 hidpp_raw_hidpp_event.cold+0x107/0x119 [hid_logitech_hidpp]
 ? lock_release+0x14f/0x460
 hidpp_raw_event+0xf2/0x610 [hid_logitech_hidpp]
 ? _raw_spin_unlock_irqrestore+0x40/0x60
 hid_input_report+0x142/0x160
 hid_irq_in+0x177/0x1a0
 __usb_hcd_giveback_urb+0x9a/0x110
 usb_giveback_urb_bh+0x97/0x110
 tasklet_action_common.constprop.0+0xe3/0x110
 __do_softirq+0xec/0x590
 __irq_exit_rcu+0xf9/0x170
 irq_exit_rcu+0xa/0x30
 common_interrupt+0xb9/0xd0
 </IRQ>
 <TASK>
 asm_common_interrupt+0x22/0x40
RIP: 0010:cpuidle_enter_state+0xeb/0x320
Code: 4b 99 75 ff 45 84 ff 74 16 9c 58 0f 1f 40 00 f6 c4 02 0f 85 05 02
00 00 31 ff e8 80 b6 7d ff e8 1b 9c 85 ff fb 0f 1f 44 00 00 <45> 85 ed
0f 88 e2 00 00 00 49 63 cd 4c 89 f2 48 8d 04 49 48 8d 04
RSP: 0018:ffffaa07c017be98 EFLAGS: 00000206
RAX: 00000000000a8327 RBX: ffffca07be8036b8 RCX: 0000000000000001
RDX: 0000000000000000 RSI: ffffffffb46ba489 RDI: ffffffffb463dd36
RBP: 0000000000000008 R08: 0000000000000001 R09: 0000000000000001
R10: 0000000000000001 R11: 0000000000000000 R12: ffffffffb50d5e00
R13: 0000000000000008 R14: 0000001da3b692b2 R15: 0000000000000000
 ? cpuidle_enter_state+0xe5/0x320
 cpuidle_enter+0x29/0x40
 do_idle+0x21d/0x2b0
 cpu_startup_entry+0x19/0x20
 start_secondary+0x113/0x140
 secondary_startup_64_no_verify+0xe5/0xeb
 </TASK>
BUG: sleeping function called from invalid context at
include/linux/sched/mm.h:274
in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0, name:
swapper/5
preempt_count: 101, expected: 0

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

[Alan Stern]

On Thu, Feb 23, 2023 at 02:24:51PM +0100, Bastien Nocera wrote:
> Allow device specific drivers to change the wireless status of a device.
> This will allow user-space to know whether the device is available,
> whether or not specific USB interfaces can detect it.
> 
> This can be used by wireless headsets with USB receivers to propagate to
> user-space whether or not the headset is turned on, so as to consider it
> as unavailable, and not switch to it just because the receiver is
> plugged in.
> 
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
>  drivers/usb/core/message.c | 13 +++++++++++++
>  drivers/usb/core/usb.c     | 24 ++++++++++++++++++++++++
>  include/linux/usb.h        |  4 ++++
>  3 files changed, 41 insertions(+)
> 
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 127fac1af676..d5c7749d515e 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1908,6 +1908,18 @@ 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);
> +
> +	usb_update_wireless_status_attr(iface);
> +	usb_put_intf(iface);	/* Undo _get_ in usb_set_wireless_status() */
> +}

Have you thought about what will happen if this routine ends up running 
after the interface has been deleted?

>  /*
>   * usb_set_configuration - Makes a particular device setting be current
> @@ -2100,6 +2112,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/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index 11b15d7b357a..5f42c5b9d209 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -871,6 +871,30 @@ int usb_get_current_frame_number(struct usb_device *dev)
>  }
>  EXPORT_SYMBOL_GPL(usb_get_current_frame_number);
>  
> +/**
> + * 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);

This routine belongs in message.c, next to __usb_wireless_status_intf().

Alan Stern

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

[Bastien Nocera]

On Thu, 2023-02-23 at 10:41 -0500, Alan Stern wrote:
> On Thu, Feb 23, 2023 at 02:24:51PM +0100, Bastien Nocera wrote:
> > Allow device specific drivers to change the wireless status of a
> > device.
> > This will allow user-space to know whether the device is available,
> > whether or not specific USB interfaces can detect it.
> > 
> > This can be used by wireless headsets with USB receivers to
> > propagate to
> > user-space whether or not the headset is turned on, so as to
> > consider it
> > as unavailable, and not switch to it just because the receiver is
> > plugged in.
> > 
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > ---
> >  drivers/usb/core/message.c | 13 +++++++++++++
> >  drivers/usb/core/usb.c     | 24 ++++++++++++++++++++++++
> >  include/linux/usb.h        |  4 ++++
> >  3 files changed, 41 insertions(+)
> > 
> > diff --git a/drivers/usb/core/message.c
> > b/drivers/usb/core/message.c
> > index 127fac1af676..d5c7749d515e 100644
> > --- a/drivers/usb/core/message.c
> > +++ b/drivers/usb/core/message.c
> > @@ -1908,6 +1908,18 @@ 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);
> > +
> > +       usb_update_wireless_status_attr(iface);
> > +       usb_put_intf(iface);    /* Undo _get_ in
> > usb_set_wireless_status() */
> > +}
> 
> Have you thought about what will happen if this routine ends up
> running 
> after the interface has been deleted?

I believe that usb_release_interface() will only be called once the
last reference to the interface is dropped, so bar any refcounting
bugs, the interface should always exist when this function is called.

> >  /*
> >   * usb_set_configuration - Makes a particular device setting be
> > current
> > @@ -2100,6 +2112,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/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > index 11b15d7b357a..5f42c5b9d209 100644
> > --- a/drivers/usb/core/usb.c
> > +++ b/drivers/usb/core/usb.c
> > @@ -871,6 +871,30 @@ int usb_get_current_frame_number(struct
> > usb_device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(usb_get_current_frame_number);
> >  
> > +/**
> > + * 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);
> 
> This routine belongs in message.c, next to
> __usb_wireless_status_intf().

Sure, done locally.

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

[Alan Stern]

On Thu, Feb 23, 2023 at 05:17:13PM +0100, Bastien Nocera wrote:
> On Thu, 2023-02-23 at 10:41 -0500, Alan Stern wrote:
> > On Thu, Feb 23, 2023 at 02:24:51PM +0100, Bastien Nocera wrote:
> > > Allow device specific drivers to change the wireless status of a
> > > device.
> > > This will allow user-space to know whether the device is available,
> > > whether or not specific USB interfaces can detect it.
> > > 
> > > This can be used by wireless headsets with USB receivers to
> > > propagate to
> > > user-space whether or not the headset is turned on, so as to
> > > consider it
> > > as unavailable, and not switch to it just because the receiver is
> > > plugged in.
> > > 
> > > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > > ---
> > >  drivers/usb/core/message.c | 13 +++++++++++++
> > >  drivers/usb/core/usb.c     | 24 ++++++++++++++++++++++++
> > >  include/linux/usb.h        |  4 ++++
> > >  3 files changed, 41 insertions(+)
> > > 
> > > diff --git a/drivers/usb/core/message.c
> > > b/drivers/usb/core/message.c
> > > index 127fac1af676..d5c7749d515e 100644
> > > --- a/drivers/usb/core/message.c
> > > +++ b/drivers/usb/core/message.c
> > > @@ -1908,6 +1908,18 @@ 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);
> > > +
> > > +       usb_update_wireless_status_attr(iface);
> > > +       usb_put_intf(iface);    /* Undo _get_ in
> > > usb_set_wireless_status() */
> > > +}
> > 
> > Have you thought about what will happen if this routine ends up
> > running 
> > after the interface has been deleted?
> 
> I believe that usb_release_interface() will only be called once the
> last reference to the interface is dropped, so bar any refcounting
> bugs, the interface should always exist when this function is called.

Yes, but what about the calls made by usb_update_wireless_status_attr(): 
sysfs_update_group(), sysfs_notify(), and kobject_uevent()?  Will they 
work properly when called for an object that has been unregistered from 
sysfs?

Alan Stern

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

[Bastien Nocera]

On Thu, 2023-02-23 at 11:25 -0500, Alan Stern wrote:
> On Thu, Feb 23, 2023 at 05:17:13PM +0100, Bastien Nocera wrote:
> > On Thu, 2023-02-23 at 10:41 -0500, Alan Stern wrote:
> > > On Thu, Feb 23, 2023 at 02:24:51PM +0100, Bastien Nocera wrote:
> > > > Allow device specific drivers to change the wireless status of
> > > > a
> > > > device.
> > > > This will allow user-space to know whether the device is
> > > > available,
> > > > whether or not specific USB interfaces can detect it.
> > > > 
> > > > This can be used by wireless headsets with USB receivers to
> > > > propagate to
> > > > user-space whether or not the headset is turned on, so as to
> > > > consider it
> > > > as unavailable, and not switch to it just because the receiver
> > > > is
> > > > plugged in.
> > > > 
> > > > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > > > ---
> > > >  drivers/usb/core/message.c | 13 +++++++++++++
> > > >  drivers/usb/core/usb.c     | 24 ++++++++++++++++++++++++
> > > >  include/linux/usb.h        |  4 ++++
> > > >  3 files changed, 41 insertions(+)
> > > > 
> > > > diff --git a/drivers/usb/core/message.c
> > > > b/drivers/usb/core/message.c
> > > > index 127fac1af676..d5c7749d515e 100644
> > > > --- a/drivers/usb/core/message.c
> > > > +++ b/drivers/usb/core/message.c
> > > > @@ -1908,6 +1908,18 @@ 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);
> > > > +
> > > > +       usb_update_wireless_status_attr(iface);
> > > > +       usb_put_intf(iface);    /* Undo _get_ in
> > > > usb_set_wireless_status() */
> > > > +}
> > > 
> > > Have you thought about what will happen if this routine ends up
> > > running 
> > > after the interface has been deleted?
> > 
> > I believe that usb_release_interface() will only be called once the
> > last reference to the interface is dropped, so bar any refcounting
> > bugs, the interface should always exist when this function is
> > called.
> 
> Yes, but what about the calls made by
> usb_update_wireless_status_attr(): 
> sysfs_update_group(), sysfs_notify(), and kobject_uevent()?  Will
> they 
> work properly when called for an object that has been unregistered
> from 
> sysfs?

Those calls are made before the last reference to the interface can be
dropped by our own call to usb_put_intf(). So, in effect, the interface
is kept alive until we're done with our work so we can't ever be poking
at objects that disappeared.

Am I missing something about the object model, or the refcounting here?

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

[Alan Stern]

On Thu, Feb 23, 2023 at 05:51:48PM +0100, Bastien Nocera wrote:
> On Thu, 2023-02-23 at 11:25 -0500, Alan Stern wrote:
> > On Thu, Feb 23, 2023 at 05:17:13PM +0100, Bastien Nocera wrote:
> > > On Thu, 2023-02-23 at 10:41 -0500, Alan Stern wrote:
> > > > On Thu, Feb 23, 2023 at 02:24:51PM +0100, Bastien Nocera wrote:
> > > > > Allow device specific drivers to change the wireless status of
> > > > > a
> > > > > device.
> > > > > This will allow user-space to know whether the device is
> > > > > available,
> > > > > whether or not specific USB interfaces can detect it.
> > > > > 
> > > > > This can be used by wireless headsets with USB receivers to
> > > > > propagate to
> > > > > user-space whether or not the headset is turned on, so as to
> > > > > consider it
> > > > > as unavailable, and not switch to it just because the receiver
> > > > > is
> > > > > plugged in.
> > > > > 
> > > > > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > > > > ---
> > > > >  drivers/usb/core/message.c | 13 +++++++++++++
> > > > >  drivers/usb/core/usb.c     | 24 ++++++++++++++++++++++++
> > > > >  include/linux/usb.h        |  4 ++++
> > > > >  3 files changed, 41 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/usb/core/message.c
> > > > > b/drivers/usb/core/message.c
> > > > > index 127fac1af676..d5c7749d515e 100644
> > > > > --- a/drivers/usb/core/message.c
> > > > > +++ b/drivers/usb/core/message.c
> > > > > @@ -1908,6 +1908,18 @@ 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);
> > > > > +
> > > > > +       usb_update_wireless_status_attr(iface);
> > > > > +       usb_put_intf(iface);    /* Undo _get_ in
> > > > > usb_set_wireless_status() */
> > > > > +}
> > > > 
> > > > Have you thought about what will happen if this routine ends up
> > > > running 
> > > > after the interface has been deleted?
> > > 
> > > I believe that usb_release_interface() will only be called once the
> > > last reference to the interface is dropped, so bar any refcounting
> > > bugs, the interface should always exist when this function is
> > > called.
> > 
> > Yes, but what about the calls made by
> > usb_update_wireless_status_attr(): 
> > sysfs_update_group(), sysfs_notify(), and kobject_uevent()?  Will
> > they 
> > work properly when called for an object that has been unregistered
> > from 
> > sysfs?
> 
> Those calls are made before the last reference to the interface can be
> dropped by our own call to usb_put_intf(). So, in effect, the interface
> is kept alive until we're done with our work so we can't ever be poking
> at objects that disappeared.
> 
> Am I missing something about the object model, or the refcounting here?

Yes, you are.

There's a difference between object _existence_ and object 
_registration_.  An object (kobject, struct device, whatever) exists for 
as long as its memory is still allocated and in use.  This is what 
refcounting protects.

An object is registered in sysfs during the time between calls to
device_add() and device_del().  During this time it shows up as a 
directory under /sys, along with its various attribute files.  Calling 
device_del() gets rid of all that -- James Bottomley calls it "removing 
an object from visibility".  The object still exists, but it isn't part 
of sysfs any more.

The refcounting in your patch guarantees that when the work function 
runs, the interface structure will still exist.  But refcounting does 
not guarantee that the interface will still be registered in sysfs, and 
this can actually happen if the work is scheduled immediately before the 
interface is unregistered.

So my question is: What will happen when sysfs_update_group(), 
sysfs_notify(), and kobject_uevent() are called after the interface has 
been unregistered from sysfs?  Maybe they will work okay -- I simply 
don't know, and I wanted to find out whether you had considered the 
issue.

Alan Stern

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

[Bastien Nocera]

On Thu, 2023-02-23 at 12:07 -0500, Alan Stern wrote:
> On Thu, Feb 23, 2023 at 05:51:48PM +0100, Bastien Nocera wrote:
> > On Thu, 2023-02-23 at 11:25 -0500, Alan Stern wrote:
> > > On Thu, Feb 23, 2023 at 05:17:13PM +0100, Bastien Nocera wrote:
> > > > On Thu, 2023-02-23 at 10:41 -0500, Alan Stern wrote:
> > > > > On Thu, Feb 23, 2023 at 02:24:51PM +0100, Bastien Nocera
> > > > > wrote:
> > > > > > Allow device specific drivers to change the wireless status
> > > > > > of
> > > > > > a
> > > > > > device.
> > > > > > This will allow user-space to know whether the device is
> > > > > > available,
> > > > > > whether or not specific USB interfaces can detect it.
> > > > > > 
> > > > > > This can be used by wireless headsets with USB receivers to
> > > > > > propagate to
> > > > > > user-space whether or not the headset is turned on, so as
> > > > > > to
> > > > > > consider it
> > > > > > as unavailable, and not switch to it just because the
> > > > > > receiver
> > > > > > is
> > > > > > plugged in.
> > > > > > 
> > > > > > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > > > > > ---
> > > > > >  drivers/usb/core/message.c | 13 +++++++++++++
> > > > > >  drivers/usb/core/usb.c     | 24 ++++++++++++++++++++++++
> > > > > >  include/linux/usb.h        |  4 ++++
> > > > > >  3 files changed, 41 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/usb/core/message.c
> > > > > > b/drivers/usb/core/message.c
> > > > > > index 127fac1af676..d5c7749d515e 100644
> > > > > > --- a/drivers/usb/core/message.c
> > > > > > +++ b/drivers/usb/core/message.c
> > > > > > @@ -1908,6 +1908,18 @@ 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);
> > > > > > +
> > > > > > +       usb_update_wireless_status_attr(iface);
> > > > > > +       usb_put_intf(iface);    /* Undo _get_ in
> > > > > > usb_set_wireless_status() */
> > > > > > +}
> > > > > 
> > > > > Have you thought about what will happen if this routine ends
> > > > > up
> > > > > running 
> > > > > after the interface has been deleted?
> > > > 
> > > > I believe that usb_release_interface() will only be called once
> > > > the
> > > > last reference to the interface is dropped, so bar any
> > > > refcounting
> > > > bugs, the interface should always exist when this function is
> > > > called.
> > > 
> > > Yes, but what about the calls made by
> > > usb_update_wireless_status_attr(): 
> > > sysfs_update_group(), sysfs_notify(), and kobject_uevent()?  Will
> > > they 
> > > work properly when called for an object that has been
> > > unregistered
> > > from 
> > > sysfs?
> > 
> > Those calls are made before the last reference to the interface can
> > be
> > dropped by our own call to usb_put_intf(). So, in effect, the
> > interface
> > is kept alive until we're done with our work so we can't ever be
> > poking
> > at objects that disappeared.
> > 
> > Am I missing something about the object model, or the refcounting
> > here?
> 
> Yes, you are.
> 
> There's a difference between object _existence_ and object 
> _registration_.  An object (kobject, struct device, whatever) exists
> for 
> as long as its memory is still allocated and in use.  This is what 
> refcounting protects.
> 
> An object is registered in sysfs during the time between calls to
> device_add() and device_del().  During this time it shows up as a 
> directory under /sys, along with its various attribute files. 
> Calling 
> device_del() gets rid of all that -- James Bottomley calls it
> "removing 
> an object from visibility".  The object still exists, but it isn't
> part 
> of sysfs any more.
> 
> The refcounting in your patch guarantees that when the work function 
> runs, the interface structure will still exist.  But refcounting does
> not guarantee that the interface will still be registered in sysfs,
> and 
> this can actually happen if the work is scheduled immediately before
> the 
> interface is unregistered.
> 
> So my question is: What will happen when sysfs_update_group(), 
> sysfs_notify(), and kobject_uevent() are called after the interface
> has 
> been unregistered from sysfs?  Maybe they will work okay -- I simply 
> don't know, and I wanted to find out whether you had considered the 
> issue.

A long week-end started for me a couple of hours ago, but I wanted to
dump my thoughts before either I forgot, or it took over my whole week-
end ;)

I had thought about the problem, and didn't think that sysfs files
would get removed before the interface got released/unref'ed and
usb_remove_sysfs_intf_files() got called.

If the device gets removed from the device bus before it's released,
then this patch should fix it:
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1917,7 +1917,8 @@ static void __usb_wireless_status_intf(struct work_struct *ws)
        struct usb_interface *iface =
                container_of(ws, struct usb_interface, wireless_status_work);
 
-       usb_update_wireless_status_attr(iface);
+       if (intf->sysfs_files_created)
+               usb_update_wireless_status_attr(iface);
        usb_put_intf(iface);    /* Undo _get_ in usb_set_wireless_status() */

The callback would be a no-op if the device's sysfs is already
unregistered, just unref'ing the reference it held.

What do you think? I'll amend that into my patchset on Monday.

Cheers

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

[Alan Stern]

On Fri, Feb 24, 2023 at 12:04:12AM +0100, Bastien Nocera wrote:
> On Thu, 2023-02-23 at 12:07 -0500, Alan Stern wrote:
> > The refcounting in your patch guarantees that when the work function 
> > runs, the interface structure will still exist.  But refcounting does
> > not guarantee that the interface will still be registered in sysfs,
> > and 
> > this can actually happen if the work is scheduled immediately before
> > the 
> > interface is unregistered.
> > 
> > So my question is: What will happen when sysfs_update_group(), 
> > sysfs_notify(), and kobject_uevent() are called after the interface
> > has 
> > been unregistered from sysfs?  Maybe they will work okay -- I simply 
> > don't know, and I wanted to find out whether you had considered the 
> > issue.
> 
> A long week-end started for me a couple of hours ago, but I wanted to
> dump my thoughts before either I forgot, or it took over my whole week-
> end ;)
> 
> I had thought about the problem, and didn't think that sysfs files
> would get removed before the interface got released/unref'ed and
> usb_remove_sysfs_intf_files() got called.
> 
> If the device gets removed from the device bus before it's released,
> then this patch should fix it:
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1917,7 +1917,8 @@ static void __usb_wireless_status_intf(struct work_struct *ws)
>         struct usb_interface *iface =
>                 container_of(ws, struct usb_interface, wireless_status_work);
>  
> -       usb_update_wireless_status_attr(iface);
> +       if (intf->sysfs_files_created)
> +               usb_update_wireless_status_attr(iface);
>         usb_put_intf(iface);    /* Undo _get_ in usb_set_wireless_status() */
> 
> The callback would be a no-op if the device's sysfs is already
> unregistered, just unref'ing the reference it held.
> 
> What do you think? I'll amend that into my patchset on Monday.

That's a good way to do it, but it does race with 
usb_remove_sysfs_intf_files().  To prevent this race, you can protect 
the test and function call with device_lock(iface->dev.parent) (that is, 
lock the interface's parent usb_device).

Alan Stern

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

[Bastien Nocera]

On Thu, 2023-02-23 at 21:34 -0500, Alan Stern wrote:
> On Fri, Feb 24, 2023 at 12:04:12AM +0100, Bastien Nocera wrote:
> > On Thu, 2023-02-23 at 12:07 -0500, Alan Stern wrote:
> > > The refcounting in your patch guarantees that when the work
> > > function 
> > > runs, the interface structure will still exist.  But refcounting
> > > does
> > > not guarantee that the interface will still be registered in
> > > sysfs,
> > > and 
> > > this can actually happen if the work is scheduled immediately
> > > before
> > > the 
> > > interface is unregistered.
> > > 
> > > So my question is: What will happen when sysfs_update_group(), 
> > > sysfs_notify(), and kobject_uevent() are called after the
> > > interface
> > > has 
> > > been unregistered from sysfs?  Maybe they will work okay -- I
> > > simply 
> > > don't know, and I wanted to find out whether you had considered
> > > the 
> > > issue.
> > 
> > A long week-end started for me a couple of hours ago, but I wanted
> > to
> > dump my thoughts before either I forgot, or it took over my whole
> > week-
> > end ;)
> > 
> > I had thought about the problem, and didn't think that sysfs files
> > would get removed before the interface got released/unref'ed and
> > usb_remove_sysfs_intf_files() got called.
> > 
> > If the device gets removed from the device bus before it's
> > released,
> > then this patch should fix it:
> > --- a/drivers/usb/core/message.c
> > +++ b/drivers/usb/core/message.c
> > @@ -1917,7 +1917,8 @@ static void __usb_wireless_status_intf(struct
> > work_struct *ws)
> >         struct usb_interface *iface =
> >                 container_of(ws, struct usb_interface,
> > wireless_status_work);
> >  
> > -       usb_update_wireless_status_attr(iface);
> > +       if (intf->sysfs_files_created)
> > +               usb_update_wireless_status_attr(iface);
> >         usb_put_intf(iface);    /* Undo _get_ in
> > usb_set_wireless_status() */
> > 
> > The callback would be a no-op if the device's sysfs is already
> > unregistered, just unref'ing the reference it held.
> > 
> > What do you think? I'll amend that into my patchset on Monday.
> 
> That's a good way to do it, but it does race with 
> usb_remove_sysfs_intf_files().  To prevent this race, you can protect
> the test and function call with device_lock(iface->dev.parent) (that
> is, 
> lock the interface's parent usb_device).

Perfect, I've done that locally.

I'll send an updated patchset once I've been able to test it against
the hardware I have.

Cheers