Patchwork ixgb: Convert to new vlan model.

login
register
mail settings
Submitter Jesse Gross
Date Dec. 14, 2010, 3:42 a.m.
Message ID <1292298163-30343-1-git-send-email-jesse@nicira.com>
Download mbox | patch
Permalink /patch/75461/
State Awaiting Upstream
Delegated to: David Miller
Headers show

Comments

Jesse Gross - Dec. 14, 2010, 3:42 a.m.
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.

Signed-off-by: Jesse Gross <jesse@nicira.com>
CC: Emil Tantilov <emil.s.tantilov@intel.com>
CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
CC: Alex Duyck <alexander.h.duyck@intel.com>
---
 drivers/net/ixgb/ixgb.h         |    2 +-
 drivers/net/ixgb/ixgb_ethtool.c |   41 +++++++++++++++++++++++++++++
 drivers/net/ixgb/ixgb_main.c    |   54 ++++++++------------------------------
 3 files changed, 54 insertions(+), 43 deletions(-)
Ben Hutchings - Dec. 14, 2010, 4:31 p.m.
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.
Tantilov, Emil S - Dec. 14, 2010, 6:09 p.m.
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.

Thanks,
Emil
Ben Hutchings - Dec. 14, 2010, 6:12 p.m.
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.
Ben Hutchings - Dec. 14, 2010, 7:15 p.m.
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.

[...]
> Either way we should update the docs as it is not intuitive.

Yes.

Ben.
Jesse Gross - Dec. 14, 2010, 9:29 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - Dec. 14, 2010, 9:47 p.m.
On Tue, 2010-12-14 at 13:29 -0800, 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:
[...]
> >> >>> I think this should reject attempts to change just one flag with
> >> >>> -EINVAL, rather than quietly 'fixing' the setting.
[...]
> 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.

That reflects a general dependency and not a driver- or hardware-
specific restriction.  But I see your point.

Perhaps the ethtool utility should check the result after applying
offload changes and report any additional automatic changes.

Ben.
Jesse Gross - Dec. 14, 2010, 9:53 p.m.
On Tue, Dec 14, 2010 at 1:47 PM, Ben Hutchings
<bhutchings@solarflare.com> wrote:
> On Tue, 2010-12-14 at 13:29 -0800, 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:
> [...]
>> >> >>> I think this should reject attempts to change just one flag with
>> >> >>> -EINVAL, rather than quietly 'fixing' the setting.
> [...]
>> 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.
>
> That reflects a general dependency and not a driver- or hardware-
> specific restriction.  But I see your point.
>
> Perhaps the ethtool utility should check the result after applying
> offload changes and report any additional automatic changes.

That sounds like the best solution to me.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tantilov, Emil S - Dec. 15, 2010, 6:09 p.m.
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.

Thanks,
Emil
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Kirsher - Dec. 15, 2010, 10:49 p.m.
On Mon, Dec 13, 2010 at 19:42, Jesse Gross <jesse@nicira.com> 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.
>
> Signed-off-by: Jesse Gross <jesse@nicira.com>
> CC: Emil Tantilov <emil.s.tantilov@intel.com>
> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> CC: Alex Duyck <alexander.h.duyck@intel.com>
> ---
>  drivers/net/ixgb/ixgb.h         |    2 +-
>  drivers/net/ixgb/ixgb_ethtool.c |   41 +++++++++++++++++++++++++++++
>  drivers/net/ixgb/ixgb_main.c    |   54 ++++++++------------------------------
>  3 files changed, 54 insertions(+), 43 deletions(-)
>

I have added this patch to my tree for testing and review.  Thanks Jesse.
Jesse Gross - Dec. 16, 2010, 4:29 a.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/ixgb/ixgb.h b/drivers/net/ixgb/ixgb.h
index 521c0c7..8f3df04 100644
--- a/drivers/net/ixgb/ixgb.h
+++ b/drivers/net/ixgb/ixgb.h
@@ -149,7 +149,7 @@  struct ixgb_desc_ring {
 
 struct ixgb_adapter {
 	struct timer_list watchdog_timer;
-	struct vlan_group *vlgrp;
+	unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)];
 	u32 bd_number;
 	u32 rx_buffer_len;
 	u32 part_num;
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;
+	}
+
+	need_reset = (data & ETH_FLAG_RXVLAN) !=
+		     (netdev->features & NETIF_F_HW_VLAN_RX);
+
+	rc = ethtool_op_set_flags(netdev, data, ETH_FLAG_RXVLAN |
+						ETH_FLAG_TXVLAN);
+	if (rc)
+		return rc;
+
+	if (need_reset) {
+		if (netif_running(netdev)) {
+			ixgb_down(adapter, true);
+			ixgb_up(adapter);
+			ixgb_set_speed_duplex(netdev);
+		} else
+			ixgb_reset(adapter);
+	}
+
+	return 0;
+}
+
 static const struct ethtool_ops ixgb_ethtool_ops = {
 	.get_settings = ixgb_get_settings,
 	.set_settings = ixgb_set_settings,
@@ -732,6 +771,8 @@  static const struct ethtool_ops ixgb_ethtool_ops = {
 	.phys_id = ixgb_phys_id,
 	.get_sset_count = ixgb_get_sset_count,
 	.get_ethtool_stats = ixgb_get_ethtool_stats,
+	.get_flags = ethtool_op_get_flags,
+	.set_flags = ixgb_set_flags,
 };
 
 void ixgb_set_ethtool_ops(struct net_device *netdev)
diff --git a/drivers/net/ixgb/ixgb_main.c b/drivers/net/ixgb/ixgb_main.c
index b021798..053ae02 100644
--- a/drivers/net/ixgb/ixgb_main.c
+++ b/drivers/net/ixgb/ixgb_main.c
@@ -100,8 +100,6 @@  static void ixgb_tx_timeout_task(struct work_struct *work);
 
 static void ixgb_vlan_strip_enable(struct ixgb_adapter *adapter);
 static void ixgb_vlan_strip_disable(struct ixgb_adapter *adapter);
-static void ixgb_vlan_rx_register(struct net_device *netdev,
-                                  struct vlan_group *grp);
 static void ixgb_vlan_rx_add_vid(struct net_device *netdev, u16 vid);
 static void ixgb_vlan_rx_kill_vid(struct net_device *netdev, u16 vid);
 static void ixgb_restore_vlan(struct ixgb_adapter *adapter);
@@ -336,7 +334,6 @@  static const struct net_device_ops ixgb_netdev_ops = {
 	.ndo_set_mac_address	= ixgb_set_mac,
 	.ndo_change_mtu		= ixgb_change_mtu,
 	.ndo_tx_timeout		= ixgb_tx_timeout,
-	.ndo_vlan_rx_register	= ixgb_vlan_rx_register,
 	.ndo_vlan_rx_add_vid	= ixgb_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid	= ixgb_vlan_rx_kill_vid,
 #ifdef CONFIG_NET_POLL_CONTROLLER
@@ -1508,7 +1505,7 @@  ixgb_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
                      DESC_NEEDED)))
 		return NETDEV_TX_BUSY;
 
-	if (adapter->vlgrp && vlan_tx_tag_present(skb)) {
+	if (vlan_tx_tag_present(skb)) {
 		tx_flags |= IXGB_TX_FLAGS_VLAN;
 		vlan_id = vlan_tx_tag_get(skb);
 	}
@@ -2049,12 +2046,11 @@  ixgb_clean_rx_irq(struct ixgb_adapter *adapter, int *work_done, int work_to_do)
 		ixgb_rx_checksum(adapter, rx_desc, skb);
 
 		skb->protocol = eth_type_trans(skb, netdev);
-		if (adapter->vlgrp && (status & IXGB_RX_DESC_STATUS_VP)) {
-			vlan_hwaccel_receive_skb(skb, adapter->vlgrp,
-			                        le16_to_cpu(rx_desc->special));
-		} else {
-			netif_receive_skb(skb);
-		}
+		if (status & IXGB_RX_DESC_STATUS_VP)
+			__vlan_hwaccel_put_tag(skb,
+					       le16_to_cpu(rx_desc->special));
+
+		netif_receive_skb(skb);
 
 rxdesc_done:
 		/* clean up descriptor, might be written over by hw */
@@ -2152,20 +2148,6 @@  map_skb:
 	}
 }
 
-/**
- * ixgb_vlan_rx_register - enables or disables vlan tagging/stripping.
- *
- * @param netdev network interface device structure
- * @param grp indicates to enable or disable tagging/stripping
- **/
-static void
-ixgb_vlan_rx_register(struct net_device *netdev, struct vlan_group *grp)
-{
-	struct ixgb_adapter *adapter = netdev_priv(netdev);
-
-	adapter->vlgrp = grp;
-}
-
 static void
 ixgb_vlan_strip_enable(struct ixgb_adapter *adapter)
 {
@@ -2200,6 +2182,7 @@  ixgb_vlan_rx_add_vid(struct net_device *netdev, u16 vid)
 	vfta = IXGB_READ_REG_ARRAY(&adapter->hw, VFTA, index);
 	vfta |= (1 << (vid & 0x1F));
 	ixgb_write_vfta(&adapter->hw, index, vfta);
+	set_bit(vid, adapter->active_vlans);
 }
 
 static void
@@ -2208,35 +2191,22 @@  ixgb_vlan_rx_kill_vid(struct net_device *netdev, u16 vid)
 	struct ixgb_adapter *adapter = netdev_priv(netdev);
 	u32 vfta, index;
 
-	ixgb_irq_disable(adapter);
-
-	vlan_group_set_device(adapter->vlgrp, vid, NULL);
-
-	/* don't enable interrupts unless we are UP */
-	if (adapter->netdev->flags & IFF_UP)
-		ixgb_irq_enable(adapter);
-
 	/* remove VID from filter table */
 
 	index = (vid >> 5) & 0x7F;
 	vfta = IXGB_READ_REG_ARRAY(&adapter->hw, VFTA, index);
 	vfta &= ~(1 << (vid & 0x1F));
 	ixgb_write_vfta(&adapter->hw, index, vfta);
+	clear_bit(vid, adapter->active_vlans);
 }
 
 static void
 ixgb_restore_vlan(struct ixgb_adapter *adapter)
 {
-	ixgb_vlan_rx_register(adapter->netdev, adapter->vlgrp);
-
-	if (adapter->vlgrp) {
-		u16 vid;
-		for (vid = 0; vid < VLAN_N_VID; vid++) {
-			if (!vlan_group_get_device(adapter->vlgrp, vid))
-				continue;
-			ixgb_vlan_rx_add_vid(adapter->netdev, vid);
-		}
-	}
+	u16 vid;
+
+	for_each_set_bit(vid, adapter->active_vlans, VLAN_N_VID)
+		ixgb_vlan_rx_add_vid(adapter->netdev, vid);
 }
 
 #ifdef CONFIG_NET_POLL_CONTROLLER