| Message ID | ac0097e4-0c3a-1a06-12f4-8cb7fcf07eb1@det.uvigo.gal (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Miguel Rodríguez Pérez <miguel@det.uvigo.gal> writes: > +static void cdc_ncm_update_filter(struct usbnet *dev) > +{ > + struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0]; > + u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber; > + struct net_device *net = dev->net; Why not change usbnet_cdc_update_filter to get the interface number from dev->intf instead? Then you can export it and reuse it here instead of creating a copy. And it will also work for any other usbnet minidriver. I.e. change this: static void usbnet_cdc_update_filter(struct usbnet *dev) { struct cdc_state *info = (void *) &dev->data; struct usb_interface *intf = info->control; into: static void usbnet_cdc_update_filter(struct usbnet *dev) { struct usb_interface *intf = dev->intf; or simply use dev->intf to deref directly into an interface number, like you do in your version. The local 'intf' variable doesn't do much good here. > + > + 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; > + > + usbnet_write_cmd(dev, This is a nice change. It should be probably done in usbnet_cdc_update_filter in any case. Unless there is some reason it doesn't alreay use usbnet_write_cmd? > + USB_CDC_SET_ETHERNET_PACKET_FILTER, > + USB_TYPE_CLASS | USB_DIR_OUT | USB_RECIP_INTERFACE, > + cdc_filter, > + iface_no, > + NULL, > + 0); > +} > + > static const struct ethtool_ops cdc_ncm_ethtool_ops = { > .get_link = usbnet_get_link, > .nway_reset = usbnet_nway_reset, > @@ -1652,6 +1679,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 = cdc_ncm_update_filter, > }; > > /* Same as cdc_ncm_info, but with FLAG_WWAN */ As the comment indicates: There are more 'struct driver_info' variants here. Please add .set_rx_mode to all of them, unless you have a reason not to? 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
On 30/06/18 14:17, Bjørn Mork wrote: > Miguel Rodríguez Pérez <miguel@det.uvigo.gal> writes: > >> +static void cdc_ncm_update_filter(struct usbnet *dev) >> +{ >> + struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0]; >> + u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber; >> + struct net_device *net = dev->net; > > Why not change usbnet_cdc_update_filter to get the interface number from > dev->intf instead? Then you can export it and reuse it here instead of > creating a copy. And it will also work for any other usbnet minidriver. Well, I just wanted to limit my changes to the minimum, as I lack experience with kernel development. Where should I declare usbnet_cdc_update then? Would linux/usb/cdc.h the right place? Is it enough to do EXPORT_GPL_SYMBOL(usbnet_cdc_update_filter) in cdc_ether.c for it to be loaded automatically from cdc_ncm.c? > > I.e. change this: > > static void usbnet_cdc_update_filter(struct usbnet *dev) > { > struct cdc_state *info = (void *) &dev->data; > struct usb_interface *intf = info->control; > > into: > > static void usbnet_cdc_update_filter(struct usbnet *dev) > { > struct usb_interface *intf = dev->intf; > > > or simply use dev->intf to deref directly into an interface number, like > you do in your version. The local 'intf' variable doesn't do much > good here. > Ok, done. > > >> + >> + 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; >> + >> + usbnet_write_cmd(dev, > > This is a nice change. It should be probably done in > usbnet_cdc_update_filter in any case. Unless there is some reason it > doesn't alreay use usbnet_write_cmd? I don't know of any, so I'll keep it changed. > >> + USB_CDC_SET_ETHERNET_PACKET_FILTER, >> + USB_TYPE_CLASS | USB_DIR_OUT | USB_RECIP_INTERFACE, >> + cdc_filter, >> + iface_no, >> + NULL, >> + 0); >> +} >> + >> static const struct ethtool_ops cdc_ncm_ethtool_ops = { >> .get_link = usbnet_get_link, >> .nway_reset = usbnet_nway_reset, >> @@ -1652,6 +1679,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 = cdc_ncm_update_filter, >> }; >> >> /* Same as cdc_ncm_info, but with FLAG_WWAN */ > > > As the comment indicates: There are more 'struct driver_info' variants > here. Please add .set_rx_mode to all of them, unless you have a reason > not to? I simply lack access to the hardware to test and I didn't want to break anybody else's hardware. In any case, the change seems quite safe, so I modified them all too. I will for your comments before sending the updated version of the patches. > > > Bjørn >
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c index d6b51e2b9495..50af1d9d0102 100644 --- a/drivers/net/usb/cdc_ncm.c +++ b/drivers/net/usb/cdc_ncm.c @@ -132,6 +132,33 @@ static void cdc_ncm_get_strings(struct net_device __always_unused *netdev, u32 s static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx); +static void cdc_ncm_update_filter(struct usbnet *dev) +{ + struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0]; + u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber; + 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; + + usbnet_write_cmd(dev, + USB_CDC_SET_ETHERNET_PACKET_FILTER, + USB_TYPE_CLASS | USB_DIR_OUT | USB_RECIP_INTERFACE, + cdc_filter, + iface_no, + NULL, + 0); +} + static const struct ethtool_ops cdc_ncm_ethtool_ops = {
Some CDC_NCM devices are used as docks for laptops. In this case, it makes sense to accept multicast Ethernet traffic, as these devices can reside in a proper LAN. Without this, mDNS or IPv6 simply do not work. Signed-off-by: Miguel Rodríguez Pérez <miguel@det.uvigo.gal> --- drivers/net/usb/cdc_ncm.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) .get_link = usbnet_get_link, .nway_reset = usbnet_nway_reset, @@ -1652,6 +1679,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 = cdc_ncm_update_filter, }; /* Same as cdc_ncm_info, but with FLAG_WWAN */