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@xxxxxx>
> Tested-by: Georgi Valkov <gvalkov@xxxxxxxxx>
> ---
> 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
