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

[Jiri Pirko <jpirko@redhat.com>]

Tue, Jul 19, 2011 at 09:24:29AM CEST, mirqus@gmail.com wrote:
>W dniu 18 lipca 2011 09:13 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
>> Sun, Jul 17, 2011 at 11:06:57PM CEST, mirqus@gmail.com wrote:
>>>W dniu 17 lipca 2011 21:44 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
>>>> Sun, Jul 17, 2011 at 10:36:04AM CEST, mirqus@gmail.com wrote:
>>>>>W dniu 17 lipca 2011 09:30 użytkownik Jiri Pirko <jpirko@redhat.com> napisał:
>>>>>> Sat, Jul 16, 2011 at 04:14:36PM CEST, mirqus@gmail.com wrote:
>>>>>>>2011/7/16 Jiri Pirko <jpirko@redhat.com>:
>>>>>>>> Some devices are not able to enable/disable rx/tw vlan accel separately.
>>>>>>>> they depend on ndo_vlan_rx_register to know if to enable of disable
>>>>>>>> hw accel. And since ndo_vlan_rx_register is going to die soon,
>>>>>>>> this must be resolved.
>>>>>>>>
>>>>>>>> One solution might be to enable accel on device start every time, even
>>>>>>>> if there are no vlan up on. But this would change behaviour and might
>>>>>>>> lead to possible regression (on older devices).
>>>>>>>[...]
>>>>>>>
>>>>>>>Please describe the possible regression. As I see it, there won't be
>>>>>>>any user visible change of behaviour - network code takes care of
>>>>>>>reinserting VLAN tag when necessary. If you think that disabling tag
>>>>>>>stripping is beneficial for cases where no VLANs are configured, it's
>>>>>>>better to do that in netdev_fix_features() for devices which advertise
>>>>>>>NETIF_F_HW_VLAN_RX in hw_features.
>>>>>>
>>>>>> Well I just wanted to preserve current behaviour which is that in many
>>>>>> drivers vlan accel is enabled only if some vid is registered upon the
>>>>>> device and it's disabled again when no vid is registered. I can see
>>>>>> no way to do this with current code after removing ndo_vlan_rx_register.
>>>>>>
>>>>>> I expect unexpected
>>>>>
>>>>>:-D
>>>>>
>>>>>> ... problems on old cards when vlan accel would be
>>>>>> enabled all the time, but maybe I'm wrong...
>>>>>
>>>>>Device has no way of knowing how the system uses VLAN tags, stripped
>>>>>or not. Any problems would be driver problems and since you're making
>>>>>it all use generic code, bugs will hit all drivers simultaneously or
>>>>>(preferably) won't happen at all.
>>>>>
>>>>>> One idea is for device which do not support sepatate rx/tx vlan accel
>>>>>> enabling/disabling they can probably use ndo_fix_features force to
>>>>>> enable/disable rx/tx pair together. That would resolve the situation as
>>>>>> well giving user possibility to turn off vlan accel in case of any issues.
>>>>>
>>>>>That is exactly the idea behind ndo_fix_features.
>>>
>>>> In netdev_fix_features add check if either one of NETIF_F_HW_VLAN_TX or
>>>> NETIF_F_HW_VLAN_RX is set and in that case set the other one. Of course
>>>> this would be done only for devices what do not support separate rx/tx
>>>> vlan on/off. But how to distinguish? NETIF_F_HW_VLAN_BOTH feature flag?
>>>
>>>Not in netdev_fix_features(). This case you describe should be handled
>>>in driver-specific
>>
>> Well since the code would be the same in many drivers I was thinking
>> about putting it in general code...
>>
>> Anyway, would you please look at following example patch and tell me if
>> it looks good to you?
>
>[...]
>>  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.

>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...

>
>BTW, the register flag name (MAC_CTRL_RMV_VLAN) suggests that it
>controls only tag stripping. Was it tested or documented that this
>also links with tag insertion?

comment says "/* enable VLAN tag insert/strip */" therefore it looks
like this register controls both.

>
>[...]
>> +static int atl1c_set_features(struct net_device *netdev, u32 features)
>> +{
>> +       u32 changed = netdev->features ^ features;
>> +
>> +       /*
>> +        * Test for NETIF_F_HW_VLAN_TX as it's paired with NETIF_F_HW_VLAN_RX
>> +        * by atl1c_fix_features.
>> +        */
>> +       if (changed & NETIF_F_HW_VLAN_TX)
>> +               atl1c_vlan_mode(netdev, features);
>> +
>
>Test for RX is better, as it will match the name of control bit
>(MAC_CTRL_RMV_VLAN).

Yeah, why not, I think this does not matter.

>
>Best Regards,
>Michał Mirosław