Re: [PATCH net-next v2 0/2] Implement udp tunnel port for txgbe

Re: [PATCH net-next v2 0/2] Implement udp tunnel port for txgbe

[Michal Kubiak]

On Mon, Apr 14, 2025 at 05:10:20PM +0800, Jiawen Wu wrote:
> Setting UDP tunnel port is supported on TXGBE devices, to implement
> hardware offload for various tunnel packets.
> 
> v2:
>  - Link to v1: https://lore.kernel.org/all/20250410074456.321847-1-jiawenwu@trustnetic.com/
>  - Remove pointless checks
>  - Adjust definition order
> 
> Jiawen Wu (2):
>   net: txgbe: Support to set UDP tunnel port
>   net: wangxun: restrict feature flags for tunnel packets
> 
>  drivers/net/ethernet/wangxun/libwx/wx_lib.c   | 27 ++++++
>  drivers/net/ethernet/wangxun/libwx/wx_lib.h   |  3 +
>  drivers/net/ethernet/wangxun/ngbe/ngbe_main.c |  1 +
>  .../net/ethernet/wangxun/txgbe/txgbe_main.c   | 87 +++++++++++++++++++
>  .../net/ethernet/wangxun/txgbe/txgbe_type.h   |  8 ++
>  5 files changed, 126 insertions(+)
> 
> -- 
> 2.27.0
> 
> 

For the series:
Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>

Thanks,
Michal

[PATCH net-next v2 0/2] Implement udp tunnel port for txgbe

[Jiawen Wu]

Setting UDP tunnel port is supported on TXGBE devices, to implement
hardware offload for various tunnel packets.

v2:
 - Link to v1: https://lore.kernel.org/all/20250410074456.321847-1-jiawenwu@trustnetic.com/
 - Remove pointless checks
 - Adjust definition order

Jiawen Wu (2):
  net: txgbe: Support to set UDP tunnel port
  net: wangxun: restrict feature flags for tunnel packets

 drivers/net/ethernet/wangxun/libwx/wx_lib.c   | 27 ++++++
 drivers/net/ethernet/wangxun/libwx/wx_lib.h   |  3 +
 drivers/net/ethernet/wangxun/ngbe/ngbe_main.c |  1 +
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   | 87 +++++++++++++++++++
 .../net/ethernet/wangxun/txgbe/txgbe_type.h   |  8 ++
 5 files changed, 126 insertions(+)

-- 
2.27.0

[PATCH net-next v2 1/2] net: txgbe: Support to set UDP tunnel port

[Jiawen Wu]

Tunnel types VXLAN/VXLAN_GPE/GENEVE are supported for txgbe devices. The
hardware supports to set only one port for each tunnel type.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   | 86 +++++++++++++++++++
 .../net/ethernet/wangxun/txgbe/txgbe_type.h   |  8 ++
 2 files changed, 94 insertions(+)

diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index 6d9134a3ce4d..c3b0f15099db 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -8,6 +8,7 @@
 #include <linux/string.h>
 #include <linux/etherdevice.h>
 #include <linux/phylink.h>
+#include <net/udp_tunnel.h>
 #include <net/ip.h>
 #include <linux/if_vlan.h>
 
@@ -392,6 +393,8 @@ static int txgbe_open(struct net_device *netdev)
 
 	txgbe_up_complete(wx);
 
+	udp_tunnel_nic_reset_ntf(netdev);
+
 	return 0;
 
 err_free_irq:
@@ -537,6 +540,87 @@ void txgbe_do_reset(struct net_device *netdev)
 		txgbe_reset(wx);
 }
 
+static int txgbe_udp_tunnel_set(struct net_device *dev, unsigned int table,
+				unsigned int entry, struct udp_tunnel_info *ti)
+{
+	struct wx *wx = netdev_priv(dev);
+	struct txgbe *txgbe = wx->priv;
+
+	switch (ti->type) {
+	case UDP_TUNNEL_TYPE_VXLAN:
+		if (txgbe->vxlan_port == ti->port)
+			break;
+
+		if (txgbe->vxlan_port) {
+			wx_err(wx, "VXLAN port %d set, not adding port %d\n",
+			       txgbe->vxlan_port, ti->port);
+			return -EINVAL;
+		}
+
+		txgbe->vxlan_port = ti->port;
+		wr32(wx, TXGBE_CFG_VXLAN, ntohs(ti->port));
+		break;
+	case UDP_TUNNEL_TYPE_VXLAN_GPE:
+		if (txgbe->vxlan_gpe_port == ti->port)
+			break;
+
+		if (txgbe->vxlan_gpe_port) {
+			wx_err(wx, "VXLAN-GPE port %d set, not adding port %d\n",
+			       txgbe->vxlan_gpe_port, ti->port);
+			return -EINVAL;
+		}
+
+		txgbe->vxlan_gpe_port = ti->port;
+		wr32(wx, TXGBE_CFG_VXLAN_GPE, ntohs(ti->port));
+		break;
+	case UDP_TUNNEL_TYPE_GENEVE:
+		if (txgbe->geneve_port == ti->port)
+			break;
+
+		if (txgbe->geneve_port) {
+			wx_err(wx, "GENEVE port %d set, not adding port %d\n",
+			       txgbe->geneve_port, ti->port);
+			return -EINVAL;
+		}
+
+		txgbe->geneve_port = ti->port;
+		wr32(wx, TXGBE_CFG_GENEVE, ntohs(ti->port));
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int txgbe_udp_tunnel_unset(struct net_device *dev,
+				  unsigned int table, unsigned int entry,
+				  struct udp_tunnel_info *ti)
+{
+	struct wx *wx = netdev_priv(dev);
+	struct txgbe *txgbe = wx->priv;
+
+	if (ti->type == UDP_TUNNEL_TYPE_VXLAN)
+		txgbe->vxlan_port = 0;
+	else if (ti->type == UDP_TUNNEL_TYPE_VXLAN_GPE)
+		txgbe->vxlan_gpe_port = 0;
+	else
+		txgbe->geneve_port = 0;
+
+	return 0;
+}
+
+static const struct udp_tunnel_nic_info txgbe_udp_tunnels = {
+	.set_port	= txgbe_udp_tunnel_set,
+	.unset_port	= txgbe_udp_tunnel_unset,
+	.flags		= UDP_TUNNEL_NIC_INFO_MAY_SLEEP,
+	.tables		= {
+		{ .n_entries = 1, .tunnel_types = UDP_TUNNEL_TYPE_VXLAN, },
+		{ .n_entries = 1, .tunnel_types = UDP_TUNNEL_TYPE_VXLAN_GPE, },
+		{ .n_entries = 1, .tunnel_types = UDP_TUNNEL_TYPE_GENEVE, },
+	},
+};
+
 static const struct net_device_ops txgbe_netdev_ops = {
 	.ndo_open               = txgbe_open,
 	.ndo_stop               = txgbe_close,
@@ -632,6 +716,7 @@ static int txgbe_probe(struct pci_dev *pdev,
 	wx->driver_name = txgbe_driver_name;
 	txgbe_set_ethtool_ops(netdev);
 	netdev->netdev_ops = &txgbe_netdev_ops;
+	netdev->udp_tunnel_nic_info = &txgbe_udp_tunnels;
 
 	/* setup the private structure */
 	err = txgbe_sw_init(wx);
@@ -677,6 +762,7 @@ static int txgbe_probe(struct pci_dev *pdev,
 	netdev->features |= NETIF_F_HIGHDMA;
 	netdev->hw_features |= NETIF_F_GRO;
 	netdev->features |= NETIF_F_GRO;
+	netdev->features |= NETIF_F_RX_UDP_TUNNEL_PORT;
 
 	netdev->priv_flags |= IFF_UNICAST_FLT;
 	netdev->priv_flags |= IFF_SUPP_NOFCS;
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
index 5937cbc6bd05..7923adb9ac73 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
@@ -88,6 +88,9 @@
 /* Port cfg registers */
 #define TXGBE_CFG_PORT_ST                       0x14404
 #define TXGBE_CFG_PORT_ST_LINK_UP               BIT(0)
+#define TXGBE_CFG_VXLAN                         0x14410
+#define TXGBE_CFG_VXLAN_GPE                     0x14414
+#define TXGBE_CFG_GENEVE                        0x14418
 
 /* I2C registers */
 #define TXGBE_I2C_BASE                          0x14900
@@ -359,6 +362,11 @@ struct txgbe {
 	union txgbe_atr_input fdir_mask;
 	int fdir_filter_count;
 	spinlock_t fdir_perfect_lock; /* spinlock for FDIR */
+
+	/* tunnel port */
+	__be16 vxlan_port;
+	__be16 vxlan_gpe_port;
+	__be16 geneve_port;
 };
 
 #endif /* _TXGBE_TYPE_H_ */
-- 
2.27.0

[PATCH net-next v2 2/2] net: wangxun: restrict feature flags for tunnel packets

[Jiawen Wu]

Implement ndo_features_check to restrict Tx checksum offload flags, since
there are some inner layer length and protocols unsupported.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>
---
 drivers/net/ethernet/wangxun/libwx/wx_lib.c   | 27 +++++++++++++++++++
 drivers/net/ethernet/wangxun/libwx/wx_lib.h   |  3 +++
 drivers/net/ethernet/wangxun/ngbe/ngbe_main.c |  1 +
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   |  1 +
 4 files changed, 32 insertions(+)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
index 18422b940dbe..2a808afeb414 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
@@ -3000,6 +3000,33 @@ netdev_features_t wx_fix_features(struct net_device *netdev,
 }
 EXPORT_SYMBOL(wx_fix_features);
 
+#define WX_MAX_TUNNEL_HDR_LEN	80
+netdev_features_t wx_features_check(struct sk_buff *skb,
+				    struct net_device *netdev,
+				    netdev_features_t features)
+{
+	struct wx *wx = netdev_priv(netdev);
+
+	if (!skb->encapsulation)
+		return features;
+
+	if (wx->mac.type == wx_mac_em)
+		return features & ~NETIF_F_CSUM_MASK;
+
+	if (unlikely(skb_inner_mac_header(skb) - skb_transport_header(skb) >
+		     WX_MAX_TUNNEL_HDR_LEN))
+		return features & ~NETIF_F_CSUM_MASK;
+
+	if (skb->inner_protocol_type == ENCAP_TYPE_ETHER &&
+	    skb->inner_protocol != htons(ETH_P_IP) &&
+	    skb->inner_protocol != htons(ETH_P_IPV6) &&
+	    skb->inner_protocol != htons(ETH_P_TEB))
+		return features & ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
+
+	return features;
+}
+EXPORT_SYMBOL(wx_features_check);
+
 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 fdeb0c315b75..919f49999308 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_lib.h
+++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.h
@@ -33,6 +33,9 @@ void wx_get_stats64(struct net_device *netdev,
 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);
+netdev_features_t wx_features_check(struct sk_buff *skb,
+				    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 fd102078f5c9..82e27b9cfc9c 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
@@ -587,6 +587,7 @@ static const struct net_device_ops ngbe_netdev_ops = {
 	.ndo_set_rx_mode        = wx_set_rx_mode,
 	.ndo_set_features       = wx_set_features,
 	.ndo_fix_features       = wx_fix_features,
+	.ndo_features_check     = wx_features_check,
 	.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 c3b0f15099db..259062b8f990 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -629,6 +629,7 @@ static const struct net_device_ops txgbe_netdev_ops = {
 	.ndo_set_rx_mode        = wx_set_rx_mode,
 	.ndo_set_features       = wx_set_features,
 	.ndo_fix_features       = wx_fix_features,
+	.ndo_features_check     = wx_features_check,
 	.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-next v2 1/2] net: txgbe: Support to set UDP tunnel port

[Jakub Kicinski]

On Mon, 14 Apr 2025 17:10:21 +0800 Jiawen Wu wrote:
> +	udp_tunnel_nic_reset_ntf(netdev);

you issue the reset here, without clearing the ports...

>  	return 0;
>  
>  err_free_irq:
> @@ -537,6 +540,87 @@ void txgbe_do_reset(struct net_device *netdev)
>  		txgbe_reset(wx);
>  }
>  
> +static int txgbe_udp_tunnel_set(struct net_device *dev, unsigned int table,
> +				unsigned int entry, struct udp_tunnel_info *ti)
> +{
> +	struct wx *wx = netdev_priv(dev);
> +	struct txgbe *txgbe = wx->priv;
> +
> +	switch (ti->type) {
> +	case UDP_TUNNEL_TYPE_VXLAN:
> +		if (txgbe->vxlan_port == ti->port)

then you ignore the set if the port is already the same.

Why do you need the udp_tunnel_nic_reset_ntf() call?
Read the kdoc on that function.
It doesn't seem like your NIC loses the state.

> +			break;
> +
> +		if (txgbe->vxlan_port) {
> +			wx_err(wx, "VXLAN port %d set, not adding port %d\n",
> +			       txgbe->vxlan_port, ti->port);
> +			return -EINVAL;
> +		}

Why...

> +		txgbe->vxlan_port = ti->port;
> +		wr32(wx, TXGBE_CFG_VXLAN, ntohs(ti->port));
> +		break;

> +static const struct udp_tunnel_nic_info txgbe_udp_tunnels = {
> +	.set_port	= txgbe_udp_tunnel_set,
> +	.unset_port	= txgbe_udp_tunnel_unset,
> +	.flags		= UDP_TUNNEL_NIC_INFO_MAY_SLEEP,

Where do the callbacks sleep?
-- 
pw-bot: cr

RE: [PATCH net-next v2 1/2] net: txgbe: Support to set UDP tunnel port

[Jiawen Wu]

On Wed, Apr 16, 2025 8:33 AM, Jakub Kicinski wrote:
> On Mon, 14 Apr 2025 17:10:21 +0800 Jiawen Wu wrote:
> > +	udp_tunnel_nic_reset_ntf(netdev);
> 
> you issue the reset here, without clearing the ports...
> 
> >  	return 0;
> >
> >  err_free_irq:
> > @@ -537,6 +540,87 @@ void txgbe_do_reset(struct net_device *netdev)
> >  		txgbe_reset(wx);
> >  }
> >
> > +static int txgbe_udp_tunnel_set(struct net_device *dev, unsigned int table,
> > +				unsigned int entry, struct udp_tunnel_info *ti)
> > +{
> > +	struct wx *wx = netdev_priv(dev);
> > +	struct txgbe *txgbe = wx->priv;
> > +
> > +	switch (ti->type) {
> > +	case UDP_TUNNEL_TYPE_VXLAN:
> > +		if (txgbe->vxlan_port == ti->port)
> 
> then you ignore the set if the port is already the same.
> 
> Why do you need the udp_tunnel_nic_reset_ntf() call?
> Read the kdoc on that function.
> It doesn't seem like your NIC loses the state.
> 
> > +			break;
> > +
> > +		if (txgbe->vxlan_port) {
> > +			wx_err(wx, "VXLAN port %d set, not adding port %d\n",
> > +			       txgbe->vxlan_port, ti->port);
> > +			return -EINVAL;
> > +		}
> 
> Why...
> 
> > +		txgbe->vxlan_port = ti->port;
> > +		wr32(wx, TXGBE_CFG_VXLAN, ntohs(ti->port));
> > +		break;
> 
> > +static const struct udp_tunnel_nic_info txgbe_udp_tunnels = {
> > +	.set_port	= txgbe_udp_tunnel_set,
> > +	.unset_port	= txgbe_udp_tunnel_unset,
> > +	.flags		= UDP_TUNNEL_NIC_INFO_MAY_SLEEP,
> 
> Where do the callbacks sleep?

I now understand how the "reset" works, it will be changed to
UDP_TUNNEL_NIC_INFO_OPEN_ONLY, and use .sync_table to simplify the flow.

RE: [PATCH net-next v2 1/2] net: txgbe: Support to set UDP tunnel port

[Jiawen Wu]

On Wed, Apr 16, 2025 2:30 PM, Jiawen Wu wrote:
> On Wed, Apr 16, 2025 8:33 AM, Jakub Kicinski wrote:
> > On Mon, 14 Apr 2025 17:10:21 +0800 Jiawen Wu wrote:
> > > +	udp_tunnel_nic_reset_ntf(netdev);
> >
> > you issue the reset here, without clearing the ports...
> >
> > >  	return 0;
> > >
> > >  err_free_irq:
> > > @@ -537,6 +540,87 @@ void txgbe_do_reset(struct net_device *netdev)
> > >  		txgbe_reset(wx);
> > >  }
> > >
> > > +static int txgbe_udp_tunnel_set(struct net_device *dev, unsigned int table,
> > > +				unsigned int entry, struct udp_tunnel_info *ti)
> > > +{
> > > +	struct wx *wx = netdev_priv(dev);
> > > +	struct txgbe *txgbe = wx->priv;
> > > +
> > > +	switch (ti->type) {
> > > +	case UDP_TUNNEL_TYPE_VXLAN:
> > > +		if (txgbe->vxlan_port == ti->port)
> >
> > then you ignore the set if the port is already the same.
> >
> > Why do you need the udp_tunnel_nic_reset_ntf() call?
> > Read the kdoc on that function.
> > It doesn't seem like your NIC loses the state.
> >
> > > +			break;
> > > +
> > > +		if (txgbe->vxlan_port) {
> > > +			wx_err(wx, "VXLAN port %d set, not adding port %d\n",
> > > +			       txgbe->vxlan_port, ti->port);
> > > +			return -EINVAL;
> > > +		}
> >
> > Why...
> >
> > > +		txgbe->vxlan_port = ti->port;
> > > +		wr32(wx, TXGBE_CFG_VXLAN, ntohs(ti->port));
> > > +		break;
> >
> > > +static const struct udp_tunnel_nic_info txgbe_udp_tunnels = {
> > > +	.set_port	= txgbe_udp_tunnel_set,
> > > +	.unset_port	= txgbe_udp_tunnel_unset,
> > > +	.flags		= UDP_TUNNEL_NIC_INFO_MAY_SLEEP,
> >
> > Where do the callbacks sleep?
> 
> I now understand how the "reset" works, it will be changed to
> UDP_TUNNEL_NIC_INFO_OPEN_ONLY, and use .sync_table to simplify the flow.

But I have a question.

There are two ethernet chips on the board with four ports eth0~eth3,
they all use txgbe driver. When I add a VXLAN port for eth0, in fact, all
4 devices are configured. So I can't add another different VXLAN port
for eth1~eth3. Is this a fixed behavior of the kernel?