Re: [PATCH] usb: gadget: ncm: Avoid dropping datagrams of properly parsed NTBs

[Krishna Kurapati PSSNV]

On 1/5/2024 2:48 AM, Jack Pham wrote:
> On Tue, Jan 02, 2024 at 05:04:01PM +0530, Krishna Kurapati PSSNV wrote:
> > > > The above might work. But just wanted to check why this 1 byte would
> > > > come actually ? Any reason for this ? ZLP must not give a 1 byte packet
> > > > of 1 byte AFAIK.
> > >
> > > I'm not a USB expert, but... my (possibly wrong) understanding is:
> > > (note I may be using bad terminology... also the 1024/16384 constants
> > > are USB3 specific, USB2 has afaik max 512 not 1024, I think USB1 is
> > > even 64, but it's likely too old to matter, etc.)
> > >
> > > USB3 payloads can be up to 16384 bytes in size,
> > > on the wire they are split up into packets of between 0 and 1024 bytes.
> > > [a Zero Length Packet is a ZLP]
> > > A usb payload is terminated with a usb packet of < 1024 bytes.
> > >
> > > So a 1524 byte payload would be sent as 2 packets 1024 + 500.
> > > While a 2048 byte payload would be sent as 3 packets 1024 + 1024 + 0 (ie. ZLP)
> > >
> > > A 16384 byte payload could be sent as 16 * 1024 + ZLP,
> > > but since 16384 is the max you might be able to get away with just 16
> > > * 1024 and skip the ZLP...
> > >
> > > I think this is why the Linux usb code base has ZLP / NO_ZLP quirks.
> > > [but do note I may be wrong, I haven't gone looking at what exactly
> > > the zlp quirks do,
> > > not even sure if they're receive or transmit side... or both]
> > >
> > > Different hardware/usb chipsets/etc have different behaviour wrt. ZLPs.
> > >
> > > In general it seems like what needs to happen is much clearer if you
> > > just avoid the need for ZLPs entirely.
> > > I think that's what windows is trying to do here: avoid ever sending a
> > > usb payload with a multiple of 1024 bytes,
> > > so it never has to send ZLPs. This seems easy enough to do...
> > > limit max to 16383 (not 16384) and add 1 byte of zero pad if the
> > > payload ends up being a multiple of 1024.
> >
> > Got it. Thanks for the explanation. Atleast this gives me an insight into
> > what might be the problem.
>
> Hooray to MS for having open-sourced a reference version of their NCM
> driver on GitHub (under MIT license)--and I think this might explain it:
>
> https://github.com/microsoft/NCM-Driver-for-Windows/blob/release_21H2/host/device.cpp#L902
>
> which states in a comment (pasted line-wrapped for mail-friendliness)
>
>          //NCM spec is not explicit if a ZLP shall be sent when
>          //wBlockLength != 0 and it happens to be
>          //multiple of wMaxPacketSize. Our interpretation is that no ZLP
>          //needed if wBlockLength is non-zero,
>          //because the non-zero wBlockLength has already told the
>          //function side the size of transfer to be expected.
>          //
>          //However, there are in-market NCM devices rely on ZLP as long
>          //as the wBlockLength is multiple of wMaxPacketSize.
>          //To deal with such devices, we pad an extra 0 at end so the
>          //transfer is no longer multiple of wMaxPacketSize
>
> If so then would be worth calling this out in commit text and/or code
> comment.

Thanks for the inputs Jack. Will make sure to add it in commit text clearly.

Regards,
Krishna,