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 <[email protected]> wrote:
>
> > 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(-)
> >
> > 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
