Date:	Sun, 17 Jul 2011 23:06:57 +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 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
ndo_fix_features callback. Because if NETIF_F_HW_VLAN_TX is disabled,
the network core will insert the tag anyway, this would cover the
restriction:

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

This works, because if HW_VLAN_TX is disabled, the driver never gets
packets which need tag insertion. Besides those already in the queue,
of course.

For netdev_fix_features() I thought that if no VLANs are configured, then it
could disable NETIF_F_HW_VLAN_RX (if set in hw_features). The
conditions for disabling it need some thought. For example, if the
device is not slave to some stacking device (bonding doesn't count, as
it passes VLAN registeration to its slaves) then packets with tags
would be either dropped or have tag reinserted (for network taps).

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

Powered by blists - more mailing lists