From: Pavan Chebbi We need to free the corresponding RSS context VNIC in FW everytime an RSS context is deleted in driver. Commit 667ac333dbb7 added a check to delete the VNIC in FW only when netif_running() is true to help delete RSS contexts with interface down. Having that condition will make the driver leak VNICs in FW whenever close() happens with active RSS contexts. On the subsequent open(), as part of RSS context restoration, we will end up trying to create extra VNICs for which we did not make any reservation. FW can fail this request, thereby making us lose active RSS contexts. Suppose an RSS context is deleted already and we try to process a delete request again, then the HWRM functions will check for validity of the request and they simply return if the resource is already freed. So, even for delete-when-down cases, netif_running() check is not necessary. Remove the netif_running() condition check when deleting an RSS context. Reported-by: Jakub Kicinski Fixes: 667ac333dbb7 ("eth: bnxt: allow deleting RSS contexts when the device is down") Reviewed-by: Andy Gospodarek Signed-off-by: Pavan Chebbi Signed-off-by: Michael Chan --- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index 8419d1eb4035..64832289e18d 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -10827,12 +10827,10 @@ void bnxt_del_one_rss_ctx(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx, struct bnxt_ntuple_filter *ntp_fltr; int i; - if (netif_running(bp->dev)) { - bnxt_hwrm_vnic_free_one(bp, &rss_ctx->vnic); - for (i = 0; i < BNXT_MAX_CTX_PER_VNIC; i++) { - if (vnic->fw_rss_cos_lb_ctx[i] != INVALID_HW_RING_ID) - bnxt_hwrm_vnic_ctx_free_one(bp, vnic, i); - } + bnxt_hwrm_vnic_free_one(bp, &rss_ctx->vnic); + for (i = 0; i < BNXT_MAX_CTX_PER_VNIC; i++) { + if (vnic->fw_rss_cos_lb_ctx[i] != INVALID_HW_RING_ID) + bnxt_hwrm_vnic_ctx_free_one(bp, vnic, i); } if (!all) return; -- 2.51.0 From: Pavan Chebbi Currently the ntuple filters installed for an RSS context use the ntuple filter's base member 'fw_vnic_id' to save the RSS context id. This is making the fw_vnic_id overloaded when it is being used with RSS context. This is very confusing. Make explicit provision to save rss_ctx_id in the ntuple filter so that we can avoid overloading fw_vnic_id. Also remove unnecessary initialization of the id to 0. This cleanup is necessary for the next patch that will remove the fw_vnic_id field. Reviewed-by: Andy Gospodarek Signed-off-by: Pavan Chebbi Signed-off-by: Michael Chan --- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 4 ++-- drivers/net/ethernet/broadcom/bnxt/bnxt.h | 1 + drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 5 ++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index 64832289e18d..6fae0490bc1c 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -6259,7 +6259,7 @@ bnxt_cfg_rfs_ring_tbl_idx(struct bnxt *bp, struct bnxt_vnic_info *vnic; ctx = xa_load(&bp->dev->ethtool->rss_ctx, - fltr->base.fw_vnic_id); + fltr->base.rss_ctx_id); if (ctx) { rss_ctx = ethtool_rxfh_context_priv(ctx); vnic = &rss_ctx->vnic; @@ -10837,7 +10837,7 @@ void bnxt_del_one_rss_ctx(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx, list_for_each_entry_safe(usr_fltr, tmp, &bp->usr_fltr_list, list) { if ((usr_fltr->flags & BNXT_ACT_RSS_CTX) && - usr_fltr->fw_vnic_id == rss_ctx->index) { + usr_fltr->rss_ctx_id == rss_ctx->index) { ntp_fltr = container_of(usr_fltr, struct bnxt_ntuple_filter, base); diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h index f88e7769a838..e2d52841fa86 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h @@ -1425,6 +1425,7 @@ struct bnxt_filter_base { u16 sw_id; u16 rxq; u16 fw_vnic_id; + u16 rss_ctx_id; u16 vf_idx; unsigned long state; #define BNXT_FLTR_VALID 0 diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c index 068e191ede19..8fd0e06ef2a7 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -1219,7 +1219,7 @@ static int bnxt_grxclsrule(struct bnxt *bp, struct ethtool_rxnfc *cmd) fs->ring_cookie = RX_CLS_FLOW_DISC; } else if (fltr->base.flags & BNXT_ACT_RSS_CTX) { fs->flow_type |= FLOW_RSS; - cmd->rss_context = fltr->base.fw_vnic_id; + cmd->rss_context = fltr->base.rss_ctx_id; } else { fs->ring_cookie = fltr->base.rxq; } @@ -1468,11 +1468,10 @@ static int bnxt_add_ntuple_cls_rule(struct bnxt *bp, if (fs->flow_type & FLOW_RSS) { struct bnxt_rss_ctx *rss_ctx; - new_fltr->base.fw_vnic_id = 0; new_fltr->base.flags |= BNXT_ACT_RSS_CTX; rss_ctx = bnxt_get_rss_ctx_from_index(bp, cmd->rss_context); if (rss_ctx) { - new_fltr->base.fw_vnic_id = rss_ctx->index; + new_fltr->base.rss_ctx_id = rss_ctx->index; } else { rc = -EINVAL; goto ntuple_err; -- 2.51.0 From: Pavan Chebbi Ntuple filters can be deleted when the interface is down. The current code blindly sends the filter delete command to FW. When the interface is down, all the VNICs are deleted in the FW. When the VNIC is freed in the FW, all the associated filters are also freed. We need not send the free command explicitly. Sending such command will generate FW error in the dmesg. In order to fix this, save a pointer to the corresponding VNIC of an Ntuple filter in the ntuple filter's base structure. Use this pointer to check if the VNIC is already freed whenever we are freeing the Ntuple filters. Now that we have access to the vnic itself, remove the redundant reference to fw_vnic_id from all places. Fixes: 8336a974f37d ("bnxt_en: Save user configured filters in a lookup list") Reviewed-by: Andy Gospodarek Signed-off-by: Pavan Chebbi Signed-off-by: Michael Chan --- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 14 ++++++++++++-- drivers/net/ethernet/broadcom/bnxt/bnxt.h | 2 +- drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 7 +++---- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index 6fae0490bc1c..6ec173a25332 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -6156,6 +6156,7 @@ int bnxt_hwrm_l2_filter_alloc(struct bnxt *bp, struct bnxt_l2_filter *fltr) { struct hwrm_cfa_l2_filter_alloc_output *resp; struct hwrm_cfa_l2_filter_alloc_input *req; + u16 dst_id = INVALID_HW_RING_ID; u16 target_id = 0xffff; int rc; @@ -6177,7 +6178,10 @@ int bnxt_hwrm_l2_filter_alloc(struct bnxt *bp, struct bnxt_l2_filter *fltr) if (!BNXT_CHIP_TYPE_NITRO_A0(bp)) req->flags |= cpu_to_le32(CFA_L2_FILTER_ALLOC_REQ_FLAGS_OUTERMOST); - req->dst_id = cpu_to_le16(fltr->base.fw_vnic_id); + + if (fltr->base.vnic) + dst_id = fltr->base.vnic->fw_vnic_id; + req->dst_id = cpu_to_le16(dst_id); req->enables = cpu_to_le32(CFA_L2_FILTER_ALLOC_REQ_ENABLES_L2_ADDR | CFA_L2_FILTER_ALLOC_REQ_ENABLES_DST_ID | @@ -6212,6 +6216,9 @@ int bnxt_hwrm_cfa_ntuple_filter_free(struct bnxt *bp, int rc; set_bit(BNXT_FLTR_FW_DELETED, &fltr->base.state); + if (fltr->base.vnic->fw_vnic_id == INVALID_HW_RING_ID) + return 0; + rc = hwrm_req_init(bp, req, HWRM_CFA_NTUPLE_FILTER_FREE); if (rc) return rc; @@ -6263,6 +6270,7 @@ bnxt_cfg_rfs_ring_tbl_idx(struct bnxt *bp, if (ctx) { rss_ctx = ethtool_rxfh_context_priv(ctx); vnic = &rss_ctx->vnic; + fltr->base.vnic = vnic; req->dst_id = cpu_to_le16(vnic->fw_vnic_id); } @@ -6273,6 +6281,7 @@ bnxt_cfg_rfs_ring_tbl_idx(struct bnxt *bp, u32 enables; vnic = &bp->vnic_info[BNXT_VNIC_NTUPLE]; + fltr->base.vnic = vnic; req->dst_id = cpu_to_le16(vnic->fw_vnic_id); enables = CFA_NTUPLE_FILTER_ALLOC_REQ_ENABLES_RFS_RING_TBL_IDX; req->enables |= cpu_to_le32(enables); @@ -6311,6 +6320,7 @@ int bnxt_hwrm_cfa_ntuple_filter_alloc(struct bnxt *bp, bnxt_cfg_rfs_ring_tbl_idx(bp, req, fltr); } else { vnic = &bp->vnic_info[fltr->base.rxq + 1]; + fltr->base.vnic = vnic; req->dst_id = cpu_to_le16(vnic->fw_vnic_id); } req->enables |= cpu_to_le32(BNXT_NTP_FLTR_FLAGS); @@ -6365,7 +6375,7 @@ static int bnxt_hwrm_set_vnic_filter(struct bnxt *bp, u16 vnic_id, u16 idx, if (IS_ERR(fltr)) return PTR_ERR(fltr); - fltr->base.fw_vnic_id = bp->vnic_info[vnic_id].fw_vnic_id; + fltr->base.vnic = &bp->vnic_info[vnic_id]; rc = bnxt_hwrm_l2_filter_alloc(bp, fltr); if (rc) bnxt_del_l2_filter(bp, fltr); diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h index e2d52841fa86..fd7201283364 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h @@ -1424,9 +1424,9 @@ struct bnxt_filter_base { #define BNXT_ACT_RSS_CTX 0x10 u16 sw_id; u16 rxq; - u16 fw_vnic_id; u16 rss_ctx_id; u16 vf_idx; + struct bnxt_vnic_info *vnic; unsigned long state; #define BNXT_FLTR_VALID 0 #define BNXT_FLTR_INSERTED 1 diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c index 8fd0e06ef2a7..f8aacd2822f9 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -1267,9 +1267,9 @@ static int bnxt_add_l2_cls_rule(struct bnxt *bp, u8 vf = ethtool_get_flow_spec_ring_vf(fs->ring_cookie); struct ethhdr *h_ether = &fs->h_u.ether_spec; struct ethhdr *m_ether = &fs->m_u.ether_spec; + struct bnxt_vnic_info *vnic = NULL; struct bnxt_l2_filter *fltr; struct bnxt_l2_key key; - u16 vnic_id; u8 flags; int rc; @@ -1291,17 +1291,16 @@ static int bnxt_add_l2_cls_rule(struct bnxt *bp, if (vf) { flags = BNXT_ACT_FUNC_DST; - vnic_id = 0xffff; vf--; } else { flags = BNXT_ACT_RING_DST; - vnic_id = bp->vnic_info[ring + 1].fw_vnic_id; + vnic = &bp->vnic_info[ring + 1]; } fltr = bnxt_alloc_new_l2_filter(bp, &key, flags); if (IS_ERR(fltr)) return PTR_ERR(fltr); - fltr->base.fw_vnic_id = vnic_id; + fltr->base.vnic = vnic; fltr->base.rxq = ring; fltr->base.vf_idx = vf; rc = bnxt_hwrm_l2_filter_alloc(bp, fltr); -- 2.51.0 From: Pavan Chebbi Add tests to verify 1. that RSS contexts persist across interface down/up 2. that RSS contexts persist across interface down/up along with their associated Ntuple fitlers Testing on bnxt_en: TAP version 13 1..1 # timeout set to 0 # selftests: drivers/net/hw: rss_ctx.py # TAP version 13 # 1..2 # ok 1 rss_ctx.test_rss_context_persist_ifupdown # ok 2 rss_ctx.test_rss_context_ntuple_persist_ifupdown # # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0 ok 1 selftests: drivers/net/hw: rss_ctx.py Reviewed-by: Andy Gospodarek Signed-off-by: Pavan Chebbi Signed-off-by: Michael Chan --- .../selftests/drivers/net/hw/rss_ctx.py | 117 +++++++++++++++++- 1 file changed, 116 insertions(+), 1 deletion(-) diff --git a/tools/testing/selftests/drivers/net/hw/rss_ctx.py b/tools/testing/selftests/drivers/net/hw/rss_ctx.py index ed7e405682f0..4e46c5931c7f 100755 --- a/tools/testing/selftests/drivers/net/hw/rss_ctx.py +++ b/tools/testing/selftests/drivers/net/hw/rss_ctx.py @@ -809,6 +809,119 @@ def test_rss_default_context_rule(cfg): 'noise' : (0, 1) }) +def test_rss_context_persist_ifupdown(cfg): + """ + Check that RSS contexts persist across an interface down/up cycle. + """ + + require_context_cnt(cfg, 10) + + # Create 10 RSS contexts and store their IDs and configurations + ctx_ids = [] + ctx_configs_before = {} + try: + for i in range(10): + ctx_id = ethtool_create(cfg, "-X", "context new") + ctx_ids.append(ctx_id) + defer(ethtool, f"-X {cfg.ifname} context {ctx_id} delete") + except CmdExitFailure: + raise KsftSkipEx(f"Could only create {len(ctx_ids)} contexts, test requires 10") + + # Bring interface down + ip(f"link set dev {cfg.ifname} down") + + # Bring interface back up + ip(f"link set dev {cfg.ifname} up") + + # Wait for interface to be fully up + cfg.wait_hw_stats_settle() + + # Verify all 10 contexts still exist after ifup + missing_contexts = [] + persisted_contexts = [] + + for ctx_id in ctx_ids: + try: + data = get_rss(cfg, context=ctx_id) + _rss_key_check(cfg, data=data, context=ctx_id) + persisted_contexts.append(ctx_id) + except CmdExitFailure: + missing_contexts.append(ctx_id) + ksft_pr(f"Context {ctx_id} is missing after ifup") + + +def test_rss_context_ntuple_persist_ifupdown(cfg): + """ + Test that RSS contexts and their associated ntuple filters persist across + an interface down/up cycle. + """ + + require_ntuple(cfg) + require_context_cnt(cfg, 10) + + # Create 10 RSS contexts with ntuple filters + ctx_ids = [] + ntuple_ids = [] + ports = [] + + try: + for i in range(10): + # Create RSS context + ctx_id = ethtool_create(cfg, "-X", "context new") + ctx_ids.append(ctx_id) + defer(ethtool, f"-X {cfg.ifname} context {ctx_id} delete") + + # Create ntuple filter for this context + port = rand_port() + ports.append(port) + flow = f"flow-type tcp{cfg.addr_ipver} dst-ip {cfg.addr} dst-port {port} context {ctx_id}" + ntuple_id = ethtool_create(cfg, "-N", flow) + ntuple_ids.append(ntuple_id) + defer(ethtool, f"-N {cfg.ifname} delete {ntuple_id}") + + except CmdExitFailure: + raise KsftSkipEx(f"Could only create {len(ctx_ids)} contexts with ntuple filters, test requires 10") + + # Bring interface down + ip(f"link set dev {cfg.ifname} down") + + # Bring interface back up + ip(f"link set dev {cfg.ifname} up") + + # Wait for interface to be fully up + cfg.wait_hw_stats_settle() + + # Verify all contexts and ntuple rules still exist after ifup + missing_contexts = [] + persisted_contexts = [] + missing_ntuple_rules = [] + persisted_ntuple_rules = [] + broken_associations = [] + + for i, ctx_id in enumerate(ctx_ids): + # Check if context persists + try: + data = get_rss(cfg, context=ctx_id) + _rss_key_check(cfg, data=data, context=ctx_id) + persisted_contexts.append(ctx_id) + except CmdExitFailure: + missing_contexts.append(ctx_id) + ksft_pr(f"Context {ctx_id} is missing after ifup") + continue + + # Check if ntuple rule persists + ntuple_id = ntuple_ids[i] + try: + _ntuple_rule_check(cfg, ntuple_id, ctx_id) + persisted_ntuple_rules.append(ntuple_id) + except CmdExitFailure: + missing_ntuple_rules.append(ntuple_id) + ksft_pr(f"Ntuple rule {ntuple_id} is missing after ifup") + except Exception as e: + broken_associations.append((ntuple_id, ctx_id)) + ksft_pr(f"Ntuple rule {ntuple_id} exists but is not properly associated with context {ctx_id}: {e}") + + def main() -> None: with NetDrvEpEnv(__file__, nsim_test=False) as cfg: cfg.context_cnt = None @@ -823,7 +936,9 @@ def main() -> None: test_rss_context_out_of_order, test_rss_context4_create_with_cfg, test_flow_add_context_missing, test_delete_rss_context_busy, test_rss_ntuple_addition, - test_rss_default_context_rule], + test_rss_default_context_rule, + test_rss_context_persist_ifupdown, + test_rss_context_ntuple_persist_ifupdown], args=(cfg, )) ksft_exit() -- 2.51.0