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

[Ratheesh Kannoth]

On 2026-04-27 at 12:02:06, Ratheesh Kannoth (rkannoth@xxxxxxxxxxx) 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.