usb: gadget: add SuperSpeed descriptor support

From: Dan Tran <[email protected]>

Add SS bulk endpoint descriptors (1024-byte MPS, bMaxBurst=15) to storage_common.c to support SuperSpeed connections. Extend fsg_ep_desc() to select them when operating at SuperSpeed, and wire up ss_descriptors in fsg_bind(). Free ss_descriptors in fsg_unbind() to match.

Signed-off-by Dan Tran <[email protected]>

Signed-off-by Thinh Nguyen <[email protected]>

---
Changes in v2:
- Removed internal Reviewed-by tags

 drivers/usb/gadget/f_mass_storage.c | 25 ++++++++++++--
 drivers/usb/gadget/storage_common.c | 52 ++++++++++++++++++++++++++++-
 2 files changed, 74 insertions(+), 3 deletions(-)

drivers/usb/gadget/f_mass_storage.c +5 -2

index 87ed25e8bb3a..a2f34c100482 100644
@@ -2225,14 +2225,16 @@ reset:
 
 	/* Enable the endpoints */
 	d = fsg_ep_desc(common->gadget,
-			&fsg_fs_bulk_in_desc, &fsg_hs_bulk_in_desc);
+			&fsg_fs_bulk_in_desc, &fsg_hs_bulk_in_desc,
+			&fsg_ss_bulk_in_desc);
 	rc = enable_endpoint(common, fsg->bulk_in, d);
 	if (rc)
 		goto reset;
 	fsg->bulk_in_enabled = 1;
 
 	d = fsg_ep_desc(common->gadget,
-			&fsg_fs_bulk_out_desc, &fsg_hs_bulk_out_desc);
+			&fsg_fs_bulk_out_desc, &fsg_hs_bulk_out_desc,
+			&fsg_ss_bulk_out_desc);
 	rc = enable_endpoint(common, fsg->bulk_out, d);
 	if (rc)
 		goto reset;

@@ -2653,6 +2655,7 @@ static void fsg_unbind(struct usb_configuration *c, struct usb_function *f)
 	fsg_common_release(fsg->common);
 	free(fsg->function.descriptors);
 	free(fsg->function.hs_descriptors);
+	free(fsg->function.ss_descriptors);
 	kfree(fsg);
 }
 

drivers/usb/gadget/f_mass_storage.c +15 -0

+
+		fsg_ss_bulk_in_desc.bEndpointAddress =
+			fsg_fs_bulk_in_desc.bEndpointAddress;
+		fsg_ss_bulk_in_comp_desc.bMaxBurst = max_burst;
+		fsg_ss_bulk_out_desc.bEndpointAddress =
+			fsg_fs_bulk_out_desc.bEndpointAddress;
+		fsg_ss_bulk_out_comp_desc.bMaxBurst = max_burst;
+		f->ss_descriptors = usb_copy_descriptors(fsg_ss_function);
+		if (unlikely(!f->ss_descriptors)) {
+			free(f->hs_descriptors);
+			free(f->descriptors);
+			return -ENOMEM;
+		}
+	}
+
 	return 0;
 
 autoconf_fail:

drivers/usb/gadget/storage_common.c +46 -0

index 7e4b542f7ce5..d745649eadb7 100644
@@ -531,11 +531,61 @@ static struct usb_descriptor_header *fsg_hs_function[] = {
 	NULL,
 };
 
+/*
+ * USB 3.0 requires SuperSpeed descriptors
+ */
+static struct usb_endpoint_descriptor
+fsg_ss_bulk_in_desc = {
+	.bLength =		USB_DT_ENDPOINT_SIZE,
+	.bDescriptorType =	USB_DT_ENDPOINT,
+
+	/* bEndpointAddress copied from fs_bulk_in_desc during fsg_bind() */
+	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
+	.wMaxPacketSize =	cpu_to_le16(1024),
+};
+
+static struct usb_ss_ep_comp_descriptor fsg_ss_bulk_in_comp_desc = {
+	.bLength =		sizeof(fsg_ss_bulk_in_comp_desc),
+	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
+	/* bMaxBurst set during fsg_bind() */
+};
+
+static struct usb_endpoint_descriptor
+fsg_ss_bulk_out_desc = {
+	.bLength =		USB_DT_ENDPOINT_SIZE,
+	.bDescriptorType =	USB_DT_ENDPOINT,
+
+	/* bEndpointAddress copied from fs_bulk_out_desc during fsg_bind() */
+	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
+	.wMaxPacketSize =	cpu_to_le16(1024),
+};
+
+static struct usb_ss_ep_comp_descriptor fsg_ss_bulk_out_comp_desc = {
+	.bLength =		sizeof(fsg_ss_bulk_out_comp_desc),
+	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
+	/* bMaxBurst set during fsg_bind() */
+};
+
+static struct usb_descriptor_header *fsg_ss_function[] = {
+#ifndef FSG_NO_OTG
+	(struct usb_descriptor_header *)&fsg_otg_desc,
+#endif
+	(struct usb_descriptor_header *)&fsg_intf_desc,
+	(struct usb_descriptor_header *)&fsg_ss_bulk_in_desc,
+	(struct usb_descriptor_header *)&fsg_ss_bulk_in_comp_desc,
+	(struct usb_descriptor_header *)&fsg_ss_bulk_out_desc,
+	(struct usb_descriptor_header *)&fsg_ss_bulk_out_comp_desc,
+	NULL,
+};

drivers/usb/gadget/storage_common.c +0 -0

 	if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH)
 		return hs;
 	return fs;

Signature · 1 line

2.53.0

Hi Thinh,

Thank you for the patch.

Quoted 50 lines · Thinh Nguyen

From: Dan Tran <[email protected]>

Add SS bulk endpoint descriptors (1024-byte MPS, bMaxBurst=15) to storage_common.c to support SuperSpeed connections. Extend fsg_ep_desc() to select them when operating at SuperSpeed, and wire up ss_descriptors in fsg_bind(). Free ss_descriptors in fsg_unbind() to match.

Signed-off-by Dan Tran <[email protected]>

Signed-off-by Thinh Nguyen <[email protected]>

---
Changes in v2:
- Removed internal Reviewed-by tags

 drivers/usb/gadget/f_mass_storage.c | 25 ++++++++++++--
 drivers/usb/gadget/storage_common.c | 52 ++++++++++++++++++++++++++++-
 2 files changed, 74 insertions(+), 3 deletions(-)

drivers/usb/gadget/f_mass_storage.c +5 -2

index 87ed25e8bb3a..a2f34c100482 100644
@@ -2225,14 +2225,16 @@ reset:
 
 	/* Enable the endpoints */
 	d = fsg_ep_desc(common->gadget,
-			&fsg_fs_bulk_in_desc, &fsg_hs_bulk_in_desc);
+			&fsg_fs_bulk_in_desc, &fsg_hs_bulk_in_desc,
+			&fsg_ss_bulk_in_desc);
 	rc = enable_endpoint(common, fsg->bulk_in, d);
 	if (rc)
 		goto reset;
 	fsg->bulk_in_enabled = 1;
 
 	d = fsg_ep_desc(common->gadget,
-			&fsg_fs_bulk_out_desc, &fsg_hs_bulk_out_desc);
+			&fsg_fs_bulk_out_desc, &fsg_hs_bulk_out_desc,
+			&fsg_ss_bulk_out_desc);
 	rc = enable_endpoint(common, fsg->bulk_out, d);
 	if (rc)
 		goto reset;

@@ -2653,6 +2655,7 @@ static void fsg_unbind(struct usb_configuration *c, struct usb_function *f)
 	fsg_common_release(fsg->common);
 	free(fsg->function.descriptors);
 	free(fsg->function.hs_descriptors);
+	free(fsg->function.ss_descriptors);
 	kfree(fsg);
 }
 

drivers/usb/gadget/f_mass_storage.c static int fsg_bind(struct usb_configuration *c, struct usb_function *f) · line 2709

Quoted 6 lines

drivers/usb/gadget/f_mass_storage.c +3 -0

@@ -2701,6 +2704,24 @@ static int fsg_bind(struct usb_configuration *c, struct usb_function *f)
 			return -ENOMEM;
 		}
 	}
+
+	if (gadget_is_superspeed(gadget)) {
+		unsigned int max_burst = min_t(unsigned int, FSG_BUFLEN / 1024, 15);

I can see that this looks similar to what we have in Linux with commit 4bb99b7c82ba ("usb: gadget: storage: add superspeed support")

However, the Linux patch added a comment as well:

(quoted diff) +2 -0

+               /* Calculate bMaxBurst, we know packet size is 1024 */
+               max_burst = min_t(unsigned, FSG_BUFLEN / 1024, 15);

Why can't we do the same here?

Quoted 73 lines

drivers/usb/gadget/f_mass_storage.c +15 -0

+
+		fsg_ss_bulk_in_desc.bEndpointAddress =
+			fsg_fs_bulk_in_desc.bEndpointAddress;
+		fsg_ss_bulk_in_comp_desc.bMaxBurst = max_burst;
+		fsg_ss_bulk_out_desc.bEndpointAddress =
+			fsg_fs_bulk_out_desc.bEndpointAddress;
+		fsg_ss_bulk_out_comp_desc.bMaxBurst = max_burst;
+		f->ss_descriptors = usb_copy_descriptors(fsg_ss_function);
+		if (unlikely(!f->ss_descriptors)) {
+			free(f->hs_descriptors);
+			free(f->descriptors);
+			return -ENOMEM;
+		}
+	}
+
 	return 0;
 
 autoconf_fail:

drivers/usb/gadget/storage_common.c +46 -0

index 7e4b542f7ce5..d745649eadb7 100644
@@ -531,11 +531,61 @@ static struct usb_descriptor_header *fsg_hs_function[] = {
 	NULL,
 };
 
+/*
+ * USB 3.0 requires SuperSpeed descriptors
+ */
+static struct usb_endpoint_descriptor
+fsg_ss_bulk_in_desc = {
+	.bLength =		USB_DT_ENDPOINT_SIZE,
+	.bDescriptorType =	USB_DT_ENDPOINT,
+
+	/* bEndpointAddress copied from fs_bulk_in_desc during fsg_bind() */
+	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
+	.wMaxPacketSize =	cpu_to_le16(1024),
+};
+
+static struct usb_ss_ep_comp_descriptor fsg_ss_bulk_in_comp_desc = {
+	.bLength =		sizeof(fsg_ss_bulk_in_comp_desc),
+	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
+	/* bMaxBurst set during fsg_bind() */
+};
+
+static struct usb_endpoint_descriptor
+fsg_ss_bulk_out_desc = {
+	.bLength =		USB_DT_ENDPOINT_SIZE,
+	.bDescriptorType =	USB_DT_ENDPOINT,
+
+	/* bEndpointAddress copied from fs_bulk_out_desc during fsg_bind() */
+	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
+	.wMaxPacketSize =	cpu_to_le16(1024),
+};
+
+static struct usb_ss_ep_comp_descriptor fsg_ss_bulk_out_comp_desc = {
+	.bLength =		sizeof(fsg_ss_bulk_out_comp_desc),
+	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
+	/* bMaxBurst set during fsg_bind() */
+};
+
+static struct usb_descriptor_header *fsg_ss_function[] = {
+#ifndef FSG_NO_OTG
+	(struct usb_descriptor_header *)&fsg_otg_desc,
+#endif
+	(struct usb_descriptor_header *)&fsg_intf_desc,
+	(struct usb_descriptor_header *)&fsg_ss_bulk_in_desc,
+	(struct usb_descriptor_header *)&fsg_ss_bulk_in_comp_desc,
+	(struct usb_descriptor_header *)&fsg_ss_bulk_out_desc,
+	(struct usb_descriptor_header *)&fsg_ss_bulk_out_comp_desc,
+	NULL,
+};

drivers/usb/gadget/storage_common.c static struct usb_descriptor_header *fsg_hs_function[] = { · line 588

Quoted 10 lines

drivers/usb/gadget/storage_common.c +5 -1

+
 /* Maxpacket and other transfer characteristics vary by speed. */
 static struct usb_endpoint_descriptor *
 fsg_ep_desc(struct usb_gadget *g, struct usb_endpoint_descriptor *fs,
-		struct usb_endpoint_descriptor *hs)
+		struct usb_endpoint_descriptor *hs,
+		struct usb_endpoint_descriptor *ss)
 {
+	if (g->speed >= USB_SPEED_SUPER)
+		return ss;

I can see that this looks similar to what we have in Linux with commit 4bb99b7c82ba ("usb: gadget: storage: add superspeed support")

However, Linux uses the following diff instead:

(quoted diff) +2 -0

+       if (gadget_is_superspeed(g) && g->speed == USB_SPEED_SUPER)
+               return ss;

Is there a reason for not doing the same here?

Quoted 6 lines

drivers/usb/gadget/storage_common.c +0 -0

 	if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH)
 		return hs;
 	return fs;

Signature · 1 line

2.53.0

Hi Mattijs,

Quoted 71 lines · Mattijs Korpershoek

Hi Thinh,

Thank you for the patch.

Quoted 50 lines · Thinh Nguyen

From: Dan Tran <[email protected]>

Add SS bulk endpoint descriptors (1024-byte MPS, bMaxBurst=15) to storage_common.c to support SuperSpeed connections. Extend fsg_ep_desc() to select them when operating at SuperSpeed, and wire up ss_descriptors in fsg_bind(). Free ss_descriptors in fsg_unbind() to match.

Signed-off-by Dan Tran <[email protected]>

Signed-off-by Thinh Nguyen <[email protected]>

---
Changes in v2:
- Removed internal Reviewed-by tags

 drivers/usb/gadget/f_mass_storage.c | 25 ++++++++++++--
 drivers/usb/gadget/storage_common.c | 52 ++++++++++++++++++++++++++++-
 2 files changed, 74 insertions(+), 3 deletions(-)

drivers/usb/gadget/f_mass_storage.c +5 -2

index 87ed25e8bb3a..a2f34c100482 100644
@@ -2225,14 +2225,16 @@ reset:
 
 	/* Enable the endpoints */
 	d = fsg_ep_desc(common->gadget,
-			&fsg_fs_bulk_in_desc, &fsg_hs_bulk_in_desc);
+			&fsg_fs_bulk_in_desc, &fsg_hs_bulk_in_desc,
+			&fsg_ss_bulk_in_desc);
 	rc = enable_endpoint(common, fsg->bulk_in, d);
 	if (rc)
 		goto reset;
 	fsg->bulk_in_enabled = 1;
 
 	d = fsg_ep_desc(common->gadget,
-			&fsg_fs_bulk_out_desc, &fsg_hs_bulk_out_desc);
+			&fsg_fs_bulk_out_desc, &fsg_hs_bulk_out_desc,
+			&fsg_ss_bulk_out_desc);
 	rc = enable_endpoint(common, fsg->bulk_out, d);
 	if (rc)
 		goto reset;

@@ -2653,6 +2655,7 @@ static void fsg_unbind(struct usb_configuration *c, struct usb_function *f)
 	fsg_common_release(fsg->common);
 	free(fsg->function.descriptors);
 	free(fsg->function.hs_descriptors);
+	free(fsg->function.ss_descriptors);
 	kfree(fsg);
 }
 

drivers/usb/gadget/f_mass_storage.c static int fsg_bind(struct usb_configuration *c, struct usb_function *f) · line 2709

Quoted 6 lines

drivers/usb/gadget/f_mass_storage.c +3 -0

@@ -2701,6 +2704,24 @@ static int fsg_bind(struct usb_configuration *c, struct usb_function *f)
 			return -ENOMEM;
 		}
 	}
+
+	if (gadget_is_superspeed(gadget)) {
+		unsigned int max_burst = min_t(unsigned int, FSG_BUFLEN / 1024, 15);

I can see that this looks similar to what we have in Linux with commit 4bb99b7c82ba ("usb: gadget: storage: add superspeed support")

However, the Linux patch added a comment as well:

(quoted diff) +2 -0

+               /* Calculate bMaxBurst, we know packet size is 1024 */
+               max_burst = min_t(unsigned, FSG_BUFLEN / 1024, 15);

Quoted 1 line

Why can't we do the same here?

The comment doesn't add much beyond what the code already expresses. The variable name max_burst and the division by 1024 make the intent clear.

That said, the Linux comment also has a minor inaccuracy: it says "packet size" when it should say "max packet size". If a comment is warranted, I'd prefer to add a corrected one. We can add it if you really think it helps with readability.

Quoted 21 lines

Quoted 21 lines

drivers/usb/gadget/f_mass_storage.c +15 -0

+
+		fsg_ss_bulk_in_desc.bEndpointAddress =
+			fsg_fs_bulk_in_desc.bEndpointAddress;
+		fsg_ss_bulk_in_comp_desc.bMaxBurst = max_burst;
+		fsg_ss_bulk_out_desc.bEndpointAddress =
+			fsg_fs_bulk_out_desc.bEndpointAddress;
+		fsg_ss_bulk_out_comp_desc.bMaxBurst = max_burst;
+		f->ss_descriptors = usb_copy_descriptors(fsg_ss_function);
+		if (unlikely(!f->ss_descriptors)) {
+			free(f->hs_descriptors);
+			free(f->descriptors);
+			return -ENOMEM;
+		}
+	}
+
 	return 0;
 
 autoconf_fail:

drivers/usb/gadget/storage_common.c +0 -0

Quoted 17 lines

drivers/usb/gadget/storage_common.c static struct usb_descriptor_header *fsg_hs_function[] = { · line 588

Quoted 13 lines

drivers/usb/gadget/storage_common.c +8 -1

+	(struct usb_descriptor_header *)&fsg_ss_bulk_out_comp_desc,
+	NULL,
+};
+
 /* Maxpacket and other transfer characteristics vary by speed. */
 static struct usb_endpoint_descriptor *
 fsg_ep_desc(struct usb_gadget *g, struct usb_endpoint_descriptor *fs,
-		struct usb_endpoint_descriptor *hs)
+		struct usb_endpoint_descriptor *hs,
+		struct usb_endpoint_descriptor *ss)
 {
+	if (g->speed >= USB_SPEED_SUPER)
+		return ss;

I can see that this looks similar to what we have in Linux with commit 4bb99b7c82ba ("usb: gadget: storage: add superspeed support")

Quoted 5 lines

However, Linux uses the following diff instead:

(quoted diff) +2 -0

+       if (gadget_is_superspeed(g) && g->speed == USB_SPEED_SUPER)
+               return ss;

Is there a reason for not doing the same here?

The gadget_is_superspeed() is a hardware capability check. It's redundant when we're already checking g->speed for connected speed.

BR,
Thinh

Quoted 6 lines

Quoted 6 lines

drivers/usb/gadget/storage_common.c +0 -0

 	if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH)
 		return hs;
 	return fs;

Signature · 1 line

2.53.0

From: Dan Tran <[email protected]>

Populate ss_descriptors to support SuperSpeed connections. DFU is control-only so no separate SS descriptor set is needed; reuse the same descriptors across all speeds.

Signed-off-by Dan Tran <[email protected]>

Signed-off-by Thinh Nguyen <[email protected]>

---
Changes in v2:
- Removed internal Reviewed-by tags

 drivers/usb/gadget/f_dfu.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/usb/gadget/f_dfu.c +2 -0

index ca8b36e077bc..68d2ec7ebbe8 100644
@@ -227,6 +227,7 @@ static inline void to_dfu_mode(struct f_dfu *f_dfu)
 	f_dfu->usb_function.strings = dfu_strings;
 	f_dfu->usb_function.hs_descriptors = f_dfu->function;
 	f_dfu->usb_function.descriptors = f_dfu->function;
+	f_dfu->usb_function.ss_descriptors = f_dfu->function;
 	f_dfu->dfu_state = DFU_STATE_dfuIDLE;
 }
 

@@ -235,6 +236,7 @@ static inline void to_runtime_mode(struct f_dfu *f_dfu)
 	f_dfu->usb_function.strings = NULL;
 	f_dfu->usb_function.hs_descriptors = dfu_runtime_descs;
 	f_dfu->usb_function.descriptors = dfu_runtime_descs;
+	f_dfu->usb_function.ss_descriptors = dfu_runtime_descs;
 }
 
 static int handle_upload(struct usb_request *req, u16 len)

drivers/usb/gadget/f_dfu.c +1 -0

 error:
 	return rv;
 }

@@ -826,6 +831,7 @@ static int dfu_bind_config(struct usb_configuration *c)
 	f_dfu->usb_function.name = "dfu";
 	f_dfu->usb_function.hs_descriptors = dfu_runtime_descs;
 	f_dfu->usb_function.descriptors = dfu_runtime_descs;
+	f_dfu->usb_function.ss_descriptors = dfu_runtime_descs;
 	f_dfu->usb_function.bind = dfu_bind;
 	f_dfu->usb_function.unbind = dfu_unbind;
 	f_dfu->usb_function.set_alt = dfu_set_alt;

Signature · 1 line

2.53.0

Hi Thinh,

Thank you for the patch.

Quoted 39 lines · Thinh Nguyen

From: Dan Tran <[email protected]>

Populate ss_descriptors to support SuperSpeed connections. DFU is control-only so no separate SS descriptor set is needed; reuse the same descriptors across all speeds.

Signed-off-by Dan Tran <[email protected]>

Signed-off-by Thinh Nguyen <[email protected]>

---
Changes in v2:
- Removed internal Reviewed-by tags

 drivers/usb/gadget/f_dfu.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/usb/gadget/f_dfu.c +2 -0

index ca8b36e077bc..68d2ec7ebbe8 100644
@@ -227,6 +227,7 @@ static inline void to_dfu_mode(struct f_dfu *f_dfu)
 	f_dfu->usb_function.strings = dfu_strings;
 	f_dfu->usb_function.hs_descriptors = f_dfu->function;
 	f_dfu->usb_function.descriptors = f_dfu->function;
+	f_dfu->usb_function.ss_descriptors = f_dfu->function;
 	f_dfu->dfu_state = DFU_STATE_dfuIDLE;
 }
 

@@ -235,6 +236,7 @@ static inline void to_runtime_mode(struct f_dfu *f_dfu)
 	f_dfu->usb_function.strings = NULL;
 	f_dfu->usb_function.hs_descriptors = dfu_runtime_descs;
 	f_dfu->usb_function.descriptors = dfu_runtime_descs;
+	f_dfu->usb_function.ss_descriptors = dfu_runtime_descs;
 }
 
 static int handle_upload(struct usb_request *req, u16 len)

@@ -752,6 +754,9 @@ static int dfu_bind(struct usb_configuration *c, struct usb_function *f)
 	if (s)
 		g_dnl_set_serialnumber((char *)s);
 

drivers/usb/gadget/f_dfu.c static int dfu_bind(struct usb_configuration *c, struct usb_function *f) · line 759

Quoted 3 lines

drivers/usb/gadget/f_dfu.c +3 -0

+	if (gadget_is_superspeed(cdev->gadget))
+		f_dfu->usb_function.ss_descriptors = f_dfu->function;
+

Can you explain why this conditional is needed? I don't see something similar for high speed so this is confusing me.