On 1/31/2024 10:33 PM, Maciej Żenczykowski wrote:
diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
index ca5d5f564998..8c314dc98952 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -1338,11 +1338,17 @@ 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 == 1 &&
+ (block_len % 512 == 0) &&
+ (*(unsigned char *)(ntb_ptr + block_len) == 0x00)) {
Hi Maciej,
I'm unconvinced this (block_len % 512) works right...
imagine you have 2 ntbs back to back (for example 400 + 624) that
together add up to 1024,
and then a padding null byte.
I think block_len will be 624 then and not 1024.
perhaps this actually needs to be:
(ntp_ptr + block_len - skb->data) % 512 == 0
The point is that AFAICT the multiple of 512/1024 that matters is in
the size of the USB transfer,
not the NTB block size itself.
Ahh !! So you mean, since this comes because the host doesn't want to
send packets of size wMaxPacketSize on the wire, we need to consider the
length of data parsed so far to be checked against 512/1024 and not
block length.
But either ways, the implementation in the patch also the same thing in
a different way right ? Process all NTB's present iteratively and while
doing so, check if there is one byte left. So if there are multiple
NTB's mixed up to form a packet of size 1024, even then, both the logics
would converge onto the point where they find that one byte is left.
+ goto done;
+ } else if (to_process > 0) {
ntb_ptr = (unsigned char *)(ntb_ptr + block_len);
note that ntb_ptr moves forward by block_len here,
so it's *not* always the start of the packet, so block_len is not
necessarily the actual on the wire size.
Apologies, I didn't understand this comment. By moving the ntb_ptr by
block length, we are pointing to the (last byte/ start of the next NTB)
right as we are fixing in [1] ?
goto parse_ntb;
}
+done:
dev_consume_skb_any(skb);
return 0;
--
2.34.1
But it would perhaps be worth confirming in the MS driver source what
exactly they're doing...
(maybe they never even generate multiple ntbs per usb segment...)
Yes they do and we made a fix for that in [1].
You may also want to mention in the commit message that 512 comes from
USB2 block size, and also works for USB3 block size of 1024.
Sure. Will update the commit message accordingly.
[1]:
https://lore.kernel.org/all/20230927105858.12950-1-quic_kriskura@xxxxxxxxxxx/
Regards,
Krishna,
