Re: [PATCH net v2 2/2] octeontx2-af: mcs: Fix SC resource cleanup loop

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
octeontx2-af: mcs: Fix SC resource cleanup loop

The SC cleanup loop in mcs_free_all_rsrc() was bounded by map->secy.max
while indexing the SC arrays, and this changes the bound to map->sc.max.
Since mcs_alloc_rsrc_bmap() sets secy.max = secy_entries - MCS_RSRC_RSVD_CNT
and sc.max = sc_entries, the two bounds differ by one.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mcs.c b/drivers/net/ethernet/marvell/octeontx2/af/mcs.c
> index a07e0b3d8d000..88658c07c391f 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/mcs.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/mcs.c
> @@ -738,7 +738,7 @@ int mcs_free_all_rsrc(struct mcs *mcs, int dir, u16 pcifunc)
>  	}
>  
>  	/* free sc entries */
> -	for (id = 0; id < map->secy.max; id++) {
> +	for (id = 0; id < map->sc.max; id++) {

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but the same
wrong bound still exists in mcs_clear_all_stats() in the same file, where the
"Clear SC stats" loop indexes map->sc2pf_map[] and clears MCS_SC_STATS:

	/* Clear SC stats */
	for (id = 0; id < map->secy.max; id++) {
		if (map->sc2pf_map[id] != pcifunc)
			continue;
		mcs_clear_stats(mcs, MCS_SC_STATS, id, dir);
	}

With mcs_alloc_rsrc_bmap() setting:

	res->secy.max = hw->secy_entries - MCS_RSRC_RSVD_CNT;
	...
	res->sc.max = hw->sc_entries;

and MCS_RSRC_RSVD_CNT == 1 with secy_entries == sc_entries (128/128 on
CN10K-B, 64/64 on CNF10K-B), secy.max == sc.max - 1, so SC id
sc_entries-1 is never visited by the stats loop.

Should this hunk also change the bound in mcs_clear_all_stats() to
map->sc.max?

Before this patch, SC id sc_entries-1 was never freed by
mcs_free_all_rsrc(), so no other function could take it over. Now that it
is freed and re-allocatable, can the next owner of that id read the
previous owner's pkt/octet counters?

The teardown path in cn10k_mcs_free_rsrc() always sends the clear-stats
request with the same "all" flag just before the free-resources request:

	clear_req = otx2_mbox_alloc_msg_mcs_clear_stats(mbox);
	...
	clear_req->all = all;
	...
	if (all)
		req->all = 1;

so with the stats loop still bounded by secy.max, MCS_GET_SC_STATS on the
re-allocated id would return the departing owner's values.

[Severity: Low]
Would a Fixes: tag be appropriate here? git blame on the pre-patch line
shows the secy.max bound came from:

	Fixes: cfc14181d497 ("octeontx2-af: cn10k: mcs: Manage MCS block hardware resources")

which is in released kernels since v6.1, so stable tooling can pick it up.

Could the changelog also state the user-visible impact? With the old bound,
the sc bitmap bit and map->sc2pf_map[] ownership for id sc_entries-1 leak
permanently (rvu_mcs_flr_handler() is the only other teardown path and
calls nothing else), making that SC id unallocatable, and on RX the SC CAM
entry stays enabled because mcs_ena_dis_sc_cam_entry(mcs, id, false) is
skipped.

One wording note on the changelog:

> The SC resource cleanup loop was incorrectly iterating over secy.max
> instead of sc.max. This patch fixes to use correct value.

Documentation/process/submitting-patches.rst asks for imperative mood and
discourages "This patch"; something like "Use sc.max as the loop bound"
would match.