diff mbox series

[net-next,10/10] bnxt_en: Implement .ndo_features_check().

Message ID 1619295271-30853-11-git-send-email-michael.chan@broadcom.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series bnxt_en: Updates for net-next. | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 4 of 4 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 69 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Michael Chan April 24, 2021, 8:14 p.m. UTC 
For UDP encapsultions, we only support the offloaded Vxlan port and
Geneve port.  All other ports included FOU and GUE are not supported so
we need to turn off TSO and checksum features.

Reviewed-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 41 +++++++++++++++++++++--
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |  2 ++
 2 files changed, 41 insertions(+), 2 deletions(-)

Comments

Jakub Kicinski April 24, 2021, 9:52 p.m. UTC | #1 
On Sat, 24 Apr 2021 16:14:31 -0400 Michael Chan wrote:
> +	features = vlan_features_check(skb, features);
> +	if (!skb->encapsulation)
> +		return features;
> +
> +	switch (vlan_get_protocol(skb)) {
> +	case htons(ETH_P_IP):
> +		l4_proto = ip_hdr(skb)->protocol;
> +		break;
> +	case htons(ETH_P_IPV6):
> +		l4_proto = ipv6_hdr(skb)->nexthdr;
> +		break;
> +	default:
> +		return features;
> +	}
> +
> +	/* For UDP, we can only handle 1 Vxlan port and 1 Geneve port. */
> +	if (l4_proto == IPPROTO_UDP) {
> +		struct bnxt *bp = netdev_priv(dev);
> +		__be16 udp_port = udp_hdr(skb)->dest;
> +
> +		if (udp_port != bp->vxlan_port && udp_port != bp->nge_port)
> +			return features & ~(NETIF_F_CSUM_MASK |
> +					    NETIF_F_GSO_MASK);
> +	}
> +	return features;

This is still written a little too much like a block list.

What if, for example it's a UDP tunnel but with extension headers?
Is there any particular case that is served by not writing it as:

	if (l4_proto == UDP && (port == bp->vxl_port ||
				port == bp->nge_port))
		return features;
	return features & ~(CSUM | GSO);
?

Sorry for not realizing this earlier.
Michael Chan April 24, 2021, 11:28 p.m. UTC | #2 
On Sat, Apr 24, 2021 at 2:52 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sat, 24 Apr 2021 16:14:31 -0400 Michael Chan wrote:
> > +     features = vlan_features_check(skb, features);
> > +     if (!skb->encapsulation)
> > +             return features;
> > +
> > +     switch (vlan_get_protocol(skb)) {
> > +     case htons(ETH_P_IP):
> > +             l4_proto = ip_hdr(skb)->protocol;
> > +             break;
> > +     case htons(ETH_P_IPV6):
> > +             l4_proto = ipv6_hdr(skb)->nexthdr;
> > +             break;
> > +     default:
> > +             return features;
> > +     }
> > +
> > +     /* For UDP, we can only handle 1 Vxlan port and 1 Geneve port. */
> > +     if (l4_proto == IPPROTO_UDP) {
> > +             struct bnxt *bp = netdev_priv(dev);
> > +             __be16 udp_port = udp_hdr(skb)->dest;
> > +
> > +             if (udp_port != bp->vxlan_port && udp_port != bp->nge_port)
> > +                     return features & ~(NETIF_F_CSUM_MASK |
> > +                                         NETIF_F_GSO_MASK);
> > +     }
> > +     return features;
>
> This is still written a little too much like a block list.
>
> What if, for example it's a UDP tunnel but with extension headers?
> Is there any particular case that is served by not writing it as:
>
>         if (l4_proto == UDP && (port == bp->vxl_port ||
>                                 port == bp->nge_port))
>                 return features;
>         return features & ~(CSUM | GSO);
> ?

Sure, I can change it to check for port == instead of port != to make
it more straight forward.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 53db073b457c..9d7c98a617f9 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -10781,6 +10781,39 @@  static int bnxt_set_features(struct net_device *dev, netdev_features_t features)
 	return rc;
 }
 
+static netdev_features_t bnxt_features_check(struct sk_buff *skb,
+					     struct net_device *dev,
+					     netdev_features_t features)
+{
+	u8 l4_proto = 0;
+
+	features = vlan_features_check(skb, features);
+	if (!skb->encapsulation)
+		return features;
+
+	switch (vlan_get_protocol(skb)) {
+	case htons(ETH_P_IP):
+		l4_proto = ip_hdr(skb)->protocol;
+		break;
+	case htons(ETH_P_IPV6):
+		l4_proto = ipv6_hdr(skb)->nexthdr;
+		break;
+	default:
+		return features;
+	}
+
+	/* For UDP, we can only handle 1 Vxlan port and 1 Geneve port. */
+	if (l4_proto == IPPROTO_UDP) {
+		struct bnxt *bp = netdev_priv(dev);
+		__be16 udp_port = udp_hdr(skb)->dest;
+
+		if (udp_port != bp->vxlan_port && udp_port != bp->nge_port)
+			return features & ~(NETIF_F_CSUM_MASK |
+					    NETIF_F_GSO_MASK);
+	}
+	return features;
+}
+
 int bnxt_dbg_hwrm_rd_reg(struct bnxt *bp, u32 reg_off, u16 num_words,
 			 u32 *reg_buf)
 {
@@ -12288,10 +12321,13 @@  static int bnxt_udp_tunnel_sync(struct net_device *netdev, unsigned int table)
 	unsigned int cmd;
 
 	udp_tunnel_nic_get_port(netdev, table, 0, &ti);
-	if (ti.type == UDP_TUNNEL_TYPE_VXLAN)
+	if (ti.type == UDP_TUNNEL_TYPE_VXLAN) {
+		bp->vxlan_port = ti.port;
 		cmd = TUNNEL_DST_PORT_FREE_REQ_TUNNEL_TYPE_VXLAN;
-	else
+	} else {
+		bp->nge_port = ti.port;
 		cmd = TUNNEL_DST_PORT_FREE_REQ_TUNNEL_TYPE_GENEVE;
+	}
 
 	if (ti.port)
 		return bnxt_hwrm_tunnel_dst_port_alloc(bp, ti.port, cmd);
@@ -12391,6 +12427,7 @@  static const struct net_device_ops bnxt_netdev_ops = {
 	.ndo_change_mtu		= bnxt_change_mtu,
 	.ndo_fix_features	= bnxt_fix_features,
 	.ndo_set_features	= bnxt_set_features,
+	.ndo_features_check	= bnxt_features_check,
 	.ndo_tx_timeout		= bnxt_tx_timeout,
 #ifdef CONFIG_BNXT_SRIOV
 	.ndo_get_vf_config	= bnxt_get_vf_config,
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index a3744247740b..24d2ad6a8740 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -1914,6 +1914,8 @@  struct bnxt {
 
 	u16			vxlan_fw_dst_port_id;
 	u16			nge_fw_dst_port_id;
+	__be16			vxlan_port;
+	__be16			nge_port;
 	u8			port_partition_type;
 	u8			port_count;
 	u16			br_mode;