Skip to contentSkip to conversation

i40evf: properly handle VLAN features

PATCH Superseded Mitch Williams 5 participants 9 messages Mar 31, 2016 – Apr 1, 2016

Subsystems (2)
Network drivers Intel Ethernet

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Mitch Williams

author
#0 lore raw

PATCH 1/1 i40evf: properly handle VLAN features

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 <[email protected]>
---
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)
drivers/net/ethernet/intel/i40evf/i40evf_main.c +11 -10
index 4b70aae..039bbd2 100644
@@ -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;
Signature · 1 line
2.5.5
#1 lore raw

PATCH 1/1 i40evf: properly handle VLAN features

Quoted 45 lines · Mitch Williams

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 <[email protected]>
---
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)
drivers/net/ethernet/intel/i40evf/i40evf_main.c +4 -10
index 4b70aae..039bbd2 100644
@@ -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 |

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter *adapter) · line 2353

Quoted 2 lines
drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
@@ -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.

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter *adapter) · line 2354

Quoted 1 line
drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
        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?

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter *adapter) · line 2355

Quoted 1 line
drivers/net/ethernet/intel/i40evf/i40evf_main.c +1 -0
+       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.

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter *adapter) · line 2363

Quoted 8 lines
drivers/net/ethernet/intel/i40evf/i40evf_main.c +6 -0
+       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

#2 lore raw

PATCH 1/1 i40evf: properly handle VLAN features

Quoted 92 lines

-----Original Message-----
From: Alexander Duyck [mailto:alexander.duyck at gmail.com]
Sent: Thursday, March 31, 2016 1:50 PM
To: Williams, Mitch A <[email protected]>
Cc: intel-wired-lan <[email protected]>
Subject: Re: [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features

Quoted 10 lines · Mitch Williams

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 <[email protected]>
---
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 21 +++++++++++---------

-

Review

Quoted 3 lines
 1 file changed, 11 insertions(+), 10 deletions(-)
(quoted diff) +0 -0

b/drivers/net/ethernet/intel/i40evf/i40evf_main.c

drivers/net/ethernet/intel/i40evf/i40evf_main.c static int i40evf_change_mtu(struct net_device

Quoted 4 lines

index 4b70aae..039bbd2 100644

drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
@@ -2259,6 +2259,10 @@ static int i40evf_change_mtu(struct net_device

*netdev, int new_mtu)

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter

Quoted 11 lines
drivers/net/ethernet/intel/i40evf/i40evf_main.c +4 -0
        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)

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter

Quoted 17 lines
drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -10
                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)

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter *adapter) · line 2353

Quoted 2 lines
drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
        /* 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.

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter *adapter) · line 2354

Quoted 1 line
drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
        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?

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter *adapter) · line 2355

Quoted 1 line
drivers/net/ethernet/intel/i40evf/i40evf_main.c +1 -0
+       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.

Quoted 7 lines
drivers/net/ethernet/intel/i40evf/i40evf_main.c +6 -0
+       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;
+       }
Quoted 8 lines

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter *adapter) · line 2363

Quoted 1 line
drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
        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.

#3 lore raw

PATCH 1/1 i40evf: properly handle VLAN features

Quoted 94 lines · Mitch A
Quoted 94 lines

-----Original Message-----
From: Alexander Duyck [mailto:alexander.duyck at gmail.com]
Sent: Thursday, March 31, 2016 1:50 PM
To: Williams, Mitch A <[email protected]>
Cc: intel-wired-lan <[email protected]>
Subject: Re: [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features

Quoted 10 lines · Mitch Williams

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 <[email protected]>
---
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 21 +++++++++++---------

-

Review

Quoted 3 lines
 1 file changed, 11 insertions(+), 10 deletions(-)
(quoted diff) +0 -0

b/drivers/net/ethernet/intel/i40evf/i40evf_main.c

drivers/net/ethernet/intel/i40evf/i40evf_main.c static int i40evf_change_mtu(struct net_device

Quoted 4 lines

index 4b70aae..039bbd2 100644

drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
@@ -2259,6 +2259,10 @@ static int i40evf_change_mtu(struct net_device

*netdev, int new_mtu)

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter

Quoted 11 lines
drivers/net/ethernet/intel/i40evf/i40evf_main.c +4 -0
        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)

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter

Quoted 17 lines
drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -10
                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)

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter *adapter) · line 2353

Quoted 2 lines
drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
        /* 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.

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter *adapter) · line 2354

Quoted 1 line
drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
        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?

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter *adapter) · line 2355

Quoted 1 line
drivers/net/ethernet/intel/i40evf/i40evf_main.c +1 -0
+       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.

Quoted 8 lines
drivers/net/ethernet/intel/i40evf/i40evf_main.c +6 -0
+       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;
Quoted 8 lines
Quoted 6 lines

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

#4 lore raw

PATCH 1/1 i40evf: properly handle VLAN features

Quoted 116 lines

-----Original Message-----
From: Alexander Duyck [mailto:alexander.duyck at gmail.com]
Sent: Friday, April 01, 2016 1:07 PM
To: Williams, Mitch A <[email protected]>
Cc: intel-wired-lan <[email protected]>
Subject: Re: [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features

Quoted 16 lines · Mitch A
Quoted 16 lines

-----Original Message-----
From: Alexander Duyck [mailto:alexander.duyck at gmail.com]
Sent: Thursday, March 31, 2016 1:50 PM
To: Williams, Mitch A <[email protected]>
Cc: intel-wired-lan <[email protected]>
Subject: Re: [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features

Quoted 4 lines · Mitch Williams

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.

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

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 <[email protected]>
---
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 21 +++++++++++------

---

Quoted 40 lines
Quoted 40 lines

-

Review

Quoted 3 lines
 1 file changed, 11 insertions(+), 10 deletions(-)
(quoted diff) +0 -0

b/drivers/net/ethernet/intel/i40evf/i40evf_main.c

drivers/net/ethernet/intel/i40evf/i40evf_main.c static int i40evf_change_mtu(struct net_device

Quoted 4 lines

index 4b70aae..039bbd2 100644

drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
@@ -2259,6 +2259,10 @@ static int i40evf_change_mtu(struct net_device

*netdev, int new_mtu)

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter

Quoted 11 lines
drivers/net/ethernet/intel/i40evf/i40evf_main.c +4 -0
        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)

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter

Quoted 17 lines
drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -10
                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)

Quoted 1 line
Quoted 1 line

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter · line 2352

Quoted 1 line
drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
        /* copy netdev features into list of user selectable features

*/

Quoted 31 lines
Quoted 31 lines

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter *adapter) · line 2353

Quoted 1 line
drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
        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.

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter *adapter) · line 2354

Quoted 1 line
drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
        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?

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter *adapter) · line 2355

Quoted 1 line
drivers/net/ethernet/intel/i40evf/i40evf_main.c +1 -0
+       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.

Quoted 8 lines
drivers/net/ethernet/intel/i40evf/i40evf_main.c +6 -0
+       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;
Quoted 8 lines
Quoted 6 lines

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

Quoted 8 lines

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

#5 lore raw

PATCH 1/1 i40evf: properly handle VLAN features

Quoted 123 lines · Mitch A
Quoted 123 lines

-----Original Message-----
From: Alexander Duyck [mailto:alexander.duyck at gmail.com]
Sent: Friday, April 01, 2016 1:07 PM
To: Williams, Mitch A <[email protected]>
Cc: intel-wired-lan <[email protected]>
Subject: Re: [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features

Quoted 16 lines · Mitch A
Quoted 16 lines

-----Original Message-----
From: Alexander Duyck [mailto:alexander.duyck at gmail.com]
Sent: Thursday, March 31, 2016 1:50 PM
To: Williams, Mitch A <[email protected]>
Cc: intel-wired-lan <[email protected]>
Subject: Re: [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN features

Quoted 4 lines · Mitch Williams

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.

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

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 <[email protected]>
---
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 21 +++++++++++------

---

Quoted 40 lines
Quoted 40 lines

-

Review

Quoted 3 lines
 1 file changed, 11 insertions(+), 10 deletions(-)
(quoted diff) +0 -0

b/drivers/net/ethernet/intel/i40evf/i40evf_main.c

drivers/net/ethernet/intel/i40evf/i40evf_main.c static int i40evf_change_mtu(struct net_device

Quoted 4 lines

index 4b70aae..039bbd2 100644

drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
@@ -2259,6 +2259,10 @@ static int i40evf_change_mtu(struct net_device

*netdev, int new_mtu)

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter

Quoted 11 lines
drivers/net/ethernet/intel/i40evf/i40evf_main.c +4 -0
        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)

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter

Quoted 17 lines
drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -10
                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)

Quoted 1 line
Quoted 1 line

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter · line 2352

Quoted 1 line
drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
        /* copy netdev features into list of user selectable features

*/

Quoted 31 lines
Quoted 31 lines

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter *adapter) · line 2353

Quoted 1 line
drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
        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.

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter *adapter) · line 2354

Quoted 1 line
drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
        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?

drivers/net/ethernet/intel/i40evf/i40evf_main.c int i40evf_process_config(struct i40evf_adapter *adapter) · line 2355

Quoted 1 line
drivers/net/ethernet/intel/i40evf/i40evf_main.c +1 -0
+       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.

Quoted 8 lines
drivers/net/ethernet/intel/i40evf/i40evf_main.c +6 -0
+       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;
Quoted 8 lines
Quoted 6 lines

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.

Quoted 4 lines
Quoted 1 line

- 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

#6 lore raw

PATCH 1/1 i40evf: properly handle VLAN features

Quoted 174 lines · Alexander Duyck
Quoted 172 lines · Mitch A
Quoted 172 lines

-----Original Message-----
From: Alexander Duyck [mailto:alexander.duyck at gmail.com]
Sent: Friday, April 01, 2016 1:07 PM
To: Williams, Mitch A <[email protected]>
Cc: intel-wired-lan <[email protected]>
Subject: Re: [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN
features

Quoted 27 lines · Mitch A
Quoted 27 lines

-----Original Message-----
From: Alexander Duyck [mailto:alexander.duyck at gmail.com]
Sent: Thursday, March 31, 2016 1:50 PM
To: Williams, Mitch A <[email protected]>
Cc: intel-wired-lan <[email protected]>
Subject: Re: [Intel-wired-lan] [next PATCH] i40evf: properly handle VLAN
features

Quoted 12 lines · Mitch Williams

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.

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Signed-off-by Mitch Williams <[email protected]>

---
?drivers/net/ethernet/intel/i40evf/i40evf_main.c | 21
+++++++++++------

---

Quoted 57 lines
Quoted 57 lines

-

Quoted 5 lines

?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

Quoted 6 lines

index 4b70aae..039bbd2 100644

drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
@@ -2259,6 +2259,10 @@ static int i40evf_change_mtu(struct

net_device

*netdev, int new_mtu)

Quoted 12 lines

????????return 0;
?}

drivers/net/ethernet/intel/i40evf/i40evf_main.c +4 -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,

drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
@@ -2320,16 +2324,6 @@ int i40evf_process_config(struct
Quoted 1 line

i40evf_adapter

*adapter)

Quoted 24 lines

????????????????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 |

drivers/net/ethernet/intel/i40evf/i40evf_main.c +0 -0
@@ -2358,6 +2352,13 @@ int i40evf_process_config(struct
Quoted 1 line

i40evf_adapter

*adapter)

Quoted 1 line
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

????????/* copy netdev features into list of user
selectable features

*/

Quoted 52 lines
Quoted 52 lines
Quoted 4 lines

????????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.

Quoted 2 lines

????????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?

Quoted 2 lines

+???????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.

Quoted 10 lines

+???????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.

Quoted 3 lines
Quoted 1 line

- 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.

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

- 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>

build bot
#7 lore raw
In reply to Mitch Williams
reproduce:

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>

build bot
#8 lore raw
In reply to Mitch Williams
reproduce:

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>