On 30/06/18 14:17, Bjørn Mork wrote:
> Miguel Rodríguez Pérez <miguel@xxxxxxxxxxxxx> 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
>
--
Miguel Rodríguez Pérez
Laboratorio de Redes
EE Telecomunicación – Universidade de Vigo
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
