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

[Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>]

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