Re: [PATCH v4 net 03/10] octeontx2-af: npc: cn20k: Propagate errors in defrag MCAM alloc rollback

Ratheesh Kannoth
2026-04-27T09:56:49Z

On 2026-04-27 at 12:02:06, Ratheesh Kannoth (rkannoth@marvell.com) wrote:
> npc_defrag_alloc_free_slots() allocates MCAM indexes in up to two passes
> on bank0 then bank1. On failure it rolls back by freeing entries already
> placed in save[].
>
> __npc_subbank_alloc() can return a negative errno while only part of the
> indexes are valid. The rollback loop used rc for
> npc_mcam_idx_2_subbank_idx() as well, so a successful lookup stored zero
> in rc and a later __npc_subbank_free() failure could still end with
> return 0 when the allocation path had also left rc at zero
> (for example shortfall after zero return values from the alloc helpers).
>
> Jump to the rollback path immediately when either __npc_subbank_alloc()
> call fails, preserving its errno. If both calls succeed but the total
> allocated count is still less than cnt, set rc to -ENOSPC before rollback.
> Use a separate err variable for npc_mcam_idx_2_subbank_idx() so a
> successful lookup no longer clears a non-zero rc from the allocation
> phase.
>

>> @@ -3533,13 +3535,14 @@ static int npc_defrag_alloc_free_slots(struct rvu *rvu,
>> __func__, cnt, alloc_cnt1, alloc_cnt2);
>> goto fail_free_alloc;
>> }
>> +
>> return 0;
>>
>> fail_free_alloc:
>> for (i = 0; i < alloc_cnt1 + alloc_cnt2; i++) {
>> - rc = npc_mcam_idx_2_subbank_idx(rvu, save[i],
>> - &sb, &sb_off);
>> - if (rc) {
>> + err = npc_mcam_idx_2_subbank_idx(rvu, save[i],
>> + &sb, &sb_off);
>> + if (err) {
>> dev_err(rvu->dev,
>> "%s: Error to find subbank for mcam idx=%u\n",
>> __func__, save[i]);
>Does the diff implement what the commit message describes?
>The commit message states:
> Jump to the rollback path immediately when either __npc_subbank_alloc()
> call fails, preserving its errno. If both calls succeed but the total
> allocated count is still less than cnt, set rc to -ENOSPC before
> rollback.
>Looking at npc_defrag_alloc_free_slots() after the patch applies, there
>is no early jump after the first __npc_subbank_alloc() call, and no
>assignment of rc = -ENOSPC before the goto:

ACK.
pw-bot: changes-requested

>> @@ -3533,13 +3535,14 @@ static int npc_defrag_alloc_free_slots(struct rvu *rvu,
>> __func__, cnt, alloc_cnt1, alloc_cnt2);
>> goto fail_free_alloc;
>> }
>> +
>> return 0;
>>
>> fail_free_alloc:
>> for (i = 0; i < alloc_cnt1 + alloc_cnt2; i++) {
>> - rc = npc_mcam_idx_2_subbank_idx(rvu, save[i],
>> - &sb, &sb_off);
>> - if (rc) {
>> + err = npc_mcam_idx_2_subbank_idx(rvu, save[i],
>> + &sb, &sb_off);
>> + if (err) {
>> dev_err(rvu->dev,
>> "%s: Error to find subbank for mcam idx=%u\n",
>> __func__, save[i]);
>Does the diff implement what the commit message describes?
>The commit message states:
> Jump to the rollback path immediately when either __npc_subbank_alloc()
> call fails, preserving its errno. If both calls succeed but the total
> allocated count is still less than cnt, set rc to -ENOSPC before
> rollback.
>Looking at npc_defrag_alloc_free_slots() after the patch applies, there
>is no early jump after the first __npc_subbank_alloc() call, and no
>assignment of rc = -ENOSPC before the goto:

ACK.