[v2,1/4] Simplify usbnet_cdc_update_filter

[v2,1/4] Simplify usbnet_cdc_update_filter

[Greg Kroah-Hartman]

On Sat, Jun 30, 2018 at 07:32:23PM +0200, Miguel Rodríguez Pérez wrote:
> Remove some unneeded varibles to make the code easier to read
> and, replace the generic usb_control_msg function for the
> more specific usbnet_write_cmd.
> ---
>  drivers/net/usb/cdc_ether.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)

None of your patches have a signed-off-by line, and your subject needs
work as well (see other patches for how to do this with the correct
prefix.)

Also work on cc: the correct people, scripts/get_maintainer.pl on the
patch will tell you that, and also use 'git send-email' to properly
thread them.

thanks,

greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[v2,1/4] Simplify usbnet_cdc_update_filter

[Miguel Rodríguez Pérez]

I have reworked the patches according to the previous round of revisions.
I hope everything is formated correctly this time.

Miguel Rodríguez Pérez (4):
  Simplify usbnet_cdc_update_filter
  Export usbnet_cdc_update_filter
  Replace the way cdc_ncm hooks into usbnet_change_mtu
  Hook into set_rx_mode to admit multicast traffic

 drivers/net/usb/cdc_ether.c | 18 +++++++-----------
 drivers/net/usb/cdc_ncm.c   | 16 ++++++----------
 include/linux/usb/usbnet.h  |  1 +
 3 files changed, 14 insertions(+), 21 deletions(-)

[v3,1/4] Simplify usbnet_cdc_update_filter

[Miguel Rodríguez Pérez]

Remove some unneded varibles to make the code easier to read
and, replace the generic usb_control_msg function for the
more specific usbnet_write_cmd.

Signed-off-by: Miguel Rodríguez Pérez <miguel@det.uvigo.gal>
---
 drivers/net/usb/cdc_ether.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 178b956501a7..815ed0dc18fe 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -77,9 +77,7 @@ static const u8 mbm_guid[16] = {
 
 static void usbnet_cdc_update_filter(struct usbnet *dev)
 {
-	struct cdc_state	*info = (void *) &dev->data;
-	struct usb_interface	*intf = info->control;
-	struct net_device	*net = dev->net;
+	struct net_device *net = dev->net;
 
 	u16 cdc_filter = USB_CDC_PACKET_TYPE_DIRECTED
 			| USB_CDC_PACKET_TYPE_BROADCAST;
@@ -93,16 +91,13 @@ static void usbnet_cdc_update_filter(struct usbnet *dev)
 	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),
+	usbnet_write_cmd(dev,
 			USB_CDC_SET_ETHERNET_PACKET_FILTER,
-			USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+			USB_TYPE_CLASS | USB_DIR_OUT | USB_RECIP_INTERFACE,
 			cdc_filter,
-			intf->cur_altsetting->desc.bInterfaceNumber,
+			dev->intf->cur_altsetting->desc.bInterfaceNumber,
 			NULL,
-			0,
-			USB_CTRL_SET_TIMEOUT
-		);
+			0);
 }
 
 /* probes control interface, claims data interface, collects the bulk

Re: [PATCH v3 1/4] Simplify usbnet_cdc_update_filter

[Wxcafé]

[-- Attachment #1: Type: text/plain, Size: 2497 bytes --]

Hey,

I've encountered that same problem a few days ago, found this thread
and these (unmerged) patches, "ported" them to a more current version
of the kernel (wasn't much work, let's be honest), and I was wondering
if it would be possible to get them merged, because this is still a
problem with cdc_ncm.

Here's the three "up to date" patches (based on 5.8-rc5), they work
insofar as I'm running with them, the bug isn't there anymore (I get
ethernet multicast packets correctly), and there's no obvious bug. I'm
not a dev though, so I have no idea if these are formatted correctly,
if the patch separation is correct, or anything like that, I just think
it would be great if this could be merged into mainline!

On Sun, 2018-07-01 at 11:05 +0200, Miguel Rodríguez Pérez wrote:
> Remove some unneded varibles to make the code easier to read
> and, replace the generic usb_control_msg function for the
> more specific usbnet_write_cmd.
> 
> Signed-off-by: Miguel Rodríguez Pérez <miguel@det.uvigo.gal>
> NACKED-BY: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/net/usb/cdc_ether.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/usb/cdc_ether.c
> b/drivers/net/usb/cdc_ether.c
> index 178b956501a7..815ed0dc18fe 100644
> --- a/drivers/net/usb/cdc_ether.c
> +++ b/drivers/net/usb/cdc_ether.c
> @@ -77,9 +77,7 @@ static const u8 mbm_guid[16] = {
>  
>  static void usbnet_cdc_update_filter(struct usbnet *dev)
>  {
> -	struct cdc_state	*info = (void *) &dev->data;
> -	struct usb_interface	*intf = info->control;
> -	struct net_device	*net = dev->net;
> +	struct net_device *net = dev->net;
>  
>  	u16 cdc_filter = USB_CDC_PACKET_TYPE_DIRECTED
>  			| USB_CDC_PACKET_TYPE_BROADCAST;
> @@ -93,16 +91,13 @@ static void usbnet_cdc_update_filter(struct
> usbnet *dev)
>  	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),
> +	usbnet_write_cmd(dev,
>  			USB_CDC_SET_ETHERNET_PACKET_FILTER,
> -			USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> +			USB_TYPE_CLASS | USB_DIR_OUT |
> USB_RECIP_INTERFACE,
>  			cdc_filter,
> -			intf->cur_altsetting->desc.bInterfaceNumber,
> +			dev->intf->cur_altsetting-
> >desc.bInterfaceNumber,
>  			NULL,
> -			0,
> -			USB_CTRL_SET_TIMEOUT
> -		);
> +			0);
>  }
>  
>  /* probes control interface, claims data interface, collects the
> bulk
-- 
Wxcafé <wxcafe@wxcafe.net>

[-- Attachment #2: 0001-net-cdc_ether-export-generic-usbnet_cdc_update_filte.patch --]
[-- Type: text/x-patch, Size: 3891 bytes --]

From fb4c24637de82a81374622d3f2f96452e0fb07d2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Wxcaf=C3=A9?= <wxcafe@wxcafe.net>
Date: Mon, 13 Jul 2020 16:16:14 -0400
Subject: [PATCH 1/3] net: cdc_ether: export generic usbnet_cdc_update_filter

This makes the function avaiable to other drivers, like cdn_ncm.

Other drivers will use different dev->data types, so the exported
function must not use it; instead the exported function takes an
additional pointer to the control interface.

Within cdc_ether the control interface is still taken from the control
field from struct cdc_state stored in dev->data.
---
 drivers/net/usb/cdc_ether.c | 24 ++++++++++++++++--------
 include/linux/usb/usbnet.h  |  2 ++
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index a657943c9f01..b201adc885f6 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -63,10 +63,8 @@ static const u8 mbm_guid[16] = {
 	0xa6, 0x07, 0xc0, 0xff, 0xcb, 0x7e, 0x39, 0x2a,
 };
 
-static void usbnet_cdc_update_filter(struct usbnet *dev)
+void usbnet_cdc_update_filter(struct usbnet *dev, struct usb_interface *control)
 {
-	struct cdc_state	*info = (void *) &dev->data;
-	struct usb_interface	*intf = info->control;
 	struct net_device	*net = dev->net;
 
 	u16 cdc_filter = USB_CDC_PACKET_TYPE_DIRECTED
@@ -86,12 +84,22 @@ static void usbnet_cdc_update_filter(struct usbnet *dev)
 			USB_CDC_SET_ETHERNET_PACKET_FILTER,
 			USB_TYPE_CLASS | USB_RECIP_INTERFACE,
 			cdc_filter,
-			intf->cur_altsetting->desc.bInterfaceNumber,
+			control->cur_altsetting->desc.bInterfaceNumber,
 			NULL,
 			0,
 			USB_CTRL_SET_TIMEOUT
 		);
 }
+EXPORT_SYMBOL_GPL(usbnet_cdc_update_filter);
+
+/* the control interface is not always necessarily the probed interface
+ * dev->intf, see rndis handling in usbnet_generic_cdc_bind.
+ */
+static void usbnet_cdc_ether_update_filter(struct usbnet *dev) {
+	struct cdc_state *info = (void *)&dev->data;
+
+	usbnet_cdc_update_filter(dev, info->control);
+}
 
 /* probes control interface, claims data interface, collects the bulk
  * endpoints, activates data interface (if needed), maybe sets MTU.
@@ -336,7 +344,7 @@ int usbnet_ether_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 	 * don't do reset all the way. So the packet filter should
 	 * be set to a sane initial value.
 	 */
-	usbnet_cdc_update_filter(dev);
+	usbnet_cdc_ether_update_filter(dev);
 
 bail_out:
 	return rv;
@@ -514,7 +522,7 @@ static const struct driver_info	cdc_info = {
 	.bind =		usbnet_cdc_bind,
 	.unbind =	usbnet_cdc_unbind,
 	.status =	usbnet_cdc_status,
-	.set_rx_mode =	usbnet_cdc_update_filter,
+	.set_rx_mode =	usbnet_cdc_ether_update_filter,
 	.manage_power =	usbnet_manage_power,
 };
 
@@ -524,7 +532,7 @@ static const struct driver_info	zte_cdc_info = {
 	.bind =		usbnet_cdc_zte_bind,
 	.unbind =	usbnet_cdc_unbind,
 	.status =	usbnet_cdc_zte_status,
-	.set_rx_mode =	usbnet_cdc_update_filter,
+	.set_rx_mode =	usbnet_cdc_ether_update_filter,
 	.manage_power =	usbnet_manage_power,
 	.rx_fixup = usbnet_cdc_zte_rx_fixup,
 };
@@ -535,7 +543,7 @@ static const struct driver_info wwan_info = {
 	.bind =		usbnet_cdc_bind,
 	.unbind =	usbnet_cdc_unbind,
 	.status =	usbnet_cdc_status,
-	.set_rx_mode =	usbnet_cdc_update_filter,
+	.set_rx_mode =	usbnet_cdc_ether_update_filter,
 	.manage_power =	usbnet_manage_power,
 };
 
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index b0bff3083278..387f3da06e9d 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -286,4 +286,6 @@ extern void usbnet_update_max_qlen(struct usbnet *dev);
 extern void usbnet_get_stats64(struct net_device *dev,
 			       struct rtnl_link_stats64 *stats);
 
+extern void usbnet_cdc_update_filter(struct usbnet *, struct usb_interface *);
+
 #endif /* __LINUX_USB_USBNET_H */
-- 
2.27.0


[-- Attachment #3: 0002-net-usbnet-export-usbnet_set_rx_mode.patch --]
[-- Type: text/x-patch, Size: 1568 bytes --]

From 315f7e56a4a49e9cbafbd9a88444f8b213524d4a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Wxcaf=C3=A9?= <wxcafe@wxcafe.net>
Date: Mon, 13 Jul 2020 16:17:21 -0400
Subject: [PATCH 2/3] net: usbnet: export usbnet_set_rx_mode

---
 drivers/net/usb/usbnet.c   | 3 ++-
 include/linux/usb/usbnet.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 5ec97def3513..e45935a5856a 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1108,12 +1108,13 @@ static void __handle_link_change(struct usbnet *dev)
 	clear_bit(EVENT_LINK_CHANGE, &dev->flags);
 }
 
-static void usbnet_set_rx_mode(struct net_device *net)
+void usbnet_set_rx_mode(struct net_device *net)
 {
 	struct usbnet		*dev = netdev_priv(net);
 
 	usbnet_defer_kevent(dev, EVENT_SET_RX_MODE);
 }
+EXPORT_SYMBOL_GPL(usbnet_set_rx_mode);
 
 static void __handle_set_rx_mode(struct usbnet *dev)
 {
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 387f3da06e9d..455b568ff4dd 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -254,6 +254,7 @@ extern int usbnet_stop(struct net_device *net);
 extern netdev_tx_t usbnet_start_xmit(struct sk_buff *skb,
 				     struct net_device *net);
 extern void usbnet_tx_timeout(struct net_device *net, unsigned int txqueue);
+extern void usbnet_set_rx_mode(struct net_device *net);
 extern int usbnet_change_mtu(struct net_device *net, int new_mtu);
 
 extern int usbnet_get_endpoints(struct usbnet *, struct usb_interface *);
-- 
2.27.0


[-- Attachment #4: 0003-net-cdc_ncm-hook-into-set_rx_mode-to-admit-multicast.patch --]
[-- Type: text/x-patch, Size: 2207 bytes --]

From b716a0fa777a9d7dee854b768efc3fff9074a2b7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Wxcaf=C3=A9?= <wxcafe@wxcafe.net>
Date: Mon, 13 Jul 2020 16:20:41 -0400
Subject: [PATCH 3/3] net: cdc_ncm: hook into set_rx_mode to admit multicast

Use usbnet_cdc_update_filter from cdc_ether to admit all multicast
traffic if there is more than one multicast filter configured.
---
 drivers/net/usb/cdc_ncm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 8929669b5e6d..688d7e9df41e 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -792,6 +792,7 @@ static const struct net_device_ops cdc_ncm_netdev_ops = {
 	.ndo_stop	     = usbnet_stop,
 	.ndo_start_xmit	     = usbnet_start_xmit,
 	.ndo_tx_timeout	     = usbnet_tx_timeout,
+	.ndo_set_rx_mode     = usbnet_set_rx_mode,
 	.ndo_get_stats64     = usbnet_get_stats64,
 	.ndo_change_mtu	     = cdc_ncm_change_mtu,
 	.ndo_set_mac_address = eth_mac_addr,
@@ -1885,6 +1886,13 @@ static void cdc_ncm_status(struct usbnet *dev, struct urb *urb)
 	}
 }
 
+/* the control interface is always the probed one */
+static void usbnet_cdc_ncm_update_filter(struct usbnet *dev)
+{
+	usbnet_cdc_update_filter(dev, dev->intf);
+}
+
+
 static const struct driver_info cdc_ncm_info = {
 	.description = "CDC NCM",
 	.flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET
@@ -1895,6 +1903,7 @@ static const struct driver_info cdc_ncm_info = {
 	.status = cdc_ncm_status,
 	.rx_fixup = cdc_ncm_rx_fixup,
 	.tx_fixup = cdc_ncm_tx_fixup,
+	.set_rx_mode = usbnet_cdc_ncm_update_filter,
 };
 
 /* Same as cdc_ncm_info, but with FLAG_WWAN */
@@ -1908,6 +1917,7 @@ static const struct driver_info wwan_info = {
 	.status = cdc_ncm_status,
 	.rx_fixup = cdc_ncm_rx_fixup,
 	.tx_fixup = cdc_ncm_tx_fixup,
+	.set_rx_mode = usbnet_cdc_ncm_update_filter,
 };
 
 /* Same as wwan_info, but with FLAG_NOARP  */
@@ -1921,6 +1931,7 @@ static const struct driver_info wwan_noarp_info = {
 	.status = cdc_ncm_status,
 	.rx_fixup = cdc_ncm_rx_fixup,
 	.tx_fixup = cdc_ncm_tx_fixup,
+	.set_rx_mode = usbnet_cdc_ncm_update_filter,
 };
 
 static const struct usb_device_id cdc_devs[] = {
-- 
2.27.0

Re: [PATCH v3 1/4] Simplify usbnet_cdc_update_filter

[Greg KH]

On Mon, Jul 13, 2020 at 04:43:11PM -0400, Wxcafé wrote:
> Hey,
> 
> I've encountered that same problem a few days ago, found this thread
> and these (unmerged) patches, "ported" them to a more current version
> of the kernel (wasn't much work, let's be honest), and I was wondering
> if it would be possible to get them merged, because this is still a
> problem with cdc_ncm.
> 
> Here's the three "up to date" patches (based on 5.8-rc5), they work
> insofar as I'm running with them, the bug isn't there anymore (I get
> ethernet multicast packets correctly), and there's no obvious bug. I'm
> not a dev though, so I have no idea if these are formatted correctly,
> if the patch separation is correct, or anything like that, I just think
> it would be great if this could be merged into mainline!

You need to submit them in a format they can be applied in, as well as
taking care of the issues that caused Oliver to not agree with them.

thanks,

greg k-h

Re: [PATCH v3 1/4] Simplify usbnet_cdc_update_filter

[Wxcafé]

Hi Greg,

Thanks for your feedback! I'm not sure what you mean by "a format they
can be applied in", I'm guessing as three emails, with a single patch
each?

As for the issues that you mention, I'm guessing you mean applying v4
of Miguel's patches as found here (
https://patchwork.kernel.org/patch/10501163/, 
https://patchwork.kernel.org/patch/10507009/) rather than v3 as I did
here (I didn't notice there was a v4 fixing those issues, and reading
the previous messages in this thread I assumed they weren't considered
blocking. Or is there a problem left with v4 of the patches? There
doesn't seem to be any replies to the messages from 2 years ago)

Sorry for all my questions, I'm totally new to all this process...

Cheers

On Tue, 2020-07-14 at 08:06 +0200, Greg KH wrote:
> On Mon, Jul 13, 2020 at 04:43:11PM -0400, Wxcafé wrote:
> > Hey,
> > 
> > I've encountered that same problem a few days ago, found this
> > thread
> > and these (unmerged) patches, "ported" them to a more current
> > version
> > of the kernel (wasn't much work, let's be honest), and I was
> > wondering
> > if it would be possible to get them merged, because this is still a
> > problem with cdc_ncm.
> > 
> > Here's the three "up to date" patches (based on 5.8-rc5), they work
> > insofar as I'm running with them, the bug isn't there anymore (I
> > get
> > ethernet multicast packets correctly), and there's no obvious bug.
> > I'm
> > not a dev though, so I have no idea if these are formatted
> > correctly,
> > if the patch separation is correct, or anything like that, I just
> > think
> > it would be great if this could be merged into mainline!
> 
> You need to submit them in a format they can be applied in, as well
> as
> taking care of the issues that caused Oliver to not agree with them.
> 
> thanks,
> 
> greg k-h
-- 
Wxcafé <wxcafe@wxcafe.net>

Re: [PATCH v3 1/4] Simplify usbnet_cdc_update_filter

[Greg KH]

On Tue, Jul 14, 2020 at 11:13:18AM -0400, Wxcafé wrote:
> Hi Greg,
> 
> Thanks for your feedback! I'm not sure what you mean by "a format they
> can be applied in", I'm guessing as three emails, with a single patch
> each?

We have loads of documentation about how to do this in the kernel tree,
you might want to read up on it starting at Documentation/process/

thanks!

greg k-h

[v2,1/4] Simplify usbnet_cdc_update_filter

[Miguel Rodríguez Pérez]

Remove some unneeded varibles to make the code easier to read
and, replace the generic usb_control_msg function for the
more specific usbnet_write_cmd.
---
 drivers/net/usb/cdc_ether.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 178b956501a7..815ed0dc18fe 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -77,9 +77,7 @@ static const u8 mbm_guid[16] = {

 static void usbnet_cdc_update_filter(struct usbnet *dev)
 {
-	struct cdc_state	*info = (void *) &dev->data;
-	struct usb_interface	*intf = info->control;
-	struct net_device	*net = dev->net;
+	struct net_device *net = dev->net;

 	u16 cdc_filter = USB_CDC_PACKET_TYPE_DIRECTED
 			| USB_CDC_PACKET_TYPE_BROADCAST;
@@ -93,16 +91,13 @@ static void usbnet_cdc_update_filter(struct usbnet *dev)
 	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),
+	usbnet_write_cmd(dev,
 			USB_CDC_SET_ETHERNET_PACKET_FILTER,
-			USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+			USB_TYPE_CLASS | USB_DIR_OUT | USB_RECIP_INTERFACE,
 			cdc_filter,
-			intf->cur_altsetting->desc.bInterfaceNumber,
+			dev->intf->cur_altsetting->desc.bInterfaceNumber,
 			NULL,
-			0,
-			USB_CTRL_SET_TIMEOUT
-		);
+			0);
 }

 /* probes control interface, claims data interface, collects the bulk