netdev - Re: [patch net-next-2.6] vlan: introduce ndo_vlan

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 19 Jul 2011 10:24:58 +0200
From:	Michał Mirosław <mirqus@...il.com>
To:	Jiri Pirko <jpirko@...hat.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net,
	shemminger@...ux-foundation.org, eric.dumazet@...il.com,
	greearb@...delatech.com
Subject: Re: [patch net-next-2.6] vlan: introduce ndo_vlan_[enable/disable]

W dniu 19 lipca 2011 10:13 użytkownik Jiri Pirko <jpirko@...hat.com> napisał:
> Tue, Jul 19, 2011 at 09:24:29AM CEST, mirqus@...il.com wrote:
>>W dniu 18 lipca 2011 09:13 użytkownik Jiri Pirko <jpirko@...hat.com> napisał:
>>>  static u32 atl1c_fix_features(struct net_device *netdev, u32 features)
>>>  {
>>> +       u32 changed = netdev->features ^ features;
>>> +
>>> +       /*
>>> +        * Since there is no support for separate rx/tx vlan accel
>>> +        * enable/disable make sure these are always set in pair.
>>> +        */
>>> +       if ((changed & NETIF_F_HW_VLAN_TX && features & NETIF_F_HW_VLAN_TX) ||
>>> +           (changed & NETIF_F_HW_VLAN_RX && features & NETIF_F_HW_VLAN_RX))
>>> +               features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
>>> +       else
>>> +               features &= ~(NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
>>> +
>>
>>You ignored my hint about combined TX/RX offload. Is that on purpose?
> Sorry but I'm probably missing what you mean.

You could replace above code with:

if (!(features & NETIF_F_HW_VLAN_RX))
  features &= ~NETIF_F_HW_VLAN_TX;

>>Your code will toggle VLAN acceleration on every
>>netdev_update_features call when user requests one offload on and one
>>off.
>
> Well for hw which cannot no/off rx/tx accel separately I thought that if
> user wants one accel on, the accel should be enabled. According to
> following table (bits set when calling driver's fix_features):
>
>  NETIF_F_HW_VLAN_TX  NETIF_F_HW_VLAN_RX -> enable
>  NETIF_F_HW_VLAN_TX !NETIF_F_HW_VLAN_RX -> enable
> !NETIF_F_HW_VLAN_TX  NETIF_F_HW_VLAN_RX -> enable
> !NETIF_F_HW_VLAN_TX !NETIF_F_HW_VLAN_RX -> disable
>
> This looks logical to me...

If user requests eg. +TX -RX, you will have ndo_fix_features called
with +TX -RX, possibly multiple times. This generates this sequence:

current features: -TX,-RX
  -> changed: TX -> return +TX,+RX
  -> set_features +TX,+TX
current_features: +TX,+RX
  -> changed: RX -> return -TX,-RX
  -> set_features -TX,-RX
...

(And you cannot control when fix_features will be called.)

If you don't agree with the hint above (about forcing TX off when RX
is off, leaving the rest alone), then its better to remove TX from
hw_features and copy it's state from RX in ndo_fix_features. Like
this:

[init]
  netdev->hw_features |= NETIF_F_HW_VLAN_RX;
[fix_features]
  if (features & NETIF_F_HW_VLAN_RX)
    features |= NETIF_F_HW_VLAN_TX;
  else
    features &= ~NETIF_F_HW_VLAN_TX;

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html