Received-SPF: pass (google.com: domain of linux-kernel+bounces-14297-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <572c21f8-e642-4d30-84aa-673051be6bb4@quicinc.com>
Date: Tue, 2 Jan 2024 17:04:01 +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: =?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>, <quic_jackp@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>
Content-Language: en-US
From: Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
In-Reply-To: <CANP3RGf5dg14DNuKOn9pqWd4oSBDsPhwwBB7AJ0c3qHbDT0sBQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


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

>>> It seems a little dangerous to just blindly ignore arbitrary amounts
>>> of trailing garbage...
>>
>> Yes. I agree, which is why I put a note in comment section of patch
>> stating that this doesn't cover all cases, just the ones found in the
>> testing so far. But the code suggestion you provided might actually work
>> out. So something like the following ?
>>
>> if (to_process == 1) && (block_len%1024 == 0) && (*payload == 0)
> 
> Assuming it compiles and works ;-) I wrote this without looking at the code. >

I will check and put a v2 with the proper check.

> I'm guessing this needs to be %512 for usb2...
> Do we know if we're connected via usb2 or usb3?
> [mayhaps there's some field that already stores this 1024 constant...]
> If not... should we just check for %512 instead to support both usb2 and usb3?
> 
>>       // extra 1 zero byte pad to prevent multiple of 1024 sized packet
>>       return
>> } else if (to_process > 1) {
> 
> this should likely continue to be != 0 or > 0
> 
>>       goto parse_ntb;
>> }
Ok, will make it (> 0)

BTW, Totally, off the conversation, can you also review: 
https://lore.kernel.org/all/20231221153216.18657-1-quic_kriskura@quicinc.com/

I made changes in this v2 as per comments on v1.

Regards,
Krishna,