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

[Jakub Kicinski <kuba@kernel.org>]

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