[PATCH net v2 0/4] Wangxun fixes

[PATCH net v2 0/4] Wangxun fixes

[Jiawen Wu]

Fixed some bugs when using ethtool to operate network devices.

v1 -> v2:
- Factor out the same code.
- Remove statistics printing with more than 64 queues.
- Detail the commit logs to describe issues.
- Remove reset flag check in wx_update_stats().
- Change to set VLAN CTAG and STAG to be consistent.

Jiawen Wu (4):
  net: wangxun: fix the incorrect display of queue number in statistics
  net: wangxun: fix to change Rx features
  net: wangxun: match VLAN CTAG and STAG features
  net: txgbe: fix to control VLAN strip

 .../net/ethernet/wangxun/libwx/wx_ethtool.c   | 23 ++++----
 drivers/net/ethernet/wangxun/libwx/wx_hw.c    |  2 +
 drivers/net/ethernet/wangxun/libwx/wx_lib.c   | 56 +++++++++++++++++--
 drivers/net/ethernet/wangxun/libwx/wx_lib.h   |  2 +
 drivers/net/ethernet/wangxun/libwx/wx_type.h  | 22 ++++++++
 .../net/ethernet/wangxun/ngbe/ngbe_ethtool.c  | 18 ++++--
 drivers/net/ethernet/wangxun/ngbe/ngbe_main.c |  1 +
 .../ethernet/wangxun/txgbe/txgbe_ethtool.c    | 18 ++++--
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   | 31 ++++++++++
 .../net/ethernet/wangxun/txgbe/txgbe_type.h   |  1 +
 10 files changed, 148 insertions(+), 26 deletions(-)

-- 
2.27.0

[PATCH net v2 1/4] net: wangxun: fix the incorrect display of queue number in statistics

[Jiawen Wu]

When using ethtool -S to print hardware statistics, the number of
Rx/Tx queues printed is greater than the number of queues actually
used.

Fixes: 46b92e10d631 ("net: libwx: support hardware statistics")
Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 .../net/ethernet/wangxun/libwx/wx_ethtool.c   | 23 +++++++++----------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
index cc3bec42ed8e..803c7f1da9a4 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
@@ -59,9 +59,12 @@ static const struct wx_stats wx_gstrings_stats[] = {
 
 int wx_get_sset_count(struct net_device *netdev, int sset)
 {
+	struct wx *wx = netdev_priv(netdev);
+
 	switch (sset) {
 	case ETH_SS_STATS:
-		return WX_STATS_LEN;
+		return WX_STATS_LEN - (WX_NUM_RX_QUEUES - wx->num_tx_queues) *
+		       (sizeof(struct wx_queue_stats) / sizeof(u64)) * 2;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -70,6 +73,7 @@ EXPORT_SYMBOL(wx_get_sset_count);
 
 void wx_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
 {
+	struct wx *wx = netdev_priv(netdev);
 	u8 *p = data;
 	int i;
 
@@ -77,11 +81,11 @@ void wx_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
 	case ETH_SS_STATS:
 		for (i = 0; i < WX_GLOBAL_STATS_LEN; i++)
 			ethtool_puts(&p, wx_gstrings_stats[i].stat_string);
-		for (i = 0; i < netdev->num_tx_queues; i++) {
+		for (i = 0; i < wx->num_tx_queues; i++) {
 			ethtool_sprintf(&p, "tx_queue_%u_packets", i);
 			ethtool_sprintf(&p, "tx_queue_%u_bytes", i);
 		}
-		for (i = 0; i < WX_NUM_RX_QUEUES; i++) {
+		for (i = 0; i < wx->num_rx_queues; i++) {
 			ethtool_sprintf(&p, "rx_queue_%u_packets", i);
 			ethtool_sprintf(&p, "rx_queue_%u_bytes", i);
 		}
@@ -107,7 +111,7 @@ void wx_get_ethtool_stats(struct net_device *netdev,
 			   sizeof(u64)) ? *(u64 *)p : *(u32 *)p;
 	}
 
-	for (j = 0; j < netdev->num_tx_queues; j++) {
+	for (j = 0; j < wx->num_tx_queues; j++) {
 		ring = wx->tx_ring[j];
 		if (!ring) {
 			data[i++] = 0;
@@ -122,7 +126,7 @@ void wx_get_ethtool_stats(struct net_device *netdev,
 		} while (u64_stats_fetch_retry(&ring->syncp, start));
 		i += 2;
 	}
-	for (j = 0; j < WX_NUM_RX_QUEUES; j++) {
+	for (j = 0; j < wx->num_rx_queues; j++) {
 		ring = wx->rx_ring[j];
 		if (!ring) {
 			data[i++] = 0;
@@ -177,13 +181,8 @@ void wx_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *info)
 	strscpy(info->driver, wx->driver_name, sizeof(info->driver));
 	strscpy(info->fw_version, wx->eeprom_id, sizeof(info->fw_version));
 	strscpy(info->bus_info, pci_name(wx->pdev), sizeof(info->bus_info));
-	if (wx->num_tx_queues <= WX_NUM_TX_QUEUES) {
-		info->n_stats = WX_STATS_LEN -
-				   (WX_NUM_TX_QUEUES - wx->num_tx_queues) *
-				   (sizeof(struct wx_queue_stats) / sizeof(u64)) * 2;
-	} else {
-		info->n_stats = WX_STATS_LEN;
-	}
+	info->n_stats = WX_STATS_LEN - (WX_NUM_TX_QUEUES - wx->num_tx_queues) *
+			(sizeof(struct wx_queue_stats) / sizeof(u64)) * 2;
 }
 EXPORT_SYMBOL(wx_get_drvinfo);
 
-- 
2.27.0

Re: [PATCH net v2 1/4] net: wangxun: fix the incorrect display of queue number in statistics

[Jakub Kicinski]

On Mon, 29 Apr 2024 18:25:16 +0800 Jiawen Wu wrote:
> When using ethtool -S to print hardware statistics, the number of
> Rx/Tx queues printed is greater than the number of queues actually
> used.

The ethtool API fetches the number of stats and the values in an
unsafe, non-atomic way. If someone increases the number of queues
while someone else is fetching the stats the memory of the latter
process will get corrupted. The code is correct as is.

RE: [PATCH net v2 1/4] net: wangxun: fix the incorrect display of queue number in statistics

[Jiawen Wu]

On Wed, May 1, 2024 10:00 AM, Jakub Kicinski wrote:
> On Mon, 29 Apr 2024 18:25:16 +0800 Jiawen Wu wrote:
> > When using ethtool -S to print hardware statistics, the number of
> > Rx/Tx queues printed is greater than the number of queues actually
> > used.
> 
> The ethtool API fetches the number of stats and the values in an
> unsafe, non-atomic way. If someone increases the number of queues
> while someone else is fetching the stats the memory of the latter
> process will get corrupted. The code is correct as is.

So should we keep the old code, showing stats with fixed maximum
number of queues?

Re: [PATCH net v2 1/4] net: wangxun: fix the incorrect display of queue number in statistics

[Jakub Kicinski]

On Thu, 9 May 2024 10:39:22 +0800 Jiawen Wu wrote:
> > The ethtool API fetches the number of stats and the values in an
> > unsafe, non-atomic way. If someone increases the number of queues
> > while someone else is fetching the stats the memory of the latter
> > process will get corrupted. The code is correct as is.  
> 
> So should we keep the old code, showing stats with fixed maximum
> number of queues?

Yes.

[PATCH net v2 2/4] net: wangxun: fix to change Rx features

[Jiawen Wu]

Fix the issue where some Rx features cannot be changed.

When using ethtool -K to turn off rx offload, it returns error and
displays "Could not change any device features". And netdev->features
is not assigned a new value to actually configure the hardware.

Fixes: 6dbedcffcf54 ("net: libwx: Implement xx_set_features ops")
Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/libwx/wx_lib.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
index 6fae161cbcb8..667a5675998c 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
@@ -2690,12 +2690,14 @@ int wx_set_features(struct net_device *netdev, netdev_features_t features)
 		wx->rss_enabled = false;
 	}
 
+	netdev->features = features;
+
 	if (changed &
 	    (NETIF_F_HW_VLAN_CTAG_RX |
 	     NETIF_F_HW_VLAN_STAG_RX))
 		wx_set_rx_mode(netdev);
 
-	return 1;
+	return 0;
 }
 EXPORT_SYMBOL(wx_set_features);
 
-- 
2.27.0

Re: [PATCH net v2 2/4] net: wangxun: fix to change Rx features

[Simon Horman]

On Mon, Apr 29, 2024 at 06:25:17PM +0800, Jiawen Wu wrote:
> Fix the issue where some Rx features cannot be changed.
> 
> When using ethtool -K to turn off rx offload, it returns error and
> displays "Could not change any device features". And netdev->features
> is not assigned a new value to actually configure the hardware.
> 
> Fixes: 6dbedcffcf54 ("net: libwx: Implement xx_set_features ops")
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>

Reviewed-by: Simon Horman <horms@kernel.org>

> ---
>  drivers/net/ethernet/wangxun/libwx/wx_lib.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> index 6fae161cbcb8..667a5675998c 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> @@ -2690,12 +2690,14 @@ int wx_set_features(struct net_device *netdev, netdev_features_t features)
>  		wx->rss_enabled = false;
>  	}
>  
> +	netdev->features = features;
> +

nit: I think it would be slightly nicer to place this
     at the end of the function, just before return.
     But it would make no difference to the logic,
     so I don't feel strongly about this.

>  	if (changed &
>  	    (NETIF_F_HW_VLAN_CTAG_RX |
>  	     NETIF_F_HW_VLAN_STAG_RX))
>  		wx_set_rx_mode(netdev);
>  
> -	return 1;
> +	return 0;
>  }
>  EXPORT_SYMBOL(wx_set_features);
>  
> -- 
> 2.27.0
> 
> 

RE: [PATCH net v2 2/4] net: wangxun: fix to change Rx features

[Jiawen Wu]

> > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > index 6fae161cbcb8..667a5675998c 100644
> > --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > @@ -2690,12 +2690,14 @@ int wx_set_features(struct net_device *netdev, netdev_features_t features)
> >  		wx->rss_enabled = false;
> >  	}
> >
> > +	netdev->features = features;
> > +
> 
> nit: I think it would be slightly nicer to place this
>      at the end of the function, just before return.
>      But it would make no difference to the logic,
>      so I don't feel strongly about this.

Thanks for your notice, but it does have to be written here.
Since 'netdev->features' will be checked in wx_set_rx_mode().

> 
> >  	if (changed &
> >  	    (NETIF_F_HW_VLAN_CTAG_RX |
> >  	     NETIF_F_HW_VLAN_STAG_RX))
> >  		wx_set_rx_mode(netdev);
> >
> > -	return 1;
> > +	return 0;
> >  }
> >  EXPORT_SYMBOL(wx_set_features);
> >
> > --
> > 2.27.0
> >
> >
> 

[PATCH net v2 3/4] net: wangxun: match VLAN CTAG and STAG features

[Jiawen Wu]

Hardware requires VLAN CTAG and STAG configuration always matches. And
whether VLAN CTAG or STAG changes, the configuration needs to be changed
as well.

Fixes: 6670f1ece2c8 ("net: txgbe: Add netdev features support")
Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/libwx/wx_lib.c   | 46 +++++++++++++++++++
 drivers/net/ethernet/wangxun/libwx/wx_lib.h   |  2 +
 drivers/net/ethernet/wangxun/ngbe/ngbe_main.c |  1 +
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   |  1 +
 4 files changed, 50 insertions(+)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
index 667a5675998c..aefd78455468 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
@@ -2701,6 +2701,52 @@ int wx_set_features(struct net_device *netdev, netdev_features_t features)
 }
 EXPORT_SYMBOL(wx_set_features);
 
+netdev_features_t wx_fix_features(struct net_device *netdev,
+				  netdev_features_t features)
+{
+	netdev_features_t changed = netdev->features ^ features;
+
+	if (changed & NETIF_F_HW_VLAN_CTAG_FILTER) {
+		if (features & NETIF_F_HW_VLAN_CTAG_FILTER)
+			features |= NETIF_F_HW_VLAN_STAG_FILTER;
+		else
+			features &= ~NETIF_F_HW_VLAN_STAG_FILTER;
+	}
+	if (changed & NETIF_F_HW_VLAN_STAG_FILTER) {
+		if (features & NETIF_F_HW_VLAN_STAG_FILTER)
+			features |= NETIF_F_HW_VLAN_CTAG_FILTER;
+		else
+			features &= ~NETIF_F_HW_VLAN_CTAG_FILTER;
+	}
+	if (changed & NETIF_F_HW_VLAN_CTAG_RX) {
+		if (features & NETIF_F_HW_VLAN_CTAG_RX)
+			features |= NETIF_F_HW_VLAN_STAG_RX;
+		else
+			features &= ~NETIF_F_HW_VLAN_STAG_RX;
+	}
+	if (changed & NETIF_F_HW_VLAN_STAG_RX) {
+		if (features & NETIF_F_HW_VLAN_STAG_RX)
+			features |= NETIF_F_HW_VLAN_CTAG_RX;
+		else
+			features &= ~NETIF_F_HW_VLAN_CTAG_RX;
+	}
+	if (changed & NETIF_F_HW_VLAN_CTAG_TX) {
+		if (features & NETIF_F_HW_VLAN_CTAG_TX)
+			features |= NETIF_F_HW_VLAN_STAG_TX;
+		else
+			features &= ~NETIF_F_HW_VLAN_STAG_TX;
+	}
+	if (changed & NETIF_F_HW_VLAN_STAG_TX) {
+		if (features & NETIF_F_HW_VLAN_STAG_TX)
+			features |= NETIF_F_HW_VLAN_CTAG_TX;
+		else
+			features &= ~NETIF_F_HW_VLAN_CTAG_TX;
+	}
+
+	return features;
+}
+EXPORT_SYMBOL(wx_fix_features);
+
 void wx_set_ring(struct wx *wx, u32 new_tx_count,
 		 u32 new_rx_count, struct wx_ring *temp_ring)
 {
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.h b/drivers/net/ethernet/wangxun/libwx/wx_lib.h
index ec909e876720..c41b29ea812f 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_lib.h
+++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.h
@@ -30,6 +30,8 @@ int wx_setup_resources(struct wx *wx);
 void wx_get_stats64(struct net_device *netdev,
 		    struct rtnl_link_stats64 *stats);
 int wx_set_features(struct net_device *netdev, netdev_features_t features);
+netdev_features_t wx_fix_features(struct net_device *netdev,
+				  netdev_features_t features);
 void wx_set_ring(struct wx *wx, u32 new_tx_count,
 		 u32 new_rx_count, struct wx_ring *temp_ring);
 
diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
index fdd6b4f70b7a..e894e01d030d 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
@@ -499,6 +499,7 @@ static const struct net_device_ops ngbe_netdev_ops = {
 	.ndo_start_xmit         = wx_xmit_frame,
 	.ndo_set_rx_mode        = wx_set_rx_mode,
 	.ndo_set_features       = wx_set_features,
+	.ndo_fix_features       = wx_fix_features,
 	.ndo_validate_addr      = eth_validate_addr,
 	.ndo_set_mac_address    = wx_set_mac,
 	.ndo_get_stats64        = wx_get_stats64,
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index bd4624d14ca0..b3c0058b045d 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -428,6 +428,7 @@ static const struct net_device_ops txgbe_netdev_ops = {
 	.ndo_start_xmit         = wx_xmit_frame,
 	.ndo_set_rx_mode        = wx_set_rx_mode,
 	.ndo_set_features       = wx_set_features,
+	.ndo_fix_features       = wx_fix_features,
 	.ndo_validate_addr      = eth_validate_addr,
 	.ndo_set_mac_address    = wx_set_mac,
 	.ndo_get_stats64        = wx_get_stats64,
-- 
2.27.0

RE: [PATCH net v2 3/4] net: wangxun: match VLAN CTAG and STAG features

[Sai Krishna Gajula]

> -----Original Message-----
> From: Jiawen Wu <jiawenwu@trustnetic.com>
> Sent: Monday, April 29, 2024 3:55 PM
> To: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; rmk+kernel@armlinux.org.uk; andrew@lunn.ch;
> netdev@vger.kernel.org
> Cc: mengyuanlou@net-swift.com; duanqiangwen@net-swift.com; Jiawen Wu
> <jiawenwu@trustnetic.com>
> Subject: [PATCH net v2 3/4] net: wangxun: match VLAN CTAG and
> STAG features
> 
> Hardware requires VLAN CTAG and STAG configuration always matches. And
> whether VLAN CTAG or STAG changes, the configuration needs to be changed
> as well.
> 
> Fixes: 6670f1ece2c8 ("net: txgbe: Add netdev features support")
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
>  drivers/net/ethernet/wangxun/libwx/wx_lib.c   | 46 +++++++++++++++++++
>  drivers/net/ethernet/wangxun/libwx/wx_lib.h   |  2 +
>  drivers/net/ethernet/wangxun/ngbe/ngbe_main.c |  1 +
>  .../net/ethernet/wangxun/txgbe/txgbe_main.c   |  1 +
>  4 files changed, 50 insertions(+)
> 
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> index 667a5675998c..aefd78455468 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> @@ -2701,6 +2701,52 @@ int wx_set_features(struct net_device *netdev,
> netdev_features_t features)  }  EXPORT_SYMBOL(wx_set_features);
> 
> +netdev_features_t wx_fix_features(struct net_device *netdev,
> +				  netdev_features_t features)
> +{
> +	netdev_features_t changed = netdev->features ^ features;
> +
> +	if (changed & NETIF_F_HW_VLAN_CTAG_FILTER) {
> +		if (features & NETIF_F_HW_VLAN_CTAG_FILTER)
> +			features |= NETIF_F_HW_VLAN_STAG_FILTER;
> +		else
> +			features &= ~NETIF_F_HW_VLAN_STAG_FILTER;
> +	}
> +	if (changed & NETIF_F_HW_VLAN_STAG_FILTER) {
> +		if (features & NETIF_F_HW_VLAN_STAG_FILTER)
> +			features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> +		else
> +			features &= ~NETIF_F_HW_VLAN_CTAG_FILTER;
> +	}
> +	if (changed & NETIF_F_HW_VLAN_CTAG_RX) {
> +		if (features & NETIF_F_HW_VLAN_CTAG_RX)
> +			features |= NETIF_F_HW_VLAN_STAG_RX;
> +		else
> +			features &= ~NETIF_F_HW_VLAN_STAG_RX;
> +	}
> +	if (changed & NETIF_F_HW_VLAN_STAG_RX) {
> +		if (features & NETIF_F_HW_VLAN_STAG_RX)
> +			features |= NETIF_F_HW_VLAN_CTAG_RX;
> +		else
> +			features &= ~NETIF_F_HW_VLAN_CTAG_RX;
> +	}
> +	if (changed & NETIF_F_HW_VLAN_CTAG_TX) {
> +		if (features & NETIF_F_HW_VLAN_CTAG_TX)
> +			features |= NETIF_F_HW_VLAN_STAG_TX;
> +		else
> +			features &= ~NETIF_F_HW_VLAN_STAG_TX;
> +	}
> +	if (changed & NETIF_F_HW_VLAN_STAG_TX) {
> +		if (features & NETIF_F_HW_VLAN_STAG_TX)
> +			features |= NETIF_F_HW_VLAN_CTAG_TX;
> +		else
> +			features &= ~NETIF_F_HW_VLAN_CTAG_TX;
> +	}
> +
> +	return features;
> +}
> +EXPORT_SYMBOL(wx_fix_features);
> +
>  void wx_set_ring(struct wx *wx, u32 new_tx_count,
>  		 u32 new_rx_count, struct wx_ring *temp_ring)  { diff --git
> a/drivers/net/ethernet/wangxun/libwx/wx_lib.h
> b/drivers/net/ethernet/wangxun/libwx/wx_lib.h
> index ec909e876720..c41b29ea812f 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.h
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.h
> @@ -30,6 +30,8 @@ int wx_setup_resources(struct wx *wx);  void
> wx_get_stats64(struct net_device *netdev,
>  		    struct rtnl_link_stats64 *stats);  int wx_set_features(struct
> net_device *netdev, netdev_features_t features);
> +netdev_features_t wx_fix_features(struct net_device *netdev,
> +				  netdev_features_t features);
>  void wx_set_ring(struct wx *wx, u32 new_tx_count,
>  		 u32 new_rx_count, struct wx_ring *temp_ring);
> 
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index fdd6b4f70b7a..e894e01d030d 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> @@ -499,6 +499,7 @@ static const struct net_device_ops ngbe_netdev_ops =
> {
>  	.ndo_start_xmit         = wx_xmit_frame,
>  	.ndo_set_rx_mode        = wx_set_rx_mode,
>  	.ndo_set_features       = wx_set_features,
> +	.ndo_fix_features       = wx_fix_features,
>  	.ndo_validate_addr      = eth_validate_addr,
>  	.ndo_set_mac_address    = wx_set_mac,
>  	.ndo_get_stats64        = wx_get_stats64,
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> index bd4624d14ca0..b3c0058b045d 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> @@ -428,6 +428,7 @@ static const struct net_device_ops txgbe_netdev_ops
> = {
>  	.ndo_start_xmit         = wx_xmit_frame,
>  	.ndo_set_rx_mode        = wx_set_rx_mode,
>  	.ndo_set_features       = wx_set_features,
> +	.ndo_fix_features       = wx_fix_features,
>  	.ndo_validate_addr      = eth_validate_addr,
>  	.ndo_set_mac_address    = wx_set_mac,
>  	.ndo_get_stats64        = wx_get_stats64,
> --
> 2.27.0
> 
Reviewed-by: Sai Krishna <saikrishnag@marvell.com

Re: [PATCH net v2 3/4] net: wangxun: match VLAN CTAG and STAG features

[Simon Horman]

On Mon, Apr 29, 2024 at 06:25:18PM +0800, Jiawen Wu wrote:
> Hardware requires VLAN CTAG and STAG configuration always matches. And
> whether VLAN CTAG or STAG changes, the configuration needs to be changed
> as well.
> 
> Fixes: 6670f1ece2c8 ("net: txgbe: Add netdev features support")
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>

Hi Jiawen Wu,

Thanks for addressing my review of v1.

Reviewed-by: Simon Horman <horms@kernel.org>

...

[PATCH net v2 4/4] net: txgbe: fix to control VLAN strip

[Jiawen Wu]

When VLAN tag strip is changed to enable or disable, the hardware requires
the Rx ring to be in a disabled state, otherwise the feature cannot be
changed.

Fixes: f3b03c655f67 ("net: wangxun: Implement vlan add and kill functions")
Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/libwx/wx_hw.c    |  2 ++
 drivers/net/ethernet/wangxun/libwx/wx_lib.c   |  6 ++--
 drivers/net/ethernet/wangxun/libwx/wx_type.h  | 22 ++++++++++++++
 .../net/ethernet/wangxun/ngbe/ngbe_ethtool.c  | 18 +++++++----
 .../ethernet/wangxun/txgbe/txgbe_ethtool.c    | 18 +++++++----
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   | 30 +++++++++++++++++++
 .../net/ethernet/wangxun/txgbe/txgbe_type.h   |  1 +
 7 files changed, 84 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
index 945c13d1a982..c09a6f744575 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
@@ -1958,6 +1958,8 @@ int wx_sw_init(struct wx *wx)
 		return -ENOMEM;
 	}
 
+	bitmap_zero(wx->state, WX_STATE_NBITS);
+
 	return 0;
 }
 EXPORT_SYMBOL(wx_sw_init);
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
index aefd78455468..ed6a168ff136 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
@@ -2692,9 +2692,9 @@ int wx_set_features(struct net_device *netdev, netdev_features_t features)
 
 	netdev->features = features;
 
-	if (changed &
-	    (NETIF_F_HW_VLAN_CTAG_RX |
-	     NETIF_F_HW_VLAN_STAG_RX))
+	if (wx->mac.type == wx_mac_sp && changed & NETIF_F_HW_VLAN_CTAG_RX)
+		wx->do_reset(netdev);
+	else if (changed & (NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_CTAG_FILTER))
 		wx_set_rx_mode(netdev);
 
 	return 0;
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
index 1fdeb464d5f4..5aaf7b1fa2db 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
+++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
@@ -982,8 +982,13 @@ struct wx_hw_stats {
 	u64 qmprc;
 };
 
+enum wx_state {
+	WX_STATE_RESETTING,
+	WX_STATE_NBITS,		/* must be last */
+};
 struct wx {
 	unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)];
+	DECLARE_BITMAP(state, WX_STATE_NBITS);
 
 	void *priv;
 	u8 __iomem *hw_addr;
@@ -1071,6 +1076,8 @@ struct wx {
 	u64 hw_csum_rx_good;
 	u64 hw_csum_rx_error;
 	u64 alloc_rx_buff_failed;
+
+	void (*do_reset)(struct net_device *netdev);
 };
 
 #define WX_INTR_ALL (~0ULL)
@@ -1131,4 +1138,19 @@ static inline struct wx *phylink_to_wx(struct phylink_config *config)
 	return container_of(config, struct wx, phylink_config);
 }
 
+static inline int wx_set_state_reset(struct wx *wx)
+{
+	u8 timeout = 50;
+
+	while (test_and_set_bit(WX_STATE_RESETTING, wx->state)) {
+		timeout--;
+		if (!timeout)
+			return -EBUSY;
+
+		usleep_range(1000, 2000);
+	}
+
+	return 0;
+}
+
 #endif /* _WX_TYPE_H_ */
diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
index 786a652ae64f..46a5a3e95202 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
@@ -52,7 +52,7 @@ static int ngbe_set_ringparam(struct net_device *netdev,
 	struct wx *wx = netdev_priv(netdev);
 	u32 new_rx_count, new_tx_count;
 	struct wx_ring *temp_ring;
-	int i;
+	int i, err = 0;
 
 	new_tx_count = clamp_t(u32, ring->tx_pending, WX_MIN_TXD, WX_MAX_TXD);
 	new_tx_count = ALIGN(new_tx_count, WX_REQ_TX_DESCRIPTOR_MULTIPLE);
@@ -64,6 +64,10 @@ static int ngbe_set_ringparam(struct net_device *netdev,
 	    new_rx_count == wx->rx_ring_count)
 		return 0;
 
+	err = wx_set_state_reset(wx);
+	if (err)
+		return err;
+
 	if (!netif_running(wx->netdev)) {
 		for (i = 0; i < wx->num_tx_queues; i++)
 			wx->tx_ring[i]->count = new_tx_count;
@@ -72,14 +76,16 @@ static int ngbe_set_ringparam(struct net_device *netdev,
 		wx->tx_ring_count = new_tx_count;
 		wx->rx_ring_count = new_rx_count;
 
-		return 0;
+		goto clear_reset;
 	}
 
 	/* allocate temporary buffer to store rings in */
 	i = max_t(int, wx->num_tx_queues, wx->num_rx_queues);
 	temp_ring = kvmalloc_array(i, sizeof(struct wx_ring), GFP_KERNEL);
-	if (!temp_ring)
-		return -ENOMEM;
+	if (!temp_ring) {
+		err = -ENOMEM;
+		goto clear_reset;
+	}
 
 	ngbe_down(wx);
 
@@ -89,7 +95,9 @@ static int ngbe_set_ringparam(struct net_device *netdev,
 	wx_configure(wx);
 	ngbe_up(wx);
 
-	return 0;
+clear_reset:
+	clear_bit(WX_STATE_RESETTING, wx->state);
+	return err;
 }
 
 static int ngbe_set_channels(struct net_device *dev,
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
index db675512ce4d..31fde3fa7c6b 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
@@ -19,7 +19,7 @@ static int txgbe_set_ringparam(struct net_device *netdev,
 	struct wx *wx = netdev_priv(netdev);
 	u32 new_rx_count, new_tx_count;
 	struct wx_ring *temp_ring;
-	int i;
+	int i, err = 0;
 
 	new_tx_count = clamp_t(u32, ring->tx_pending, WX_MIN_TXD, WX_MAX_TXD);
 	new_tx_count = ALIGN(new_tx_count, WX_REQ_TX_DESCRIPTOR_MULTIPLE);
@@ -31,6 +31,10 @@ static int txgbe_set_ringparam(struct net_device *netdev,
 	    new_rx_count == wx->rx_ring_count)
 		return 0;
 
+	err = wx_set_state_reset(wx);
+	if (err)
+		return err;
+
 	if (!netif_running(wx->netdev)) {
 		for (i = 0; i < wx->num_tx_queues; i++)
 			wx->tx_ring[i]->count = new_tx_count;
@@ -39,14 +43,16 @@ static int txgbe_set_ringparam(struct net_device *netdev,
 		wx->tx_ring_count = new_tx_count;
 		wx->rx_ring_count = new_rx_count;
 
-		return 0;
+		goto clear_reset;
 	}
 
 	/* allocate temporary buffer to store rings in */
 	i = max_t(int, wx->num_tx_queues, wx->num_rx_queues);
 	temp_ring = kvmalloc_array(i, sizeof(struct wx_ring), GFP_KERNEL);
-	if (!temp_ring)
-		return -ENOMEM;
+	if (!temp_ring) {
+		err = -ENOMEM;
+		goto clear_reset;
+	}
 
 	txgbe_down(wx);
 
@@ -55,7 +61,9 @@ static int txgbe_set_ringparam(struct net_device *netdev,
 
 	txgbe_up(wx);
 
-	return 0;
+clear_reset:
+	clear_bit(WX_STATE_RESETTING, wx->state);
+	return err;
 }
 
 static int txgbe_set_channels(struct net_device *dev,
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index b3c0058b045d..8c7a74981b90 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -269,6 +269,8 @@ static int txgbe_sw_init(struct wx *wx)
 	wx->tx_work_limit = TXGBE_DEFAULT_TX_WORK;
 	wx->rx_work_limit = TXGBE_DEFAULT_RX_WORK;
 
+	wx->do_reset = txgbe_do_reset;
+
 	return 0;
 }
 
@@ -421,6 +423,34 @@ int txgbe_setup_tc(struct net_device *dev, u8 tc)
 	return 0;
 }
 
+static void txgbe_reinit_locked(struct wx *wx)
+{
+	int err = 0;
+
+	netif_trans_update(wx->netdev);
+
+	err = wx_set_state_reset(wx);
+	if (err) {
+		wx_err(wx, "wait device reset timeout\n");
+		return;
+	}
+
+	txgbe_down(wx);
+	txgbe_up(wx);
+
+	clear_bit(WX_STATE_RESETTING, wx->state);
+}
+
+void txgbe_do_reset(struct net_device *netdev)
+{
+	struct wx *wx = netdev_priv(netdev);
+
+	if (netif_running(netdev))
+		txgbe_reinit_locked(wx);
+	else
+		txgbe_reset(wx);
+}
+
 static const struct net_device_ops txgbe_netdev_ops = {
 	.ndo_open               = txgbe_open,
 	.ndo_stop               = txgbe_close,
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
index 1b4ff50d5857..f434a7865cb7 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
@@ -134,6 +134,7 @@ extern char txgbe_driver_name[];
 void txgbe_down(struct wx *wx);
 void txgbe_up(struct wx *wx);
 int txgbe_setup_tc(struct net_device *dev, u8 tc);
+void txgbe_do_reset(struct net_device *netdev);
 
 #define NODE_PROP(_NAME, _PROP)			\
 	(const struct software_node) {		\
-- 
2.27.0

Re: [PATCH net v2 4/4] net: txgbe: fix to control VLAN strip

[Simon Horman]

On Mon, Apr 29, 2024 at 06:25:19PM +0800, Jiawen Wu wrote:
> When VLAN tag strip is changed to enable or disable, the hardware requires
> the Rx ring to be in a disabled state, otherwise the feature cannot be
> changed.
> 
> Fixes: f3b03c655f67 ("net: wangxun: Implement vlan add and kill functions")
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
>  drivers/net/ethernet/wangxun/libwx/wx_hw.c    |  2 ++
>  drivers/net/ethernet/wangxun/libwx/wx_lib.c   |  6 ++--
>  drivers/net/ethernet/wangxun/libwx/wx_type.h  | 22 ++++++++++++++
>  .../net/ethernet/wangxun/ngbe/ngbe_ethtool.c  | 18 +++++++----
>  .../ethernet/wangxun/txgbe/txgbe_ethtool.c    | 18 +++++++----
>  .../net/ethernet/wangxun/txgbe/txgbe_main.c   | 30 +++++++++++++++++++
>  .../net/ethernet/wangxun/txgbe/txgbe_type.h   |  1 +
>  7 files changed, 84 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> index 945c13d1a982..c09a6f744575 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> @@ -1958,6 +1958,8 @@ int wx_sw_init(struct wx *wx)
>  		return -ENOMEM;
>  	}
>  
> +	bitmap_zero(wx->state, WX_STATE_NBITS);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(wx_sw_init);
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> index aefd78455468..ed6a168ff136 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> @@ -2692,9 +2692,9 @@ int wx_set_features(struct net_device *netdev, netdev_features_t features)
>  
>  	netdev->features = features;
>  
> -	if (changed &
> -	    (NETIF_F_HW_VLAN_CTAG_RX |
> -	     NETIF_F_HW_VLAN_STAG_RX))
> +	if (wx->mac.type == wx_mac_sp && changed & NETIF_F_HW_VLAN_CTAG_RX)
> +		wx->do_reset(netdev);
> +	else if (changed & (NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_CTAG_FILTER))
>  		wx_set_rx_mode(netdev);
>  
>  	return 0;

Hi Jiawen Wu,

NETIF_F_HW_VLAN_CTAG_RX appears in both the "if" and "if else" condition.
Should "if else" be changed to "if" ?

...

RE: [PATCH net v2 4/4] net: txgbe: fix to control VLAN strip

[Jiawen Wu]

> > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > index aefd78455468..ed6a168ff136 100644
> > --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > @@ -2692,9 +2692,9 @@ int wx_set_features(struct net_device *netdev, netdev_features_t features)
> >
> >  	netdev->features = features;
> >
> > -	if (changed &
> > -	    (NETIF_F_HW_VLAN_CTAG_RX |
> > -	     NETIF_F_HW_VLAN_STAG_RX))
> > +	if (wx->mac.type == wx_mac_sp && changed & NETIF_F_HW_VLAN_CTAG_RX)
> > +		wx->do_reset(netdev);
> > +	else if (changed & (NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_CTAG_FILTER))
> >  		wx_set_rx_mode(netdev);
> >
> >  	return 0;
> 
> Hi Jiawen Wu,
> 
> NETIF_F_HW_VLAN_CTAG_RX appears in both the "if" and "if else" condition.
> Should "if else" be changed to "if" ?

There are 4 cases where wx_set_rx_mode() is called, CTAG_RX and CTAG_FILTER
combined with wx_mac_sp and wx_mac_em. But only one special case that
changing CTAG_RX requires wx_mac_sp device to do reset, and wx_set_rx_mode()
also will be called during the reset process. So I think "if else" is more appropriate
here.

Re: [PATCH net v2 4/4] net: txgbe: fix to control VLAN strip

[Simon Horman]

On Thu, May 09, 2024 at 11:08:46AM +0800, Jiawen Wu wrote:
> > > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > > index aefd78455468..ed6a168ff136 100644
> > > --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > > +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > > @@ -2692,9 +2692,9 @@ int wx_set_features(struct net_device *netdev, netdev_features_t features)
> > >
> > >  	netdev->features = features;
> > >
> > > -	if (changed &
> > > -	    (NETIF_F_HW_VLAN_CTAG_RX |
> > > -	     NETIF_F_HW_VLAN_STAG_RX))
> > > +	if (wx->mac.type == wx_mac_sp && changed & NETIF_F_HW_VLAN_CTAG_RX)
> > > +		wx->do_reset(netdev);
> > > +	else if (changed & (NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_CTAG_FILTER))
> > >  		wx_set_rx_mode(netdev);
> > >
> > >  	return 0;
> > 
> > Hi Jiawen Wu,
> > 
> > NETIF_F_HW_VLAN_CTAG_RX appears in both the "if" and "if else" condition.
> > Should "if else" be changed to "if" ?
> 
> There are 4 cases where wx_set_rx_mode() is called, CTAG_RX and CTAG_FILTER
> combined with wx_mac_sp and wx_mac_em. But only one special case that
> changing CTAG_RX requires wx_mac_sp device to do reset, and wx_set_rx_mode()
> also will be called during the reset process. So I think "if else" is more appropriate
> here.

Hi Jiawen Wu,

Thanks for your response.

Looking over this again it seems that I misread the code the first time
around. And i think that the current if ... else if ...  construction is
fine. Sorry for the noise.

Reviewed-by: Simon Horman <horms@kernel.org>