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

[Greg KH]

On Wed, Feb 15, 2023 at 11:05:58AM +1000, David Sands wrote:
> Add the ability for FunctionFS driver to be able to create DFU Run-Time
> descriptors.
> 
> Signed-off-by: David Sands <david.sands@xxxxxxxxx>
> ---
>  drivers/usb/gadget/function/f_fs.c  | 12 ++++++++++--
>  include/uapi/linux/usb/ch9.h        |  4 ++++
>  include/uapi/linux/usb/functionfs.h | 10 ++++++++++
>  3 files changed, 24 insertions(+), 2 deletions(-)

Ah, DFU, bad memories (hint, my name is on that spec...)

Why is this a RFC?  What needs to be done on this to make it "real"?


> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 73dc10a77cde..ac77ded9e6ad 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -2049,7 +2049,7 @@ typedef int (*ffs_os_desc_callback)(enum ffs_os_desc_type entity,
>  
>  static int __must_check ffs_do_single_desc(char *data, unsigned len,
>  					   ffs_entity_callback entity,
> -					   void *priv, int *current_class)
> +					   void *priv, int *current_class, int *current_subclass)
>  {
>  	struct usb_descriptor_header *_ds = (void *)data;
>  	u8 length;
> @@ -2108,6 +2108,7 @@ static int __must_check ffs_do_single_desc(char *data, unsigned len,
>  		if (ds->iInterface)
>  			__entity(STRING, ds->iInterface);
>  		*current_class = ds->bInterfaceClass;
> +		*current_subclass = ds->bInterfaceSubClass;
>  	}
>  		break;
>  
> @@ -2132,6 +2133,12 @@ static int __must_check ffs_do_single_desc(char *data, unsigned len,
>  			if (length != sizeof(struct ccid_descriptor))
>  				goto inv_length;
>  			break;
> +		} else if (*current_class == USB_CLASS_APP_SPEC &&
> +				*current_subclass == USB_SUBCLASS_DFU) {
> +			pr_vdebug("dfu functional descriptor\n");
> +			if (length != sizeof(struct usb_dfu_functional_descriptor))
> +				goto inv_length;
> +			break;
>  		} else {
>  			pr_vdebug("unknown descriptor: %d for class %d\n",
>  			      _ds->bDescriptorType, *current_class);
> @@ -2194,6 +2201,7 @@ static int __must_check ffs_do_descs(unsigned count, char *data, unsigned len,
>  	const unsigned _len = len;
>  	unsigned long num = 0;
>  	int current_class = -1;
> +	int current_subclass = -1;
>  
>  	ENTER();
>  
> @@ -2215,7 +2223,7 @@ static int __must_check ffs_do_descs(unsigned count, char *data, unsigned len,
>  			return _len - len;
>  
>  		ret = ffs_do_single_desc(data, len, entity, priv,
> -			&current_class);
> +			&current_class, &current_subclass);
>  		if (ret < 0) {
>  			pr_debug("%s returns %d\n", __func__, ret);
>  			return ret;
> diff --git a/include/uapi/linux/usb/ch9.h b/include/uapi/linux/usb/ch9.h
> index 31fcfa084e63..4683b3b508c7 100644
> --- a/include/uapi/linux/usb/ch9.h
> +++ b/include/uapi/linux/usb/ch9.h
> @@ -266,6 +266,9 @@ struct usb_ctrlrequest {
>  /* From the USB 3.1 spec */
>  #define	USB_DT_SSP_ISOC_ENDPOINT_COMP	0x31
>  
> +/* From USB Device Firmware Upgrade Specification, Revision 1.1 */
> +#define USB_DT_DFU_FUNCTIONAL		0x21
> +
>  /* 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.
> @@ -335,6 +338,7 @@ struct usb_device_descriptor {
>  #define USB_CLASS_VENDOR_SPEC		0xff
>  
>  #define USB_SUBCLASS_VENDOR_SPEC	0xff
> +#define USB_SUBCLASS_DFU		0x01
>  
>  /*-------------------------------------------------------------------------*/
>  
> diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
> index d77ee6b65328..3d15b193b260 100644
> --- a/include/uapi/linux/usb/functionfs.h
> +++ b/include/uapi/linux/usb/functionfs.h
> @@ -37,6 +37,16 @@ struct usb_endpoint_descriptor_no_audio {
>  	__u8  bInterval;
>  } __attribute__((packed));
>  
> +/* DFU Functional descriptor */
> +struct usb_dfu_functional_descriptor {
> +	__u8  bLength;
> +	__u8  bDescriptorType;
> +	__u8  bmAttributes;

Did you forget bEndpointAddress?

I can't remember if that is needed or not, but without it, it's an odd
layout of this structure.

thanks,

greg k-h