Message ID | 1619372727-19187-11-git-send-email-michael.chan@broadcom.com (mailing list archive) |
---|---|
State | Accepted |
Commit | 1698d600b361915fbe5eda63a613da55c435bd34 |
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, 70 lines checked |
netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
netdev/header_inline | success | Link |
On Sun, 25 Apr 2021 13:45:27 -0400 Michael Chan wrote: > + if (l4_proto != IPPROTO_UDP) > + return features; This should have been "features & ~(CSUM | GSO)" if you actually accepted my feedback. I mentioned extension headers as an example, v2 looks like a minor refactoring, no functional changes :/ > + bp = netdev_priv(dev); > + /* For UDP, we can only handle 1 Vxlan port and 1 Geneve port. */ > + udp_port = udp_hdr(skb)->dest; > + if (udp_port == bp->vxlan_port || udp_port == bp->nge_port) > + return features; > + return features & ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
On Mon, Apr 26, 2021 at 9:29 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Sun, 25 Apr 2021 13:45:27 -0400 Michael Chan wrote: > > + if (l4_proto != IPPROTO_UDP) > > + return features; > > This should have been "features & ~(CSUM | GSO)" if you actually > accepted my feedback. I mentioned extension headers as an example, > v2 looks like a minor refactoring, no functional changes :/ > > > + bp = netdev_priv(dev); > > + /* For UDP, we can only handle 1 Vxlan port and 1 Geneve port. */ > > + udp_port = udp_hdr(skb)->dest; > > + if (udp_port == bp->vxlan_port || udp_port == bp->nge_port) > > + return features; > > + return features & ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
On Mon, Apr 26, 2021 at 9:35 AM Michael Chan <michael.chan@broadcom.com> wrote: > > On Mon, Apr 26, 2021 at 9:29 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Sun, 25 Apr 2021 13:45:27 -0400 Michael Chan wrote: > > > + if (l4_proto != IPPROTO_UDP) > > > + return features; > > > > This should have been "features & ~(CSUM | GSO)" if you actually > > accepted my feedback. Sorry, hit send too early. If it is not UDP, it could be GRE or IP over IP for example, right? Why do we need to turn off offload? > I mentioned extension headers as an example, Extension headers (Geneve for example) are supported.
On Mon, 26 Apr 2021 09:45:17 -0700 Michael Chan wrote: > On Mon, Apr 26, 2021 at 9:35 AM Michael Chan <michael.chan@broadcom.com> wrote: > > On Mon, Apr 26, 2021 at 9:29 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > On Sun, 25 Apr 2021 13:45:27 -0400 Michael Chan wrote: > > > This should have been "features & ~(CSUM | GSO)" if you actually > > > accepted my feedback. > > Sorry, hit send too early. If it is not UDP, it could be GRE or IP > over IP for example, right? Why do we need to turn off offload? All supported protocols can be included in the allow list. That's one of the costs of NETIF_F_IP_CSUM, the driver needs to make sure the device can understand the packet. > > I mentioned extension headers as an example, > > Extension headers (Geneve for example) are supported. I thought the Geneve things were called options. I meant IPv6 extension headers, which the device may also support, but then the right thing to do is something like a call to ipv6_skip_exthdr() to retrieve the L4 proto.
On Mon, Apr 26, 2021 at 10:00 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 26 Apr 2021 09:45:17 -0700 Michael Chan wrote: > > On Mon, Apr 26, 2021 at 9:35 AM Michael Chan <michael.chan@broadcom.com> wrote: > > > On Mon, Apr 26, 2021 at 9:29 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Sun, 25 Apr 2021 13:45:27 -0400 Michael Chan wrote: > > > > This should have been "features & ~(CSUM | GSO)" if you actually > > > > accepted my feedback. > > > > Sorry, hit send too early. If it is not UDP, it could be GRE or IP > > over IP for example, right? Why do we need to turn off offload? > > All supported protocols can be included in the allow list. > That's one of the costs of NETIF_F_IP_CSUM, the driver needs > to make sure the device can understand the packet. Only UDP encapsulations have the 2 port limitations. The rest are supported. > > > > I mentioned extension headers as an example, > > > > Extension headers (Geneve for example) are supported. > > I thought the Geneve things were called options. I meant IPv6 extension > headers, which the device may also support, but then the right thing to > do is something like a call to ipv6_skip_exthdr() to retrieve the L4 > proto. You're right and I misunderstood you. I will double check for ipv6 extension headers support and will send a follow up patch. Thanks.
On Mon, 26 Apr 2021 10:09:29 -0700 Michael Chan wrote: > On Mon, Apr 26, 2021 at 10:00 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 26 Apr 2021 09:45:17 -0700 Michael Chan wrote: > > > Sorry, hit send too early. If it is not UDP, it could be GRE or IP > > > over IP for example, right? Why do we need to turn off offload? > > > > All supported protocols can be included in the allow list. > > That's one of the costs of NETIF_F_IP_CSUM, the driver needs > > to make sure the device can understand the packet. > > Only UDP encapsulations have the 2 port limitations. The rest are supported. AFAIU you claim that other than certain UDP formats your device can parse _every_ possible packet encapsulation. To which I have no reply.
On Mon, Apr 26, 2021 at 10:36 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 26 Apr 2021 10:09:29 -0700 Michael Chan wrote: > > On Mon, Apr 26, 2021 at 10:00 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > On Mon, 26 Apr 2021 09:45:17 -0700 Michael Chan wrote: > > > > Sorry, hit send too early. If it is not UDP, it could be GRE or IP > > > > over IP for example, right? Why do we need to turn off offload? > > > > > > All supported protocols can be included in the allow list. > > > That's one of the costs of NETIF_F_IP_CSUM, the driver needs > > > to make sure the device can understand the packet. > > > > Only UDP encapsulations have the 2 port limitations. The rest are supported. > > AFAIU you claim that other than certain UDP formats your device can > parse _every_ possible packet encapsulation. To which I have no reply. Fair enough. I'll get the list of supported encapsulations and check for all supported types.
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index 53db073b457c..5d0ab5629aa4 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -10781,6 +10781,40 @@ 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) +{ + struct bnxt *bp; + __be16 udp_port; + 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; + } + + if (l4_proto != IPPROTO_UDP) + return features; + + bp = netdev_priv(dev); + /* For UDP, we can only handle 1 Vxlan port and 1 Geneve port. */ + udp_port = udp_hdr(skb)->dest; + if (udp_port == bp->vxlan_port || udp_port == bp->nge_port) + return features; + return features & ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK); +} + int bnxt_dbg_hwrm_rd_reg(struct bnxt *bp, u32 reg_off, u16 num_words, u32 *reg_buf) { @@ -12288,10 +12322,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 +12428,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;