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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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