Received-SPF: pass (google.com: domain of linux-kernel+bounces-17470-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <50e7cf06-dc3c-4324-9a5d-d82bec9cca89@quicinc.com>
Date: Fri, 5 Jan 2024 10:11:53 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] usb: gadget: ncm: Avoid dropping datagrams of properly
 parsed NTBs
To: Jack Pham <quic_jackp@quicinc.com>,
        =?UTF-8?Q?Maciej_=C5=BBenczykowski?=
	<maze@google.com>
CC: 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>
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>
 <20240104211858.GB4127689@hu-jackp-lv.qualcomm.com>
Content-Language: en-US
From: Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
In-Reply-To: <20240104211858.GB4127689@hu-jackp-lv.qualcomm.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


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,