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

[Subrat Pandey]

________________________________________
From: Jakub Kicinski <kuba@xxxxxxxxxx>
Sent: Thursday, August 6, 2026 8:57 PM
To: Subrat Pandey
Cc: Jakub Kicinski; linux-kernel@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; pabeni@xxxxxxxxxx; edumazet@xxxxxxxxxx; davem@xxxxxxxxxxxxx; andrew+netdev@xxxxxxx; Subbaraya Sundeep Bhatta; Ratheesh Kannoth; Geethasowjanya Akula; Sunil Kovvuri Goutham
Subject: [EXTERNAL] Re: [PATCH net v2 2/2] octeontx2-af: mcs: Fix SC resource cleanup loop

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

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

Thanks for the feedback.
We will fix it and submit next version.