| Message ID | 20230516210127.35841-1-forst@pen.gy (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: usb: ipheth: add CDC NCM support | expand |
On Tue, May 16, 2023 at 09:01:51PM +0000, Foster Snowhill wrote: > Recent iOS releases support CDC NCM encapsulation on RX. This mode is > the default on macOS and Windows. In this mode, an iOS device may include > one or more Ethernet frames inside a single URB. > > Freshly booted iOS devices start in legacy mode, but are put into > NCM mode by the official Apple driver. When reconnecting such a device > from a macOS/Windows machine to a Linux host, the device stays in > NCM mode, making it unusable with the legacy ipheth driver code. > > To correctly support such a device, the driver has to either support > the NCM mode too, or put the device back into legacy mode. > > To match the behaviour of the macOS/Windows driver, and since there > is no documented control command to revert to legacy mode, implement > NCM support. The device is attempted to be put into NCM mode by default, > and falls back to legacy mode if the attempt fails. > > Signed-off-by: Foster Snowhill <forst@pen.gy> > Tested-by: Georgi Valkov <gvalkov@gmail.com> > --- > Georgi Valkov and I have been using this patch for one year with OpenWrt, > Linux kernel versions 5.10 and 5.15 on the following devices: > > * Linksys WRT3200ACM (Marvell Armada 385, ARMv7-A LE), with iPhone 7 Plus > * Linksys EA8300 (Qualcomm IPQ4019, ARMv7-A LE), with iPhone Xs Max > > Georgi also performed limited tests on > > * TP-Link TL-WR1043ND (QCA9558, MIPS 74Kc BE) > * OrangePi Zero (Allwinner H2+, ARMv7-A LE) > > There is an interest, specifically from Georgi, to have this patch > backported to v5.15 to then be used in OpenWrt. > > Neither me nor Georgi are by any means skilled in developing for the > Linux kernel. Please review the patch carefully and advise if any > changes are needed in regards to security (e.g. data validation) > or code formatting. Hi Foster, thanks for your patch. Some initial feedback follows (hopefully there will be feedback from others too). nit: The target tree for this patch is probably net-next. As such it should be included in the Subject: Subject: [PATCH net-next v2] ... Link: https://kernel.org/doc/html/latest/process/maintainer-netdev.html nit: Looking at Git history, probably the patch prefix should be 'usbnet: ipheth: ' Subject: [PATCH net-next v2] usbnet: ipheth: ... Above you mention 5.10 and 5.15. I see that this patch applies to current net-next, which is where development occurs. Has the patch been tested there? > --- > drivers/net/usb/ipheth.c | 186 ++++++++++++++++++++++++++++++++------- > 1 file changed, 152 insertions(+), 34 deletions(-) > > diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c > index 6a769df0b..3cdf92b39 100644 > --- a/drivers/net/usb/ipheth.c > +++ b/drivers/net/usb/ipheth.c > @@ -52,6 +52,7 @@ > #include <linux/ethtool.h> > #include <linux/usb.h> > #include <linux/workqueue.h> > +#include <linux/usb/cdc.h> > > #define USB_VENDOR_APPLE 0x05ac > > @@ -59,8 +60,11 @@ > #define IPHETH_USBINTF_SUBCLASS 253 > #define IPHETH_USBINTF_PROTO 1 > > -#define IPHETH_BUF_SIZE 1514 > #define IPHETH_IP_ALIGN 2 /* padding at front of URB */ > +#define IPHETH_NCM_HEADER_SIZE (12 + 96) /* NCMH + NCM0 */ > +#define IPHETH_TX_BUF_SIZE ETH_FRAME_LEN > +#define IPHETH_RX_BUF_SIZE 65536 I wonder if there are any issues with increasing the RX size from 1514 to 65536. Not that I have anything specific in mind. > + > #define IPHETH_TX_TIMEOUT (5 * HZ) > > #define IPHETH_INTFNUM 2 ... > +static int ipheth_rcvbulk_callback_ncm(struct urb *urb) > +{ > + struct ipheth_device *dev; > + struct usb_cdc_ncm_nth16 *ncmh; > + struct usb_cdc_ncm_ndp16 *ncm0; > + struct usb_cdc_ncm_dpe16 *dpe; > + char *buf; > + int len; > + int retval = -EINVAL; For networking code, please arrange local variables in reverse xmas tree order - longest line to shortest. There is, IMHO, no need to go and fix any existing instances. But please follow this for new code. Something like this: struct usb_cdc_ncm_nth16 *ncmh; struct usb_cdc_ncm_ndp16 *ncm0; struct usb_cdc_ncm_dpe16 *dpe; struct ipheth_device *dev; int retval = -EINVAL; char *buf; int len; > + > + dev = urb->context; > + > + if (urb->actual_length < IPHETH_NCM_HEADER_SIZE) { > + dev->net->stats.rx_length_errors++; > + return retval; > + } > + > + ncmh = (struct usb_cdc_ncm_nth16 *)(urb->transfer_buffer); nit: There is no need to cast a void pointer. > + if (ncmh->dwSignature != cpu_to_le32(USB_CDC_NCM_NTH16_SIGN) || > + le16_to_cpu(ncmh->wNdpIndex) >= urb->actual_length) { > + dev->net->stats.rx_errors++; > + return retval; > + } > + > + ncm0 = (struct usb_cdc_ncm_ndp16 *)(urb->transfer_buffer + le16_to_cpu(ncmh->wNdpIndex)); Ditto. > + if (ncm0->dwSignature != cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN) || > + le16_to_cpu(ncmh->wHeaderLength) + le16_to_cpu(ncm0->wLength) >= urb->actual_length) { nit: Lines less than 80 columns wide a re a bit nicer IMHO > + dev->net->stats.rx_errors++; > + return retval; > + } > + > + dpe = ncm0->dpe16; > + while (le16_to_cpu(dpe->wDatagramIndex) != 0 && > + le16_to_cpu(dpe->wDatagramLength) != 0) { > + if (le16_to_cpu(dpe->wDatagramIndex) >= urb->actual_length || > + (le16_to_cpu(dpe->wDatagramIndex) + le16_to_cpu(dpe->wDatagramLength)) > + > urb->actual_length) { > + dev->net->stats.rx_length_errors++; > + return retval; > + } > + > + buf = urb->transfer_buffer + le16_to_cpu(dpe->wDatagramIndex); > + len = le16_to_cpu(dpe->wDatagramLength); > + > + retval = ipheth_consume_skb(buf, len, dev); > + if (retval != 0) > + return retval; > + > + dpe++; > + } > + > + return 0; > +} ... > @@ -481,6 +595,10 @@ static int ipheth_probe(struct usb_interface *intf, > if (retval) > goto err_get_macaddr; > > + retval = ipheth_enable_ncm(dev); > + if (retval == 0) nit: if (!retval) > + dev->rcvbulk_callback = ipheth_rcvbulk_callback_ncm; > + > INIT_DELAYED_WORK(&dev->carrier_work, ipheth_carrier_check_work); > > retval = ipheth_alloc_urbs(dev); > @@ -510,8 +628,8 @@ static int ipheth_probe(struct usb_interface *intf, > ipheth_free_urbs(dev); > err_alloc_urbs: > err_get_macaddr: > -err_alloc_ctrl_buf: > kfree(dev->ctrl_buf); > +err_alloc_ctrl_buf: > err_endpoints: > free_netdev(netdev); > return retval; nit: this hunk seems unrelated to the rest of the patch
Hello Simon, Thank you for the speedy review. > nit: The target tree for this patch is probably net-next. > As such it should be included in the Subject: Ack, will label the next patch revision accordingly. The netdev workflow doc has been very useful. > nit: Looking at Git history, probably the patch prefix should be > 'usbnet: ipheth: ' I looked at the commit log for drivers/net/usb, and saw "net: usb:" prefix a lot more often than "usbnet:", that's why I chose the former. As for ipheth, you're right, "usbnet:" is more frequent. Will change to "usbnet:" in v2, unless there's a different opinion on this. Naming things is difficult. :) > I see that this patch applies to current net-next, which is where> development occurs. Has the patch been tested there? I will make sure to test it on net-next before submitting v2, and make an explicit note about that in the comments. > I wonder if there are any issues with increasing the RX size > from 1514 to 65536. Not that I have anything specific in mind. The RX buffer increase was to accommodate for multiple Ethernet frames encapsulated in a single URB. Per the USB NCM spec [1], an NTB-16 is "shorter than 65,536 bytes", may include "up to forty 1514-byte Ethernet frames". Frames themselves are still up to 1514 bytes each. Somebody else may have to comment whether this has any other implications from the kernel side of things. > For networking code, please arrange local variables in reverse xmas tree > order - longest line to shortest. Ack >> + ncmh = (struct usb_cdc_ncm_nth16 *)(urb->transfer_buffer); > nit: There is no need to cast a void pointer. > >> + ncm0 = (struct usb_cdc_ncm_ndp16 *)(urb->transfer_buffer + le16_to_cpu(ncmh->wNdpIndex)); > Ditto. Ack > nit: Lines less than 80 columns wide a re a bit nicer IMHO Ack >> + if (retval == 0) > nit: if (!retval) Ack >> @@ -510,8 +628,8 @@ static int ipheth_probe(struct usb_interface *intf, >> ipheth_free_urbs(dev); >> err_alloc_urbs: >> err_get_macaddr: >> -err_alloc_ctrl_buf: >> kfree(dev->ctrl_buf); >> +err_alloc_ctrl_buf: >> err_endpoints: >> free_netdev(netdev); >> return retval; > > nit: this hunk seems unrelated to the rest of the patch You're right, this should live in a separate patch. Would you suggest making this a second patch in the same series, or submitting it separately? [1]: https://www.usb.org/document-library/network-control-model-devices-specification-v10-and-errata-and-adopters-agreement
On Wed, 17 May 2023 22:30:18 +0000 Forst wrote:
> [encrypted.asc application/octet-stream (5747 bytes)]
FWIW people on CC are not getting your emails.
Could you disable the encryption or whatever you have going? :|
diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c index 6a769df0b..3cdf92b39 100644 --- a/drivers/net/usb/ipheth.c +++ b/drivers/net/usb/ipheth.c @@ -52,6 +52,7 @@ #include <linux/ethtool.h> #include <linux/usb.h> #include <linux/workqueue.h> +#include <linux/usb/cdc.h> #define USB_VENDOR_APPLE 0x05ac @@ -59,8 +60,11 @@ #define IPHETH_USBINTF_SUBCLASS 253 #define IPHETH_USBINTF_PROTO 1 -#define IPHETH_BUF_SIZE 1514 #define IPHETH_IP_ALIGN 2 /* padding at front of URB */ +#define IPHETH_NCM_HEADER_SIZE (12 + 96) /* NCMH + NCM0 */ +#define IPHETH_TX_BUF_SIZE ETH_FRAME_LEN +#define IPHETH_RX_BUF_SIZE 65536 + #define IPHETH_TX_TIMEOUT (5 * HZ) #define IPHETH_INTFNUM 2 @@ -71,6 +75,7 @@ #define IPHETH_CTRL_TIMEOUT (5 * HZ) #define IPHETH_CMD_GET_MACADDR 0x00 +#define IPHETH_CMD_ENABLE_NCM 0x04 #define IPHETH_CMD_CARRIER_CHECK 0x45 #define IPHETH_CARRIER_CHECK_TIMEOUT round_jiffies_relative(1 * HZ) @@ -84,6 +89,8 @@ static const struct usb_device_id ipheth_table[] = { }; MODULE_DEVICE_TABLE(usb, ipheth_table); +static const char ipheth_start_packet[] = { 0x00, 0x01, 0x01, 0x00 }; + struct ipheth_device { struct usb_device *udev; struct usb_interface *intf; @@ -97,6 +104,7 @@ struct ipheth_device { u8 bulk_out; struct delayed_work carrier_work; bool confirmed_pairing; + int (*rcvbulk_callback)(struct urb *urb); }; static int ipheth_rx_submit(struct ipheth_device *dev, gfp_t mem_flags); @@ -116,12 +124,12 @@ static int ipheth_alloc_urbs(struct ipheth_device *iphone) if (rx_urb == NULL) goto free_tx_urb; - tx_buf = usb_alloc_coherent(iphone->udev, IPHETH_BUF_SIZE, + tx_buf = usb_alloc_coherent(iphone->udev, IPHETH_TX_BUF_SIZE, GFP_KERNEL, &tx_urb->transfer_dma); if (tx_buf == NULL) goto free_rx_urb; - rx_buf = usb_alloc_coherent(iphone->udev, IPHETH_BUF_SIZE + IPHETH_IP_ALIGN, + rx_buf = usb_alloc_coherent(iphone->udev, IPHETH_RX_BUF_SIZE, GFP_KERNEL, &rx_urb->transfer_dma); if (rx_buf == NULL) goto free_tx_buf; @@ -134,7 +142,7 @@ static int ipheth_alloc_urbs(struct ipheth_device *iphone) return 0; free_tx_buf: - usb_free_coherent(iphone->udev, IPHETH_BUF_SIZE, tx_buf, + usb_free_coherent(iphone->udev, IPHETH_TX_BUF_SIZE, tx_buf, tx_urb->transfer_dma); free_rx_urb: usb_free_urb(rx_urb); @@ -146,9 +154,9 @@ static int ipheth_alloc_urbs(struct ipheth_device *iphone) static void ipheth_free_urbs(struct ipheth_device *iphone) { - usb_free_coherent(iphone->udev, IPHETH_BUF_SIZE + IPHETH_IP_ALIGN, iphone->rx_buf, + usb_free_coherent(iphone->udev, IPHETH_RX_BUF_SIZE, iphone->rx_buf, iphone->rx_urb->transfer_dma); - usb_free_coherent(iphone->udev, IPHETH_BUF_SIZE, iphone->tx_buf, + usb_free_coherent(iphone->udev, IPHETH_TX_BUF_SIZE, iphone->tx_buf, iphone->tx_urb->transfer_dma); usb_free_urb(iphone->rx_urb); usb_free_urb(iphone->tx_urb); @@ -160,14 +168,104 @@ static void ipheth_kill_urbs(struct ipheth_device *dev) usb_kill_urb(dev->rx_urb); } -static void ipheth_rcvbulk_callback(struct urb *urb) +static int ipheth_consume_skb(char *buf, int len, struct ipheth_device *dev) { - struct ipheth_device *dev; struct sk_buff *skb; - int status; + + skb = dev_alloc_skb(len); + if (!skb) { + dev->net->stats.rx_dropped++; + return -ENOMEM; + } + + skb_put_data(skb, buf, len); + skb->dev = dev->net; + skb->protocol = eth_type_trans(skb, dev->net); + + dev->net->stats.rx_packets++; + dev->net->stats.rx_bytes += len; + netif_rx(skb); + + return 0; +} + +static int ipheth_rcvbulk_callback_legacy(struct urb *urb) +{ + struct ipheth_device *dev; char *buf; int len; + dev = urb->context; + + if (urb->actual_length <= IPHETH_IP_ALIGN) { + dev->net->stats.rx_length_errors++; + return -EINVAL; + } + len = urb->actual_length - IPHETH_IP_ALIGN; + buf = urb->transfer_buffer + IPHETH_IP_ALIGN; + + return ipheth_consume_skb(buf, len, dev); +} + +static int ipheth_rcvbulk_callback_ncm(struct urb *urb) +{ + struct ipheth_device *dev; + struct usb_cdc_ncm_nth16 *ncmh; + struct usb_cdc_ncm_ndp16 *ncm0; + struct usb_cdc_ncm_dpe16 *dpe; + char *buf; + int len; + int retval = -EINVAL; + + dev = urb->context; + + if (urb->actual_length < IPHETH_NCM_HEADER_SIZE) { + dev->net->stats.rx_length_errors++; + return retval; + } + + ncmh = (struct usb_cdc_ncm_nth16 *)(urb->transfer_buffer); + if (ncmh->dwSignature != cpu_to_le32(USB_CDC_NCM_NTH16_SIGN) || + le16_to_cpu(ncmh->wNdpIndex) >= urb->actual_length) { + dev->net->stats.rx_errors++; + return retval; + } + + ncm0 = (struct usb_cdc_ncm_ndp16 *)(urb->transfer_buffer + le16_to_cpu(ncmh->wNdpIndex)); + if (ncm0->dwSignature != cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN) || + le16_to_cpu(ncmh->wHeaderLength) + le16_to_cpu(ncm0->wLength) >= urb->actual_length) { + dev->net->stats.rx_errors++; + return retval; + } + + dpe = ncm0->dpe16; + while (le16_to_cpu(dpe->wDatagramIndex) != 0 && + le16_to_cpu(dpe->wDatagramLength) != 0) { + if (le16_to_cpu(dpe->wDatagramIndex) >= urb->actual_length || + (le16_to_cpu(dpe->wDatagramIndex) + le16_to_cpu(dpe->wDatagramLength)) + > urb->actual_length) { + dev->net->stats.rx_length_errors++; + return retval; + } + + buf = urb->transfer_buffer + le16_to_cpu(dpe->wDatagramIndex); + len = le16_to_cpu(dpe->wDatagramLength); + + retval = ipheth_consume_skb(buf, len, dev); + if (retval != 0) + return retval; + + dpe++; + } + + return 0; +} + +static void ipheth_rcvbulk_callback(struct urb *urb) +{ + struct ipheth_device *dev; + int retval, status; + dev = urb->context; if (dev == NULL) return; @@ -187,29 +285,25 @@ static void ipheth_rcvbulk_callback(struct urb *urb) return; } - if (urb->actual_length <= IPHETH_IP_ALIGN) { - dev->net->stats.rx_length_errors++; - return; - } - len = urb->actual_length - IPHETH_IP_ALIGN; - buf = urb->transfer_buffer + IPHETH_IP_ALIGN; - - skb = dev_alloc_skb(len); - if (!skb) { - dev_err(&dev->intf->dev, "%s: dev_alloc_skb: -ENOMEM\n", - __func__); - dev->net->stats.rx_dropped++; + /* The very first frame we receive from device has a fixed 4-byte value + * We can safely skip it + */ + if (unlikely + (urb->actual_length == sizeof(ipheth_start_packet) && + memcmp(urb->transfer_buffer, ipheth_start_packet, + sizeof(ipheth_start_packet)) == 0 + )) + goto rx_submit; + + retval = dev->rcvbulk_callback(urb); + if (retval != 0) { + dev_err(&dev->intf->dev, "%s: callback retval: %d\n", + __func__, retval); return; } - skb_put_data(skb, buf, len); - skb->dev = dev->net; - skb->protocol = eth_type_trans(skb, dev->net); - - dev->net->stats.rx_packets++; - dev->net->stats.rx_bytes += len; +rx_submit: dev->confirmed_pairing = true; - netif_rx(skb); ipheth_rx_submit(dev, GFP_ATOMIC); } @@ -310,6 +404,27 @@ static int ipheth_get_macaddr(struct ipheth_device *dev) return retval; } +static int ipheth_enable_ncm(struct ipheth_device *dev) +{ + struct usb_device *udev = dev->udev; + int retval; + + retval = usb_control_msg(udev, + usb_sndctrlpipe(udev, IPHETH_CTRL_ENDP), + IPHETH_CMD_ENABLE_NCM, /* request */ + 0x40, /* request type */ + 0x00, /* value */ + 0x02, /* index */ + NULL, + 0, + IPHETH_CTRL_TIMEOUT); + + dev_info(&dev->intf->dev, "%s: usb_control_msg: %d\n", + __func__, retval); + + return retval; +} + static int ipheth_rx_submit(struct ipheth_device *dev, gfp_t mem_flags) { struct usb_device *udev = dev->udev; @@ -317,7 +432,7 @@ static int ipheth_rx_submit(struct ipheth_device *dev, gfp_t mem_flags) usb_fill_bulk_urb(dev->rx_urb, udev, usb_rcvbulkpipe(udev, dev->bulk_in), - dev->rx_buf, IPHETH_BUF_SIZE + IPHETH_IP_ALIGN, + dev->rx_buf, IPHETH_RX_BUF_SIZE, ipheth_rcvbulk_callback, dev); dev->rx_urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; @@ -365,7 +480,7 @@ static netdev_tx_t ipheth_tx(struct sk_buff *skb, struct net_device *net) int retval; /* Paranoid */ - if (skb->len > IPHETH_BUF_SIZE) { + if (skb->len > IPHETH_TX_BUF_SIZE) { WARN(1, "%s: skb too large: %d bytes\n", __func__, skb->len); dev->net->stats.tx_dropped++; dev_kfree_skb_any(skb); @@ -373,12 +488,10 @@ static netdev_tx_t ipheth_tx(struct sk_buff *skb, struct net_device *net) } 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; @@ -450,6 +563,7 @@ static int ipheth_probe(struct usb_interface *intf, dev->net = netdev; dev->intf = intf; dev->confirmed_pairing = false; + dev->rcvbulk_callback = ipheth_rcvbulk_callback_legacy; /* Set up endpoints */ hintf = usb_altnum_to_altsetting(intf, IPHETH_ALT_INTFNUM); if (hintf == NULL) { @@ -481,6 +595,10 @@ static int ipheth_probe(struct usb_interface *intf, if (retval) goto err_get_macaddr; + retval = ipheth_enable_ncm(dev); + if (retval == 0) + dev->rcvbulk_callback = ipheth_rcvbulk_callback_ncm; + INIT_DELAYED_WORK(&dev->carrier_work, ipheth_carrier_check_work); retval = ipheth_alloc_urbs(dev); @@ -510,8 +628,8 @@ static int ipheth_probe(struct usb_interface *intf, ipheth_free_urbs(dev); err_alloc_urbs: err_get_macaddr: -err_alloc_ctrl_buf: kfree(dev->ctrl_buf); +err_alloc_ctrl_buf: err_endpoints: free_netdev(netdev); return retval;