|WARNING| [14/14] common/sfc_efx/base: cleanup wider type comparisons in loops

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/167524

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-11

# DPDK Patch Review

Reviewing patch series against DPDK coding standards and contribution guidelines.

---

## Patch 01/14: common/sfc_efx/base: reduce stack in RSS context table write

### Errors

**1. Resource leak on error path before KMEM_ALLOC**
The error path at `fail2` is reached after checking `nentries > encp->enc_rx_scale_indirection_max_nqueues`, but before allocating `payload`. At this point, jumping to `fail2` will execute `EFSYS_PROBE(fail2);` and then fall through to `fail1`, which is correct. However, the new code introduces `fail3` after the allocation check. If `EFSYS_KMEM_ALLOC` fails and we jump to `fail3`, the code does:
```c
fail3:
    EFSYS_PROBE(fail3);
fail2:
    EFSYS_PROBE(fail2);
fail1:
    EFSYS_PROBE1(fail1, efx_rc_t, rc);
```
This is correct. But the old `fail3` (queue bounds check) now becomes `fail4`, and if it fails, the code jumps to `fail4` without freeing `payload`. Let me trace the paths:

- Allocation fails - `fail3` - no free (payload is NULL, correct)
- Queue bounds check fails (line "if (table[i] >= encp->enc_rx_scale_indirection_max_nqueues)") - `fail4` - but `fail4` does not free `payload`
- MCDI execution fails - `fail5` - but `fail5` does not free `payload`

Looking at the patch:
```c
    for (i = 0; i < nentries; ++i) {
        if (table[i] >= encp->enc_rx_scale_indirection_max_nqueues) {
            rc = EINVAL;
            goto fail4;  // ERROR: payload not freed
        }
        ...
    }
    
    efx_mcdi_execute(enp, &req);
    if (req.emr_rc != 0) {
        rc = req.emr_rc;
        goto fail5;  // ERROR: payload not freed
    }
```

The only path that frees `payload` is the success path. The error paths at `fail4` and `fail5` do not free it.

**Expected:**
```c
fail5:
    EFSYS_PROBE(fail5);
fail4:
    EFSYS_PROBE(fail4);
    EFSYS_KMEM_FREE(enp->en_esip, size, payload);
fail3:
    EFSYS_PROBE(fail3);
```

---

## Patch 02/14: common/sfc_efx/base: reduce stack in get addr regions MCDI

### Errors

**1. Resource leak on error path after MCDI execution**
After `efx_mcdi_execute()`, if the MCDI command fails (`req.emr_rc != 0`), the code jumps to `fail2`, which does not free `payload`. Similarly, if `req.emr_out_length_used` is too small, it jumps to `fail3`, which also does not free `payload`.

The patch shows:
```c
    if (req.emr_rc != 0) {
        rc = req.emr_rc;
        goto fail2;  // ERROR: payload not freed
    }

    if (req.emr_out_length_used < MC_CMD_GET_DESC_ADDR_REGIONS_OUT_LENMIN) {
        rc = EMSGSIZE;
        goto fail3;  // ERROR: payload not freed
    }
```

Later error paths (`fail4`, `fail5`) also do not free `payload`.

**Expected:** Add `EFSYS_KMEM_FREE(enp->en_esip, size, payload);` before the first error probe that occurs after the allocation.

```c
fail5:
    EFSYS_PROBE(fail5);
fail4:
    EFSYS_PROBE(fail4);
fail3:
    EFSYS_PROBE(fail3);
fail2:
    EFSYS_PROBE(fail2);
    EFSYS_KMEM_FREE(enp->en_esip, size, payload);
fail1:
    EFSYS_PROBE1(fail1, efx_rc_t, rc);
```

---

## Patch 03/14: common/sfc_efx/base: reduce stack in set addr regions MCDI

### Errors

**1. Resource leak on MCDI failure**
After `efx_mcdi_execute()`, if `req.emr_rc != 0`, the code jumps to `fail3`. The patch shows `EFSYS_KMEM_FREE` at `fail3`, so this is correct.

No issues found.

---

## Patch 04/14: common/sfc_efx/base: reduce stack in netport stat describe

### Errors

**1. Missing payload free on multiple error paths**
The function has an early return path:
```c
    if (lut_nentries == 0 || lut == NULL || nprocessedp == NULL)
        goto out;
```
But `out:` frees `payload`, so that's fine. However, the error paths after MCDI execution do not free `payload`:

```c
    if (req.emr_rc != 0) {
        rc = req.emr_rc;
        goto fail2;  // ERROR: payload not freed
    }

    out_sz = req.emr_out_length_used;
    if (out_sz < MC_CMD_MAC_STATISTICS_DESCRIPTOR_OUT_LENMIN) {
        rc = EMSGSIZE;
        goto fail3;  // ERROR: payload not freed
    }
```

Later:
```c
    if (nprocessed == 0) {
        rc = EMSGSIZE;
        goto fail4;  // ERROR: payload not freed
    }
```

The patch adds the free at `fail2`, which is correct. Let me re-check the original code:

Looking at the original patch more carefully, the `goto out;` path is changed. The new code has:
```c
    if (lut_nentries == 0 || lut == NULL || nprocessedp == NULL)
        goto out;

out:
    EFSYS_KMEM_FREE(enp->en_esip, size, payload);
    return (0);

fail4:
    EFSYS_PROBE(fail4);
fail3:
    EFSYS_PROBE(fail3);
fail2:
    EFSYS_PROBE(fail2);
    EFSYS_KMEM_FREE(enp->en_esip, size, payload);
fail1:
    EFSYS_PROBE1(fail1, efx_rc_t, rc);
```

Wait, the condition `if (lut_nentries == 0 || lut == NULL || nprocessedp == NULL) goto out;` is checked AFTER the MCDI call. So if the MCDI call fails, we jump to `fail2`, which now frees `payload`. If the MCDI call succeeds but the early-return condition is true, we jump to `out`, which frees `payload`. This looks correct.

Let me trace again:
- KMEM_ALLOC fails - `fail1` (payload is NULL, no free needed)
- MCDI fails (`req.emr_rc != 0`) - `fail2` - frees `payload` 
- Output too small - `fail3` - does NOT free `payload` 

Actually, the patch shows:
```c
fail4:
    EFSYS_PROBE(fail4);
fail3:
    EFSYS_PROBE(fail3);

fail2:
    EFSYS_PROBE(fail2);
    EFSYS_KMEM_FREE(enp->en_esip, size, payload);
```

So `fail3` falls through to `fail2`, which frees. That's correct. But `fail4` also falls through, so it will free. This is correct.

No issues found (the free is placed correctly to catch all error paths after allocation).

---

## Patch 05/14: common/sfc_efx/base: fix filter saved spec handling

### Warnings

**1. Inconsistent NULL check pattern**
The patch adds `if (saved_spec != NULL)` checks before dereferencing `saved_spec`. However, it only adds these checks in some places, not all places where `saved_spec` is used. For example:

In `ef10_filter_add_select_action`, `saved_spec` is used:
```c
    if (saved_spec->efs_overridden_spec != NULL) {
        ...
    }
```
But the function signature now says `__in_opt`, meaning it can be NULL. This would be a NULL dereference if `saved_spec` is NULL.

Wait, looking at the patch more carefully, the changes are only in `ef10_filter_add_execute_action`, not in `ef10_filter_add_select_action`. Let me check if `ef10_filter_add_select_action` dereferences `saved_spec`:

The patch shows:
```diff
 static			void
 ef10_filter_add_select_action(
-	__in		efx_filter_spec_t *saved_spec,
+	__in_opt	efx_filter_spec_t *saved_spec,
```

But I don't see any NULL checks added in `ef10_filter_add_select_action` in the diff. Looking at the original code (not shown in the patch), I would need to verify whether `ef10_filter_add_select_action` dereferences `saved_spec`. Since the patch changes the annotation to `__in_opt`, it implies `saved_spec` can be NULL. But if the function body dereferences it without checking, that's a bug.

However, I can only review what's in the patch. The patch does not show any dereferences of `saved_spec` in `ef10_filter_add_select_action`, so I cannot confirm a NULL dereference there. The patch does add NULL checks in `ef10_filter_add_execute_action` before dereferencing `saved_spec`.

**Potential issue (cannot confirm without full context):** If `ef10_filter_add_select_action` dereferences `saved_spec` without checking, changing the annotation to `__in_opt` creates a mismatch.

---

## Patch 06/14: common/sfc_efx/base: fix annotations in client MAC addr get

No correctness issues found. This is an annotation-only change.

---

## Patch 07/14: common/sfc_efx/base: fix annotations in HW-SW mask converter

### Errors

**1. Potential uninitialized write to sw_cap_maskp**
The patch changes the annotation of `sw_cap_maskp` from `__out` to `__inout`, and adds:
```c
    *sw_cap_maskp = 0;
```
in `efx_np_cap_hw_data_to_sw_mask`.

However, `efx_np_cap_mask_hw_to_sw` is called with this pointer:
```c
    EFX_NP_CAP_MASK_HW_TO_SW(efx_np_cap_map_tech, ETH_AN_FIELDS_TECH_MASK,
        hw_data, sw_maskp);
```

Looking at the macro expansion (not shown), `efx_np_cap_mask_hw_to_sw` does:
```c
    if ((hw_cap_data[byte_idx] & flag_hw) == flag_hw)
        *sw_cap_maskp |= flag_sw;
```

So it uses `|=`, which means it reads the current value of `*sw_cap_maskp`. The patch changes the annotation to `__inout` to reflect this. And it initializes `*sw_cap_maskp = 0;` in the caller `efx_np_cap_hw_data_to_sw_mask`.

But there's a second call in the same function:
```c
    EFX_NP_CAP_MASK_HW_TO_SW(efx_np_cap_map_fec_req, ...);
```

After the first call, `*sw_maskp` is modified. The second call will OR in additional bits. This is intentional (accumulating multiple capability masks).

Wait, let me re-read the code. The patch adds:
```c
+	*sw_cap_maskp = 0;
+
 	EFX_NP_CAP_MASK_HW_TO_SW(efx_np_cap_map_tech, ETH_AN_FIELDS_TECH_MASK,
 	    hw_data, sw_maskp);
```

So `*sw_maskp` is zeroed once at the start of `efx_np_cap_hw_data_to_sw_mask`, then the two macro calls accumulate bits into it. This is correct.

However, I need to check where `efx_np_cap_hw_data_to_sw_mask` is called. Looking at the patch:
```diff
 	efx_np_cap_hw_data_to_sw_mask(
 	    MCDI_OUT2(req, const uint8_t, LINK_STATE_OUT_ADVERTISED_ABILITIES),
 	    &lsp->enls_adv_cap_mask);
 
+	if (lsp->enls_an_supported != B_FALSE)
+		lsp->enls_adv_cap_mask |= 1U << EFX_PHY_CAP_AN;
```

So the caller initializes `lsp->enls_adv_cap_mask` elsewhere (not shown), then `efx_np_cap_hw_data_to_sw_mask` is called. But now `efx_np_cap_hw_data_to_sw_mask` does `*sw_maskp = 0;`, which **overwrites** the caller's value.

Wait, looking at the original code:
```c
    if (lsp->enls_an_supported != B_FALSE)
        lsp->enls_adv_cap_mask |= 1U << EFX_PHY_CAP_AN;

    efx_np_cap_hw_data_to_sw_mask(..., &lsp->enls_adv_cap_mask);
```

The old code set the AN bit BEFORE calling the function. The new code sets it AFTER:
```c
    efx_np_cap_hw_data_to_sw_mask(..., &lsp->enls_adv_cap_mask);

    if (lsp->enls_an_supported != B_FALSE)
        lsp->enls_adv_cap_mask |= 1U << EFX_PHY_CAP_AN;
```

So the order is swapped to account for the fact that the function now zeroes the mask first. This is correct.

But is `lsp->enls_adv_cap_mask` initialized before the call? Looking at the broader context (not fully shown in the patch), `lsp` is a `link_state` structure. Typically these are zero-initialized. If `lsp->enls_adv_cap_mask` is not explicitly initialized before the call, and the function now does `*sw_maskp = 0;`, then the behavior is the same as before (it will be zero). This seems correct.

No issues found.

---

## Patch 08/14: common/sfc_efx/base: fix annotations in get fixed port props

No correctness issues found. This is an annotation-only change.

---

## Patch 09/14: common/sfc_efx/base: fix annotations in SW-HW enum converter

### Errors

**1. Function may not write to `*enum_hwp` on success**
The patch adds the annotation `__success(*supportedp != 0)` to indicate that `*enum_hwp` is only written when `*supportedp` is true (non-zero). However, looking at the function logic:

```c
static void
__success(*supportedp != 0)
efx_np_cap_enum_sw_to_hw(..., __out *enum_hwp)
{
    ...
}
```

Wait, the patch only adds the annotation, it doesn't change the function body. So I need to verify whether the function always writes to `*enum_hwp` when `*supportedp` is set to non-zero.

Actually, the annotation `