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

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

[Chris Wulff]

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>
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(-)

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
+
 /* 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
 
 /*-------------------------------------------------------------------------*/
 
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;
+	__le16 wDetachTimeOut;
+	__le16 wTransferSize;
+	__le16 bcdDFUVersion;
+} __attribute__ ((packed));
+
 struct usb_functionfs_descs_head_v2 {
 	__le32 magic;
 	__le32 length;
-- 
2.34.1

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

[Greg KH]

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

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

[Chris Wulff]

From: Greg KH <gregkh@linuxfoundation.org>
>>  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?

I haven't found anything in Documentation that talks about any of these structures or
constants. I will add kerneldoc sytle comments for the fields in the usb_dfu_functional_descriptor
structure though.

>> 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?
>

I will move it up so it is in 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?

I will re-group USB_SUBCLASS_DFU so it is associated with its corresponding
class code (USB_CLASS_APP_SPEC).

The DFU interface descriptor just uses the standard usb_interface_descriptor
structure so nothing new needed for that.

>> 
>>  /*-------------------------------------------------------------------------*/
>> 
>> 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?

I have them defined in the associated userspace program, but I will include
them here instead. The kernel doesn't need any of those constants, but I see a few
other structs that have bmAttributes related defines below them.

-- Chris Wulff