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

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

Hi Thinh,

On Thu, Jul 23, 2026 at 20:40, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:

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

Overall, my rule of thumb is "keep the code as close as possible to the
Linux driver to ease maintenance in U-Boot".

Please consider adding the corrected comment for v3.

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

[...]

>> > +	(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.

So does that mean that the dualspeed conditional just below is doing a
redundant check as well?

As for the previous comment, I'd prefer if we can stay closer to the
Linux code.

If we can't, I'd like to see a strong justification for not doing so.

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