Re: [PATCH] usb: gadget: f_ncm: allow using NCM in SuperSpeed Plus gadgets.

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2488 bytes --]

Lorenzo Colitti <lorenzo@google.com> writes:

> On Wed, Aug 5, 2020 at 7:21 PM Felipe Balbi <balbi@kernel.org> wrote:
>> But anyway, if we change 13 to 16, then we 1Gbps. How did you get
>> 1.7Gbps? I think we should really update ncm_bitrate() to contain the
>> correct equations for bitrate on different speeds.
>
> I got 1.7 Gbps because that's what I measured in iperf. :-)
>
> But actually after reading the spec I think that for SuperSpeed and
> above that calculation is meaningless, because bulk transfers are no
> longer limited by a set number of packets per microframe. The USB 3.2
> spec has mostly replaced the words "microframe" with "bus interval",
> but I don't see any place in the spec that says the number of packets
> per bus interval is limited. Section 8.12.1.2 (Bulk IN Transactions)
> just says that "when the host is ready to receive bulk data, it sends
> an ACK TP" saying how many packets it's willing to accept, where the
> maximum is the burst size supported by the endpoint. After that, the
> host has to respond with an ACK to every data packet received, and
> every ACK specifies the number of data packets it expects from then
> on.
>
> It seems more correct that for SS and above the driver should simply
> just report the link speed minus theoretical bus overhead? Section
> 4.4.11 suggests that's about 10%, so it would announce 4.5 Gbps.

makes sense to me :-)

>> > +static struct usb_ss_ep_comp_descriptor ssp_ncm_bulk_comp_desc = {
>> > +     .bLength =              sizeof(ssp_ncm_bulk_comp_desc),
>> > +     .bDescriptorType =      USB_DT_SS_ENDPOINT_COMP,
>> > +
>> > +     /* the following 2 values can be tweaked if necessary */
>> > +     /* .bMaxBurst =         0, */
>>
>> should you add bMaxBurst = 15 here?
>
> I'm not sure. On my setup, it provides a fair performance boost (goes
> from ~1.7Gbps to ~2.3Gbps in, and ~620Mbps to ~720Mbps out). But I
> don't know whether there might be any compatibility constraints or
> hardware dependencies. I do see that the f_mass_storage driver sets it
> to 15:

there shouldn't be any compatibility issues, no.

>          /* Calculate bMaxBurst, we know packet size is 1024 */
>         max_burst = min_t(unsigned, FSG_BUFLEN / 1024, 15);
>
> so perhaps this is fine to do in NCM too? If we want to set bMaxBurst
> to 15, should that be in this patch, or in a separate patch?

yup, should be fine. A separate patch is okay too :-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]