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

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Apr 18, 2024 at 08:39:03PM +0000, Chris Wulff wrote:
> From: David Sands <david.sands@biamp.com>
> 
> Add the ability for FunctionFS driver to be able to create DFU Run-Time
> descriptors.
> 
> Signed-off-by: David Sands <david.sands@biamp.com>

Why didn't you also cc: them?

> Co-developed-by: Chris Wulff <chris.wulff@biamp.com>
> Signed-off-by: Chris Wulff <chris.wulff@biamp.com>
> 
> ---
> v2: Fixed attribution and signoff
> 
>  drivers/usb/gadget/function/f_fs.c  | 11 +++++++++--
>  include/uapi/linux/usb/ch9.h        |  4 ++++
>  include/uapi/linux/usb/functionfs.h | 10 ++++++++++
>  3 files changed, 23 insertions(+), 2 deletions(-)

Does this require a Documentation/ABI/ update?

> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index bffbc1dc651f..4d39e5e30b73 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -2467,7 +2467,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;
> @@ -2524,6 +2524,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;
>  
> @@ -2548,6 +2549,11 @@ 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);
> @@ -2610,6 +2616,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;
>  
>  	for (;;) {
>  		int ret;
> @@ -2629,7 +2636,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 44d73ba8788d..dcd962d1a75a 100644
> --- a/include/uapi/linux/usb/ch9.h
> +++ b/include/uapi/linux/usb/ch9.h
> @@ -263,6 +263,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

Why is this not in sorted order?

And it really conflicts with USB_DT_WIRE_ADAPTER?  That seems odd given
that DFU came first.  Hm, it is that value, odd.

> +
>  /* 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.
> @@ -332,6 +335,7 @@ struct usb_device_descriptor {
>  #define USB_CLASS_VENDOR_SPEC		0xff
>  
>  #define USB_SUBCLASS_VENDOR_SPEC	0xff
> +#define USB_SUBCLASS_DFU		0x01

This should be up next to the other DFU stuff, right?  WHat about the
DFU interface type?




>  
>  /*-------------------------------------------------------------------------*/
>  
> diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
> index 9f88de9c3d66..cf3f55234a5e 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;

Do you need #defines for the different attribute bits that DFU wants
here?

thanks,

greg k-h