Message-ID: <ZGSb4l8XcxclFsB1@corigine.com>
Date: Wed, 17 May 2023 11:18:26 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Foster Snowhill <forst@....gy>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-usb@...r.kernel.org, netdev@...r.kernel.org,
	Georgi Valkov <gvalkov@...il.com>
Subject: Re: [PATCH] net: usb: ipheth: add CDC NCM support

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@....gy>
> Tested-by: Georgi Valkov <gvalkov@...il.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

Powered by blists - more mailing lists