Re: [patch net-next-2.6] vlan: introduce ndo_vlan_[enable/disable]

W dniu 19 lipca 2011 10:13 użytkownik Jiri Pirko [off-list ref] napisał:

Tue, Jul 19, 2011 at 09:24:29AM CEST, mirqus@gmail.com wrote:

quoted

W dniu 18 lipca 2011 09:13 użytkownik Jiri Pirko [off-list ref] napisał:

quoted

 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;

quoted

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