It is observed sometimes when tethering is used over NCM with Windows 11
as host, at some instances, the gadget_giveback has one byte appended at
the end of a proper NTB. When the NTB is parsed, unwrap call looks for
any leftover bytes in SKB provided by u_ether and if there are any pending
bytes, it treats them as a separate NTB and parses it. But in case the
second NTB (as per unwrap call) is faulty/corrupt, all the datagrams that
were parsed properly in the first NTB and saved in rx_list are dropped.
Adding a few custom traces showed the following:
[002] d..1 7828.532866: dwc3_gadget_giveback: ep1out:
req 000000003868811a length 1025/16384 zsI ==> 0
[002] d..1 7828.532867: ncm_unwrap_ntb: K: ncm_unwrap_ntb toprocess: 1025
[002] d..1 7828.532867: ncm_unwrap_ntb: K: ncm_unwrap_ntb nth: 1751999342
[002] d..1 7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb seq: 0xce67
[002] d..1 7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb blk_len: 0x400
[002] d..1 7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb ndp_len: 0x10
[002] d..1 7828.532869: ncm_unwrap_ntb: K: Parsed NTB with 1 frames
In this case, the giveback is of 1025 bytes and block length is 1024.
The rest 1 byte (which is 0x00) won't be parsed resulting in drop of
all datagrams in rx_list.
Same is case with packets of size 2048:
[002] d..1 7828.557948: dwc3_gadget_giveback: ep1out:
req 0000000011dfd96e length 2049/16384 zsI ==> 0
[002] d..1 7828.557949: ncm_unwrap_ntb: K: ncm_unwrap_ntb nth: 1751999342
[002] d..1 7828.557950: ncm_unwrap_ntb: K: ncm_unwrap_ntb blk_len: 0x800
Lecroy shows one byte coming in extra confirming that the byte is coming
in from PC:
Transfer 2959 - Bytes Transferred(1025) Timestamp((18.524 843 590)
- Transaction 8391 - Data(1025 bytes) Timestamp(18.524 843 590)
--- Packet 4063861
Data(1024 bytes)
Duration(2.117us) Idle(14.700ns) Timestamp(18.524 843 590)
--- Packet 4063863
Data(1 byte)
Duration(66.160ns) Time(282.000ns) Timestamp(18.524 845 722)
Fix this by checking if the leftover bytes before parsing next NTB is of
size more than the expected header.
Fixes: 427694cfaafa ("usb: gadget: ncm: Handle decoding of multiple NTB's in unwrap call")
Signed-off-by: Krishna Kurapati <[email protected]>
---
There could probably be cases where the first NTB is proper and the second
NTB's header is proper but the NDP is corrupt, and in those cases too, all
the datagrams are dropped. But I haven't seen such case practically.
drivers/usb/gadget/function/f_ncm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
index cc0ed29a4adc..a75b6dc8b0cb 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -1325,7 +1325,7 @@ static int ncm_unwrap_ntb(struct gether *port,
"Parsed NTB with %d frames\n", dgram_counter);
to_process -= block_len;
- if (to_process != 0) {
+ if (to_process > opts->nth_size) {
ntb_ptr = (unsigned char *)(ntb_ptr + block_len);
goto parse_ntb;
}
--
2.42.0
On Tue, Jan 02, 2024 at 11:21:43AM +0530, Krishna Kurapati wrote:
> It is observed sometimes when tethering is used over NCM with Windows 11
> as host, at some instances, the gadget_giveback has one byte appended at
> the end of a proper NTB. When the NTB is parsed, unwrap call looks for
> any leftover bytes in SKB provided by u_ether and if there are any pending
> bytes, it treats them as a separate NTB and parses it. But in case the
> second NTB (as per unwrap call) is faulty/corrupt, all the datagrams that
> were parsed properly in the first NTB and saved in rx_list are dropped.
>
> Adding a few custom traces showed the following:
>
> [002] d..1 7828.532866: dwc3_gadget_giveback: ep1out:
> req 000000003868811a length 1025/16384 zsI ==> 0
> [002] d..1 7828.532867: ncm_unwrap_ntb: K: ncm_unwrap_ntb toprocess: 1025
> [002] d..1 7828.532867: ncm_unwrap_ntb: K: ncm_unwrap_ntb nth: 1751999342
> [002] d..1 7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb seq: 0xce67
> [002] d..1 7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb blk_len: 0x400
> [002] d..1 7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb ndp_len: 0x10
> [002] d..1 7828.532869: ncm_unwrap_ntb: K: Parsed NTB with 1 frames
>
> In this case, the giveback is of 1025 bytes and block length is 1024.
> The rest 1 byte (which is 0x00) won't be parsed resulting in drop of
> all datagrams in rx_list.
>
> Same is case with packets of size 2048:
> [002] d..1 7828.557948: dwc3_gadget_giveback: ep1out:
> req 0000000011dfd96e length 2049/16384 zsI ==> 0
> [002] d..1 7828.557949: ncm_unwrap_ntb: K: ncm_unwrap_ntb nth: 1751999342
> [002] d..1 7828.557950: ncm_unwrap_ntb: K: ncm_unwrap_ntb blk_len: 0x800
>
> Lecroy shows one byte coming in extra confirming that the byte is coming
> in from PC:
>
> Transfer 2959 - Bytes Transferred(1025) Timestamp((18.524 843 590)
> - Transaction 8391 - Data(1025 bytes) Timestamp(18.524 843 590)
> --- Packet 4063861
> Data(1024 bytes)
> Duration(2.117us) Idle(14.700ns) Timestamp(18.524 843 590)
> --- Packet 4063863
> Data(1 byte)
> Duration(66.160ns) Time(282.000ns) Timestamp(18.524 845 722)
>
> Fix this by checking if the leftover bytes before parsing next NTB is of
> size more than the expected header.
>
> Fixes: 427694cfaafa ("usb: gadget: ncm: Handle decoding of multiple NTB's in unwrap call")
> Signed-off-by: Krishna Kurapati <[email protected]>
> ---
> There could probably be cases where the first NTB is proper and the second
> NTB's header is proper but the NDP is corrupt, and in those cases too, all
> the datagrams are dropped. But I haven't seen such case practically.
>
> drivers/usb/gadget/function/f_ncm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- You have marked a patch with a "Fixes:" tag for a commit that is in an
older released kernel, yet you do not have a cc: stable line in the
signed-off-by area at all, which means that the patch will not be
applied to any older kernel releases. To properly fix this, please
follow the documented rules in the
Documentation/process/stable-kernel-rules.rst file for how to resolve
this.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
en
On Mon, Jan 1, 2024 at 9:52 PM Krishna Kurapati
<[email protected]> wrote:
>
> It is observed sometimes when tethering is used over NCM with Windows 11
> as host, at some instances, the gadget_giveback has one byte appended at
> the end of a proper NTB. When the NTB is parsed, unwrap call looks for
> any leftover bytes in SKB provided by u_ether and if there are any pending
> bytes, it treats them as a separate NTB and parses it. But in case the
> second NTB (as per unwrap call) is faulty/corrupt, all the datagrams that
> were parsed properly in the first NTB and saved in rx_list are dropped.
I think this is likely Windows trying to avoid generating 0 length frames.
(usb max single datagram [frame?] size is 1024 bytes).
My guess is this extra byte will only ever happen at the end of a
multiple of 1024 bytes,
and it will always be exactly one byte, and it will likely be a '0' pad byte.
Could you check if a more specific test of this sort would make sense?
(ie. fix the problem)
Something like
if (to_process == 1) && (current_offset & 1023 == 0) && (*payload == 0)
// extra 1 zero byte pad to prevent multiple of 1024 sized packet
return
}
It seems a little dangerous to just blindly ignore arbitrary amounts
of trailing garbage...
>
> Adding a few custom traces showed the following:
>
> [002] d..1 7828.532866: dwc3_gadget_giveback: ep1out:
> req 000000003868811a length 1025/16384 zsI ==> 0
> [002] d..1 7828.532867: ncm_unwrap_ntb: K: ncm_unwrap_ntb toprocess: 1025
> [002] d..1 7828.532867: ncm_unwrap_ntb: K: ncm_unwrap_ntb nth: 1751999342
> [002] d..1 7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb seq: 0xce67
> [002] d..1 7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb blk_len: 0x400
> [002] d..1 7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb ndp_len: 0x10
> [002] d..1 7828.532869: ncm_unwrap_ntb: K: Parsed NTB with 1 frames
>
> In this case, the giveback is of 1025 bytes and block length is 1024.
> The rest 1 byte (which is 0x00) won't be parsed resulting in drop of
> all datagrams in rx_list.
>
> Same is case with packets of size 2048:
> [002] d..1 7828.557948: dwc3_gadget_giveback: ep1out:
> req 0000000011dfd96e length 2049/16384 zsI ==> 0
> [002] d..1 7828.557949: ncm_unwrap_ntb: K: ncm_unwrap_ntb nth: 1751999342
> [002] d..1 7828.557950: ncm_unwrap_ntb: K: ncm_unwrap_ntb blk_len: 0x800
>
> Lecroy shows one byte coming in extra confirming that the byte is coming
> in from PC:
>
> Transfer 2959 - Bytes Transferred(1025) Timestamp((18.524 843 590)
> - Transaction 8391 - Data(1025 bytes) Timestamp(18.524 843 590)
> --- Packet 4063861
> Data(1024 bytes)
> Duration(2.117us) Idle(14.700ns) Timestamp(18.524 843 590)
> --- Packet 4063863
> Data(1 byte)
> Duration(66.160ns) Time(282.000ns) Timestamp(18.524 845 722)
>
> Fix this by checking if the leftover bytes before parsing next NTB is of
> size more than the expected header.
>
> Fixes: 427694cfaafa ("usb: gadget: ncm: Handle decoding of multiple NTB's in unwrap call")
> Signed-off-by: Krishna Kurapati <[email protected]>
> ---
> There could probably be cases where the first NTB is proper and the second
> NTB's header is proper but the NDP is corrupt, and in those cases too, all
> the datagrams are dropped. But I haven't seen such case practically.
>
> drivers/usb/gadget/function/f_ncm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
> index cc0ed29a4adc..a75b6dc8b0cb 100644
> --- a/drivers/usb/gadget/function/f_ncm.c
> +++ b/drivers/usb/gadget/function/f_ncm.c
> @@ -1325,7 +1325,7 @@ static int ncm_unwrap_ntb(struct gether *port,
> "Parsed NTB with %d frames\n", dgram_counter);
>
> to_process -= block_len;
> - if (to_process != 0) {
> + if (to_process > opts->nth_size) {
shouldn't this check actually be >= not > ?
> ntb_ptr = (unsigned char *)(ntb_ptr + block_len);
> goto parse_ntb;
> }
> --
> 2.42.0
>
--
Maciej Żenczykowski, Kernel Networking Developer @ Google
>> It is observed sometimes when tethering is used over NCM with Windows 11
>> as host, at some instances, the gadget_giveback has one byte appended at
>> the end of a proper NTB. When the NTB is parsed, unwrap call looks for
>> any leftover bytes in SKB provided by u_ether and if there are any pending
>> bytes, it treats them as a separate NTB and parses it. But in case the
>> second NTB (as per unwrap call) is faulty/corrupt, all the datagrams that
>> were parsed properly in the first NTB and saved in rx_list are dropped.
>
> I think this is likely Windows trying to avoid generating 0 length frames.
>
> (usb max single datagram [frame?] size is 1024 bytes).
>
> My guess is this extra byte will only ever happen at the end of a
> multiple of 1024 bytes,
> and it will always be exactly one byte, and it will likely be a '0' pad byte.
>
You are right. This happens only with packet sizes of 1024/2048/3072
etc., and it is 0x00 only.
> Could you check if a more specific test of this sort would make sense?
> (ie. fix the problem)
>
> Something like
>
> if (to_process == 1) && (current_offset & 1023 == 0) && (*payload == 0)
> // extra 1 zero byte pad to prevent multiple of 1024 sized packet
> return
> }
>
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.
> 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)
// extra 1 zero byte pad to prevent multiple of 1024 sized packet
return
} else if (to_process > 1) {
goto parse_ntb;
}
Just modified in current_offset with block_len and checked it with
%1024. Let me know if it is fine and I will give the change to testing
team. The issue is easily reproducible.
Regards,
Krishna,
>>
>> Adding a few custom traces showed the following:
>>
>> [002] d..1 7828.532866: dwc3_gadget_giveback: ep1out:
>> req 000000003868811a length 1025/16384 zsI ==> 0
>> [002] d..1 7828.532867: ncm_unwrap_ntb: K: ncm_unwrap_ntb toprocess: 1025
>> [002] d..1 7828.532867: ncm_unwrap_ntb: K: ncm_unwrap_ntb nth: 1751999342
>> [002] d..1 7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb seq: 0xce67
>> [002] d..1 7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb blk_len: 0x400
>> [002] d..1 7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb ndp_len: 0x10
>> [002] d..1 7828.532869: ncm_unwrap_ntb: K: Parsed NTB with 1 frames
>>
>> In this case, the giveback is of 1025 bytes and block length is 1024.
>> The rest 1 byte (which is 0x00) won't be parsed resulting in drop of
>> all datagrams in rx_list.
>>
>> Same is case with packets of size 2048:
>> [002] d..1 7828.557948: dwc3_gadget_giveback: ep1out:
>> req 0000000011dfd96e length 2049/16384 zsI ==> 0
>> [002] d..1 7828.557949: ncm_unwrap_ntb: K: ncm_unwrap_ntb nth: 1751999342
>> [002] d..1 7828.557950: ncm_unwrap_ntb: K: ncm_unwrap_ntb blk_len: 0x800
>>
>> Lecroy shows one byte coming in extra confirming that the byte is coming
>> in from PC:
>>
>> Transfer 2959 - Bytes Transferred(1025) Timestamp((18.524 843 590)
>> - Transaction 8391 - Data(1025 bytes) Timestamp(18.524 843 590)
>> --- Packet 4063861
>> Data(1024 bytes)
>> Duration(2.117us) Idle(14.700ns) Timestamp(18.524 843 590)
>> --- Packet 4063863
>> Data(1 byte)
>> Duration(66.160ns) Time(282.000ns) Timestamp(18.524 845 722)
>>
>> Fix this by checking if the leftover bytes before parsing next NTB is of
>> size more than the expected header.
>>
>> Fixes: 427694cfaafa ("usb: gadget: ncm: Handle decoding of multiple NTB's in unwrap call")
>> Signed-off-by: Krishna Kurapati <[email protected]>
>> ---
>> There could probably be cases where the first NTB is proper and the second
>> NTB's header is proper but the NDP is corrupt, and in those cases too, all
>> the datagrams are dropped. But I haven't seen such case practically.
>>
>> drivers/usb/gadget/function/f_ncm.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
>> index cc0ed29a4adc..a75b6dc8b0cb 100644
>> --- a/drivers/usb/gadget/function/f_ncm.c
>> +++ b/drivers/usb/gadget/function/f_ncm.c
>> @@ -1325,7 +1325,7 @@ static int ncm_unwrap_ntb(struct gether *port,
>> "Parsed NTB with %d frames\n", dgram_counter);
>>
>> to_process -= block_len;
>> - if (to_process != 0) {
>> + if (to_process > opts->nth_size) {
>
> shouldn't this check actually be >= not > ?
>
Yes. But what is header is present and no data further ? Just to fix
another harmless corner case, I added the "=".
Regards,
Krishna,
On Tue, Jan 2, 2024 at 2:48 AM Krishna Kurapati PSSNV
<[email protected]> wrote:
>
>
> >> It is observed sometimes when tethering is used over NCM with Windows 11
> >> as host, at some instances, the gadget_giveback has one byte appended at
> >> the end of a proper NTB. When the NTB is parsed, unwrap call looks for
> >> any leftover bytes in SKB provided by u_ether and if there are any pending
> >> bytes, it treats them as a separate NTB and parses it. But in case the
> >> second NTB (as per unwrap call) is faulty/corrupt, all the datagrams that
> >> were parsed properly in the first NTB and saved in rx_list are dropped.
> >
> > I think this is likely Windows trying to avoid generating 0 length frames.
> >
> > (usb max single datagram [frame?] size is 1024 bytes).
> >
> > My guess is this extra byte will only ever happen at the end of a
> > multiple of 1024 bytes,
> > and it will always be exactly one byte, and it will likely be a '0' pad byte.
> >
>
> You are right. This happens only with packet sizes of 1024/2048/3072
> etc., and it is 0x00 only.
>
> > Could you check if a more specific test of this sort would make sense?
> > (ie. fix the problem)
> >
> > Something like
> >
> > if (to_process == 1) && (current_offset & 1023 == 0) && (*payload == 0)
> > // extra 1 zero byte pad to prevent multiple of 1024 sized packet
> > return
> > }
> >
>
> 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.
> > 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'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;
> }
>
> Just modified in current_offset with block_len and checked it with
> %1024. Let me know if it is fine and I will give the change to testing
> team. The issue is easily reproducible.
>
> Regards,
> Krishna,
>
> >>
> >> Adding a few custom traces showed the following:
> >>
> >> [002] d..1 7828.532866: dwc3_gadget_giveback: ep1out:
> >> req 000000003868811a length 1025/16384 zsI ==> 0
> >> [002] d..1 7828.532867: ncm_unwrap_ntb: K: ncm_unwrap_ntb toprocess: 1025
> >> [002] d..1 7828.532867: ncm_unwrap_ntb: K: ncm_unwrap_ntb nth: 1751999342
> >> [002] d..1 7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb seq: 0xce67
> >> [002] d..1 7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb blk_len: 0x400
> >> [002] d..1 7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb ndp_len: 0x10
> >> [002] d..1 7828.532869: ncm_unwrap_ntb: K: Parsed NTB with 1 frames
> >>
> >> In this case, the giveback is of 1025 bytes and block length is 1024.
> >> The rest 1 byte (which is 0x00) won't be parsed resulting in drop of
> >> all datagrams in rx_list.
> >>
> >> Same is case with packets of size 2048:
> >> [002] d..1 7828.557948: dwc3_gadget_giveback: ep1out:
> >> req 0000000011dfd96e length 2049/16384 zsI ==> 0
> >> [002] d..1 7828.557949: ncm_unwrap_ntb: K: ncm_unwrap_ntb nth: 1751999342
> >> [002] d..1 7828.557950: ncm_unwrap_ntb: K: ncm_unwrap_ntb blk_len: 0x800
> >>
> >> Lecroy shows one byte coming in extra confirming that the byte is coming
> >> in from PC:
> >>
> >> Transfer 2959 - Bytes Transferred(1025) Timestamp((18.524 843 590)
> >> - Transaction 8391 - Data(1025 bytes) Timestamp(18.524 843 590)
> >> --- Packet 4063861
> >> Data(1024 bytes)
> >> Duration(2.117us) Idle(14.700ns) Timestamp(18.524 843 590)
> >> --- Packet 4063863
> >> Data(1 byte)
> >> Duration(66.160ns) Time(282.000ns) Timestamp(18.524 845 722)
> >>
> >> Fix this by checking if the leftover bytes before parsing next NTB is of
> >> size more than the expected header.
> >>
> >> Fixes: 427694cfaafa ("usb: gadget: ncm: Handle decoding of multiple NTB's in unwrap call")
> >> Signed-off-by: Krishna Kurapati <[email protected]>
> >> ---
> >> There could probably be cases where the first NTB is proper and the second
> >> NTB's header is proper but the NDP is corrupt, and in those cases too, all
> >> the datagrams are dropped. But I haven't seen such case practically.
> >>
> >> drivers/usb/gadget/function/f_ncm.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
> >> index cc0ed29a4adc..a75b6dc8b0cb 100644
> >> --- a/drivers/usb/gadget/function/f_ncm.c
> >> +++ b/drivers/usb/gadget/function/f_ncm.c
> >> @@ -1325,7 +1325,7 @@ static int ncm_unwrap_ntb(struct gether *port,
> >> "Parsed NTB with %d frames\n", dgram_counter);
> >>
> >> to_process -= block_len;
> >> - if (to_process != 0) {
> >> + if (to_process > opts->nth_size) {
> >
> > shouldn't this check actually be >= not > ?
> >
>
> Yes. But what is header is present and no data further ? Just to fix
> another harmless corner case, I added the "=".
>
> Regards,
> Krishna,
>> 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/[email protected]/
I made changes in this v2 as per comments on v1.
Regards,
Krishna,
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.
Jack
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,