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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Tue, 19 Jul 2011 09:24:29 +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 18 lipca 2011 09:13 użytkownik Jiri Pirko <jpirko@...hat.com> napisał:
> Sun, Jul 17, 2011 at 11:06:57PM CEST, mirqus@...il.com wrote:
>>W dniu 17 lipca 2011 21:44 użytkownik Jiri Pirko <jpirko@...hat.com> napisał:
>>> Sun, Jul 17, 2011 at 10:36:04AM CEST, mirqus@...il.com wrote:
>>>>W dniu 17 lipca 2011 09:30 użytkownik Jiri Pirko <jpirko@...hat.com> napisał:
>>>>> Sat, Jul 16, 2011 at 04:14:36PM CEST, mirqus@...il.com wrote:
>>>>>>2011/7/16 Jiri Pirko <jpirko@...hat.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?
Your code will toggle VLAN acceleration on every
netdev_update_features call when user requests one offload on and one
off.

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?

[...]
> +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).

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