[Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features

[Alexander Duyck]

On Fri, Apr 1, 2016 at 1:52 PM, Williams, Mitch A
<mitch.a.williams at intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Alexander Duyck [mailto:alexander.duyck at gmail.com]
>> Sent: Friday, April 01, 2016 1:07 PM
>> To: Williams, Mitch A <mitch.a.williams at intel.com>
>> Cc: intel-wired-lan <intel-wired-lan at lists.osuosl.org>
>> Subject: Re: [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN
>> features
>>
>> On Fri, Apr 1, 2016 at 10:32 AM, Williams, Mitch A
>> <mitch.a.williams at intel.com> wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: Alexander Duyck [mailto:alexander.duyck at gmail.com]
>> >> Sent: Thursday, March 31, 2016 1:50 PM
>> >> To: Williams, Mitch A <mitch.a.williams at intel.com>
>> >> Cc: intel-wired-lan <intel-wired-lan at lists.osuosl.org>
>> >> Subject: Re: [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN
>> >> features
>> >>
>> >> On Thu, Mar 31, 2016 at 10:35 AM, Mitch Williams
>> >> <mitch.a.williams at intel.com> wrote:
>> >> > Correctly set the VLAN feature flags after setting the rest of the
>> >> > netdev flags. And don't set them in hw_features, because these can't be
>> >> > controlled by the VF driver.
>> >> >
>> >> > Testing Hints: Make sure VLAN offloads work correctly when not using
>> >> > port VLANs, and make sure they're not availble when using port VLANs.
>> >> >
>> >> > Signed-off-by: Mitch Williams <mitch.a.williams at intel.com>
>> >> > ---
>> >> >  drivers/net/ethernet/intel/i40evf/i40evf_main.c | 21 +++++++++++------
>> ---
>> >> -
>> >> >  1 file changed, 11 insertions(+), 10 deletions(-)
>> >> >
>> >> > diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_main.c
>> >> b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
>> >> > index 4b70aae..039bbd2 100644
>> >> > --- a/drivers/net/ethernet/intel/i40evf/i40evf_main.c
>> >> > +++ b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
>> >> > @@ -2259,6 +2259,10 @@ static int i40evf_change_mtu(struct net_device
>> >> *netdev, int new_mtu)
>> >> >         return 0;
>> >> >  }
>> >> >
>> >> > +#define I40EVF_VLAN_FEATURES (NETIF_F_HW_VLAN_CTAG_TX |\
>> >> > +                              NETIF_F_HW_VLAN_CTAG_RX |\
>> >> > +                              NETIF_F_HW_VLAN_CTAG_FILTER)
>> >> > +
>> >> >  static const struct net_device_ops i40evf_netdev_ops = {
>> >> >         .ndo_open               = i40evf_open,
>> >> >         .ndo_stop               = i40evf_close,
>> >> > @@ -2320,16 +2324,6 @@ int i40evf_process_config(struct i40evf_adapter
>> >> *adapter)
>> >> >                 return -ENODEV;
>> >> >         }
>> >> >
>> >> > -       if (adapter->vf_res->vf_offload_flags
>> >> > -           & I40E_VIRTCHNL_VF_OFFLOAD_VLAN) {
>> >> > -               netdev->vlan_features = netdev->features &
>> >> > -                                       ~(NETIF_F_HW_VLAN_CTAG_TX |
>> >> > -                                         NETIF_F_HW_VLAN_CTAG_RX |
>> >> > -                                         NETIF_F_HW_VLAN_CTAG_FILTER);
>> >> > -               netdev->features |= NETIF_F_HW_VLAN_CTAG_TX |
>> >> > -                                   NETIF_F_HW_VLAN_CTAG_RX |
>> >> > -                                   NETIF_F_HW_VLAN_CTAG_FILTER;
>> >> > -       }
>> >> >         netdev->features |= NETIF_F_HIGHDMA |
>> >> >                             NETIF_F_SG |
>> >> >                             NETIF_F_IP_CSUM |
>> >> > @@ -2358,6 +2352,13 @@ int i40evf_process_config(struct i40evf_adapter
>> >> *adapter)
>> >> >         /* copy netdev features into list of user selectable features
>> */
>> >> >         netdev->hw_features |= netdev->features;
>> >>
>> >> This line here is actually a bit problematic.  I found while working
>> >> on NETIF_F_GSO_PARTIAL that we end up calling i40evf_process_config
>> >> multiple times.  What ends up happening is that features end up
>> >> bleeding through into hw_features.  Same thing if you try going the
>> >> other way.  Your best bend ends up actually being to populate
>> >> hw_enc_features and then feed that into features and hw_features.
>> >>
>> >> >         netdev->hw_features &= ~NETIF_F_RXCSUM;
>> >>
>> >> I never noticed before but why are you clearing the NETIF_F_RXCSUM
>> >> flag?  That is a feature you control via software isn't it?  You could
>> >> just ignore the results from the hardware.  Why lock this feature for
>> >> a VF?
>> >>
>> >> > +       netdev->hw_features &= ~(I40EVF_VLAN_FEATURES);
>> >>
>> >> There is no need to mask hw_features if you never set it.  I assume
>> >> this is needed to deal with the bleeding through issue I referenced
>> >> above where you are writing hw_features from features.
>> >>
>> >> > +       netdev->features &= ~(I40EVF_VLAN_FEATURES);
>> >> > +
>> >> > +       if (vfres->vf_offload_flags & I40E_VIRTCHNL_VF_OFFLOAD_VLAN) {
>> >> > +               netdev->vlan_features = netdev->features;
>> >> > +               netdev->features |= I40EVF_VLAN_FEATURES;
>> >> > +       }
>> >> >
>> >> >         adapter->vsi.id = adapter->vsi_res->vsi_id;
>> >>
>> >> You can probably just set vlan_features always.  As far as I know the
>> >> i40e/i40evf hardware doesn't penalize you if you have some extra data
>> >> like a VLAN tag in the L2 header so TSO and checksums should all still
>> >> work.
>> >>
>> >> - Alex
>> >
>> > This looks like it's causing build problems, so let's drop it and I'll
>> respin. Again.
>>
>> If you want I can just submit a patch in a couple hours to take care
>> of most of this and a few other things I have found.  I was already
>> working in the area as I have the GSO partial stuff and some code to
>> add IPIP and SIT tunnel support for the PF and VF drivers.
>>
>> - Alex
>
> Thanks for the offer. Just pushed my v2.
> -Mitch

Yeah I saw that.  I'll probably wait for Jeff to apply it then I will
rebase my patches and submit the stuff for IPIP and SIT offload
support.

- Alex