Re: [REVIEW] eal: RTE_PTR_ADD/SUB API improvements

Scott Mitchell
2026-02-02T05:17:36Z

> 2. **Missing `Cc: stable@dpdk.org`**
> - This patch changes API behavior (NULL to `RTE_PTR_ADD`/`RTE_PTR_SUB` is now undefined). If this is considered a fix for existing issues, it should include `Cc: stable@dpdk.org`.

Morten opened a discussion on this. It's API breaking so I'm not
proposing backporting. If backporting is desired and API compatibility
is required an option is to deprecate existing macros, and introduce
new implementation with a distinct naming convention (e.g.
RTE_PTR_QUAL_ALL - qualified add), then remove old macros in the main
branch.

> 3. **Missing explicit NULL checks before some `RTE_PTR_ADD` calls**
> - `lib/eal/common/eal_common_fbarray.c:1062`: Added `RTE_ASSERT(arr->data)` - good, but this is a debug-only check. Consider returning error in release builds.

I'll add a runtime check here. Note there are other cases where APIs
didn't have any null handling and would have crashed, and I added an
assert to at least be explicit about expectations.

> 5. **`volatile` qualifier handling in `idxd_pci.c`**
> ```c
> // Line 62
> static volatile uint32_t *
> idxd_get_wq_cfg(struct idxd_pci_common *pci, uint8_t wq_idx)
> ```
> Good fix, but the FIXME comment at line 398-400 indicates incomplete resolution:
> ```c
> /* FIXME: cast drops volatile propagation to idxd_dmadev.portal
> * See: https://bugs.dpdk.org/show_bug.cgi?id=1871
> */
> idxd.portal = RTE_PTR_ADD(RTE_PTR_UNQUAL(idxd.u.pci->portals),
> ```
> This should be tracked in the commit message or release notes.

Can you clarify what would be captured in this commit message or
release notes? This commit isn't introducing any issues, and a bug was
filed to follow up.

>
> 6. **`RTE_PTR_UNQUAL` macro is used but not defined in this patch**
> - The patch uses `RTE_PTR_UNQUAL` in multiple places but the macro definition is not visible in the diff. Ensure it exists or is added.

False positive.

>
> 8. **`drivers/dma/odm/odm_dmadev.c` changes remove `const`**
> ```c
> // Before: const uint32_t *base_addr = vq->cring_mz->addr;
> // After: uint32_t *base_addr = vq->cring_mz->addr;
> ```
> This removes a useful const qualifier. If the intent is to allow writes, document why. If not, preserve const.

The comment would be similar to "this value is used in a non-const
way, so don't use const", and I don't think this adds too much value.

>
> 9. **Release notes format**
> - The API Changes section uses bullet points correctly but should reference the specific macros more prominently for searchability.

Clarified.

>
> **`lib/eal/common/eal_common_memory.c:1563`:**
> ```c
> struct malloc_elem *volatile elem;
> ```
> The `volatile` on a local pointer variable is unusual. This appears to be preventing optimization, but the rationale should be documented.

Comment added.