[PATCH net 0/5] Wangxun fixes

[PATCH net 0/5] Wangxun fixes

[Jiawen Wu]

Fixed some bugs when using ethtool to operate network devices.

Jiawen Wu (5):
  net: wangxun: fix the incorrect display of queue number in statistics
  net: wangxun: fix error statistics when the device is reset
  net: wangxun: fix to change Rx features
  net: wangxun: change NETIF_F_HW_VLAN_STAG_* to fixed features
  net: txgbe: fix to control VLAN strip

 .../net/ethernet/wangxun/libwx/wx_ethtool.c   | 19 ++++--
 drivers/net/ethernet/wangxun/libwx/wx_hw.c    |  3 +
 drivers/net/ethernet/wangxun/libwx/wx_lib.c   | 26 +++++++-
 drivers/net/ethernet/wangxun/libwx/wx_lib.h   |  2 +
 drivers/net/ethernet/wangxun/libwx/wx_type.h  |  6 ++
 .../net/ethernet/wangxun/ngbe/ngbe_ethtool.c  | 24 +++++--
 drivers/net/ethernet/wangxun/ngbe/ngbe_main.c |  6 +-
 .../ethernet/wangxun/txgbe/txgbe_ethtool.c    | 24 +++++--
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   | 63 ++++++++++++++++++-
 9 files changed, 154 insertions(+), 19 deletions(-)

-- 
2.27.0

[PATCH net 1/5] 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   | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
index cc3bec42ed8e..3847c909ba1a 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
@@ -59,9 +59,17 @@ 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;
+		if (wx->num_tx_queues <= WX_NUM_RX_QUEUES) {
+			return WX_STATS_LEN -
+			       (WX_NUM_RX_QUEUES - wx->num_tx_queues) *
+			       (sizeof(struct wx_queue_stats) / sizeof(u64)) * 2;
+		} else {
+			return WX_STATS_LEN;
+		}
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -70,6 +78,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 +86,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 +116,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 +131,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;
-- 
2.27.0

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

[Francois Romieu]

Jiawen Wu <jiawenwu@trustnetic.com> :
> 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   | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> index cc3bec42ed8e..3847c909ba1a 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> @@ -59,9 +59,17 @@ 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;
> +		if (wx->num_tx_queues <= WX_NUM_RX_QUEUES) {
> +			return WX_STATS_LEN -
> +			       (WX_NUM_RX_QUEUES - wx->num_tx_queues) *
> +			       (sizeof(struct wx_queue_stats) / sizeof(u64)) * 2;
> +		} else {
> +			return WX_STATS_LEN;
> +		}

The same code appears in wx_get_drvinfo.

1) It may be factored out.
2) The size of stats depends on num_{rx, tx}_queues. Unless there is some
   reason to keep WX_STATS_LEN, you may remove it, avoid the conditional code
   and always perform the required arithmetic.

By the way, I understand that driver allocates num_tx_queues and num_rx_queues
symmetrically as outlined in the comment at the start of wx_ethtool.c.
However, it's a bit unexpected to see this dependency elsewhere.

-- 
Ueimor

[PATCH net 2/5] net: wangxun: fix error statistics when the device is reset

[Jiawen Wu]

Add flag for reset state to avoid reading statistics when hardware
is reset.

Fixes: 883b5984a5d2 ("net: wangxun: add ethtool_ops for ring parameters")
Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/libwx/wx_hw.c    |  3 +++
 drivers/net/ethernet/wangxun/libwx/wx_type.h  |  6 +++++
 .../net/ethernet/wangxun/ngbe/ngbe_ethtool.c  | 24 +++++++++++++++----
 .../ethernet/wangxun/txgbe/txgbe_ethtool.c    | 24 +++++++++++++++----
 4 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
index 945c13d1a982..e4e6a14c4efc 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
@@ -2285,6 +2285,9 @@ void wx_update_stats(struct wx *wx)
 	u64 restart_queue = 0, tx_busy = 0;
 	u32 i;
 
+	if (test_bit(WX_STATE_RESETTING, wx->state))
+		return;
+
 	/* gather some stats to the wx struct that are per queue */
 	for (i = 0; i < wx->num_rx_queues; i++) {
 		struct wx_ring *rx_ring = wx->rx_ring[i];
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
index 1fdeb464d5f4..3726ec2fec06 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
+++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
@@ -982,8 +982,14 @@ 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;
diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
index 786a652ae64f..0e85c5a6633e 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
@@ -52,7 +52,8 @@ 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;
+	u8 timeout = 50;
+	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 +65,15 @@ static int ngbe_set_ringparam(struct net_device *netdev,
 	    new_rx_count == wx->rx_ring_count)
 		return 0;
 
+	while (test_and_set_bit(WX_STATE_RESETTING, wx->state)) {
+		timeout--;
+		if (!timeout) {
+			err = -EBUSY;
+			goto clear_reset;
+		}
+		usleep_range(1000, 2000);
+	}
+
 	if (!netif_running(wx->netdev)) {
 		for (i = 0; i < wx->num_tx_queues; i++)
 			wx->tx_ring[i]->count = new_tx_count;
@@ -72,14 +82,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 +101,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..216599bbf9de 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
@@ -19,7 +19,8 @@ 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;
+	u8 timeout = 50;
+	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 +32,15 @@ static int txgbe_set_ringparam(struct net_device *netdev,
 	    new_rx_count == wx->rx_ring_count)
 		return 0;
 
+	while (test_and_set_bit(WX_STATE_RESETTING, wx->state)) {
+		timeout--;
+		if (!timeout) {
+			err = -EBUSY;
+			goto clear_reset;
+		}
+		usleep_range(1000, 2000);
+	}
+
 	if (!netif_running(wx->netdev)) {
 		for (i = 0; i < wx->num_tx_queues; i++)
 			wx->tx_ring[i]->count = new_tx_count;
@@ -39,14 +49,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 +67,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,
-- 
2.27.0

Re: [PATCH net 2/5] net: wangxun: fix error statistics when the device is reset

[Francois Romieu]

Jiawen Wu <jiawenwu@trustnetic.com> :
> Add flag for reset state to avoid reading statistics when hardware
> is reset.
> 
> Fixes: 883b5984a5d2 ("net: wangxun: add ethtool_ops for ring parameters")
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
>  drivers/net/ethernet/wangxun/libwx/wx_hw.c    |  3 +++
>  drivers/net/ethernet/wangxun/libwx/wx_type.h  |  6 +++++
>  .../net/ethernet/wangxun/ngbe/ngbe_ethtool.c  | 24 +++++++++++++++----
>  .../ethernet/wangxun/txgbe/txgbe_ethtool.c    | 24 +++++++++++++++----
>  4 files changed, 47 insertions(+), 10 deletions(-)
> 
[...]
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
> index 786a652ae64f..0e85c5a6633e 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
> @@ -52,7 +52,8 @@ 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;
> +	u8 timeout = 50;
> +	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 +65,15 @@ static int ngbe_set_ringparam(struct net_device *netdev,
>  	    new_rx_count == wx->rx_ring_count)
>  		return 0;
>  
> +	while (test_and_set_bit(WX_STATE_RESETTING, wx->state)) {
> +		timeout--;
> +		if (!timeout) {
> +			err = -EBUSY;
> +			goto clear_reset;
> +		}
> +		usleep_range(1000, 2000);
> +	}
> +

This code appears twice. It may be factored out.

[...]
> @@ -89,7 +101,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;
>  }

This function always clears the bit but it does not necessarily owns it.

-- 
Ueimor

Re: [PATCH net 2/5] net: wangxun: fix error statistics when the device is reset

[Andrew Lunn]

On Tue, Apr 16, 2024 at 02:29:49PM +0800, Jiawen Wu wrote:
> Add flag for reset state to avoid reading statistics when hardware
> is reset.

This explains the what, which you can also get by reading the code
change. The commit message should also explain the why? What goes
wrong if you read the statistics when the hardware is in reset? Do you
get 42 for every statistic? Does the hardware lockup and the reset
never completes?

      Andrew

RE: [PATCH net 2/5] net: wangxun: fix error statistics when the device is reset

[Jiawen Wu]

On Tue, April 16, 2024 10:56 PM, Andrew Lunn wrote:
> On Tue, Apr 16, 2024 at 02:29:49PM +0800, Jiawen Wu wrote:
> > Add flag for reset state to avoid reading statistics when hardware
> > is reset.
> 
> This explains the what, which you can also get by reading the code
> change. The commit message should also explain the why? What goes
> wrong if you read the statistics when the hardware is in reset? Do you
> get 42 for every statistic? Does the hardware lockup and the reset
> never completes?

I think I should discard this patch, and add the resetting flag to the patch 5/5 to avoid
device reset collision. Since wx_update_stats() is called in txgbe_disable_device() while
device is resetting. And I haven't found a situation that causes statistics confusion.

[PATCH net 3/5] net: wangxun: fix to change Rx features

[Jiawen Wu]

Fix the issue where some Rx features cannot be changed.

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 6dff2c85682d..5c511feb97f2 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 3/5] net: wangxun: fix to change Rx features

[Andrew Lunn]

On Tue, Apr 16, 2024 at 02:29:50PM +0800, Jiawen Wu wrote:
> Fix the issue where some Rx features cannot be changed.

For fixes, please describe the observed behaviour being fixed. Does
ethtool -k return the wrong values?

Somebody might run into a problem, and look at commits to see if it
has already been fixed. However, it is hard to tell what this actually
fixes with the current description.

	Andrew

[PATCH net 4/5] net: wangxun: change NETIF_F_HW_VLAN_STAG_* to fixed features

[Jiawen Wu]

Because the hardware doesn't support the configuration of VLAN STAG,
remove NETIF_F_HW_VLAN_STAG_* in netdev->features, and set their state
to be consistent with NETIF_F_HW_VLAN_CTAG_*.

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

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
index 5c511feb97f2..40612cd29f1b 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
@@ -2701,6 +2701,28 @@ 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)
+{
+	if (features & NETIF_F_HW_VLAN_CTAG_FILTER)
+		features |= NETIF_F_HW_VLAN_STAG_FILTER;
+	else
+		features &= ~NETIF_F_HW_VLAN_STAG_FILTER;
+
+	if (features & NETIF_F_HW_VLAN_CTAG_RX)
+		features |= NETIF_F_HW_VLAN_STAG_RX;
+	else
+		features &= ~NETIF_F_HW_VLAN_STAG_RX;
+
+	if (features & NETIF_F_HW_VLAN_CTAG_TX)
+		features |= NETIF_F_HW_VLAN_STAG_TX;
+	else
+		features &= ~NETIF_F_HW_VLAN_STAG_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..12e47a1056d7 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,
@@ -584,7 +585,10 @@ static int ngbe_probe(struct pci_dev *pdev,
 			   NETIF_F_RXHASH | NETIF_F_RXCSUM;
 	netdev->features |= NETIF_F_SCTP_CRC | NETIF_F_TSO_MANGLEID;
 	netdev->vlan_features |= netdev->features;
-	netdev->features |= NETIF_F_IPV6_CSUM | NETIF_F_VLAN_FEATURES;
+	netdev->features |= NETIF_F_IPV6_CSUM;
+	netdev->features |= NETIF_F_HW_VLAN_CTAG_FILTER |
+			    NETIF_F_HW_VLAN_CTAG_RX |
+			    NETIF_F_HW_VLAN_CTAG_TX;
 	/* copy netdev features into list of user selectable features */
 	netdev->hw_features |= netdev->features | NETIF_F_RXALL;
 	netdev->hw_features |= NETIF_F_NTUPLE | NETIF_F_HW_TC;
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index bd4624d14ca0..af0c548e52b0 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,
@@ -547,7 +548,9 @@ static int txgbe_probe(struct pci_dev *pdev,
 	netdev->features |= NETIF_F_SCTP_CRC;
 	netdev->vlan_features |= netdev->features | NETIF_F_TSO_MANGLEID;
 	netdev->hw_enc_features |= netdev->vlan_features;
-	netdev->features |= NETIF_F_VLAN_FEATURES;
+	netdev->features |= NETIF_F_HW_VLAN_CTAG_FILTER |
+			    NETIF_F_HW_VLAN_CTAG_RX |
+			    NETIF_F_HW_VLAN_CTAG_TX;
 	/* copy netdev features into list of user selectable features */
 	netdev->hw_features |= netdev->features | NETIF_F_RXALL;
 	netdev->hw_features |= NETIF_F_NTUPLE | NETIF_F_HW_TC;
-- 
2.27.0

Re: [PATCH net 4/5] net: wangxun: change NETIF_F_HW_VLAN_STAG_* to fixed features

[Simon Horman]

On Tue, Apr 16, 2024 at 02:29:51PM +0800, Jiawen Wu wrote:
> Because the hardware doesn't support the configuration of VLAN STAG,
> remove NETIF_F_HW_VLAN_STAG_* in netdev->features, and set their state
> to be consistent with NETIF_F_HW_VLAN_CTAG_*.
> 
> Fixes: 6670f1ece2c8 ("net: txgbe: Add netdev features support")
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>

Hi Jiawen Wu,

I am having trouble reconciling "hardware doesn't support the configuration
of VLAN STAG" with both "set their state to be consistent with
NETIF_F_HW_VLAN_CTAG_*" and the code changes.

Is the problem here not that VLAN STAGs aren't supported by
the HW, but rather that the HW requires that corresponding
CTAG and STAG configuration always matches?

I.e, the HW requires:

  f & NETIF_F_HW_VLAN_CTAG_FILTER == f & NETIF_F_HW_VLAN_STAG_FILTER
  f & NETIF_F_HW_VLAN_CTAG_RX     == f & NETIF_F_HW_VLAN_STAG_RX
  f & NETIF_F_HW_VLAN_CTAG_TX     == f & NETIF_F_HW_VLAN_STAG_TX

If so, I wonder if only the wx_fix_features() portion of
this patch is required.

...

RE: [PATCH net 4/5] net: wangxun: change NETIF_F_HW_VLAN_STAG_* to fixed features

[Jiawen Wu]

On Fri, April 19, 2024 2:59 AM, Simon Horman wrote:
> On Tue, Apr 16, 2024 at 02:29:51PM +0800, Jiawen Wu wrote:
> > Because the hardware doesn't support the configuration of VLAN STAG,
> > remove NETIF_F_HW_VLAN_STAG_* in netdev->features, and set their state
> > to be consistent with NETIF_F_HW_VLAN_CTAG_*.
> >
> > Fixes: 6670f1ece2c8 ("net: txgbe: Add netdev features support")
> > Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> 
> Hi Jiawen Wu,
> 
> I am having trouble reconciling "hardware doesn't support the configuration
> of VLAN STAG" with both "set their state to be consistent with
> NETIF_F_HW_VLAN_CTAG_*" and the code changes.
> 
> Is the problem here not that VLAN STAGs aren't supported by
> the HW, but rather that the HW requires that corresponding
> CTAG and STAG configuration always matches?
> 
> I.e, the HW requires:
> 
>   f & NETIF_F_HW_VLAN_CTAG_FILTER == f & NETIF_F_HW_VLAN_STAG_FILTER
>   f & NETIF_F_HW_VLAN_CTAG_RX     == f & NETIF_F_HW_VLAN_STAG_RX
>   f & NETIF_F_HW_VLAN_CTAG_TX     == f & NETIF_F_HW_VLAN_STAG_TX
> 
> If so, I wonder if only the wx_fix_features() portion of
> this patch is required.

You are right. I need to set their state to be consistent in wx_fix_features(),
this patch is missing the case when STAG changes.

Re: [PATCH net 4/5] net: wangxun: change NETIF_F_HW_VLAN_STAG_* to fixed features

[Simon Horman]

On Thu, Apr 25, 2024 at 02:53:24PM +0800, Jiawen Wu wrote:
> On Fri, April 19, 2024 2:59 AM, Simon Horman wrote:
> > On Tue, Apr 16, 2024 at 02:29:51PM +0800, Jiawen Wu wrote:
> > > Because the hardware doesn't support the configuration of VLAN STAG,
> > > remove NETIF_F_HW_VLAN_STAG_* in netdev->features, and set their state
> > > to be consistent with NETIF_F_HW_VLAN_CTAG_*.
> > >
> > > Fixes: 6670f1ece2c8 ("net: txgbe: Add netdev features support")
> > > Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> > 
> > Hi Jiawen Wu,
> > 
> > I am having trouble reconciling "hardware doesn't support the configuration
> > of VLAN STAG" with both "set their state to be consistent with
> > NETIF_F_HW_VLAN_CTAG_*" and the code changes.
> > 
> > Is the problem here not that VLAN STAGs aren't supported by
> > the HW, but rather that the HW requires that corresponding
> > CTAG and STAG configuration always matches?
> > 
> > I.e, the HW requires:
> > 
> >   f & NETIF_F_HW_VLAN_CTAG_FILTER == f & NETIF_F_HW_VLAN_STAG_FILTER
> >   f & NETIF_F_HW_VLAN_CTAG_RX     == f & NETIF_F_HW_VLAN_STAG_RX
> >   f & NETIF_F_HW_VLAN_CTAG_TX     == f & NETIF_F_HW_VLAN_STAG_TX
> > 
> > If so, I wonder if only the wx_fix_features() portion of
> > this patch is required.
> 
> You are right. I need to set their state to be consistent in wx_fix_features(),
> this patch is missing the case when STAG changes.

Yes, agreed. The case where STAG changes also occurred to me after I sent
my previous email. Sorry for forgetting on follow-up on that.

[PATCH net 5/5] 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>
---
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   | 58 ++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index af0c548e52b0..2a6b35036fce 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -421,13 +421,69 @@ int txgbe_setup_tc(struct net_device *dev, u8 tc)
 	return 0;
 }
 
+static void txgbe_reinit_locked(struct wx *wx)
+{
+	u8 timeout = 50;
+
+	netif_trans_update(wx->netdev);
+
+	while (test_and_set_bit(WX_STATE_RESETTING, wx->state)) {
+		timeout--;
+		if (!timeout) {
+			wx_err(wx, "wait device reset timeout\n");
+			goto clear_reset;
+		}
+		usleep_range(1000, 2000);
+	}
+
+	txgbe_down(wx);
+	txgbe_up(wx);
+
+clear_reset:
+	clear_bit(WX_STATE_RESETTING, wx->state);
+}
+
+static 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 int txgbe_set_features(struct net_device *netdev, netdev_features_t features)
+{
+	netdev_features_t changed = netdev->features ^ features;
+	struct wx *wx = netdev_priv(netdev);
+
+	if (features & NETIF_F_RXHASH) {
+		wr32m(wx, WX_RDB_RA_CTL, WX_RDB_RA_CTL_RSS_EN,
+		      WX_RDB_RA_CTL_RSS_EN);
+		wx->rss_enabled = true;
+	} else {
+		wr32m(wx, WX_RDB_RA_CTL, WX_RDB_RA_CTL_RSS_EN, 0);
+		wx->rss_enabled = false;
+	}
+
+	netdev->features = features;
+
+	if (changed & NETIF_F_HW_VLAN_CTAG_RX)
+		txgbe_do_reset(netdev);
+	else if (changed & NETIF_F_HW_VLAN_CTAG_FILTER)
+		wx_set_rx_mode(netdev);
+
+	return 0;
+}
+
 static const struct net_device_ops txgbe_netdev_ops = {
 	.ndo_open               = txgbe_open,
 	.ndo_stop               = txgbe_close,
 	.ndo_change_mtu         = wx_change_mtu,
 	.ndo_start_xmit         = wx_xmit_frame,
 	.ndo_set_rx_mode        = wx_set_rx_mode,
-	.ndo_set_features       = wx_set_features,
+	.ndo_set_features       = txgbe_set_features,
 	.ndo_fix_features       = wx_fix_features,
 	.ndo_validate_addr      = eth_validate_addr,
 	.ndo_set_mac_address    = wx_set_mac,
-- 
2.27.0

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

[Simon Horman]

On Tue, Apr 16, 2024 at 02:29:52PM +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>
> ---
>  .../net/ethernet/wangxun/txgbe/txgbe_main.c   | 58 ++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> index af0c548e52b0..2a6b35036fce 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c

...

> +static int txgbe_set_features(struct net_device *netdev, netdev_features_t features)

There seems to be a lot of overlap between this function and
wx_set_features(). To aid maintenance, the common code be
factored out into a shared function?

> +{
> +	netdev_features_t changed = netdev->features ^ features;
> +	struct wx *wx = netdev_priv(netdev);
> +
> +	if (features & NETIF_F_RXHASH) {
> +		wr32m(wx, WX_RDB_RA_CTL, WX_RDB_RA_CTL_RSS_EN,
> +		      WX_RDB_RA_CTL_RSS_EN);
> +		wx->rss_enabled = true;
> +	} else {
> +		wr32m(wx, WX_RDB_RA_CTL, WX_RDB_RA_CTL_RSS_EN, 0);
> +		wx->rss_enabled = false;
> +	}
> +
> +	netdev->features = features;
> +
> +	if (changed & NETIF_F_HW_VLAN_CTAG_RX)
> +		txgbe_do_reset(netdev);
> +	else if (changed & NETIF_F_HW_VLAN_CTAG_FILTER)
> +		wx_set_rx_mode(netdev);

I see the following in wx_set_features():

        if (changed &
	    (NETIF_F_HW_VLAN_CTAG_RX |
	     NETIF_F_HW_VLAN_STAG_RX))
		wx_set_rx_mode(netdev);

Should NETIF_F_HW_VLAN_STAG_RX and NETIF_F_HW_VLAN_STAG_FILTER
also be checked in this function?

> +
> +	return 0;
> +}
> +
>  static const struct net_device_ops txgbe_netdev_ops = {
>  	.ndo_open               = txgbe_open,
>  	.ndo_stop               = txgbe_close,
>  	.ndo_change_mtu         = wx_change_mtu,
>  	.ndo_start_xmit         = wx_xmit_frame,
>  	.ndo_set_rx_mode        = wx_set_rx_mode,
> -	.ndo_set_features       = wx_set_features,
> +	.ndo_set_features       = txgbe_set_features,
>  	.ndo_fix_features       = wx_fix_features,
>  	.ndo_validate_addr      = eth_validate_addr,
>  	.ndo_set_mac_address    = wx_set_mac,
> -- 
> 2.27.0
> 
>