Hello Paolo, Thank you for the review! >> - rx_buf = usb_alloc_coherent(iphone->udev, IPHETH_BUF_SIZE + IPHETH_IP_ALIGN, >> + rx_buf = usb_alloc_coherent(iphone->udev, IPHETH_RX_BUF_SIZE, > > Here the driver already knows if the device is in NCM or legacy mode, > so perhaps we could select the buffer size accordingly? You would > probably need to store the actual buffer size somewhere to keep the > buffer handling consistent and simple in later code. Agreed, I will make the buffer size dynamic in the next revision. The RX buffer size is 1516 bytes for legacy mode (2 bytes padding + 1514 bytes Ethernet frame), and 65536 bytes for NCM mode. >> memcpy(dev->tx_buf, skb->data, skb->len); >> - if (skb->len < IPHETH_BUF_SIZE) >> - memset(dev->tx_buf + skb->len, 0, IPHETH_BUF_SIZE - skb->len); >> >> usb_fill_bulk_urb(dev->tx_urb, udev, >> usb_sndbulkpipe(udev, dev->bulk_out), >> - dev->tx_buf, IPHETH_BUF_SIZE, >> + dev->tx_buf, skb->len, >> ipheth_sndbulk_callback, >> dev); >> dev->tx_urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; > > This chunk looks unrelated from NCM support, and unconditionally > changes the established behaviour even with legacy mode, why? > > Does that works even with old(er) devices? I see Georgi Valkov said he tested v3 of the patch on older iOS devices and confirmed it working. I'll chat with him to get some USB traffic captures, to check what is macOS' behaviour with such devices (to make sure we behave the same way as the official driver). I also wanted to investigate a bit, when was NCM support even added to iOS. Personally I remember testing this in legacy mode a while ago, before I implemented NCM. I will re-test it again in legacy mode in addition to Georgi's efforts. >From my side, I think it's reasonable to split this out into a separate patch, since it technically applies to the legacy mode as well, and doesn't (directly) relate to NCM support, as you pointed out. There's no reason to send the full buffer every time including padding, but I'll check out traffic captures on macOS + older devices, maybe that's what they're doing.
