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 |
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 |
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.
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 --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;