> 2. **Missing `Cc: [email protected]`** > - 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: [email protected]`.
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.
