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