Re: [PATCH v3] usb: gadget: f_fs: add capability for dfu run-time descriptor

[Greg Kroah-Hartman]

On Wed, Apr 24, 2024 at 10:14:58PM +0000, Chris Wulff wrote:
> From: David Sands <david.sands@xxxxxxxxx>
> 
> Add the ability for FunctionFS driver to be able to create DFU Run-Time
> descriptors.

Don't you need some userspace documentation for this as well?

> --- a/include/uapi/linux/usb/ch9.h
> +++ b/include/uapi/linux/usb/ch9.h
> @@ -254,6 +254,9 @@ struct usb_ctrlrequest {
>  #define USB_DT_DEVICE_CAPABILITY	0x10
>  #define USB_DT_WIRELESS_ENDPOINT_COMP	0x11
>  #define USB_DT_WIRE_ADAPTER		0x21
> +/* From USB Device Firmware Upgrade Specification, Revision 1.1 */
> +#define USB_DT_DFU_FUNCTIONAL		0x21

So USB_DT_WIRE_ADAPTER and USB_DT_DFU_FUNCTIONAL are the same?  That
seems wrong.

> +/* these are from the Wireless USB spec */

What spec?  What "these"?

>  #define USB_DT_RPIPE			0x22
>  #define USB_DT_CS_RADIO_CONTROL		0x23
>  /* From the T10 UAS specification */
> @@ -263,6 +266,7 @@ struct usb_ctrlrequest {
>  /* From the USB 3.1 spec */
>  #define	USB_DT_SSP_ISOC_ENDPOINT_COMP	0x31
>  
> +
>  /* Conventional codes for class-specific descriptors.  The convention is
>   * defined in the USB "Common Class" Spec (3.11).  Individual class specs
>   * are authoritative for their usage, not the "common class" writeup.

Unneeded change?

> @@ -329,9 +333,10 @@ struct usb_device_descriptor {
>  #define USB_CLASS_USB_TYPE_C_BRIDGE	0x12
>  #define USB_CLASS_MISC			0xef
>  #define USB_CLASS_APP_SPEC		0xfe
> -#define USB_CLASS_VENDOR_SPEC		0xff
> +#define USB_SUBCLASS_DFU			0x01
>  
> -#define USB_SUBCLASS_VENDOR_SPEC	0xff
> +#define USB_CLASS_VENDOR_SPEC		0xff
> +#define USB_SUBCLASS_VENDOR_SPEC		0xff

Why reorder these?

>  
>  /*-------------------------------------------------------------------------*/
>  
> diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
> index 9f88de9c3d66..6d2061500184 100644
> --- a/include/uapi/linux/usb/functionfs.h
> +++ b/include/uapi/linux/usb/functionfs.h
> @@ -37,6 +37,31 @@ struct usb_endpoint_descriptor_no_audio {
>  	__u8  bInterval;
>  } __attribute__((packed));
>  
> +/**
> + * struct usb_dfu_functional_descriptor - DFU Functional descriptor
> + * @bLength:		Size of the descriptor (bytes)
> + * @bDescriptorType:	USB_DT_DFU_FUNCTIONAL
> + * @bmAttributes:	DFU attributes
> + * @wDetachTimeOut:	Maximum time to wait after DFU_DETACH (ms, le16)
> + * @wTransferSize:	Maximum number of bytes per control-write (le16)
> + * @bcdDFUVersion:	DFU Spec version (BCD, le16)
> + */
> +struct usb_dfu_functional_descriptor {
> +	__u8  bLength;
> +	__u8  bDescriptorType;
> +	__u8  bmAttributes;
> +	__le16 wDetachTimeOut;
> +	__le16 wTransferSize;
> +	__le16 bcdDFUVersion;
> +} __attribute__ ((packed));
> +
> +/* from DFU functional descriptor bmAttributes */
> +#define DFU_FUNC_ATT_WILL_DETACH		(1 << 3)
> +#define DFU_FUNC_ATT_MANIFEST_TOLERANT		(1 << 2)
> +#define DFU_FUNC_ATT_CAN_UPLOAD			(1 << 1)
> +#define DFU_FUNC_ATT_CAN_DOWNLOAD		(1 << 0)

Please use proper BIT macros here to make this more obvious.  And in
sorted order?

thanks,

greg k-h