On Mon, Jan 13, 2025 at 7:46 PM Junzhong Pan <panjunzhong@xxxxxxxxxxx> wrote: > > Hi Maciej, > > Thanks for your quick reply. > > On 2025/1/14 3:22, Maciej Żenczykowski Wrote: > > Looking at https://github.com/microsoft/NCM-Driver-for-Windows > > > > commit ded4839c5103ab91822bfde1932393bbb14afda3 (tag: > > windows_10.0.22000, origin/master) > > Author: Brandon Jiang <jiangyue@xxxxxxxxxxxxx> > > Date: Mon Oct 4 14:30:44 2021 -0700 > > > > update NCM to Windows 11 (21H2) release, built with Windows 11 > > (22000) WDK and DMF v1.1.82 > > > > -- vs previous change to host/device.cpp > > > > commit 40118f55a0843221f04c8036df8b97fa3512a007 (tag: > > windows_10.0.19041, origin/release_2004) > > Author: Brandon Jiang <jiangyue@xxxxxxxxxxxxx> > > Date: Sun Feb 23 19:53:06 2020 -0800 > > > > update NCM to 20H1 Windows release, built with 20H1 WDK and DMF v1.1.20 > > > > it introduced > > > > if (bufferRequest->TransferLength < bufferRequest->BufferLength && > > bufferRequest->TransferLength % > > hostDevice->m_DataBulkOutPipeMaximumPacketSize == 0) > > { > > //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, > > In NCM10, 3.2.2 dwBlockLength description, it states: > > If exactly dwNtbInMaxSize or dwNtbOutMaxSize bytes are sent, and the > > size is a multiple of wMaxPacketSize for the given pipe, then no ZLP > > shall be sent. But the Linux ncm gadget driver sets both of those (dwNtbIn/OutMaxSize) to 16 kiB (I can never remember which one is relevant to which direction, I think in this case it's 'In' cause it's relevant to the gadget/device and thus affects what is sent by Windows and parsed by Linux). So with 15.5 kiB this is not relevant, right? (Please correct me if I'm wrong) Furthermore that 16 kiB is also the size of the preallocated receive buffer it passes to the usb stack, so there won't be a problem without ZLP (post 16 kiB xfer), because the buffer will naturally terminate. > I don't know if its a Microsoft's problem or really **not explicit**. I *think* (in this case) this is very much an MS problem (and the fix in the newer driver confirms it). Short packet / ZLP termination is simply how USB works to transfer packets... Unfortunately MS is not the only one with problems with ZLP. See Linux's drivers/net/usb/cdc_ncm.c 'NO ZLP' vs 'SEND ZLP' and the FLAG_SEND_ZLP. and note it's set on Apple tethering... [I think your quote up above is exactly why the standard requires 'NO ZLP' operation] So there's very clearly ample confusion here... > Maybe most of the device implementations treat the incoming data as a > stream and do contiguous parsing on it. > > > //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 > > > > bufferRequest->Buffer[bufferRequest->TransferLength] = 0; > > bufferRequest->TransferLength++; > > } > > > > Which I think is literally the fix for this bug you're reporting. > > That 'fix' is what then caused us to add the patch at the top of this thread. > > > > So that fix was present in the very first official Win11 release > > (build 22000), but is likely not present in any Win10 release... > > As you mentioned before to fix it in the gadget side, it seems very > complicated, maybe we need a extra skb with size=ntb_size as an intermediate > buffer to move around those ntb data before parsing it, but may (or may not) > lead to performance drop. Any other idea? It would definitely lead to a fair bit of code complication, and in the particular case of this happening it would involve (at least) an extra copy (to linearize), so definitely be a performance hit. I think we'd have to have a potential extra buffer/offset/length. Initially it would be null/0/0. Whenever we receive a frame and parsing it leaves us with leftover bytes, we'd have to allocate this buffer, and copy the leftover stuff into this temporary area. Try to parse it (note: potentially repeatedly, because we could have 8 2kiB merged pkts...) and swallow the part that parsed, but if the buffer is too short, then hold on to it until we receive more data. If we ever manage to fully parse it - we could potentially deallocate it (or hold on to the memory to avoid multiple alloc/deallocs). When we receive a usb xfer, if the buffer already exists (or is non zero size), the new xfer needs to be appended to it, and parsing repeats. This btw. implies this needs to be a 32 kiB (2*16) buffer... vmalloc would be fine. I think we'd likely need to get rid of the way this stuff abuses skbs for usb anyway. I've wanted to do this anyway (note: not sure I've seen this in the gadget or host side ncm driver). Ugh... > Do you think hacking in the gadget side to fix this compatible issue is > a good idea consider that there are still a large number of users using > Win10? I'm still thinking about it, but I'd far prefer for MS to fix their Win10 driver. This just seems really hard to fix in the gadget. > (Though Win10 will reach end of support on October 14, 2025, Far longer than that. Since there's (purchasable) extended support (+2 years), and non consumer Win10 EOL is further out as well (Enterprise is Jan 2027, business/school Oct 2028, IoT Jan 2032, etc). > but people may still use it for a long time Yeah, Win10 will be around for many many years to come... > since many PCs in good condition cannot install win11.) Or people don't want to, even when they could :-) I personally have a win11 capable PC that I'm refusing to upgrade... (and a pair that are too old) This is only the *second* time Win11 actually has something (beyond what's in Win10) I could potentially maybe actually want (the first being related to WSL, though I've since stopped using it). I miss XP with classic UI (maybe Win7 was better? not sure).
