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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jan 13, 2025 at 5:31 AM Junzhong Pan <panjunzhong@xxxxxxxxxxx> wrote:
>
> Hi Maciej,
>
> On 2025/1/13 1:49, Maciej Żenczykowski Wrote:> (a) I think this looks like a bug on the sending Win10 side, rather
> > than a parsing bug in Linux,
> > with there being no ZLP, and no short (<512) frame, there's simply no
> > way for the receiver to split at the right spot.
> >
> > Indeed, fixing this on the Linux/parsing side seems non-trivial...
> > I guess we could try to treat the connection as simply a serial
> > connection (ie. ignore frame boundaries), but then we might have
> > issues with other senders...
> >
> > I guess the most likely 'correct' hack/fix would be to hold on to the
> > extra 'N*512' bytes (it doesn't even have to be 1, though likely the N
> > is odd), if it starts with a NTH header...Make sence, it seems we only need to save the rest data beside
> dwBlockLength for next unwrap if a hack is acceptable, otherwise I may
> need to check if a custom host driver for Windows10 user feasible.
>
> I didn't look carefully into the 1byte and padding stuff with Windows11
> host yet, I will take a look then.
>
> > (b) I notice the '512' not '1024', I think this implies a USB2
> > connection instead of USB3
> > -- could you try replicating this with a USB3 capable data cable (and
> > USB3 ports), this should result in 1024 block size instead of 512.
> >
> > I'm wondering if the win10 stack is avoiding generating N*1024, but
> > then hitting N*512 with odd N...Yes, I am using USB2.0 connection to better capture the crime scene.
>
> Normally the OUT transfer on USB3.0 SuperSpeed connection comes with a bunch
> of 1024B Data Pakcet along with a Short Packet less than 1024B in the end from
> the Lecroy trace.
>
> It's also reproducible on USB3.0 SuperSpeed connection using dwc3 controller,
> but it will cost more time and make it difficult to capture the online data
> (limited tracer HW buffer), I can try using software tracing or custom logs
> later:
>
> [  5]  26.00-27.00  sec   183 MBytes  1.54 Gbits/sec
> [  5]  27.00-28.00  sec   182 MBytes  1.53 Gbits/sec
> [  206.123935] configfs.gadget.2: Wrong NDP SIGN
> [  206.129785] configfs.gadget.2: Wrong NTH SIGN, skblen 12208
> [  206.136802] HEAD:0000000004f66a88: 80 06 bc f9 c0 a8 24 66 c0 a8 24 65 f7 24 14 51 aa 1a 30 d5 01 f8 01 26 50 10 20 14 27 3d 00 00
> [  5]  28.00-29.00  sec   128 MBytes  1.07 Gbits/sec
> [  5]  29.00-30.00  sec   191 MBytes  1.61 Gbits/sec>
> > Presumably '512' would be '64' with USB1.0/1.1, but I guess finding a
> > USB1.x port/host to test against is likely to be near impossible...
> >
> > I'll try to see if I can find the source of the bug in the Win
> > driver's sources (though based on it being Win10 only, may need to
> > search history)
> > It's great if you can analyze from the host driver.
> I didn't know if the NCM driver open-sourced on github by M$ is the correspond
> version. They said that only Win 11 officially support NCM in the issue on github
> yet they do have a built-in driver in Win10 and 2004 tag there in the repo.

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

https://en.wikipedia.org/wiki/Windows_10_version_history (2004 - 20H1
- May 2020 Update - 19041 - May 27, 2020)
https://en.wikipedia.org/wiki/Windows_11 (first version is 21H2 - Sun
Valley - 22000 - October 5, 2021)





[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux