[PATCH net] sfc: ef10: don't overwrite offload features at NIC reset

[PATCH net] sfc: ef10: don't overwrite offload features at NIC reset

[Íñigo Huguet]

At NIC reset, some offload features related to encapsulated traffic
might have changed (this mainly happens if the firmware-variant is
changed with the sfboot userspace tool). Because of this, features are
checked and set again at reset time.

However, this was not done right, and some features were improperly
overwritten at NIC reset:
- Tunneled IPv6 segmentation was always disabled
- Features disabled with ethtool were reenabled
- Features that becomes unsupported after the reset were not disabled

Also, cleanup a bit the setting of other features not related to
encapsulation. Now that Siena devices are unsupported, some checks are
unnecessary because they're always supported in all ef10 models.

Fixes: ffffd2454a7a ("sfc: correctly advertise tunneled IPv6 segmentation")
Fixes: 24b2c3751aa3 ("sfc: advertise encapsulated offloads on EF10")
Reported-by: Tianhao Zhao <tizhao@redhat.com>
Suggested-by: Jonathan Cooper <jonathan.s.cooper@amd.com>
Tested-by: Jonathan Cooper <jonathan.s.cooper@amd.com>
Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
---
 drivers/net/ethernet/sfc/ef10.c | 38 ++++++++++++++++++++++-----------
 drivers/net/ethernet/sfc/efx.c  | 17 ++++++---------
 2 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
index 7022fb2005a2..d30459dbfe8f 100644
--- a/drivers/net/ethernet/sfc/ef10.c
+++ b/drivers/net/ethernet/sfc/ef10.c
@@ -1304,7 +1304,8 @@ static void efx_ef10_fini_nic(struct efx_nic *efx)
 static int efx_ef10_init_nic(struct efx_nic *efx)
 {
 	struct efx_ef10_nic_data *nic_data = efx->nic_data;
-	netdev_features_t hw_enc_features = 0;
+	struct net_device *net_dev = efx->net_dev;
+	netdev_features_t tun_feats, tso_feats;
 	int rc;
 
 	if (nic_data->must_check_datapath_caps) {
@@ -1349,20 +1350,30 @@ static int efx_ef10_init_nic(struct efx_nic *efx)
 		nic_data->must_restore_piobufs = false;
 	}
 
-	/* add encapsulated checksum offload features */
+	/* encap features might change during reset if fw variant changed */
 	if (efx_has_cap(efx, VXLAN_NVGRE) && !efx_ef10_is_vf(efx))
-		hw_enc_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
-	/* add encapsulated TSO features */
-	if (efx_has_cap(efx, TX_TSO_V2_ENCAP)) {
-		netdev_features_t encap_tso_features;
+		net_dev->hw_enc_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
+	else
+		net_dev->hw_enc_features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM);
 
-		encap_tso_features = NETIF_F_GSO_UDP_TUNNEL | NETIF_F_GSO_GRE |
-			NETIF_F_GSO_UDP_TUNNEL_CSUM | NETIF_F_GSO_GRE_CSUM;
+	tun_feats = NETIF_F_GSO_UDP_TUNNEL | NETIF_F_GSO_GRE |
+		    NETIF_F_GSO_UDP_TUNNEL_CSUM | NETIF_F_GSO_GRE_CSUM;
+	tso_feats = NETIF_F_TSO | NETIF_F_TSO6;
 
-		hw_enc_features |= encap_tso_features | NETIF_F_TSO;
-		efx->net_dev->features |= encap_tso_features;
+	if (efx_has_cap(efx, TX_TSO_V2_ENCAP)) {
+		/* If this is first nic_init, or if it is a reset and a new fw
+		 * variant has added new features, enable them by default.
+		 * If the features are not new, maintain their current value.
+		 */
+		if (!(net_dev->hw_features & tun_feats))
+			net_dev->features |= tun_feats;
+		net_dev->hw_enc_features |= tun_feats | tso_feats;
+		net_dev->hw_features |= tun_feats;
+	} else {
+		net_dev->hw_enc_features &= ~(tun_feats | tso_feats);
+		net_dev->hw_features &= ~tun_feats;
+		net_dev->features &= ~tun_feats;
 	}
-	efx->net_dev->hw_enc_features = hw_enc_features;
 
 	/* don't fail init if RSS setup doesn't work */
 	rc = efx->type->rx_push_rss_config(efx, false,
@@ -4021,7 +4032,10 @@ static unsigned int efx_ef10_recycle_ring_size(const struct efx_nic *efx)
 	 NETIF_F_HW_VLAN_CTAG_FILTER |	\
 	 NETIF_F_IPV6_CSUM |		\
 	 NETIF_F_RXHASH |		\
-	 NETIF_F_NTUPLE)
+	 NETIF_F_NTUPLE |		\
+	 NETIF_F_SG |			\
+	 NETIF_F_RXCSUM |		\
+	 NETIF_F_RXALL)
 
 const struct efx_nic_type efx_hunt_a0_vf_nic_type = {
 	.is_vf = true,
diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index 02c2adeb0a12..884d8d168862 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -1001,21 +1001,18 @@ static int efx_pci_probe_post_io(struct efx_nic *efx)
 	}
 
 	/* Determine netdevice features */
-	net_dev->features |= (efx->type->offload_features | NETIF_F_SG |
-			      NETIF_F_TSO | NETIF_F_RXCSUM | NETIF_F_RXALL);
-	if (efx->type->offload_features & (NETIF_F_IPV6_CSUM | NETIF_F_HW_CSUM)) {
-		net_dev->features |= NETIF_F_TSO6;
-		if (efx_has_cap(efx, TX_TSO_V2_ENCAP))
-			net_dev->hw_enc_features |= NETIF_F_TSO6;
-	}
-	/* Check whether device supports TSO */
-	if (!efx->type->tso_versions || !efx->type->tso_versions(efx))
-		net_dev->features &= ~NETIF_F_ALL_TSO;
+	net_dev->features |= efx->type->offload_features;
+
+	/* Add TSO features */
+	if (efx->type->tso_versions && efx->type->tso_versions(efx))
+		net_dev->features |= NETIF_F_TSO | NETIF_F_TSO6;
+
 	/* Mask for features that also apply to VLAN devices */
 	net_dev->vlan_features |= (NETIF_F_HW_CSUM | NETIF_F_SG |
 				   NETIF_F_HIGHDMA | NETIF_F_ALL_TSO |
 				   NETIF_F_RXCSUM);
 
+	/* Determine user configurable features */
 	net_dev->hw_features |= net_dev->features & ~efx->fixed_features;
 
 	/* Disable receiving frames with bad FCS, by default. */
-- 
2.39.2

Re: [PATCH net] sfc: ef10: don't overwrite offload features at NIC reset

[Edward Cree]

On 08/03/2023 11:32, Íñigo Huguet wrote:
> At NIC reset, some offload features related to encapsulated traffic
> might have changed (this mainly happens if the firmware-variant is
> changed with the sfboot userspace tool). Because of this, features are
> checked and set again at reset time.
> 
> However, this was not done right, and some features were improperly
> overwritten at NIC reset:
> - Tunneled IPv6 segmentation was always disabled
> - Features disabled with ethtool were reenabled
> - Features that becomes unsupported after the reset were not disabled
> 
> Also, cleanup a bit the setting of other features not related to
> encapsulation. Now that Siena devices are unsupported, some checks are
> unnecessary because they're always supported in all ef10 models.

Could you clarify what checks were removed?  All I can see is the
 'NETIF_F_TSO6 requires NETIF_F_IPV6_CSUM' check, and Siena already
 supported NETIF_F_IPV6_CSUM (it's only Falcon that didn't).
Or are you also referring to some items moving from efx.c to the
 definition of EF10_OFFLOAD_FEATURES?  That's fine and matches more
 closely to what we do for ef100, but again the commit message could
 explain this better.
In any case this should really be two separate patches, with the
 cleanup part going to net-next.
That said, the above is all nit-picky, and the fix looks good, so:
Acked-by: Edward Cree <ecree.xilinx@gmail.com>

Re: [PATCH net] sfc: ef10: don't overwrite offload features at NIC reset

[Íñigo Huguet]

On Wed, Mar 8, 2023 at 4:53 PM Edward Cree <ecree.xilinx@gmail.com> wrote:
>
> On 08/03/2023 11:32, Íñigo Huguet wrote:
> > At NIC reset, some offload features related to encapsulated traffic
> > might have changed (this mainly happens if the firmware-variant is
> > changed with the sfboot userspace tool). Because of this, features are
> > checked and set again at reset time.
> >
> > However, this was not done right, and some features were improperly
> > overwritten at NIC reset:
> > - Tunneled IPv6 segmentation was always disabled
> > - Features disabled with ethtool were reenabled
> > - Features that becomes unsupported after the reset were not disabled
> >
> > Also, cleanup a bit the setting of other features not related to
> > encapsulation. Now that Siena devices are unsupported, some checks are
> > unnecessary because they're always supported in all ef10 models.
>
> Could you clarify what checks were removed?  All I can see is the
>  'NETIF_F_TSO6 requires NETIF_F_IPV6_CSUM' check, and Siena already
>  supported NETIF_F_IPV6_CSUM (it's only Falcon that didn't).

Yes, those are the removed checks. It's only one check, actually,
sorry for the inaccuracy.

I didn't know since what device family was this supported. Then this
check was unnecessary since the sfc_falcon split.

> Or are you also referring to some items moving from efx.c to the
>  definition of EF10_OFFLOAD_FEATURES?  That's fine and matches more
>  closely to what we do for ef100, but again the commit message could
>  explain this better.

Yes, a very poor explanation. Moving those items is part of the "cleanup", yes.

> In any case this should really be two separate patches, with the
>  cleanup part going to net-next.

I thought about doing that, but I'm always reluctant to increase
reviewers' work by sending small cleanups, so being small and very
related to this patch, I included it here.

> That said, the above is all nit-picky, and the fix looks good, so:
> Acked-by: Edward Cree <ecree.xilinx@gmail.com>
>


-- 
Íñigo Huguet

Re: [PATCH net] sfc: ef10: don't overwrite offload features at NIC reset

[Íñigo Huguet]

On Wed, Mar 8, 2023 at 4:53 PM Edward Cree <ecree.xilinx@gmail.com> wrote:
>
> On 08/03/2023 11:32, Íñigo Huguet wrote:
> > At NIC reset, some offload features related to encapsulated traffic
> > might have changed (this mainly happens if the firmware-variant is
> > changed with the sfboot userspace tool). Because of this, features are
> > checked and set again at reset time.
> >
> > However, this was not done right, and some features were improperly
> > overwritten at NIC reset:
> > - Tunneled IPv6 segmentation was always disabled
> > - Features disabled with ethtool were reenabled
> > - Features that becomes unsupported after the reset were not disabled
> >
> > Also, cleanup a bit the setting of other features not related to
> > encapsulation. Now that Siena devices are unsupported, some checks are
> > unnecessary because they're always supported in all ef10 models.
>
> Could you clarify what checks were removed?  All I can see is the
>  'NETIF_F_TSO6 requires NETIF_F_IPV6_CSUM' check, and Siena already
>  supported NETIF_F_IPV6_CSUM (it's only Falcon that didn't).
> Or are you also referring to some items moving from efx.c to the
>  definition of EF10_OFFLOAD_FEATURES?  That's fine and matches more
>  closely to what we do for ef100, but again the commit message could
>  explain this better.
> In any case this should really be two separate patches, with the
>  cleanup part going to net-next.
> That said, the above is all nit-picky, and the fix looks good, so:
> Acked-by: Edward Cree <ecree.xilinx@gmail.com>
>

Hi. Kindly asking about the state of this patch, it is acked since 2
weeks ago and it appears in patchwork as "changes requested". Is there
something else I need to do? Thanks!

-- 
Íñigo Huguet

Re: [PATCH net] sfc: ef10: don't overwrite offload features at NIC reset

[Jakub Kicinski]

On Wed, 22 Mar 2023 07:47:42 +0100 Íñigo Huguet wrote:
> > Could you clarify what checks were removed?  All I can see is the
> >  'NETIF_F_TSO6 requires NETIF_F_IPV6_CSUM' check, and Siena already
> >  supported NETIF_F_IPV6_CSUM (it's only Falcon that didn't).
> > Or are you also referring to some items moving from efx.c to the
> >  definition of EF10_OFFLOAD_FEATURES?  That's fine and matches more
> >  closely to what we do for ef100, but again the commit message could
> >  explain this better.
> > In any case this should really be two separate patches, with the
> >  cleanup part going to net-next.
> > That said, the above is all nit-picky, and the fix looks good, so:
> > Acked-by: Edward Cree <ecree.xilinx@gmail.com>
> 
> Hi. Kindly asking about the state of this patch, it is acked since 2
> weeks ago and it appears in patchwork as "changes requested". Is there
> something else I need to do? Thanks!

The commit message needs to be beefed up to answer all questions Ed had.