|WARNING| [2/2] net/sfc: provide cached dev info to use in secondary process

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## PATCH 1/2: net/sfc: fix shared adapter pointer set in secondary process

### Errors

None.

### Warnings

None.

### Info

None.

---

## PATCH 2/2: net/sfc: provide cached dev info to use in secondary process

### Errors

**1. Memory leak: `dev_info_cache` pointer fields not initialized**

The `rte_eth_dev_info` structure contains pointer fields that are populated by `sfc_dev_infos_get()`.
When this structure is copied in `sfc_dev_infos_get_secondary()`, the pointers are copied verbatim,
creating multiple references to the same allocated memory.
When the device is closed in the secondary process, if these pointers are freed,
the primary process will be left with dangling pointers.
Conversely, if they are not freed, the memory is leaked.

**Affected fields in `struct rte_eth_dev_info`:**
- `dev_info->tx_desc_lim` (pointer to static data, safe to share)
- `dev_info->rx_desc_lim` (pointer to static data, safe to share)
- `dev_info->default_rxconf`, `dev_info->default_txconf` (pointers to static data, safe to share)
- `dev_info->flow_type_rss_offloads` (uint64_t bitmask, not a pointer)
- Other fields depend on the driver implementation

**Review the `sfc_dev_infos_get()` implementation to determine:**
1. Which pointer fields it populates
2. Whether those pointers reference dynamically allocated memory or static data
3. If dynamic, whether the cache copy needs separate allocations or reference counting

If `sfc_dev_infos_get()` only populates pointers to static data (descriptor limits, default configs),
the shallow copy is safe.
If it allocates memory, the cache initialization must either:
- Duplicate the allocations (and free them on adapter cleanup)
- Use reference counting
- Document that the cached pointers are owned by the primary process and must not be freed by secondary

**Suggested fix:**
Add a comment documenting the ownership of pointer fields, or allocate separate storage if needed.

**2. Missing bounds check on `sfc_dev_infos_get()` return value**

The comment states "`sfc_dev_infos_get()` helper always returns 0", but the code casts the return value to `void`.
If `sfc_dev_infos_get()` is ever modified to return an error (e.g., due to hardware probe failure),
the initialization of `dev_info_cache` may be incomplete or uninitialized, and the cast to `void` suppresses the warning.

```c
/* Current code */
(void)sfc_dev_infos_get(dev, &sas->dev_info_cache);

/* Suggested */
rc = sfc_dev_infos_get(dev, &sas->dev_info_cache);
if (rc != 0) {
	/* Handle error: either fail init or mark cache as invalid */
	sfc_err(sa, "failed to populate dev_info_cache: %s",
		rte_strerror(-rc));
	goto fail_dev_info_cache;
}
```

Or, if `sfc_dev_infos_get()` is guaranteed to succeed after successful attach,
add an assertion instead of casting to void:

```c
rc = sfc_dev_infos_get(dev, &sas->dev_info_cache);
SFC_ASSERT(rc == 0);  /* or RTE_VERIFY */
```

### Warnings

**1. `dev_info->device` set to NULL may confuse callers**

The code sets `sas->dev_info_cache.device = NULL` with the intent that secondary processes will restore it.
However, if `sfc_dev_infos_get()` is called in the primary process after initialization
(e.g., from a control plane query), it will see the stale NULL pointer unless it re-calls `sfc_dev_infos_get()`.

**Clarify the intended behavior:**
- Is `dev_info_cache` only for secondary processes?
- Should primary process re-populate it on each query, or cache it once?

**Suggested improvement:**
Add a comment explaining that `dev_info_cache.device` is intentionally NULL and will be restored by `sfc_dev_infos_get_secondary()`.

```c
/*
 * Set device pointer to NULL in the cache.
 * Secondary processes will restore their process-local device pointer
 * in sfc_dev_infos_get_secondary() before returning the cached data.
 */
sas->dev_info_cache.device = NULL;
```

**2. Switch domain ID initialization redundant**

The code initializes `sas->dev_info_cache.switch_info.domain_id` to `RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID`
before calling `sfc_dev_infos_get()`, which will overwrite it.
This is harmless but unnecessary.

```c
/* Current code */
sas->dev_info_cache.switch_info.domain_id =
	RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID;
(void)sfc_dev_infos_get(dev, &sas->dev_info_cache);  /* overwrites domain_id */
```

**Suggested:**
Either remove the pre-initialization (if `sfc_dev_infos_get()` always sets it),
or add a comment explaining why it is needed (e.g., if `sfc_dev_infos_get()` conditionally sets it).

### Info

**1. Consider zeroing `dev_info_cache` before populating**

To ensure no uninitialized fields are present (in case `sfc_dev_infos_get()` does not write all fields),
consider zeroing the structure first:

```c
memset(&sas->dev_info_cache, 0, sizeof(sas->dev_info_cache));
sas->dev_info_cache.switch_info.domain_id =
	RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID;
```

This is defensive programming and may not be strictly necessary if `sfc_dev_infos_get()` always writes all fields.

---

## Summary

**Patch 1/2:** Correct fix for NULL pointer dereference. No issues.

**Patch 2/2:** Needs review of pointer field ownership in `dev_info_cache` to avoid memory leaks or dangling pointers.
Missing error check on `sfc_dev_infos_get()` could hide future issues.