|WARNING| [v3] net/ice: support link flow control

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-22

# DPDK Patch Review

## Summary

This patch adds IEEE 802.3x link flow control support to the Intel ice PMD. While the core functionality appears sound, there are several correctness issues and style violations that need to be addressed.

---

## Errors

### 1. Race condition in link event handler

**File:** `drivers/net/intel/ice/ice_ethdev.c`
**Location:** `ice_handle_aq_msg()` at lines 1478-1485

The link-up check and MIB configuration are performed without holding `pf->link_lock`, but `ice_link_update()` acquires this lock. The link status could change between the `ice_link_update()` call and the subsequent check, causing the MIB to be programmed when the link is actually down.

```c
/* BAD - link_info read without lock after ice_link_update released it */
if ((hw->port_info->phy.link_info.link_info & ICE_AQ_LINK_UP) &&
        !(dev->data->dev_conf.rxmode.mq_mode &
            RTE_ETH_MQ_RX_DCB_FLAG))
    ice_set_dflt_mib(dev);
```

**Fix:** Capture the link state inside `ice_link_update()` or acquire `pf->link_lock` around this check:

```c
ret = ice_link_update(dev, 0);
if (!ret) {
    rte_spinlock_lock(&pf->link_lock);
    bool link_up = hw->port_info->phy.link_info.link_info & ICE_AQ_LINK_UP;
    bool dcb_disabled = !(dev->data->dev_conf.rxmode.mq_mode & RTE_ETH_MQ_RX_DCB_FLAG);
    rte_spinlock_unlock(&pf->link_lock);

    if (link_up && dcb_disabled)
        ice_set_dflt_mib(dev);

    rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC, NULL);
}
```

---

### 2. Error path in `ice_flow_ctrl_set` does not restore state

**File:** `drivers/net/intel/ice/ice_ethdev.c`
**Location:** `ice_flow_ctrl_set()` at lines 4194-4198

When `ice_set_fc()` succeeds but `ice_cfg_fc_params()` fails, the `pf->fc_conf.mode` has already been updated. On a subsequent call to `ice_flow_ctrl_get()`, the application will see the requested mode even though the pause parameters were not applied.

```c
pf->fc_conf.mode = fc_conf->mode;  /* Saved before ice_cfg_fc_params */

tmp_fc_conf = *fc_conf;
ret = ice_cfg_fc_params(dev, 0, &tmp_fc_conf);
if (ret != 0) {
    PMD_DRV_LOG(WARNING, "Flow control mode applied, pause parameters left unchanged");
    /* Mode already saved above, partially applied config visible to user */
}
```

**Fix:** Only update `pf->fc_conf` after both operations succeed, or revert `pi->fc.req_mode` on `ice_cfg_fc_params()` failure:

```c
pi->fc.req_mode = req_mode;
ret = ice_set_fc(pi, &aq_failures, link_up);
if (ret != 0 && aq_failures != ICE_SET_FC_AQ_FAIL_UPDATE) {
    PMD_DRV_LOG(ERR, "Failed to set flow control mode, ret %d aq_failures 0x%x",
                ret, aq_failures);
    return -EIO;
}

tmp_fc_conf = *fc_conf;
ret = ice_cfg_fc_params(dev, 0, &tmp_fc_conf);
if (ret != 0) {
    /* Revert pi->fc.req_mode here or return error instead of warning */
    PMD_DRV_LOG(ERR, "Failed to set pause parameters");
    return ret;
}

/* Only update cached config after both operations succeed */
pf->fc_conf.mode = fc_conf->mode;
if (tmp_fc_conf.pause_time)
    pf->fc_conf.pause_time = tmp_fc_conf.pause_time;
/* ... */
```

---

### 3. Missing error check on `ice_aq_set_lldp_mib`

**File:** `drivers/net/intel/ice/ice_ethdev.c`
**Location:** `ice_set_dflt_mib()` at line 4145

The function logs an error but does not propagate it to the caller. If the MIB write fails, link flow control will not operate correctly, but the user has no indication that the feature is broken.

```c
if (ice_aq_set_lldp_mib(hw, SET_LOCAL_MIB_TYPE_LOCAL_MIB, lldpmib, miblen, NULL))
    PMD_DRV_LOG(ERR, "Failed to set default LLDP MIB");
```

**Fix:** Return `int` and propagate the error, or at minimum document that this failure is non-fatal:

```c
static int
ice_set_dflt_mib(struct rte_eth_dev *dev)
{
    /* ... */
    int ret = ice_aq_set_lldp_mib(hw, SET_LOCAL_MIB_TYPE_LOCAL_MIB,
                                  lldpmib, miblen, NULL);
    if (ret) {
        PMD_DRV_LOG(ERR, "Failed to set default LLDP MIB");
        return -EIO;
    }
    return 0;
}

/* In caller (ice_handle_aq_msg): */
ret = ice_set_dflt_mib(dev);
if (ret)
    PMD_DRV_LOG(WARNING, "Link is up but flow control not operational");
```

---

### 4. `ice_cfg_fc_params` ignores return value from `ice_aq_set_mac_pfc_cfg`

**File:** `drivers/net/intel/ice/ice_ethdev.c`
**Location:** `ice_cfg_fc_params()` at lines 4138-4141

The function returns `ret` from `ice_aq_set_mac_pfc_cfg()`, but has already written to hardware registers. If the AQ command fails, the watermarks have been updated but the pause quanta has not, leaving the hardware in an inconsistent state.

**Fix:** Either write watermarks *after* the AQ command succeeds, or document that partial configuration may occur:

```c
/* Option 1: Reorder operations */
ret = ice_aq_set_mac_pfc_cfg(hw, max_frame_size, tc_bitmap,
                             fc_conf->pause_time,
                             ((u32)fc_conf->pause_time + 1) / 2,
                             false, NULL);
if (ret) {
    PMD_DRV_LOG(ERR, "Can not update MAC configuration");
    return ret;
}

/* Only update watermarks after AQ command succeeds */
if (high_water)
    wr32(hw, GLRPB_TCHW(cgd_idx), high_watermark);
if (low_water)
    wr32(hw, GLRPB_TCLW(cgd_idx), low_watermark);
```

---

## Warnings

### 1. Missing `RTE_EXPORT_SYMBOL` for new dev_ops

**File:** `drivers/net/intel/ice/ice_ethdev.c`

The new functions `ice_flow_ctrl_get` and `ice_flow_ctrl_set` are assigned to `eth_dev_ops` but do not have `RTE_EXPORT_SYMBOL` annotations. While these are not public API (they are PMD internal), other PMDs in the tree have similar patterns where the dev_ops functions are exported.

**Guidance:** Verify whether these should be `RTE_EXPORT_INTERNAL_SYMBOL`. If they are only called via function pointers in `eth_dev_ops`, no export may be needed.

---

### 2. Cached config not initialized

**File:** `drivers/net/intel/ice/ice_ethdev.h`
**Location:** Line 616

The new `fc_conf` field in `struct ice_pf` is not explicitly initialized. When `ice_flow_ctrl_get()` is called before `ice_flow_ctrl_set()`, it returns uninitialized watermark values.

**Fix:** Initialize `fc_conf` in the PF init path:

```c
/* In ice PF initialization: */
pf->fc_conf.mode = RTE_ETH_FC_NONE;
pf->fc_conf.high_water = 0;
pf->fc_conf.low_water = 0;
pf->fc_conf.pause_time = 0;
pf->fc_conf.autoneg = false;
pf->fc_conf.send_xon = false;
pf->fc_conf.mac_ctrl_frame_fwd = false;
```

---

### 3. Incomplete MTU + overhead calculation

**File:** `drivers/net/intel/ice/ice_ethdev.c`
**Location:** `ice_cfg_fc_params()` at lines 4133-4135

The frame size calculation uses a hardcoded `ICE_ETH_OVERHEAD` constant. Per the guidelines, Ethernet overhead should be derived from device capabilities to account for VLAN/QinQ support.

```c
max_frame_size = pf->dev_data->mtu ?
    pf->dev_data->mtu + ICE_ETH_OVERHEAD :
    ICE_FRAME_SIZE_MAX;
```

**Suggested fix:** Use `dev_info.max_rx_pktlen - dev_info.max_mtu` if available, or document why the hardcoded overhead is correct for this device.

---

### 4. `ice_priority_flow_ctrl_set` warning message downgraded

**File:** `drivers/net/intel/ice/ice_ethdev.c`
**Location:** Line 4410

The patch changes the error from returning `-EIO` (original code at line 4192) to a warning when `ice_cfg_fc_params()` fails in the PFC path. This inconsistency means PFC configuration failures are logged but not reported, while the same failure in LFC returns an error.

**Suggested fix:** Keep the error return consistent with the LFC path, or document why PFC should tolerate this failure.

---

### 5. Boolean comparison style

**File:** `drivers/net/intel/ice/ice_ethdev.c`
**Location:** Lines 1478, 4163

Implicit boolean comparison used on `enable_lse` flag:

```c
bool enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
```

**Note:** The ternary is redundant. Simplify to:

```c
bool enable_lse = dev->data->dev_conf.intr_conf.lsc != 0;
```

This is a minor style preference and not flagged as an error since the code is correct.

---

## Info

### 1. Documentation clarity: "MAC control frame forwarding"

**File:** `doc/guides/nics/ice.rst`
**Location:** Lines 539-540

The description states "Forwarding of MAC control frames other than pause frames," but does not explain *where* they are forwarded (to the host application? dropped?). Consider clarifying the behavior.

---

### 2. Missing `ice_get_link_info_safe` declaration

**File:** `drivers/net/intel/ice/ice_ethdev.c`
**Location:** Line 4055

The forward declaration of `ice_get_link_info_safe` is added but the function definition is not present in the patch. Verify this function exists in the base code or is added in a previous patch in the series.

---

## Positive Observations

- The patch correctly retains flow control configuration across port stop/start by caching it in `pf->fc_conf`.
- The mutual exclusion check between LFC and PFC is appropriate.
- Documentation is thorough and matches the code behavior.
- The use of a `tmp_fc_conf` local variable to avoid modifying the user's struct on error is good practice.

---

## Recommendation

**Do not merge** until the race condition (Error #1), error path state inconsistency (Error #2), and error propagation issues (Errors #3, #4) are addressed. The warnings should also be fixed to ensure robustness.