On Fri, 5 Dec 2025 11:21:32 +0530
Nawal Kishor <[email protected]> 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.
