Re: [PATCH v2] usb: Export BOS descriptor to sysfs

[Greg KH]

On Mon, Mar 04, 2024 at 04:23:01PM -0800, Elbert Mai wrote:
> Motivation
> ----------
> 
> The binary device object store (BOS) of a USB device consists of the BOS
> descriptor followed by a set of device capability descriptors. One that is
> of interest to users is the platform descriptor. This contains a 128-bit
> UUID and arbitrary data, and it allows parties outside of USB-IF to add
> additional metadata about a USB device in a standards-compliant manner.
> Notable examples include the WebUSB and Microsoft OS 2.0 descriptors.
> 
> The kernel already retrieves and caches the BOS from USB devices if its
> bcdUSB is >= 0x0201. Because the BOS is flexible and extensible, we export
> the entire BOS to sysfs so users can retrieve whatever device capabilities
> they desire, without requiring USB I/O or elevated permissions.
> 
> Implementation
> --------------
> 
> Add bos_descriptors attribute to sysfs. This is a binary file and it works
> the same way as the existing descriptors attribute. The file exists only if
> the BOS is present in the USB device.
> 
> Also create a binary attribute group, so the driver core can handle the
> creation of both the descriptors and bos_descriptors attributes in sysfs.
> 
> Signed-off-by: Elbert Mai <code@xxxxxxxxxxxxx>
> ---
> Changes in v2:
>  - Rename to bos_descriptors (plural) since the attribute contains the
>    entire BOS, not just the first descriptor in it.
>  - Use binary attribute groups to let driver core handle attribute
>    creation for both descriptors and bos_descriptors.
>  - The attribute is visible in sysfs only if the BOS is present in the
>    USB device.

Very nice, thanks for this!

One very minor comment, you can send a follow-on patch for this if you
like:

> +static umode_t dev_bin_attrs_are_visible(struct kobject *kobj,
> +		struct bin_attribute *a, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct usb_device *udev = to_usb_device(dev);
> +
> +	/* All USB devices have a device descriptor, so the descriptors
> +	 * attribute always exists. No need to check for its visibility.
> +	 */

This comment is in the "wrong" style, I think checkpatch will complain
about that, right?

But it's a bit confusing, you say "no need to check", and then you:

> +	if (a == &bin_attr_bos_descriptors) {
> +		if (udev->bos == NULL)
> +			return 0;
> +	}

check something :)

How about this as a comment instead:
	/*
	 * If this is the BOS descriptor, check to verify if the device
	 * has that descriptor at all or not.
	 */

That's all you need here, right?

Anyway, again, very nice, I'll go queue this up and run it through the
0-day tests.

thanks!

greg k -h