* Re: [PATCH net] octeontx2-pf: mcs: Fix mcs resources free on PF shutdown
2026-05-29 11:38 [PATCH net] octeontx2-pf: mcs: Fix mcs resources free on PF shutdown Subbaraya Sundeep
@ 2026-06-03 19:46 ` Simon Horman
0 siblings, 0 replies; 2+ messages in thread
From: Simon Horman @ 2026-06-03 19:46 UTC (permalink / raw)
To: sbhatta
Cc: 'Simon Horman', andrew+netdev, davem, edumazet, kuba,
pabeni, sgoutham, gakula, bbhushan2, netdev, linux-kernel
From: 'Simon Horman' <horms@kernel.org>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
Simon says:
There is also an AI-generated review available at https://sashiko.dev
However, I do not believe it raises anything that needs to
be addressed in the context of this patch that is not already
raised below.
---
Subject: octeontx2-pf: mcs: Fix mcs resources free on PF shutdown
This patch gates the MACSEC resource cleanup in cn10k_mcs_free() on
the txsc_list being non-empty, with the intent of skipping the
mailbox messages when no MCS resources were ever allocated to the
PF. It also sets clear_req->all in cn10k_mcs_free_rsrc().
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
> index 2cc1bdfd9b2e..f25809c9d393 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
> @@ -182,6 +182,7 @@ static void cn10k_mcs_free_rsrc(struct otx2_nic *pfvf, enum mcs_direction dir,
> clear_req->id = hw_rsrc_id;
> clear_req->type = type;
> clear_req->dir = dir;
> + clear_req->all = all;
[Low]
Could the commit message mention this change? Before the patch,
clear_req->all was never assigned and the mcs_clear_stats mailbox
always asked the AF to clear stats for the single resource named by
id. After the patch, calls from cn10k_mcs_free() pass all=true and
the AF will now be asked to clear all PF-mapped resource stats.
That looks consistent with the pre-existing req->all = 1 on the
matching mcs_free_resources message, but it is an additional fix to
the AF mailbox protocol that the changelog does not describe and
that may want its own Fixes: tag for backporting.
>
> req = otx2_mbox_alloc_msg_mcs_free_resources(mbox);
> if (!req)
> @@ -1776,9 +1777,14 @@ int cn10k_mcs_init(struct otx2_nic *pfvf)
>
> void cn10k_mcs_free(struct otx2_nic *pfvf)
> {
> + struct cn10k_mcs_cfg *cfg = pfvf->macsec_cfg;
> +
> if (!test_bit(CN10K_HW_MACSEC, &pfvf->hw.cap_flag))
> return;
>
> + if (list_empty(&cfg->txsc_list))
> + return;
> +
[High]
Does this early return leak pfvf->macsec_cfg?
cn10k_mcs_init() unconditionally allocates cfg whenever
CN10K_HW_MACSEC is set, and initializes both lists empty:
cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
...
INIT_LIST_HEAD(&cfg->txsc_list);
INIT_LIST_HEAD(&cfg->rxsc_list);
pfvf->macsec_cfg = cfg;
txsc_list only becomes non-empty once the user creates a SecY via
cn10k_mdo_add_secy(). On any MACSEC-capable PF where the user never
configures MACSEC offload, txsc_list is still empty at otx2_remove()
time, and the new early return skips the trailing kfree:
> cn10k_mcs_free_rsrc(pfvf, MCS_TX, MCS_RSRC_TYPE_SECY, 0, true);
> cn10k_mcs_free_rsrc(pfvf, MCS_RX, MCS_RSRC_TYPE_SECY, 0, true);
> kfree(pfvf->macsec_cfg);
> pfvf->macsec_cfg = NULL;
> }
The same path runs from the err_mcs_free probe error label in
otx2_probe() when cn10k_ipsec_init() fails, so the leak also fires
there.
Would gating only the two cn10k_mcs_free_rsrc() mailbox calls on
the empty-list condition, and keeping the kfree unconditional, work
better? Something like:
if (!list_empty(&cfg->txsc_list)) {
cn10k_mcs_free_rsrc(pfvf, MCS_TX, MCS_RSRC_TYPE_SECY, 0, true);
cn10k_mcs_free_rsrc(pfvf, MCS_RX, MCS_RSRC_TYPE_SECY, 0, true);
}
kfree(pfvf->macsec_cfg);
pfvf->macsec_cfg = NULL;
^ permalink raw reply [flat|nested] 2+ messages in thread