Message ID | 57c4e599df3fff7bf678c1813445bd6016c6db79.1679603051.git.ecree.xilinx@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | sfc: support TC decap rules | expand |
On Thu, 23 Mar 2023 20:45:13 +0000 edward.cree@amd.com wrote: > +__always_unused > +static int efx_tc_flower_record_encap_match(struct efx_nic *efx, > + struct efx_tc_match *match, > + enum efx_encap_type type, > + struct netlink_ext_ack *extack) > +{ > + struct efx_tc_encap_match *encap, *old; > + bool ipv6; > + int rc; clang sayeth drivers/net/ethernet/sfc/tc.c:414:43: warning: variable 'ipv6' is uninitialized when used here [-Wuninitialized] rc = efx_mae_check_encap_match_caps(efx, ipv6, extack); ^~~~ drivers/net/ethernet/sfc/tc.c:356:11: note: initialize the variable 'ipv6' to silence this warning bool ipv6; ^ = 0
On Thu, Mar 23, 2023 at 10:05:30PM -0700, Jakub Kicinski wrote: > On Thu, 23 Mar 2023 20:45:13 +0000 edward.cree@amd.com wrote: > > +__always_unused > > +static int efx_tc_flower_record_encap_match(struct efx_nic *efx, > > + struct efx_tc_match *match, > > + enum efx_encap_type type, > > + struct netlink_ext_ack *extack) > > +{ > > + struct efx_tc_encap_match *encap, *old; > > + bool ipv6; > > + int rc; > > clang sayeth > > drivers/net/ethernet/sfc/tc.c:414:43: warning: variable 'ipv6' is uninitialized when used here [-Wuninitialized] > rc = efx_mae_check_encap_match_caps(efx, ipv6, extack); > ^~~~ > drivers/net/ethernet/sfc/tc.c:356:11: note: initialize the variable 'ipv6' to silence this warning > bool ipv6; > ^ > = 0 If CONFIG_IPV6 is unset it is never used, to is needs a __maybe_unused as well. Martin
Hi, I love your patch! Perhaps something to improve: [auto build test WARNING on net-next/main] url: https://github.com/intel-lab-lkp/linux/commits/edward-cree-amd-com/sfc-document-TC-to-EF100-MAE-action-translation-concepts/20230324-044845 patch link: https://lore.kernel.org/r/57c4e599df3fff7bf678c1813445bd6016c6db79.1679603051.git.ecree.xilinx%40gmail.com patch subject: [PATCH net-next v2 5/6] sfc: add code to register and unregister encap matches config: arm-randconfig-r025-20230322 (https://download.01.org/0day-ci/archive/20230325/202303250154.HsaEs3hh-lkp@intel.com/config) compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install arm cross compiling tool for clang build # apt-get install binutils-arm-linux-gnueabi # https://github.com/intel-lab-lkp/linux/commit/48db650a79ec4f4091360a2c1363d1cac6235707 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review edward-cree-amd-com/sfc-document-TC-to-EF100-MAE-action-translation-concepts/20230324-044845 git checkout 48db650a79ec4f4091360a2c1363d1cac6235707 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/net/ethernet/sfc/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> | Link: https://lore.kernel.org/oe-kbuild-all/202303250154.HsaEs3hh-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/net/ethernet/sfc/tc.c:414:43: warning: variable 'ipv6' is uninitialized when used here [-Wuninitialized] rc = efx_mae_check_encap_match_caps(efx, ipv6, extack); ^~~~ drivers/net/ethernet/sfc/tc.c:356:11: note: initialize the variable 'ipv6' to silence this warning bool ipv6; ^ = 0 1 warning generated. vim +/ipv6 +414 drivers/net/ethernet/sfc/tc.c 348 349 __always_unused 350 static int efx_tc_flower_record_encap_match(struct efx_nic *efx, 351 struct efx_tc_match *match, 352 enum efx_encap_type type, 353 struct netlink_ext_ack *extack) 354 { 355 struct efx_tc_encap_match *encap, *old; 356 bool ipv6; 357 int rc; 358 359 /* We require that the socket-defining fields (IP addrs and UDP dest 360 * port) are present and exact-match. Other fields are currently not 361 * allowed. This meets what OVS will ask for, and means that we don't 362 * need to handle difficult checks for overlapping matches as could 363 * come up if we allowed masks or varying sets of match fields. 364 */ 365 if (match->mask.enc_dst_ip | match->mask.enc_src_ip) { 366 if (!IS_ALL_ONES(match->mask.enc_dst_ip)) { 367 NL_SET_ERR_MSG_MOD(extack, 368 "Egress encap match is not exact on dst IP address"); 369 return -EOPNOTSUPP; 370 } 371 if (!IS_ALL_ONES(match->mask.enc_src_ip)) { 372 NL_SET_ERR_MSG_MOD(extack, 373 "Egress encap match is not exact on src IP address"); 374 return -EOPNOTSUPP; 375 } 376 #ifdef CONFIG_IPV6 377 if (!ipv6_addr_any(&match->mask.enc_dst_ip6) || 378 !ipv6_addr_any(&match->mask.enc_src_ip6)) { 379 NL_SET_ERR_MSG_MOD(extack, 380 "Egress encap match on both IPv4 and IPv6, don't understand"); 381 return -EOPNOTSUPP; 382 } 383 } else { 384 ipv6 = true; 385 if (!efx_ipv6_addr_all_ones(&match->mask.enc_dst_ip6)) { 386 NL_SET_ERR_MSG_MOD(extack, 387 "Egress encap match is not exact on dst IP address"); 388 return -EOPNOTSUPP; 389 } 390 if (!efx_ipv6_addr_all_ones(&match->mask.enc_src_ip6)) { 391 NL_SET_ERR_MSG_MOD(extack, 392 "Egress encap match is not exact on src IP address"); 393 return -EOPNOTSUPP; 394 } 395 #endif 396 } 397 if (!IS_ALL_ONES(match->mask.enc_dport)) { 398 NL_SET_ERR_MSG_MOD(extack, "Egress encap match is not exact on dst UDP port"); 399 return -EOPNOTSUPP; 400 } 401 if (match->mask.enc_sport) { 402 NL_SET_ERR_MSG_MOD(extack, "Egress encap match on src UDP port not supported"); 403 return -EOPNOTSUPP; 404 } 405 if (match->mask.enc_ip_tos) { 406 NL_SET_ERR_MSG_MOD(extack, "Egress encap match on IP ToS not supported"); 407 return -EOPNOTSUPP; 408 } 409 if (match->mask.enc_ip_ttl) { 410 NL_SET_ERR_MSG_MOD(extack, "Egress encap match on IP TTL not supported"); 411 return -EOPNOTSUPP; 412 } 413 > 414 rc = efx_mae_check_encap_match_caps(efx, ipv6, extack); 415 if (rc) { 416 NL_SET_ERR_MSG_FMT_MOD(extack, "MAE hw reports no support for IPv%d encap matches", 417 ipv6 ? 6 : 4); 418 return -EOPNOTSUPP; 419 } 420 421 encap = kzalloc(sizeof(*encap), GFP_USER); 422 if (!encap) 423 return -ENOMEM; 424 encap->src_ip = match->value.enc_src_ip; 425 encap->dst_ip = match->value.enc_dst_ip; 426 #ifdef CONFIG_IPV6 427 encap->src_ip6 = match->value.enc_src_ip6; 428 encap->dst_ip6 = match->value.enc_dst_ip6; 429 #endif 430 encap->udp_dport = match->value.enc_dport; 431 encap->tun_type = type; 432 old = rhashtable_lookup_get_insert_fast(&efx->tc->encap_match_ht, 433 &encap->linkage, 434 efx_tc_encap_match_ht_params); 435 if (old) { 436 /* don't need our new entry */ 437 kfree(encap); 438 if (old->tun_type != type) { 439 NL_SET_ERR_MSG_FMT_MOD(extack, 440 "Egress encap match with conflicting tun_type %u != %u", 441 old->tun_type, type); 442 return -EEXIST; 443 } 444 if (!refcount_inc_not_zero(&old->ref)) 445 return -EAGAIN; 446 /* existing entry found */ 447 encap = old; 448 } else { 449 rc = efx_mae_register_encap_match(efx, encap); 450 if (rc) { 451 NL_SET_ERR_MSG_MOD(extack, "Failed to record egress encap match in HW"); 452 goto fail; 453 } 454 refcount_set(&encap->ref, 1); 455 } 456 match->encap = encap; 457 return 0; 458 fail: 459 rhashtable_remove_fast(&efx->tc->encap_match_ht, &encap->linkage, 460 efx_tc_encap_match_ht_params); 461 kfree(encap); 462 return rc; 463 } 464
On 24/03/2023 09:10, Martin Habets wrote: > On Thu, Mar 23, 2023 at 10:05:30PM -0700, Jakub Kicinski wrote: >> clang sayeth >> >> drivers/net/ethernet/sfc/tc.c:414:43: warning: variable 'ipv6' is uninitialized when used here [-Wuninitialized] >> rc = efx_mae_check_encap_match_caps(efx, ipv6, extack); > > If CONFIG_IPV6 is unset it is never used, to is needs a __maybe_unused > as well. efx_mae_check_encap_match_caps() uses it regardless of the CONFIG, as does the extack setting just below it. So it just needs initialising to `false`, will fix in v3. (No idea why gcc doesn't warn about it, not even on W=1.)
On Thu, Mar 23, 2023 at 08:45:13PM +0000, edward.cree@amd.com wrote: > From: Edward Cree <ecree.xilinx@gmail.com> > > Add a hashtable to detect duplicate and conflicting matches. If match > is not a duplicate, call MAE functions to add/remove it from OR table. > Calling code not added yet, so mark the new functions as unused. > > Signed-off-by: Edward Cree <ecree.xilinx@gmail.com> > --- > Changed in v2: replace `unsigned char ipv` with `bool ipv6`, simplifying > code (suggested by Michal) Many bits have been spilt on a minor problem around initialising 'ipv6 [1][2]. But that aside, this looks good to me. Reviewed-by: Simon Horman <simon.horman@corigine.com> [1] https://lore.kernel.org/all/20230323220530.0d979b62@kernel.org/ [2] https://lore.kernel.org/all/202303250154.HsaEs3hh-lkp@intel.com/
diff --git a/drivers/net/ethernet/sfc/tc.c b/drivers/net/ethernet/sfc/tc.c index 21eb79b20978..f18ac2a7697a 100644 --- a/drivers/net/ethernet/sfc/tc.c +++ b/drivers/net/ethernet/sfc/tc.c @@ -57,6 +57,12 @@ static s64 efx_tc_flower_external_mport(struct efx_nic *efx, struct efx_rep *efv return mport; } +static const struct rhashtable_params efx_tc_encap_match_ht_params = { + .key_len = offsetof(struct efx_tc_encap_match, linkage), + .key_offset = 0, + .head_offset = offsetof(struct efx_tc_encap_match, linkage), +}; + static const struct rhashtable_params efx_tc_match_action_ht_params = { .key_len = sizeof(unsigned long), .key_offset = offsetof(struct efx_tc_flow_rule, cookie), @@ -340,6 +346,144 @@ static int efx_tc_flower_parse_match(struct efx_nic *efx, return 0; } +__always_unused +static int efx_tc_flower_record_encap_match(struct efx_nic *efx, + struct efx_tc_match *match, + enum efx_encap_type type, + struct netlink_ext_ack *extack) +{ + struct efx_tc_encap_match *encap, *old; + bool ipv6; + int rc; + + /* We require that the socket-defining fields (IP addrs and UDP dest + * port) are present and exact-match. Other fields are currently not + * allowed. This meets what OVS will ask for, and means that we don't + * need to handle difficult checks for overlapping matches as could + * come up if we allowed masks or varying sets of match fields. + */ + if (match->mask.enc_dst_ip | match->mask.enc_src_ip) { + if (!IS_ALL_ONES(match->mask.enc_dst_ip)) { + NL_SET_ERR_MSG_MOD(extack, + "Egress encap match is not exact on dst IP address"); + return -EOPNOTSUPP; + } + if (!IS_ALL_ONES(match->mask.enc_src_ip)) { + NL_SET_ERR_MSG_MOD(extack, + "Egress encap match is not exact on src IP address"); + return -EOPNOTSUPP; + } +#ifdef CONFIG_IPV6 + if (!ipv6_addr_any(&match->mask.enc_dst_ip6) || + !ipv6_addr_any(&match->mask.enc_src_ip6)) { + NL_SET_ERR_MSG_MOD(extack, + "Egress encap match on both IPv4 and IPv6, don't understand"); + return -EOPNOTSUPP; + } + } else { + ipv6 = true; + if (!efx_ipv6_addr_all_ones(&match->mask.enc_dst_ip6)) { + NL_SET_ERR_MSG_MOD(extack, + "Egress encap match is not exact on dst IP address"); + return -EOPNOTSUPP; + } + if (!efx_ipv6_addr_all_ones(&match->mask.enc_src_ip6)) { + NL_SET_ERR_MSG_MOD(extack, + "Egress encap match is not exact on src IP address"); + return -EOPNOTSUPP; + } +#endif + } + if (!IS_ALL_ONES(match->mask.enc_dport)) { + NL_SET_ERR_MSG_MOD(extack, "Egress encap match is not exact on dst UDP port"); + return -EOPNOTSUPP; + } + if (match->mask.enc_sport) { + NL_SET_ERR_MSG_MOD(extack, "Egress encap match on src UDP port not supported"); + return -EOPNOTSUPP; + } + if (match->mask.enc_ip_tos) { + NL_SET_ERR_MSG_MOD(extack, "Egress encap match on IP ToS not supported"); + return -EOPNOTSUPP; + } + if (match->mask.enc_ip_ttl) { + NL_SET_ERR_MSG_MOD(extack, "Egress encap match on IP TTL not supported"); + return -EOPNOTSUPP; + } + + rc = efx_mae_check_encap_match_caps(efx, ipv6, extack); + if (rc) { + NL_SET_ERR_MSG_FMT_MOD(extack, "MAE hw reports no support for IPv%d encap matches", + ipv6 ? 6 : 4); + return -EOPNOTSUPP; + } + + encap = kzalloc(sizeof(*encap), GFP_USER); + if (!encap) + return -ENOMEM; + encap->src_ip = match->value.enc_src_ip; + encap->dst_ip = match->value.enc_dst_ip; +#ifdef CONFIG_IPV6 + encap->src_ip6 = match->value.enc_src_ip6; + encap->dst_ip6 = match->value.enc_dst_ip6; +#endif + encap->udp_dport = match->value.enc_dport; + encap->tun_type = type; + old = rhashtable_lookup_get_insert_fast(&efx->tc->encap_match_ht, + &encap->linkage, + efx_tc_encap_match_ht_params); + if (old) { + /* don't need our new entry */ + kfree(encap); + if (old->tun_type != type) { + NL_SET_ERR_MSG_FMT_MOD(extack, + "Egress encap match with conflicting tun_type %u != %u", + old->tun_type, type); + return -EEXIST; + } + if (!refcount_inc_not_zero(&old->ref)) + return -EAGAIN; + /* existing entry found */ + encap = old; + } else { + rc = efx_mae_register_encap_match(efx, encap); + if (rc) { + NL_SET_ERR_MSG_MOD(extack, "Failed to record egress encap match in HW"); + goto fail; + } + refcount_set(&encap->ref, 1); + } + match->encap = encap; + return 0; +fail: + rhashtable_remove_fast(&efx->tc->encap_match_ht, &encap->linkage, + efx_tc_encap_match_ht_params); + kfree(encap); + return rc; +} + +__always_unused +static void efx_tc_flower_release_encap_match(struct efx_nic *efx, + struct efx_tc_encap_match *encap) +{ + int rc; + + if (!refcount_dec_and_test(&encap->ref)) + return; /* still in use */ + + rc = efx_mae_unregister_encap_match(efx, encap); + if (rc) + /* Display message but carry on and remove entry from our + * SW tables, because there's not much we can do about it. + */ + netif_err(efx, drv, efx->net_dev, + "Failed to release encap match %#x, rc %d\n", + encap->fw_id, rc); + rhashtable_remove_fast(&efx->tc->encap_match_ht, &encap->linkage, + efx_tc_encap_match_ht_params); + kfree(encap); +} + /* For details of action order constraints refer to SF-123102-TC-1§12.6.1 */ enum efx_tc_action_order { EFX_TC_AO_VLAN_POP, @@ -974,6 +1118,18 @@ void efx_fini_tc(struct efx_nic *efx) efx->tc->up = false; } +/* At teardown time, all TC filter rules (and thus all resources they created) + * should already have been removed. If we find any in our hashtables, make a + * cursory attempt to clean up the software side. + */ +static void efx_tc_encap_match_free(void *ptr, void *__unused) +{ + struct efx_tc_encap_match *encap = ptr; + + WARN_ON(refcount_read(&encap->ref)); + kfree(encap); +} + int efx_init_struct_tc(struct efx_nic *efx) { int rc; @@ -996,6 +1152,9 @@ int efx_init_struct_tc(struct efx_nic *efx) rc = efx_tc_init_counters(efx); if (rc < 0) goto fail_counters; + rc = rhashtable_init(&efx->tc->encap_match_ht, &efx_tc_encap_match_ht_params); + if (rc < 0) + goto fail_encap_match_ht; rc = rhashtable_init(&efx->tc->match_action_ht, &efx_tc_match_action_ht_params); if (rc < 0) goto fail_match_action_ht; @@ -1008,6 +1167,8 @@ int efx_init_struct_tc(struct efx_nic *efx) efx->extra_channel_type[EFX_EXTRA_CHANNEL_TC] = &efx_tc_channel_type; return 0; fail_match_action_ht: + rhashtable_destroy(&efx->tc->encap_match_ht); +fail_encap_match_ht: efx_tc_destroy_counters(efx); fail_counters: mutex_destroy(&efx->tc->mutex); @@ -1030,6 +1191,8 @@ void efx_fini_struct_tc(struct efx_nic *efx) MC_CMD_MAE_ACTION_RULE_INSERT_OUT_ACTION_RULE_ID_NULL); rhashtable_free_and_destroy(&efx->tc->match_action_ht, efx_tc_flow_free, efx); + rhashtable_free_and_destroy(&efx->tc->encap_match_ht, + efx_tc_encap_match_free, NULL); efx_tc_fini_counters(efx); mutex_unlock(&efx->tc->mutex); mutex_destroy(&efx->tc->mutex); diff --git a/drivers/net/ethernet/sfc/tc.h b/drivers/net/ethernet/sfc/tc.h index 19782c9a4354..d70c0ba86669 100644 --- a/drivers/net/ethernet/sfc/tc.h +++ b/drivers/net/ethernet/sfc/tc.h @@ -18,6 +18,13 @@ #define IS_ALL_ONES(v) (!(typeof (v))~(v)) +#ifdef CONFIG_IPV6 +static inline bool efx_ipv6_addr_all_ones(struct in6_addr *addr) +{ + return !memchr_inv(addr, 0xff, sizeof(*addr)); +} +#endif + struct efx_tc_action_set { u16 vlan_push:2; u16 vlan_pop:2; @@ -70,7 +77,9 @@ struct efx_tc_encap_match { __be32 src_ip, dst_ip; struct in6_addr src_ip6, dst_ip6; __be16 udp_dport; + struct rhash_head linkage; u16 tun_type; /* enum efx_encap_type */ + refcount_t ref; u32 fw_id; /* index of this entry in firmware encap match table */ }; @@ -107,6 +116,7 @@ enum efx_tc_rule_prios { * @mutex: Used to serialise operations on TC hashtables * @counter_ht: Hashtable of TC counters (FW IDs and counter values) * @counter_id_ht: Hashtable mapping TC counter cookies to counters + * @encap_match_ht: Hashtable of TC encap matches * @match_action_ht: Hashtable of TC match-action rules * @reps_mport_id: MAE port allocated for representor RX * @reps_filter_uc: VNIC filter for representor unicast RX (promisc) @@ -130,6 +140,7 @@ struct efx_tc_state { struct mutex mutex; struct rhashtable counter_ht; struct rhashtable counter_id_ht; + struct rhashtable encap_match_ht; struct rhashtable match_action_ht; u32 reps_mport_id, reps_mport_vport_id; s32 reps_filter_uc, reps_filter_mc;