Re: [PATCH net-next 1/4] net: usb: move updating filter and status from cdc to usbnet

[Manuel Ebner <manuelebner@mailbox.org>]

Hi,
I have a couple suggestions for you patch. Some parts are
weird to read - it could also be that my perception today is off.

On Thu, 2026-07-02 at 16:25 +0200, Oliver Neukum wrote:
> These helpers are used by multiple drivers and do not depend

"and do not only depend on cdc core. They also depend on
other cdc drivers."
is this better? 

> on the rest of cdc. Leavin them in a cdc driver means that

/Leavin/Leaving/

> more drivers are loaded just for infrastructure, not hardware

just for the support of other drivers, not their hardware.

> support.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/net/usb/cdc_ether.c | 75 ------------------------------------
>  drivers/net/usb/usbnet.c    | 76 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 76 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
> index a0a5740590b9..b4df32e18461 100644
> --- a/drivers/net/usb/cdc_ether.c
> +++ b/drivers/net/usb/cdc_ether.c
> @@ -63,35 +63,6 @@ static const u8 mbm_guid[16] = {
>  	0xa6, 0x07, 0xc0, 0xff, 0xcb, 0x7e, 0x39, 0x2a,
>  };
>  
> -void usbnet_cdc_update_filter(struct usbnet *dev)
> -{
> -	struct net_device	*net = dev->net;
> -
> -	u16 cdc_filter = USB_CDC_PACKET_TYPE_DIRECTED
> -			| USB_CDC_PACKET_TYPE_BROADCAST;
> -
> -	/* filtering on the device is an optional feature and not worth
> -	 * the hassle so we just roughly care about snooping and if any
> -	 * multicast is requested, we take every multicast
> -	 */
> -	if (net->flags & IFF_PROMISC)
> -		cdc_filter |= USB_CDC_PACKET_TYPE_PROMISCUOUS;
> -	if (!netdev_mc_empty(net) || (net->flags & IFF_ALLMULTI))
> -		cdc_filter |= USB_CDC_PACKET_TYPE_ALL_MULTICAST;
> -
> -	usb_control_msg(dev->udev,
> -			usb_sndctrlpipe(dev->udev, 0),
> -			USB_CDC_SET_ETHERNET_PACKET_FILTER,
> -			USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> -			cdc_filter,
> -			dev->intf->cur_altsetting->desc.bInterfaceNumber,
> -			NULL,
> -			0,
> -			USB_CTRL_SET_TIMEOUT
> -		);
> -}
> -EXPORT_SYMBOL_GPL(usbnet_cdc_update_filter);
> -
>  /* We need to override usbnet_*_link_ksettings in bind() */
>  static const struct ethtool_ops cdc_ether_ethtool_ops = {
>  	.get_link		= usbnet_get_link,
> @@ -400,52 +371,6 @@ EXPORT_SYMBOL_GPL(usbnet_cdc_unbind);
>   * (by Brad Hards) talked with, with more functionality.
>   */
>  
> -static void speed_change(struct usbnet *dev, __le32 *speeds)
> -{
> -	dev->tx_speed = __le32_to_cpu(speeds[0]);
> -	dev->rx_speed = __le32_to_cpu(speeds[1]);
> -}
> -
> -void usbnet_cdc_status(struct usbnet *dev, struct urb *urb)
> -{
> -	struct usb_cdc_notification	*event;
> -
> -	if (urb->actual_length < sizeof(*event))
> -		return;
> -
> -	/* SPEED_CHANGE can get split into two 8-byte packets */
> -	if (test_and_clear_bit(EVENT_STS_SPLIT, &dev->flags)) {
> -		speed_change(dev, (__le32 *) urb->transfer_buffer);
> -		return;
> -	}
> -
> -	event = urb->transfer_buffer;
> -	switch (event->bNotificationType) {
> -	case USB_CDC_NOTIFY_NETWORK_CONNECTION:
> -		netif_dbg(dev, timer, dev->net, "CDC: carrier %s\n",
> -			  event->wValue ? "on" : "off");
> -		if (netif_carrier_ok(dev->net) != !!event->wValue)
> -			usbnet_link_change(dev, !!event->wValue, 0);
> -		break;
> -	case USB_CDC_NOTIFY_SPEED_CHANGE:	/* tx/rx rates */
> -		netif_dbg(dev, timer, dev->net, "CDC: speed change (len %d)\n",
> -			  urb->actual_length);
> -		if (urb->actual_length != (sizeof(*event) + 8))
> -			set_bit(EVENT_STS_SPLIT, &dev->flags);
> -		else
> -			speed_change(dev, (__le32 *) &event[1]);
> -		break;
> -	/* USB_CDC_NOTIFY_RESPONSE_AVAILABLE can happen too (e.g. RNDIS),
> -	 * but there are no standard formats for the response data.
> -	 */
> -	default:
> -		netdev_err(dev->net, "CDC: unexpected notification %02x!\n",
> -			   event->bNotificationType);
> -		break;
> -	}
> -}
> -EXPORT_SYMBOL_GPL(usbnet_cdc_status);
> -
>  int usbnet_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
>  {
>  	int				status;
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 25518635b7b7..5544af1f4aa5 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -22,6 +22,7 @@
>  #include <linux/init.h>
>  #include <linux/netdevice.h>
>  #include <linux/etherdevice.h>
> +#include <linux/usb/cdc.h>

why do you put it there?
it could be alphabetical instead.

>  #include <linux/ctype.h>
>  #include <linux/ethtool.h>
>  #include <linux/workqueue.h>
> @@ -2271,6 +2272,81 @@ int usbnet_write_cmd_async(struct usbnet *dev, u8 cmd, u8
> reqtype,
>  
>  }
>  EXPORT_SYMBOL_GPL(usbnet_write_cmd_async);
> +
> +void usbnet_cdc_update_filter(struct usbnet *dev)
> +{
> +	struct net_device	*net = dev->net;
> +
> +	u16 cdc_filter = USB_CDC_PACKET_TYPE_DIRECTED
> +			| USB_CDC_PACKET_TYPE_BROADCAST;
> +
Multi-line-comments in coding-style:
	/*
	 * filtering on ..
> +	/* filtering on the device is an optional feature and not worth
> +	 * the hassle so we just roughly care about snooping and if any
> +	 * multicast is requested, we take every multicast
				 , we take that multicast.
also that's a pretty long sentence.



> +	 */
> +	if (net->flags & IFF_PROMISC)
> +		cdc_filter |= USB_CDC_PACKET_TYPE_PROMISCUOUS;
> +	if (!netdev_mc_empty(net) || (net->flags & IFF_ALLMULTI))
> +		cdc_filter |= USB_CDC_PACKET_TYPE_ALL_MULTICAST;
> +
> +	usb_control_msg(dev->udev,
> +			usb_sndctrlpipe(dev->udev, 0),
> +			USB_CDC_SET_ETHERNET_PACKET_FILTER,
> +			USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> +			cdc_filter,
> +			dev->intf->cur_altsetting->desc.bInterfaceNumber,
> +			NULL,
> +			0,
> +			USB_CTRL_SET_TIMEOUT
> +		);
> +}
> +EXPORT_SYMBOL_GPL(usbnet_cdc_update_filter);
> +
> +static void speed_change(struct usbnet *dev, __le32 *speeds)
> +{
> +	dev->tx_speed = __le32_to_cpu(speeds[0]);
> +	dev->rx_speed = __le32_to_cpu(speeds[1]);
> +}
> +
> +void usbnet_cdc_status(struct usbnet *dev, struct urb *urb)
> +{
> +	struct usb_cdc_notification	*event;
> +
> +	if (urb->actual_length < sizeof(*event))
> +		return;
> +
> +	/* SPEED_CHANGE can get split into two 8-byte packets */
> +	if (test_and_clear_bit(EVENT_STS_SPLIT, &dev->flags)) {
> +		speed_change(dev, (__le32 *)urb->transfer_buffer);
> +		return;
> +	}
> +
> +	event = urb->transfer_buffer;
> +	switch (event->bNotificationType) {
> +	case USB_CDC_NOTIFY_NETWORK_CONNECTION:
> +		netif_dbg(dev, timer, dev->net, "CDC: carrier %s\n",
> +			  event->wValue ? "on" : "off");
> +		if (netif_carrier_ok(dev->net) != !!event->wValue)
> +			usbnet_link_change(dev, !!event->wValue, 0);
> +		break;
> +	case USB_CDC_NOTIFY_SPEED_CHANGE:	/* tx/rx rates */
> +		netif_dbg(dev, timer, dev->net, "CDC: speed change (len %d)\n",
> +			  urb->actual_length);
> +		if (urb->actual_length != (sizeof(*event) + 8))
> +			set_bit(EVENT_STS_SPLIT, &dev->flags);
> +		else
> +			speed_change(dev, (__le32 *)&event[1]);
> +		break;

	/*
	 * USB ..
> +	/* USB_CDC_NOTIFY_RESPONSE_AVAILABLE can happen too (e.g. RNDIS),
> +	 * but there are no standard formats for the response data.
> +	 */
> +	default:
> +		netdev_err(dev->net, "CDC: unexpected notification %02x!\n",
> +			   event->bNotificationType);
> +		break;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(usbnet_cdc_status);
>  /*-------------------------------------------------------------------------*/
>  
>  static int __init usbnet_init(void)

You can add my 
Reviewed-by: Manuel Ebner <manuelebner@mailbox.org>

Thanks
 Manuel