Re: [PATCH v2 0/2] cnxk: add HALO support for CN20K mempool

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 5 Dec 2025 11:21:32 +0530
Nawal Kishor <nkishor@marvell.com> wrote:

> This series of patches adds support for HALO in CNXK mempool driver.
> 
> Changes in v2:
>  * Fixed compilation warnings.
> 
> Nawal Kishor (2):
>   common/cnxk: add support for halos
>   mempool/cnxk: add halo support in mempool
> 
>  drivers/common/cnxk/hw/npa.h                  |  81 ++++++
>  drivers/common/cnxk/roc_idev.c                |  25 ++
>  drivers/common/cnxk/roc_idev.h                |   3 +
>  drivers/common/cnxk/roc_idev_priv.h           |   1 +
>  drivers/common/cnxk/roc_mbox.h                |   6 +
>  drivers/common/cnxk/roc_nix.h                 |   1 +
>  drivers/common/cnxk/roc_nix_queue.c           |  46 ++-
>  drivers/common/cnxk/roc_npa.c                 | 268 ++++++++++++++++--
>  drivers/common/cnxk/roc_npa.h                 |  20 +-
>  drivers/common/cnxk/roc_npa_debug.c           | 201 ++++++++++++-
>  drivers/common/cnxk/roc_npa_priv.h            |   3 +
>  .../common/cnxk/roc_platform_base_symbols.c   |   2 +
>  drivers/common/cnxk/roc_sso.c                 |  35 ++-
>  drivers/common/cnxk/roc_sso.h                 |   1 +
>  drivers/mempool/cnxk/cn10k_mempool_ops.c      |  19 +-
>  drivers/mempool/cnxk/cn20k_mempool_ops.c      |  60 ++++
>  drivers/mempool/cnxk/cn9k_mempool_ops.c       |   2 +-
>  drivers/mempool/cnxk/cnxk_mempool.c           |  40 ++-
>  drivers/mempool/cnxk/cnxk_mempool.h           |  16 +-
>  drivers/mempool/cnxk/cnxk_mempool_ops.c       |  11 +-
>  drivers/mempool/cnxk/meson.build              |   1 +
>  21 files changed, 750 insertions(+), 92 deletions(-)
>  create mode 100644 drivers/mempool/cnxk/cn20k_mempool_ops.c
> 

This patch never got merged because of CI build failures.
Fix and resubmit.

AI review also says:


## Patch Review: cnxk halo support (2-patch series)

### Patch 1/2: `common/cnxk: add support for halos`

#### Commit Message ✓

| Criterion | Status | Notes |
|-----------|--------|-------|
| Subject ≤60 chars | ✓ | 33 characters |
| Correct prefix | ✓ | `common/cnxk:` is appropriate |
| Lowercase after colon | ✓ | |
| Imperative mood | ✓ | "add support" |
| No trailing period | ✓ | |
| Body wrapped ≤75 chars | ✓ | |
| Body not starting with "It" | ✓ | Starts with "In cn9k/cn10k..." |
| Signed-off-by present | ✓ | Real name and valid email |

#### Code Issues

**Warning: Implicit pointer comparison (multiple locations)**

The AGENTS.md specifies:
> `if (!p)             /* Bad - don't use ! on pointers */`

Several instances use `!ptr` style:

```c
// roc_npa.c - npa_halo_alloc()
if (!lf || !halo || !aura_handle)
    return NPA_ERR_PARAM;

// roc_npa.c - npa_halo_free()  
if (!lf || !aura_handle)
    return NPA_ERR_PARAM;
```

**Suggested fix:**
```c
if (lf == NULL || halo == NULL || aura_handle == NULL)
    return NPA_ERR_PARAM;
```

---

**Warning: Non-portable format specifier**

In `roc_npa_debug.c`, function `npa_halo_dump()`:
```c
npa_dump("W9: thresh \t\t%lu\nW9: fc_msh_dst \t\t%d",
         (unsigned long)halo->thresh, halo->fc_msh_dst);
```

The AGENTS.md specifies:
> Forbidden: `%lld`, `%llu`, `%llx` → Preferred: `%PRId64`, `%PRIu64`, `%PRIx64`

While `%lu` with an explicit cast is technically different, for consistency with the rest of the file (which uses `%"PRIx64"` patterns), this should use:
```c
npa_dump("W9: thresh \t\t%" PRIu64 "\nW9: fc_msh_dst \t\t%d",
         (uint64_t)halo->thresh, halo->fc_msh_dst);
```

---

**Warning: New API functions not marked `__rte_experimental`**

New public APIs added to `roc_idev.h`:
```c
int __roc_api roc_idev_npa_halo_ena_get(void);
void __roc_api roc_idev_npa_halo_ena_set(int halo_ena);
```

And new inline function in `roc_npa.h`:
```c
static inline uint32_t
roc_npa_max_block_size(void)
```

Per AGENTS.md:
> New APIs must be marked as `__rte_experimental`

**Note:** These are internal (`__roc_api`) functions in the ROC layer, so `__rte_experimental` may not strictly apply - this depends on whether the ROC API is considered public. However, the pattern in other ROC files should be followed.

---

**Info: Good practices observed**
- Proper error code additions (`NPA_ERR_HALO_INIT`, `NPA_ERR_HALO_FINI`)
- Symbol exports added via `RTE_EXPORT_INTERNAL_SYMBOL`
- Hardware structure properly defined with bitfields
- Consistent use of `mbox_get()`/`mbox_put()` patterns

---

### Patch 2/2: `mempool/cnxk: add halo support in mempool`

#### Commit Message ✓

| Criterion | Status | Notes |
|-----------|--------|-------|
| Subject ≤60 chars | ✓ | 40 characters |
| Correct prefix | ✓ | `mempool/cnxk:` is appropriate |
| Lowercase after colon | ✓ | |
| Imperative mood | ✓ | "add" |
| No trailing period | ✓ | |
| Body wrapped ≤75 chars | ✓ | |
| Body not starting with "It" | ✓ | Starts with "This patch..." |
| Signed-off-by present | ✓ | |

#### License ✓

New file `cn20k_mempool_ops.c`:
```c
/* SPDX-License-Identifier: BSD-3-Clause
 * Copyright(C) 2025 Marvell.
 */
```
- SPDX on first line ✓
- Copyright follows ✓
- Blank line before includes ✓
- BSD-3-Clause appropriate for driver ✓

#### Code Issues

**Warning: Devargs parameter string formatting**

In `cnxk_mempool.c`:
```c
RTE_PMD_REGISTER_PARAM_STRING(mempool_cnxk,
                  CNXK_NPA_MAX_POOLS_PARAM "=<128-1048576>"
                  CNXK_NPA_HALO_ENA_PARAM "=<0-1>");
```

The two parameter strings will concatenate without a space separator, resulting in:
`max_pools=<128-1048576>halo_ena=<0-1>`

**Suggested fix:**
```c
RTE_PMD_REGISTER_PARAM_STRING(mempool_cnxk,
                  CNXK_NPA_MAX_POOLS_PARAM "=<128-1048576> "
                  CNXK_NPA_HALO_ENA_PARAM "=<0-1>");
```

---

**Info: meson.build ordering**

The file list in `meson.build` is not alphabetically ordered:
```python
sources = files(
        ...
        'cn9k_mempool_ops.c',
        'cn10k_mempool_ops.c',
        'cn20k_mempool_ops.c',
        'cn10k_hwpool_ops.c',
)
```

Per AGENTS.md: "Lists alphabetically ordered". However, this follows the existing convention in the file (which was already non-alphabetical), so this is an existing technical debt rather than a new issue introduced by this patch.

---

**Info: Documentation consideration**

The new `halo_ena` devargs parameter should ideally be documented in the driver documentation (`doc/guides/mempool/cnxk.rst` or similar) so users understand what it does and when to use it.

---

### Summary

| Severity | Count | Items |
|----------|-------|-------|
| **Error** | 0 | None |
| **Warning** | 4 | Implicit pointer checks, format specifier, experimental marking, devargs spacing |
| **Info** | 2 | meson.build ordering (pre-existing), documentation gap |

#### Recommended Actions

1. **Should fix:** Change `!ptr` to `ptr == NULL` for pointer comparisons
2. **Should fix:** Use `PRIu64` format specifier in `npa_halo_dump()`
3. **Should fix:** Add space between devargs parameter strings
4. **Consider:** Add documentation for the new `halo_ena` parameter
5. **Consider:** Verify `__rte_experimental` policy for internal ROC APIs

The patches are generally well-structured and follow DPDK conventions. The issues identified are relatively minor style/consistency items rather than functional problems.