Re: [PATCH] ixgb: Convert to new vlan model.

[Jesse Gross <jesse@nicira.com>]

On Wed, Dec 15, 2010 at 10:09 AM, Tantilov, Emil S
<emil.s.tantilov@intel.com> wrote:
> Jesse Gross wrote:
>> On Tue, Dec 14, 2010 at 11:15 AM, Ben Hutchings
>> <bhutchings@solarflare.com> wrote:
>>> On Tue, 2010-12-14 at 12:08 -0700, Tantilov, Emil S wrote:
>>>> Ben Hutchings wrote:
>>>>> On Tue, 2010-12-14 at 11:09 -0700, Tantilov, Emil S wrote:
>>>>>> Ben Hutchings wrote:
>>>>>>> On Mon, 2010-12-13 at 19:42 -0800, Jesse Gross wrote:
>>>>>>>> This switches the ixgb driver to use the new vlan interfaces.
>>>>>>>> In doing this, it completes the work begun in
>>>>>>>> ae54496f9e8d40c89e5668205c181dccfa9ecda1 allowing the use of
>>>>>>>> hardware vlan insertion without having a vlan group configured.
>>>>>>>> [...] diff --git a/drivers/net/ixgb/ixgb_ethtool.c
>>>>>>>> b/drivers/net/ixgb/ixgb_ethtool.c
>>>>>>>> index 43994c1..0e4c527 100644
>>>>>>>> --- a/drivers/net/ixgb/ixgb_ethtool.c
>>>>>>>> +++ b/drivers/net/ixgb/ixgb_ethtool.c
>>>>>>>> @@ -706,6 +706,45 @@ ixgb_get_strings(struct net_device *netdev,
>>>>>>>> u32  stringset, u8 *data)        } }
>>>>>>>>
>>>>>>>> +static int ixgb_set_flags(struct net_device *netdev, u32 data)
>>>>>>>> +{ +        struct ixgb_adapter *adapter = netdev_priv(netdev);
>>>>>>>> +   bool need_reset; +    int rc; + +        /* The hardware
>>>>>>>> requires that RX vlan stripping and TX vlan insertion +       *
>>>>>>>> be configured together.  Therefore, if one setting changes
>>>>>>>> adjust the +      * other one to match. +         */ +
>>>>>>>> if (!!(data & ETH_FLAG_RXVLAN) != !!(data & ETH_FLAG_TXVLAN)) {
>>>>>>>> +                if ((data & ETH_FLAG_RXVLAN) != +
>>>>>>>> (netdev->features & NETIF_F_HW_VLAN_RX)) +
>>>>>>>> data ^= ETH_FLAG_TXVLAN; +                else if ((data &
>>>>>>>> ETH_FLAG_TXVLAN) != +                    (netdev->features &
>>>>>>>> NETIF_F_HW_VLAN_TX)) +                        data ^=
>>>>>>>> ETH_FLAG_RXVLAN; +        }
>>>>>>> [...]
>>>>>>>
>>>>>>> I think this should reject attempts to change just one flag with
>>>>>>> -EINVAL, rather than quietly 'fixing' the setting.
>>>>>>>
>>>>>>> Ben.
>>>>>>
>>>>>> I'm not sure this is a good idea. At least not without some sort
>>>>>> of explanation. Since there is no way for the user to know that
>>>>>> he needs to disable both.
>>>>>
>>>>> Document the limitation in Documentation/networking/ixgb.txt.  You
>>>>> could also send a patch for the ethtool manual page stating that
>>>>> this restriction might exist.
>>>>>
>>>>> Ben.
>>>>
>>>> Just to make sure it's clear - there is no hard requirement for both
>>>> settings to be set at the same time. So setting:
>>>> ethtool -K eth0 rxvlan off
>>>>
>>>> Is a valid setting and will disable stripping on Rx, but because of
>>>> the design, stripping on Tx will also be disabled.
>>>
>>> Then it's *not* a valid setting for your hardware/driver.
>>
>> Ben, I agree that limiting the settings to what is actually supported
>> is conceptually cleaner but in practice it's not very intuitive.  If
>> you try to turn something off and the response is that it's invalid,
>> most people are going to assume that you just can't do it.  This is
>> especially true since you actually can't turn these settings off in
>> most drivers.
>>
>> There's a precedent for this type of thing: turn off TX checksum
>> offloading and watch scatter/gather and TSO be automatically disabled
>> as well.  It makes sense - the user requested a change, we do what is
>> necessary to make that happen without requiring them to understand why
>> these features are interrelated.
>>
>> Emil, I realized afterwards that, as you pointed out, TX vlan
>> offloading can be disabled without requiring RX offloading to be
>> disabled.  Feel free to make the modification yourself or I can
>> resubmit, whichever is easier.
>
> If it's OK with you I can make whatever changes are needed since I need
> to test this first, so I don't want to go back and forth in case we find
> other issues in testing.

Yes, please go ahead and make the necessary changes - I'm really just
interested in the end result.