[PATCH v5 net-next 0/5] usbnet: multicast filter support for cdc ncm devices

[PATCH v5 net-next 0/5] usbnet: multicast filter support for cdc ncm devices

[Bjørn Mork]

This revives a 2 year old patch set from Miguel Rodríguez
Pérez, which appears to have been lost somewhere along the
way.  I've based it on the last version I found (v4), and
added one patch which I believe must have been missing in
the original.

I kept Oliver's ack on one of the patches, since both the patch and
the motivation still is the same.  Hope this is OK..

Thanks to the anonymous user <wxcafe@wxcafe.net> for bringing up this
problem in https://bugs.debian.org/965074

This is only build and load tested by me.  I don't have any device
where I can test the actual functionality.


Changes v5:
 - added missing symbol export
 - formatted patch subjects with subsystem


Bjørn Mork (1):
  net: usbnet: export usbnet_set_rx_mode()

Miguel Rodríguez Pérez (4):
  net: cdc_ether: use dev->intf to get interface information
  net: cdc_ether: export usbnet_cdc_update_filter
  net: cdc_ncm: add .ndo_set_rx_mode to cdc_ncm_netdev_ops
  net: cdc_ncm: hook into set_rx_mode to admit multicast traffic

 drivers/net/usb/cdc_ether.c | 7 +++----
 drivers/net/usb/cdc_ncm.c   | 4 ++++
 drivers/net/usb/usbnet.c    | 3 ++-
 include/linux/usb/usbnet.h  | 2 ++
 4 files changed, 11 insertions(+), 5 deletions(-)

-- 
2.27.0

[PATCH v5 net-next 1/5] net: cdc_ether: use dev->intf to get interface information

[Bjørn Mork]

From: Miguel Rodríguez Pérez <miguel@det.uvigo.gal>

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_ctx instead. Getting intf directly from struct usbnet solves
the problem.

Signed-off-by: Miguel Rodríguez Pérez <miguel@det.uvigo.gal>
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 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 a657943c9f01..2afe258e3648 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -65,8 +65,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
@@ -86,7 +84,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.27.0

[PATCH v5 net-next 2/5] net: cdc_ether: export usbnet_cdc_update_filter

[Bjørn Mork]

From: Miguel Rodríguez Pérez <miguel@det.uvigo.gal>

This makes the function available to other drivers, like cdc_ncm.

Signed-off-by: Miguel Rodríguez Pérez <miguel@det.uvigo.gal>
Acked-by: Oliver Neukum <oneukum@suse.com>
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 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 2afe258e3648..8c1d61c2cbac 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -63,7 +63,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;
 
@@ -90,6 +90,7 @@ static void usbnet_cdc_update_filter(struct usbnet *dev)
 			USB_CTRL_SET_TIMEOUT
 		);
 }
+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 b0bff3083278..3a856963a363 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -207,6 +207,7 @@ struct cdc_state {
 	struct usb_interface		*data;
 };
 
+extern void usbnet_cdc_update_filter(struct usbnet *dev);
 extern int usbnet_generic_cdc_bind(struct usbnet *, struct usb_interface *);
 extern int usbnet_ether_cdc_bind(struct usbnet *dev, struct usb_interface *intf);
 extern int usbnet_cdc_bind(struct usbnet *, struct usb_interface *);
-- 
2.27.0

Re: [PATCH v5 net-next 2/5] net: cdc_ether: export usbnet_cdc_update_filter

[Oliver Neukum]

Am Mittwoch, den 15.07.2020, 20:40 +0200 schrieb Bjørn Mork:
> 
> @@ -90,6 +90,7 @@ static void usbnet_cdc_update_filter(struct usbnet *dev)
>  			USB_CTRL_SET_TIMEOUT
>  		);
>  }
> +EXPORT_SYMBOL_GPL(usbnet_cdc_update_filter);

Hi,

this function is pretty primitive. In fact it more or less
is a straight take from the spec. Can this justify the _GPL
version?

	Regards
		Oliver

Re: [PATCH v5 net-next 2/5] net: cdc_ether: export usbnet_cdc_update_filter

[Bjørn Mork]

On July 21, 2020 11:00:08 AM GMT+02:00, Oliver Neukum <oneukum@suse.de> wrote:
>Am Mittwoch, den 15.07.2020, 20:40 +0200 schrieb Bjørn Mork:
>> 
>> @@ -90,6 +90,7 @@ static void usbnet_cdc_update_filter(struct usbnet
>*dev)
>>  			USB_CTRL_SET_TIMEOUT
>>  		);
>>  }
>> +EXPORT_SYMBOL_GPL(usbnet_cdc_update_filter);
>
>Hi,
>
>this function is pretty primitive. In fact it more or less
>is a straight take from the spec. Can this justify the _GPL
>version?

Maybe not? I must admit I didn't put much thought into it. 

I will not object to changing it. And you're the boss anyway :-)


Bjørn

Re: [PATCH v5 net-next 2/5] net: cdc_ether: export usbnet_cdc_update_filter

[Oliver Neukum]

Am Freitag, den 24.07.2020, 16:18 +0200 schrieb Bjørn Mork:
> 
> On July 21, 2020 11:00:08 AM GMT+02:00, Oliver Neukum <oneukum@suse.de> wrote:
> > Am Mittwoch, den 15.07.2020, 20:40 +0200 schrieb Bjørn Mork:
> > > 
> > > @@ -90,6 +90,7 @@ static void usbnet_cdc_update_filter(struct usbnet
> > 
> > *dev)
> > >  			USB_CTRL_SET_TIMEOUT
> > >  		);
> > >  }
> > > +EXPORT_SYMBOL_GPL(usbnet_cdc_update_filter);
> > 
> > Hi,
> > 
> > this function is pretty primitive. In fact it more or less
> > is a straight take from the spec. Can this justify the _GPL
> > version?
> 
> Maybe not? I must admit I didn't put much thought into it. 
> 
> I will not object to changing it. And you're the boss anyway :-)

Well,

it has been applied. I don't care enough to change it unless
we are violating a policy. I am looking for some ground rules
on that issue.

Leading us to the thorny issue of binary modules, yes I know.
Yet up to now it was my understanding that plain EXPORT_SYMBOL
is the default and EXPORT_SYMBOL_GPL needs a reason.
Now, I like the GPL as much as everybody else and I will
not challenge people on their reason if they state it
and I am willing to assume that there is a reason if the code
behind the symbol is substantial.
My job as maintainer is to check things and to ensure some
consistency. And I am seeing a certain lack of consistency here.
As I do not want to make developers unhappy I would very much
appreciate some guide lines I can point at.

I really want to preclude some lawyers sending me conflicting
patches in the future. I fear this coming.

	Regards
		Oliver

[PATCH v5 net-next 3/5] net: usbnet: export usbnet_set_rx_mode()

[Bjørn Mork]

This function can be reused by other usbnet minidrivers.

Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 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 3a856963a363..2e4f7721fc4e 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -274,6 +274,7 @@ extern int usbnet_set_link_ksettings(struct net_device *net,
 extern u32 usbnet_get_link(struct net_device *net);
 extern u32 usbnet_get_msglevel(struct net_device *);
 extern void usbnet_set_msglevel(struct net_device *, u32);
+extern void usbnet_set_rx_mode(struct net_device *net);
 extern void usbnet_get_drvinfo(struct net_device *, struct ethtool_drvinfo *);
 extern int usbnet_nway_reset(struct net_device *net);
 
-- 
2.27.0

[PATCH v5 net-next 4/5] net: cdc_ncm: add .ndo_set_rx_mode to cdc_ncm_netdev_ops

[Bjørn Mork]

From: Miguel Rodríguez Pérez <miguel@det.uvigo.gal>

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>
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 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 8929669b5e6d..f5d7b933792b 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,
-- 
2.27.0

[PATCH v5 net-next 5/5] net: cdc_ncm: hook into set_rx_mode to admit multicast traffic

[Bjørn Mork]

From: Miguel Rodríguez Pérez <miguel@det.uvigo.gal>

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>
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 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 f5d7b933792b..e04f588538cc 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1896,6 +1896,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 */
@@ -1909,6 +1910,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  */
@@ -1922,6 +1924,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.27.0

Re: [PATCH v5 net-next 0/5] usbnet: multicast filter support for cdc ncm devices

[David Miller]

From: Bjørn Mork <bjorn@mork.no>
Date: Wed, 15 Jul 2020 20:40:55 +0200

> This revives a 2 year old patch set from Miguel Rodríguez
> Pérez, which appears to have been lost somewhere along the
> way.  I've based it on the last version I found (v4), and
> added one patch which I believe must have been missing in
> the original.
> 
> I kept Oliver's ack on one of the patches, since both the patch and
> the motivation still is the same.  Hope this is OK..
> 
> Thanks to the anonymous user <wxcafe@wxcafe.net> for bringing up this
> problem in https://bugs.debian.org/965074
> 
> This is only build and load tested by me.  I don't have any device
> where I can test the actual functionality.
> 
> 
> Changes v5:
>  - added missing symbol export
>  - formatted patch subjects with subsystem

Series applied, thank you.