Received-SPF: pass (google.com: domain of linux-kernel+bounces-17229-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 4 Jan 2024 13:18:58 -0800
From: Jack Pham <quic_jackp@quicinc.com>
To: Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
CC: Maciej =?utf-8?Q?=C5=BBenczykowski?= <maze@google.com>,
        Greg Kroah-Hartman
	<gregkh@linuxfoundation.org>,
        Hardik Gajjar <hgajjar@de.adit-jv.com>, <linux-usb@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <quic_ppratap@quicinc.com>,
        <quic_wcheng@quicinc.com>
Subject: Re: [PATCH] usb: gadget: ncm: Avoid dropping datagrams of properly
 parsed NTBs
Message-ID: <20240104211858.GB4127689@hu-jackp-lv.qualcomm.com>
References: <20240102055143.3889-1-quic_kriskura@quicinc.com>
 <CANP3RGeirg+f8cBbw_3YR5AvuB1ZxJC_9-wcn+Tb-GXf1ESKCQ@mail.gmail.com>
 <ad60f399-5c6a-4f16-8c28-f4d4e0fde1ff@quicinc.com>
 <CANP3RGf5dg14DNuKOn9pqWd4oSBDsPhwwBB7AJ0c3qHbDT0sBQ@mail.gmail.com>
 <572c21f8-e642-4d30-84aa-673051be6bb4@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <572c21f8-e642-4d30-84aa-673051be6bb4@quicinc.com>

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.

Jack