[PATCH] net: usb: cdc_ncm: add Apple MacBook Pro USB product ID 0x1902

[PATCH] net: usb: cdc_ncm: add Apple MacBook Pro USB product ID 0x1902

[Mehrdad Afshari]

The cdc_devs[] quirk table special-cases the Mac CDC-NCM private
interface personality only for USB product ID 0x1905.
Some MacBook Pro models (e.g. M1 Max) connected over a
USB4/Thunderbolt 3/4 cable to a host whose Thunderbolt controller
lacks PCIe tunneling support (no NHI function, USB4-only mode)
present themselves with product ID 0x1902 instead,
using the same descriptor layout as 0x1905: a Communications
control interface with zero endpoints (no interrupt/status endpoint)
paired with a CDC Data interface, at
interface numbers 0 and 2.

Because 0x1902 is unmatched, these devices fall through to the
generic cdc_ncm_info driver_info, which sets FLAG_LINK_INTR and
therefore requires an interrupt endpoint on the control interface.
Apple's private NCM interface never provides one, so cdc_ncm_bind()
fails outright:

  cdc_ncm 2-1:1.0: bind() failure
  cdc_ncm 2-1:1.2: bind() failure

and no network device is created, breaking Ethernet-over-USB4
between the Mac and any USB4 host lacking Thunderbolt PCIe
tunneling.

Add matching entries for 0x1902 alongside the existing 0x1905
ones, reusing apple_private_interface_info as with the other Mac
ID.

Signed-off-by: Mehrdad Afshari <mehrdad@signeen.com>
---
 drivers/net/usb/cdc_ncm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 0223a17..35db38c 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -2013,6 +2013,12 @@ static const struct usb_device_id cdc_devs[] = {
 	},
 
 	/* Mac */
+	{ USB_DEVICE_INTERFACE_NUMBER(0x05ac, 0x1902, 0),
+		.driver_info = (unsigned long)&apple_private_interface_info,
+	},
+	{ USB_DEVICE_INTERFACE_NUMBER(0x05ac, 0x1902, 2),
+		.driver_info = (unsigned long)&apple_private_interface_info,
+	},
 	{ USB_DEVICE_INTERFACE_NUMBER(0x05ac, 0x1905, 0),
 		.driver_info = (unsigned long)&apple_private_interface_info,
 	},
-- 
2.55.0

Re: [PATCH] net: usb: cdc_ncm: add Apple MacBook Pro USB product ID 0x1902

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: usb: cdc_ncm: add Apple MacBook Pro USB product ID 0x1902

This adds two cdc_devs[] entries for USB ID 0x05ac:0x1902 (interface
numbers 0 and 2) pointing at apple_private_interface_info, so MacBook Pro
models presenting that product ID over USB4 no longer fall through to the
generic cdc_ncm_info entry, which requires an interrupt endpoint via
FLAG_LINK_INTR.

No Link: tags are present in the commit header.

>     using the same descriptor layout as 0x1905: a Communications
>     control interface with zero endpoints (no interrupt/status endpoint)
>     paired with a CDC Data interface, at
>     interface numbers 0 and 2.

[Severity: Low]
Is this description of the descriptor layout what the hardware actually
presents?  Read literally, it says interface 0 is the Communications
control interface and interface 2 is its CDC Data interface.  If that were
the case, the second entry added below for interface number 2 would never
bind, since cdc_ncm_bind_common() notes:

drivers/net/usb/cdc_ncm.c:cdc_ncm_bind_common() {
	/* only the control interface can be successfully probed */
	ctx->control = intf;
	...
}

and a CDC Data interface carries no CDC Union / NCM functional
descriptors, so bind can only reach the error path.

The pre-patch dmesg quoted in the changelog seems to say something
different:

>       cdc_ncm 2-1:1.0: bind() failure
>       cdc_ncm 2-1:1.2: bind() failure

That string is only emitted from cdc_ncm_bind_common():

drivers/net/usb/cdc_ncm.c:cdc_ncm_bind_common() {
	...
error:
	cdc_ncm_free((struct cdc_ncm_ctx *)dev->data[0]);
	dev->data[0] = 0;
	dev_info(&intf->dev, "bind() failure\n");
	return -ENODEV;
}

usb_driver_claim_interface(), which is how the data interface is taken,
does not invoke probe/bind, so interface 2 must have matched the id_table
on its own.  Pre-patch the only entry it could match is the generic

	{ USB_INTERFACE_INFO(USB_CLASS_COMM,
		USB_CDC_SUBCLASS_NCM, USB_CDC_PROTO_NONE),
		.driver_info = (unsigned long)&cdc_ncm_info,
	},

which a CDC Data interface (bInterfaceClass 0x0a) cannot match.  Would it
be more accurate to say that interfaces 0 and 2 are both zero-endpoint
COMM/NCM control interfaces, each paired with its own CDC Data interface,
as with 0x1905?

> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 0223a172851ec..35db38cb3e4a2 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -2013,6 +2013,12 @@ static const struct usb_device_id cdc_devs[] = {
>  	},
>  
>  	/* Mac */
> +	{ USB_DEVICE_INTERFACE_NUMBER(0x05ac, 0x1902, 0),
> +		.driver_info = (unsigned long)&apple_private_interface_info,
> +	},
> +	{ USB_DEVICE_INTERFACE_NUMBER(0x05ac, 0x1902, 2),
> +		.driver_info = (unsigned long)&apple_private_interface_info,
> +	},

The code change itself looks right, and the two entries appear to be
needed.  Could the changelog also state the post-patch result on the
hardware, the way the precedent commit a5148bc2fa27 ("net: usb: cdc_ncm:
add Apple Mac USB-C direct networking quirk") does?  That commit ends with
the netdevs appearing, ip -br link showing them UP and DHCP working.  Here
the message stops at the pre-patch failure, so there is nothing recording
that apple_private_interface_info (FLAG_POINTTOPOINT | FLAG_NO_SETINT |
FLAG_MULTI_PACKET | FLAG_ETHER | FLAG_SEND_ZLP, without FLAG_LINK_INTR) is
the right driver_info for this product ID and that both entries are
required.  Including the /sys/kernel/debug/usb/devices or lsusb dump would
also make the interface layout unambiguous.

>  	{ USB_DEVICE_INTERFACE_NUMBER(0x05ac, 0x1905, 0),
>  		.driver_info = (unsigned long)&apple_private_interface_info,
>  	},

[ ... ]
-- 
pw-bot: cr