Re: [net-next 08/12] ixgb: convert to new VLAN model

On Sun, Jan 23, 2011 at 4:25 PM, Tantilov, Emil S
[off-list ref] wrote:

Jesse Gross wrote:

quoted

On Thu, Jan 6, 2011 at 7:29 PM,  [off-list ref] wrote:

quoted

+static int ixgb_set_flags(struct net_device *netdev, u32 data) +{
+       struct ixgb_adapter *adapter = netdev_priv(netdev); +
bool need_reset; +       int rc;
+
+       /*
+        * TX vlan insertion does not work per HW design when Rx
stripping is +        * disabled.  Disable txvlan when rxvlan is
off. +        */ +       if ((data & ETH_FLAG_RXVLAN) !=
(netdev->features & NETIF_F_HW_VLAN_RX)) +               data ^=
ETH_FLAG_TXVLAN;

Does this really do the right thing?  If the RX vlan setting is
changed, it will do the opposite of what the user requested for TX
vlan?

So if I start with both on (the default) and turn them both off in one
command (a valid setting), I will get RX off and TX on (an invalid
setting).

Why not:

if (!(data & ETH_FLAG_RXVLAN))
        data &= ~ETH_FLAG_TXVLAN;

Yeah that works for disabling rxvlan, but what if rxvlan is disabled, and the user attempts to enable txvlan? At least our validation argued that we should make it work both ways. Perhaps something like the following?

       if (!(data & ETH_FLAG_RXVLAN) &&
          (netdev->features & NETIF_F_HW_VLAN_TX))
               data &= ~ETH_FLAG_TXVLAN;
       else if (data & ETH_FLAG_TXVLAN)
               data |= ETH_FLAG_RXVLAN;

I think the logic above does what you describe and will always result
in a consistent state.  Turning dependent features on when needed is a
little bit inconsistent with the rest of Ethtool (for example, turning
on TSO when checksum offloading is off will not enable checksum
offloading, it will produce an error).  However, I know that drivers
aren't completely consistent here and the most important part is that
it enforces valid states, so I don't have a strong opinion.  Ben's
previous suggestion of Ethtool querying again after the operation and
reporting any flags that were automatically changed would help a lot
here.