[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

Re: [PATCH v2 1/4] Simplify usbnet_cdc_update_filter

[Greg KH]

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

[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(-)

[PATCH v3 0/4] usbnet: Admit multicast traffic in cdc ncm devices

[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(-)

-- 
2.17.1

[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

[PATCH 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
-- 
2.17.1

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

[v3,4/4] Hook into set_rx_mode to admit multicast traffic

[kbuild test robot]

Hi Miguel,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on v4.18-rc4 next-20180706]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Miguel-Rodr-guez-P-rez/usbnet-Admit-multicast-traffic-in-cdc-ncm-devices/20180701-170924
config: x86_64-randconfig-u0-07091340 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> drivers/net/usb/cdc_ncm.o:(.rodata+0x338): undefined reference to `usbnet_cdc_update_filter'
   drivers/net/usb/cdc_ncm.o:(.rodata+0x3d8): undefined reference to `usbnet_cdc_update_filter'
   drivers/net/usb/cdc_ncm.o:(.rodata+0x478): undefined reference to `usbnet_cdc_update_filter'
---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH v3 4/4] Hook into set_rx_mode to admit multicast traffic

[kbuild test robot]

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

Hi Miguel,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on v4.18-rc4 next-20180706]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Miguel-Rodr-guez-P-rez/usbnet-Admit-multicast-traffic-in-cdc-ncm-devices/20180701-170924
config: x86_64-randconfig-u0-07091340 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> drivers/net/usb/cdc_ncm.o:(.rodata+0x338): undefined reference to `usbnet_cdc_update_filter'
   drivers/net/usb/cdc_ncm.o:(.rodata+0x3d8): undefined reference to `usbnet_cdc_update_filter'
   drivers/net/usb/cdc_ncm.o:(.rodata+0x478): undefined reference to `usbnet_cdc_update_filter'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33140 bytes --]

[v4,3/4] Add .ndo_set_rx_mode to cdc_ncm_netdev_ops

[Miguel Rodríguez Pérez]

This is another alternative, but it would require exporting
usbnet_set_rx_mode. The main drawback of this approach is that it would
need to make use that net_device_ops copy in cdc_ncm remains updated,
should usbnet.c version change.
On the other hand, the structure can be kept const.

Which alternative do you think is best?

On 04/07/18 16:32, Miguel Rodríguez Pérez wrote:
> The cdc_ncm driver overrides the net_device_ops structure used by usbnet
> to be able to hook into .ndo_change_mtu. However, the structure was
> missing the .ndo_set_rx_mode field, preventing the driver from
> hooking into usbnet's set_rx_mode. This patch adds the missing callback to
> usbnet_set_rx_mode in net_device_ops.
> 
> Signed-off-by: Miguel Rodríguez Pérez <miguel@det.uvigo.gal>
> ---
>  drivers/net/usb/cdc_ncm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 9e1b74590682..342bf9bb91b5 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -755,6 +755,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,
>

Re: [PATCH v4 3/4] Add .ndo_set_rx_mode to cdc_ncm_netdev_ops

[Miguel Rodríguez Pérez]

This is another alternative, but it would require exporting
usbnet_set_rx_mode. The main drawback of this approach is that it would
need to make use that net_device_ops copy in cdc_ncm remains updated,
should usbnet.c version change.
On the other hand, the structure can be kept const.

Which alternative do you think is best?

On 04/07/18 16:32, Miguel Rodríguez Pérez wrote:
> The cdc_ncm driver overrides the net_device_ops structure used by usbnet
> to be able to hook into .ndo_change_mtu. However, the structure was
> missing the .ndo_set_rx_mode field, preventing the driver from
> hooking into usbnet's set_rx_mode. This patch adds the missing callback to
> usbnet_set_rx_mode in net_device_ops.
> 
> Signed-off-by: Miguel Rodríguez Pérez <miguel@det.uvigo.gal>
> ---
>  drivers/net/usb/cdc_ncm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 9e1b74590682..342bf9bb91b5 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -755,6 +755,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,
> 

-- 
Miguel Rodríguez Pérez
Laboratorio de Redes
EE Telecomunicación – Universidade de Vigo

[v4,3/4] Add .ndo_set_rx_mode to cdc_ncm_netdev_ops

[Miguel Rodríguez Pérez]

The cdc_ncm driver overrides the net_device_ops structure used by usbnet
to be able to hook into .ndo_change_mtu. However, the structure was
missing the .ndo_set_rx_mode field, preventing the driver from
hooking into usbnet's set_rx_mode. This patch adds the missing callback to
usbnet_set_rx_mode in net_device_ops.

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

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 9e1b74590682..342bf9bb91b5 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -755,6 +755,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,

[PATCH v4 3/4] Add .ndo_set_rx_mode to cdc_ncm_netdev_ops

[Miguel Rodríguez Pérez]

The cdc_ncm driver overrides the net_device_ops structure used by usbnet
to be able to hook into .ndo_change_mtu. However, the structure was
missing the .ndo_set_rx_mode field, preventing the driver from
hooking into usbnet's set_rx_mode. This patch adds the missing callback to
usbnet_set_rx_mode in net_device_ops.

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

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 9e1b74590682..342bf9bb91b5 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -755,6 +755,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,
-- 
2.17.1

[v3,1/4] Simplify usbnet_cdc_update_filter

[Bjørn Mork]

Oliver Neukum <oneukum@suse.com> writes:
> On So, 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>
>
> No,
>
> sorry, but this is not good. The reason is a bit subtle.
> Drivers need to reset the filters when handling post_reset()
> [ and reset_resume() ] usbnet_write_cmd() falls back to
> kmemdup() with GFP_KERNEL. Usbnet is a framework with class
> drivers and some of the devices we drive have a storage
> interface. Thence we are on the block error handling path here.

Right.  I knew there had to be some reason this was left out when the
rest of these were converted.  But I don't think the reason is
valid... usbnet_write_cmd() will never call kmemdup when data is NULL.

There is of course also little advantage in using usbnet_write_cmd when
we don't need to allocate a buffer.  But I'd still prefer to see it for
consistency, power management and debug logging.

Or if it is left as-is: Maybe add a comment so that I don't ask the same
stupid questions in 3 weeks time? :-)  My memory is los^Husy...

> The simplest solution is to leave out this patch in the sequence.

As Miguel noted: That won't work. The switch from dev->data->control to
dev->intf is necessary to make this function reusable by other
minidrivers.




Bjørn
---
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

Re: [PATCH v3 1/4] Simplify usbnet_cdc_update_filter

[Bjørn Mork]

Oliver Neukum <oneukum@suse.com> writes:
> On So, 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>
>
> No,
>
> sorry, but this is not good. The reason is a bit subtle.
> Drivers need to reset the filters when handling post_reset()
> [ and reset_resume() ] usbnet_write_cmd() falls back to
> kmemdup() with GFP_KERNEL. Usbnet is a framework with class
> drivers and some of the devices we drive have a storage
> interface. Thence we are on the block error handling path here.

Right.  I knew there had to be some reason this was left out when the
rest of these were converted.  But I don't think the reason is
valid... usbnet_write_cmd() will never call kmemdup when data is NULL.

There is of course also little advantage in using usbnet_write_cmd when
we don't need to allocate a buffer.  But I'd still prefer to see it for
consistency, power management and debug logging.

Or if it is left as-is: Maybe add a comment so that I don't ask the same
stupid questions in 3 weeks time? :-)  My memory is los^Husy...

> The simplest solution is to leave out this patch in the sequence.

As Miguel noted: That won't work. The switch from dev->data->control to
dev->intf is necessary to make this function reusable by other
minidrivers.




Bjørn

[v4,1/4] Use dev->intf to get interface information

[Miguel Rodríguez Pérez]

usbnet_cdc_update_filter was getting the interface number from the
usb_interface struct in cdc_state->control. However, cdc_ncm does
not initialize that structure in its bind function, but uses
cdc_ncm_cts instead. Getting intf directly from struct usbnet solves
the problem.

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

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 178b956501a7..beac02cbde51 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -77,8 +77,6 @@ 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;
 
 	u16 cdc_filter = USB_CDC_PACKET_TYPE_DIRECTED
@@ -98,7 +96,7 @@ 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,
+			dev->intf->cur_altsetting->desc.bInterfaceNumber,
 			NULL,
 			0,
 			USB_CTRL_SET_TIMEOUT

[PATCH v4 1/4] Use dev->intf to get interface information

[Miguel Rodríguez Pérez]

usbnet_cdc_update_filter was getting the interface number from the
usb_interface struct in cdc_state->control. However, cdc_ncm does
not initialize that structure in its bind function, but uses
cdc_ncm_cts instead. Getting intf directly from struct usbnet solves
the problem.

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

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 178b956501a7..beac02cbde51 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -77,8 +77,6 @@ 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;
 
 	u16 cdc_filter = USB_CDC_PACKET_TYPE_DIRECTED
@@ -98,7 +96,7 @@ 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,
+			dev->intf->cur_altsetting->desc.bInterfaceNumber,
 			NULL,
 			0,
 			USB_CTRL_SET_TIMEOUT
-- 
2.17.1

[v3,1/4] Simplify usbnet_cdc_update_filter

[Miguel Rodríguez Pérez]

I get a panic if I remove this patch, because intf comes NULL for
cdc_ncm devices. I'll send an updated patch that solves this issue while
still using usb_control_msg.

On 02/07/18 10:25, Oliver Neukum wrote:
> On So, 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>
> 
> No,
> 
> sorry, but this is not good. The reason is a bit subtle.
> Drivers need to reset the filters when handling post_reset()
> [ and reset_resume() ] usbnet_write_cmd() falls back to
> kmemdup() with GFP_KERNEL. Usbnet is a framework with class
> drivers and some of the devices we drive have a storage
> interface. Thence we are on the block error handling path here.
> 
> The simplest solution is to leave out this patch in the sequence.
> 
> 	Regards
> 		Oliver
> 
> 
> 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
>

Re: [PATCH v3 1/4] Simplify usbnet_cdc_update_filter

[Miguel Rodríguez Pérez]

I get a panic if I remove this patch, because intf comes NULL for
cdc_ncm devices. I'll send an updated patch that solves this issue while
still using usb_control_msg.

On 02/07/18 10:25, Oliver Neukum wrote:
> On So, 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>
> 
> No,
> 
> sorry, but this is not good. The reason is a bit subtle.
> Drivers need to reset the filters when handling post_reset()
> [ and reset_resume() ] usbnet_write_cmd() falls back to
> kmemdup() with GFP_KERNEL. Usbnet is a framework with class
> drivers and some of the devices we drive have a storage
> interface. Thence we are on the block error handling path here.
> 
> The simplest solution is to leave out this patch in the sequence.
> 
> 	Regards
> 		Oliver
> 
> 
> 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
> 

-- 
Miguel Rodríguez Pérez
Laboratorio de Redes
EE Telecomunicación – Universidade de Vigo

[v3,2/4] Export usbnet_cdc_update_filter

[Oliver Neukum]

On So, 2018-07-01 at 11:05 +0200, Miguel Rodríguez Pérez         wrote:
> This makes the function avaiable to other drivers, like cdn_ncm.
> 
> Signed-off-by: Miguel Rodríguez Pérez <miguel@det.uvigo.gal>
Acked-by: Oliver Neukum <oneukum@suse.com>
---
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

Re: [PATCH v3 2/4] Export usbnet_cdc_update_filter

[Oliver Neukum]

On So, 2018-07-01 at 11:05 +0200, Miguel Rodríguez Pérez         wrote:
> This makes the function avaiable to other drivers, like cdn_ncm.
> 
> Signed-off-by: Miguel Rodríguez Pérez <miguel@det.uvigo.gal>
Acked-by: Oliver Neukum <oneukum@suse.com>

[v3,1/4] Simplify usbnet_cdc_update_filter

[Oliver Neukum]

On So, 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>

No,

sorry, but this is not good. The reason is a bit subtle.
Drivers need to reset the filters when handling post_reset()
[ and reset_resume() ] usbnet_write_cmd() falls back to
kmemdup() with GFP_KERNEL. Usbnet is a framework with class
drivers and some of the devices we drive have a storage
interface. Thence we are on the block error handling path here.

The simplest solution is to leave out this patch in the sequence.

	Regards
		Oliver


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
---
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

Re: [PATCH v3 1/4] Simplify usbnet_cdc_update_filter

[Oliver Neukum]

On So, 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>

No,

sorry, but this is not good. The reason is a bit subtle.
Drivers need to reset the filters when handling post_reset()
[ and reset_resume() ] usbnet_write_cmd() falls back to
kmemdup() with GFP_KERNEL. Usbnet is a framework with class
drivers and some of the devices we drive have a storage
interface. Thence we are on the block error handling path here.

The simplest solution is to leave out this patch in the sequence.

	Regards
		Oliver


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

[v3,4/4] Hook into set_rx_mode to admit multicast traffic

[Miguel Rodríguez Pérez]

We set set_rx_mode to usbnet_cdc_update_filter provided
by cdc_ether that simply admits all multicast traffic
if there is more than one multicast filter configured.

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

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index d6b51e2b9495..fda0af0b5d3c 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1652,6 +1652,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_update_filter,
 };
 
 /* Same as cdc_ncm_info, but with FLAG_WWAN */
@@ -1665,6 +1666,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_update_filter,
 };
 
 /* Same as wwan_info, but with FLAG_NOARP  */
@@ -1678,6 +1680,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_update_filter,
 };
 
 static const struct usb_device_id cdc_devs[] = {

[PATCH v3 4/4] Hook into set_rx_mode to admit multicast traffic

[Miguel Rodríguez Pérez]

We set set_rx_mode to usbnet_cdc_update_filter provided
by cdc_ether that simply admits all multicast traffic
if there is more than one multicast filter configured.

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

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index d6b51e2b9495..fda0af0b5d3c 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1652,6 +1652,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_update_filter,
 };
 
 /* Same as cdc_ncm_info, but with FLAG_WWAN */
@@ -1665,6 +1666,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_update_filter,
 };
 
 /* Same as wwan_info, but with FLAG_NOARP  */
@@ -1678,6 +1680,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_update_filter,
 };
 
 static const struct usb_device_id cdc_devs[] = {
-- 
2.17.1

[v3,3/4] Replace the way cdc_ncm hooks into usbnet_change_mtu

[Miguel Rodríguez Pérez]

Previously cdc_ncm overwrited netdev_ops used by usbnet
thus preventing hooking into set_rx_mode. This patch
preserves usbnet hooks into netdev_ops, and add an
additional one for change_mtu needed by cdc_ncm.

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

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 9e1b74590682..d6b51e2b9495 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -750,16 +750,7 @@ int cdc_ncm_change_mtu(struct net_device *net, int new_mtu)
 }
 EXPORT_SYMBOL_GPL(cdc_ncm_change_mtu);
 
-static const struct net_device_ops cdc_ncm_netdev_ops = {
-	.ndo_open	     = usbnet_open,
-	.ndo_stop	     = usbnet_stop,
-	.ndo_start_xmit	     = usbnet_start_xmit,
-	.ndo_tx_timeout	     = usbnet_tx_timeout,
-	.ndo_get_stats64     = usbnet_get_stats64,
-	.ndo_change_mtu	     = cdc_ncm_change_mtu,
-	.ndo_set_mac_address = eth_mac_addr,
-	.ndo_validate_addr   = eth_validate_addr,
-};
+static struct net_device_ops cdc_ncm_netdev_ops;
 
 int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_altsetting, int drvflags)
 {
@@ -939,6 +930,8 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
 	dev->net->sysfs_groups[0] = &cdc_ncm_sysfs_attr_group;
 
 	/* must handle MTU changes */
+	cdc_ncm_netdev_ops = *dev->net->netdev_ops;
+	cdc_ncm_netdev_ops.ndo_change_mtu = cdc_ncm_change_mtu;
 	dev->net->netdev_ops = &cdc_ncm_netdev_ops;
 	dev->net->max_mtu = cdc_ncm_max_dgram_size(dev) - cdc_ncm_eth_hlen(dev);
 

[PATCH v3 3/4] Replace the way cdc_ncm hooks into usbnet_change_mtu

[Miguel Rodríguez Pérez]

Previously cdc_ncm overwrited netdev_ops used by usbnet
thus preventing hooking into set_rx_mode. This patch
preserves usbnet hooks into netdev_ops, and add an
additional one for change_mtu needed by cdc_ncm.

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

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 9e1b74590682..d6b51e2b9495 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -750,16 +750,7 @@ int cdc_ncm_change_mtu(struct net_device *net, int new_mtu)
 }
 EXPORT_SYMBOL_GPL(cdc_ncm_change_mtu);
 
-static const struct net_device_ops cdc_ncm_netdev_ops = {
-	.ndo_open	     = usbnet_open,
-	.ndo_stop	     = usbnet_stop,
-	.ndo_start_xmit	     = usbnet_start_xmit,
-	.ndo_tx_timeout	     = usbnet_tx_timeout,
-	.ndo_get_stats64     = usbnet_get_stats64,
-	.ndo_change_mtu	     = cdc_ncm_change_mtu,
-	.ndo_set_mac_address = eth_mac_addr,
-	.ndo_validate_addr   = eth_validate_addr,
-};
+static struct net_device_ops cdc_ncm_netdev_ops;
 
 int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_altsetting, int drvflags)
 {
@@ -939,6 +930,8 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
 	dev->net->sysfs_groups[0] = &cdc_ncm_sysfs_attr_group;
 
 	/* must handle MTU changes */
+	cdc_ncm_netdev_ops = *dev->net->netdev_ops;
+	cdc_ncm_netdev_ops.ndo_change_mtu = cdc_ncm_change_mtu;
 	dev->net->netdev_ops = &cdc_ncm_netdev_ops;
 	dev->net->max_mtu = cdc_ncm_max_dgram_size(dev) - cdc_ncm_eth_hlen(dev);
 
-- 
2.17.1

[v3,2/4] Export usbnet_cdc_update_filter

[Miguel Rodríguez Pérez]

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

Signed-off-by: Miguel Rodríguez Pérez <miguel@det.uvigo.gal>
---
 drivers/net/usb/cdc_ether.c | 3 ++-
 include/linux/usb/usbnet.h  | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 815ed0dc18fe..54472ab77b90 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -75,7 +75,7 @@ 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 net_device *net = dev->net;
 
@@ -99,6 +99,7 @@ static void usbnet_cdc_update_filter(struct usbnet *dev)
 			NULL,
 			0);
 }
+EXPORT_SYMBOL_GPL(usbnet_cdc_update_filter);
 
 /* probes control interface, claims data interface, collects the bulk
  * endpoints, activates data interface (if needed), maybe sets MTU.
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index e2ec3582e549..7821cf1dcd60 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -286,4 +286,5 @@ 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 *);
 #endif /* __LINUX_USB_USBNET_H */

[PATCH v3 2/4] Export usbnet_cdc_update_filter

[Miguel Rodríguez Pérez]

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

Signed-off-by: Miguel Rodríguez Pérez <miguel@det.uvigo.gal>
---
 drivers/net/usb/cdc_ether.c | 3 ++-
 include/linux/usb/usbnet.h  | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 815ed0dc18fe..54472ab77b90 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -75,7 +75,7 @@ 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 net_device *net = dev->net;
 
@@ -99,6 +99,7 @@ static void usbnet_cdc_update_filter(struct usbnet *dev)
 			NULL,
 			0);
 }
+EXPORT_SYMBOL_GPL(usbnet_cdc_update_filter);
 
 /* probes control interface, claims data interface, collects the bulk
  * endpoints, activates data interface (if needed), maybe sets MTU.
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index e2ec3582e549..7821cf1dcd60 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -286,4 +286,5 @@ 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 *);
 #endif /* __LINUX_USB_USBNET_H */
-- 
2.17.1

[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

[PATCH 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
-- 
2.17.1

-- 
Miguel Rodríguez Pérez
Laboratorio de Redes
EE Telecomunicación – Universidade de Vigo