* [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features
@ 2016-03-31 17:35 Mitch Williams
2016-03-31 20:50 ` Alexander Duyck
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Mitch Williams @ 2016-03-31 17:35 UTC (permalink / raw)
To: intel-wired-lan
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@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;
netdev->hw_features &= ~NETIF_F_RXCSUM;
+ netdev->hw_features &= ~(I40EVF_VLAN_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;
--
2.5.5
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features 2016-03-31 17:35 [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features Mitch Williams @ 2016-03-31 20:50 ` Alexander Duyck 2016-04-01 17:32 ` Williams, Mitch A 2016-04-01 5:17 ` kbuild test robot 2016-04-01 13:10 ` kbuild test robot 2 siblings, 1 reply; 9+ messages in thread From: Alexander Duyck @ 2016-03-31 20:50 UTC (permalink / raw) To: intel-wired-lan On Thu, Mar 31, 2016 at 10:35 AM, Mitch Williams <mitch.a.williams@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@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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features 2016-03-31 20:50 ` Alexander Duyck @ 2016-04-01 17:32 ` Williams, Mitch A 2016-04-01 20:06 ` Alexander Duyck 0 siblings, 1 reply; 9+ messages in thread From: Williams, Mitch A @ 2016-04-01 17:32 UTC (permalink / raw) To: intel-wired-lan > -----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@intel.com> > Cc: intel-wired-lan <intel-wired-lan@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@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@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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features 2016-04-01 17:32 ` Williams, Mitch A @ 2016-04-01 20:06 ` Alexander Duyck 2016-04-01 20:52 ` Williams, Mitch A 0 siblings, 1 reply; 9+ messages in thread From: Alexander Duyck @ 2016-04-01 20:06 UTC (permalink / raw) To: intel-wired-lan On Fri, Apr 1, 2016 at 10:32 AM, Williams, Mitch A <mitch.a.williams@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@intel.com> >> Cc: intel-wired-lan <intel-wired-lan@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@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@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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features 2016-04-01 20:06 ` Alexander Duyck @ 2016-04-01 20:52 ` Williams, Mitch A 2016-04-01 20:55 ` Alexander Duyck 0 siblings, 1 reply; 9+ messages in thread From: Williams, Mitch A @ 2016-04-01 20:52 UTC (permalink / raw) To: intel-wired-lan > -----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@intel.com> > Cc: intel-wired-lan <intel-wired-lan@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@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@intel.com> > >> Cc: intel-wired-lan <intel-wired-lan@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@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@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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features 2016-04-01 20:52 ` Williams, Mitch A @ 2016-04-01 20:55 ` Alexander Duyck 2016-04-01 23:41 ` Jeff Kirsher 0 siblings, 1 reply; 9+ messages in thread From: Alexander Duyck @ 2016-04-01 20:55 UTC (permalink / raw) To: intel-wired-lan On Fri, Apr 1, 2016 at 1:52 PM, Williams, Mitch A <mitch.a.williams@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@intel.com> >> Cc: intel-wired-lan <intel-wired-lan@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@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@intel.com> >> >> Cc: intel-wired-lan <intel-wired-lan@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@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@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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features 2016-04-01 20:55 ` Alexander Duyck @ 2016-04-01 23:41 ` Jeff Kirsher 0 siblings, 0 replies; 9+ messages in thread From: Jeff Kirsher @ 2016-04-01 23:41 UTC (permalink / raw) To: intel-wired-lan On Fri, 2016-04-01 at 13:55 -0700, Alexander Duyck wrote: > On Fri, Apr 1, 2016 at 1:52 PM, Williams, Mitch A > <mitch.a.williams@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@intel.com> > > > Cc: intel-wired-lan <intel-wired-lan@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@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@intel.com> > > > > > Cc: intel-wired-lan <intel-wired-lan@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@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@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_C > > > > > > TAG_TX | > > > > > > -?????????????????????????????????????????NETIF_F_HW_VLAN_C > > > > > > TAG_RX | > > > > > > -?????????????????????????????????????????NETIF_F_HW_VLAN_C > > > > > > TAG_FILTER); > > > > > > -???????????????netdev->features |= NETIF_F_HW_VLAN_CTAG_TX > > > > > > | > > > > > > -???????????????????????????????????NETIF_F_HW_VLAN_CTAG_RX > > > > > > | > > > > > > -???????????????????????????????????NETIF_F_HW_VLAN_CTAG_FI > > > > > > LTER; > > > > > > -???????} > > > > > > ????????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. I have applied Mitch's patch to dev-queue branch in my next-queue tree. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: This is a digitally signed message part URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20160401/2fe95d20/attachment.asc> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features 2016-03-31 17:35 [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features Mitch Williams 2016-03-31 20:50 ` Alexander Duyck @ 2016-04-01 5:17 ` kbuild test robot 2016-04-01 13:10 ` kbuild test robot 2 siblings, 0 replies; 9+ messages in thread From: kbuild test robot @ 2016-04-01 5:17 UTC (permalink / raw) To: intel-wired-lan Hi Mitch, [auto build test WARNING on net/master] [also build test WARNING on v4.6-rc1 next-20160401] [if your patch is applied to the wrong git tree, please drop us a note to help improving the system] url: https://github.com/0day-ci/linux/commits/Mitch-Williams/i40evf-properly-handle-VLAN-features/20160401-014314 config: i386-randconfig-x011-04011139 (attached as .config) reproduce: # save the attached .config to linux build tree make ARCH=i386 All warnings (new ones prefixed by >>): In file included from include/uapi/linux/stddef.h:1:0, from include/linux/stddef.h:4, from include/uapi/linux/posix_types.h:4, from include/uapi/linux/types.h:13, from include/linux/types.h:5, from include/linux/list.h:4, from include/linux/module.h:9, from drivers/net/ethernet/intel/i40evf/i40evf.h:30, from drivers/net/ethernet/intel/i40evf/i40evf_main.c:27: drivers/net/ethernet/intel/i40evf/i40evf_main.c: In function 'i40evf_process_config': drivers/net/ethernet/intel/i40evf/i40evf_main.c:2358:6: error: 'vfres' undeclared (first use in this function) if (vfres->vf_offload_flags & I40E_VIRTCHNL_VF_OFFLOAD_VLAN) { ^ include/linux/compiler.h:151:30: note: in definition of macro '__trace_if' if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ ^ >> drivers/net/ethernet/intel/i40evf/i40evf_main.c:2358:2: note: in expansion of macro 'if' if (vfres->vf_offload_flags & I40E_VIRTCHNL_VF_OFFLOAD_VLAN) { ^ drivers/net/ethernet/intel/i40evf/i40evf_main.c:2358:6: note: each undeclared identifier is reported only once for each function it appears in if (vfres->vf_offload_flags & I40E_VIRTCHNL_VF_OFFLOAD_VLAN) { ^ include/linux/compiler.h:151:30: note: in definition of macro '__trace_if' if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ ^ >> drivers/net/ethernet/intel/i40evf/i40evf_main.c:2358:2: note: in expansion of macro 'if' if (vfres->vf_offload_flags & I40E_VIRTCHNL_VF_OFFLOAD_VLAN) { ^ vim +/if +2358 drivers/net/ethernet/intel/i40evf/i40evf_main.c 2342 NETIF_F_TSO | 2343 NETIF_F_TSO6 | 2344 NETIF_F_TSO_ECN | 2345 NETIF_F_GSO_GRE | 2346 NETIF_F_GSO_UDP_TUNNEL | 2347 NETIF_F_GSO_UDP_TUNNEL_CSUM; 2348 2349 if (adapter->flags & I40EVF_FLAG_OUTER_UDP_CSUM_CAPABLE) 2350 netdev->features |= NETIF_F_GSO_UDP_TUNNEL_CSUM; 2351 2352 /* copy netdev features into list of user selectable features */ 2353 netdev->hw_features |= netdev->features; 2354 netdev->hw_features &= ~NETIF_F_RXCSUM; 2355 netdev->hw_features &= ~(I40EVF_VLAN_FEATURES); 2356 netdev->features &= ~(I40EVF_VLAN_FEATURES); 2357 > 2358 if (vfres->vf_offload_flags & I40E_VIRTCHNL_VF_OFFLOAD_VLAN) { 2359 netdev->vlan_features = netdev->features; 2360 netdev->features |= I40EVF_VLAN_FEATURES; 2361 } 2362 2363 adapter->vsi.id = adapter->vsi_res->vsi_id; 2364 2365 adapter->vsi.back = adapter; 2366 adapter->vsi.base_vector = 1; --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation -------------- next part -------------- A non-text attachment was scrubbed... Name: .config.gz Type: application/octet-stream Size: 29120 bytes Desc: not available URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20160401/94f07b57/attachment-0001.obj> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features 2016-03-31 17:35 [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features Mitch Williams 2016-03-31 20:50 ` Alexander Duyck 2016-04-01 5:17 ` kbuild test robot @ 2016-04-01 13:10 ` kbuild test robot 2 siblings, 0 replies; 9+ messages in thread From: kbuild test robot @ 2016-04-01 13:10 UTC (permalink / raw) To: intel-wired-lan Hi Mitch, [auto build test ERROR on net/master] [also build test ERROR on v4.6-rc1 next-20160401] [if your patch is applied to the wrong git tree, please drop us a note to help improving the system] url: https://github.com/0day-ci/linux/commits/Mitch-Williams/i40evf-properly-handle-VLAN-features/20160401-014314 config: i386-allmodconfig (attached as .config) reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): drivers/net/ethernet/intel/i40evf/i40evf_main.c: In function 'i40evf_process_config': >> drivers/net/ethernet/intel/i40evf/i40evf_main.c:2358:6: error: 'vfres' undeclared (first use in this function) if (vfres->vf_offload_flags & I40E_VIRTCHNL_VF_OFFLOAD_VLAN) { ^ drivers/net/ethernet/intel/i40evf/i40evf_main.c:2358:6: note: each undeclared identifier is reported only once for each function it appears in vim +/vfres +2358 drivers/net/ethernet/intel/i40evf/i40evf_main.c 2352 /* copy netdev features into list of user selectable features */ 2353 netdev->hw_features |= netdev->features; 2354 netdev->hw_features &= ~NETIF_F_RXCSUM; 2355 netdev->hw_features &= ~(I40EVF_VLAN_FEATURES); 2356 netdev->features &= ~(I40EVF_VLAN_FEATURES); 2357 > 2358 if (vfres->vf_offload_flags & I40E_VIRTCHNL_VF_OFFLOAD_VLAN) { 2359 netdev->vlan_features = netdev->features; 2360 netdev->features |= I40EVF_VLAN_FEATURES; 2361 } --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation -------------- next part -------------- A non-text attachment was scrubbed... Name: .config.gz Type: application/octet-stream Size: 54390 bytes Desc: not available URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20160401/99664dce/attachment-0001.obj> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-04-01 23:41 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-03-31 17:35 [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features Mitch Williams 2016-03-31 20:50 ` Alexander Duyck 2016-04-01 17:32 ` Williams, Mitch A 2016-04-01 20:06 ` Alexander Duyck 2016-04-01 20:52 ` Williams, Mitch A 2016-04-01 20:55 ` Alexander Duyck 2016-04-01 23:41 ` Jeff Kirsher 2016-04-01 5:17 ` kbuild test robot 2016-04-01 13:10 ` kbuild test robot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.