Re: [PATCH v2 1/2] usb: gadget: mass_storage: add SuperSpeed descriptor support

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

Hi Mattijs,

On Thu, Jul 23, 2026, Mattijs Korpershoek wrote:
> Hi Thinh,
> 
> Thank you for the patch.
> 
> On Thu, Jul 09, 2026 at 18:19, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> 
> > From: Dan Tran <trandan@synopsys.com>
> >
> > 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 <trandan@synopsys.com>
> > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> > ---
> >  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(-)
> >
> > diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
> > index 87ed25e8bb3a..a2f34c100482 100644
> > --- a/drivers/usb/gadget/f_mass_storage.c
> > +++ b/drivers/usb/gadget/f_mass_storage.c
> > @@ -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);
> >  }
> >  
> > @@ -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:
> 
> +               /* 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?

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.

> 
> > +
> > +		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:
> > diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c
> > index 7e4b542f7ce5..d745649eadb7 100644
> > --- a/drivers/usb/gadget/storage_common.c
> > +++ b/drivers/usb/gadget/storage_common.c
> > @@ -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,
> > +};
> > +
> >  /* 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:
> +       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

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