From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7D92BE63CA4 for ; Sun, 25 Jan 2026 19:36:26 +0000 (UTC) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6503E40276; Sun, 25 Jan 2026 20:36:25 +0100 (CET) Received: from mail-wr1-f50.google.com (mail-wr1-f50.google.com [209.85.221.50]) by mails.dpdk.org (Postfix) with ESMTP id 6B4284026F for ; Sun, 25 Jan 2026 20:36:23 +0100 (CET) Received: by mail-wr1-f50.google.com with SMTP id ffacd0b85a97d-42fb6ce71c7so3623381f8f.1 for ; Sun, 25 Jan 2026 11:36:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20230601.gappssmtp.com; s=20230601; t=1769369783; x=1769974583; darn=dpdk.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=trSWm1MNx2UPBMNPe03yW4mRymhT/Py3OXM26NnusTE=; b=p9EBECpP4BLqe0GukAxEnYud+0us6PDXFR9xT5O7F6XTqmHOBoWkHXSgTs+QhxMwvz JFgnIfNmCKLHUJdxZNndUwmcTKG1KrOf5RG5EjPeYE2AhDTC+BhBtwL/7UdwRMKl7Mne n5Wn5K1LoJM0fxIbjygpqn5zV1lWDq3qI4z2L0mAGLOo6GRrXHfUwyKp+4pD8uIyCiCx xLPxJOqKcyUthWbVfBB2QplHPHNTCNyuUeugbd1OTid3igRmpSndJrKDdX9dvqO0JlNr uZYqB6yp+hn3XYdS86RotYDA4/mB0JbTgZszqFf9Wqc4DGEkqc1BA4jN2c8+mBZSllQw XPjQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1769369783; x=1769974583; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=trSWm1MNx2UPBMNPe03yW4mRymhT/Py3OXM26NnusTE=; b=pggKPtzHyMXMugP1zYLybl8qMoH6Tc4RcqyEvDnAq/Fo1kIx2lix//tA9fpP8kO3jb JhT4cI67xtBSVXTbTRt6RnKL6G4Zp4skhcwErC7mR13L6447EjCVbjgu5BahKxPOhMuI 1SqNir0x7gud9nkP8IfbmtwhDW+sOUCL6re2NY2YiQtVgcFf9eOP8bVsh7qEROo8+M3K KDikYNClooNUEOQyCZT016d1mJFh4BH3CpLxzvEqGIwSDe6z4vLUhQ3VStSyMbNL7RvX 80qSJ+bFkZIWgf2hujAWviJidQJdEBEVcy1byMJqDZIEJaIoLT3Tqj/sybJIZ/ocFha1 zc5Q== X-Gm-Message-State: AOJu0YzMnm+Ru9we/ep+h+kwnIy8+lkset6gXeW1E8Q88cLdpvXEGBN3 ynDjTb9dIoAI4sD3u2Giv0/L2PaBWryt6nO+bg0CURgzckRM13Tq9akEBRU+0xt8HK+DwOVvgvj yPOJI X-Gm-Gg: AZuq6aKZfKpOpuntyQSftb9vmBtTOVpLb7nFYxMTYT2VQhdzYjZ8+Ml11YSkkdozCB7 VR5yk1c3+7dHB2nv8T0ZgQ15eYVwVvHdc8esDXqC0tkeq4/zt8k0Kis6yPqstYYdXQ/X4d0Xt+H QvARtaUVTUmpKvg3CgvTszd9QW6DGcAG+3Vys1Dybccv0QjbGD2tCJmp0DRap2TQ1YZ0q1opvVl TKygUqkSUe/9Yl3dq6U7Kd+Ct7YRY4oRZYLD10r0ccJLxnohqegfxWAWB/YKrbNMDFn4+nyC/gV ij0M80q5uk4hfJ0JJcLzRUNcXWL6IYyWTU49PRJjKQ3S8LNxDWjzJf3jcJ9zSG8k3WrGk3mxzuV 9bWzn/ft4M2o+OfrZwfBu07mEEG6QQJb/Rv2oiznBaM2cnHCnP/Kfw3xC4yKSUw2AIqv2zqm5IK 3UOORP00YxYX168UVeHqc6QOKe7GPHe6xm1jgePbUEMUxp5vTbBg== X-Received: by 2002:a05:6000:1787:b0:435:8dd5:ad4c with SMTP id ffacd0b85a97d-435ca0e2b27mr3183072f8f.6.1769369782864; Sun, 25 Jan 2026 11:36:22 -0800 (PST) Received: from phoenix.lan (204-195-96-226.wavecable.com. [204.195.96.226]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-435b1f7c9cesm24678626f8f.41.2026.01.25.11.36.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 25 Jan 2026 11:36:22 -0800 (PST) From: Stephen Hemminger To: dev@dpdk.org Cc: Stephen Hemminger , Scott Mitchell Subject: [REVIEW] eal: RTE_PTR_ADD/SUB API improvements Date: Sun, 25 Jan 2026 11:36:18 -0800 Message-ID: <20260125193618.499170-1-stephen@networkplumber.org> X-Mailer: git-send-email 2.51.0 In-Reply-To: <20260125111211.34800-1-scott.k.mitch1@gmail.com> References: <20260125111211.34800-1-scott.k.mitch1@gmail.com> Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org AI-generated review of ptr-add.mbo Reviewed using Claude (claude-opus-4-5-20251101) This is an automated review. Please verify all suggestions. --- # DPDK Patch Review: eal: RTE_PTR_ADD/SUB API improvements ## Summary This patch introduces improvements to the RTE_PTR_ADD/SUB macros by splitting the API into pointer-based and integer-based variants to preserve pointer provenance and type qualifiers. --- ## Errors (Must Fix) ### 1. Subject Line Exceeds 60 Characters **Location:** Commit subject **Issue:** Subject line "eal: RTE_PTR_ADD/SUB API improvements" is 39 characters, which is acceptable. However, the format should use lowercase after the colon. **Severity:** The subject looks acceptable. ### 2. Missing Blank Line Between Tag Groups **Location:** Commit message tags **Issue:** The `Signed-off-by` tag appears without proper separation from the body text. According to guidelines, there should be a blank line before `Reported-by`, `Suggested-by`, `Signed-off-by` group. **Current:** ``` ...the new API exposes. Signed-off-by: Scott Mitchell ``` **Expected:** Blank line separation is present, so this is acceptable. ### 3. Unnecessary Cast of void* in Test File **Location:** `app/test/test_ptr_add_sub.c`, line 145 ```c uint32_t *u32p = (uint32_t *)buffer; ``` **Issue:** While this cast is necessary for the type conversion, consider using a union or aligned buffer to avoid potential alignment issues. --- ## Warnings (Should Fix) ### 1. Documentation Does Not Match Code for RTE_PTR_UNQUAL **Location:** `lib/eal/include/rte_common.h` **Issue:** The patch adds `RTE_PTR_UNQUAL` macro usage throughout the codebase but the macro definition is not shown in the patch. If this is a new macro, it needs documentation. ### 2. Use of `__auto_type` Without Fallback for All Compilers **Location:** `lib/eal/include/rte_common.h`, lines 106-111 ```c #ifdef __cplusplus #define __rte_auto_type auto #elif defined(RTE_CC_GCC) || defined(RTE_CC_CLANG) #define __rte_auto_type __auto_type #endif ``` **Issue:** No fallback defined for compilers other than GCC/Clang in C mode. This could cause compilation failures with other compilers. ### 3. Implicit Pointer Comparison **Location:** `drivers/bus/cdx/cdx_vfio.c`, line 374 ```c if (msl->base_va == NULL) ``` **Status:** This is correct per guidelines (explicit NULL comparison). ### 4. Missing `Cc: stable@dpdk.org` Tag **Location:** Commit message **Issue:** This patch fixes issues that could affect existing code (compiler optimization issues, qualifier dropping). If these are considered bug fixes for stable releases, the `Cc: stable@dpdk.org` tag should be added. ### 5. Variable Declaration Style Inconsistency **Location:** `drivers/bus/cdx/cdx_vfio.c`, lines 371-372 ```c size_t sz = msl->len; void *end_va; ``` **Issue:** Mixed declaration styles - `sz` is initialized at declaration while `end_va` is not. Consider consistency within the function. ### 6. FIXME Comment Left in Code **Location:** `drivers/dma/idxd/idxd_pci.c`, line 398 ```c /* FIXME: cast drops volatile propagation to idxd_dmadev.portal */ ``` **Issue:** FIXME comments should be resolved before merging, or tracked in a bug tracker. ### 7. Missing Test Registration Constant Documentation **Location:** `app/test/test_ptr_add_sub.c`, line 197 ```c REGISTER_FAST_TEST(ptr_add_sub_autotest, NOHUGE_OK, ASAN_OK, test_ptr_add_sub_suite); ``` **Status:** Good - uses named constants `NOHUGE_OK` and `ASAN_OK` as required. ### 8. Inconsistent Error Handling Pattern **Location:** Multiple files **Issue:** Some NULL checks return early, others use goto. For example: - `drivers/bus/cdx/cdx_vfio.c`: returns 0 - `lib/eal/linux/eal_memalloc.c`: uses goto While both may be correct for their contexts, ensure consistency within each file. --- ## Info (Consider) ### 1. Good Practice: Using TEST_ASSERT Macros **Location:** `app/test/test_ptr_add_sub.c` **Status:** The test file correctly uses `TEST_ASSERT_EQUAL` and follows the `unit_test_suite_runner` pattern as required by guidelines. ### 2. Good Practice: Proper SPDX and Copyright **Location:** `app/test/test_ptr_add_sub.c`, lines 1-3 ```c /* SPDX-License-Identifier: BSD-3-Clause * Copyright(c) 2026 Apple Inc. */ ``` **Status:** Correct format with SPDX on first line, copyright following, blank line before includes. ### 3. Good Practice: Include Order **Location:** `app/test/test_ptr_add_sub.c`, lines 5-9 ```c #include #include #include "test.h" ``` **Status:** Correct order - system includes, DPDK includes, application includes, with blank lines separating groups. ### 4. Complex Macro Implementation **Location:** `lib/eal/include/rte_common.h`, RTE_PTR_ADD/SUB macros **Observation:** The new implementation using `__extension__`, `__rte_auto_type`, and diagnostic pragmas is complex. Consider adding implementation notes as regular comments (which was done in v8 per changelog). ### 5. Potential ABI Impact **Location:** `lib/eal/include/rte_common.h` **Observation:** While these are macro changes (not ABI), the behavioral change could affect compiled code. The release notes should document this change clearly. ### 6. Line Length Compliance **Location:** Various files **Status:** Lines appear to be within the 100-character limit for source code. ### 7. Good Practice: Defensive NULL Checks Added **Location:** Multiple files (`vhost_user.c`, `ena_ethdev.c`, `rte_mbuf.h`, etc.) **Status:** The patch adds appropriate NULL checks before pointer operations, improving robustness. ### 8. Cast Style in Tests **Location:** `app/test/test_common.c`, line 40 ```c RTE_TEST_ASSERT_EQUAL(RTE_INT_PTR_ADD(SMALLER, PTR_DIFF), (void *) BIGGER, ``` **Note:** Space before `BIGGER` in cast `(void *) BIGGER` - minor style inconsistency but not an error. --- ## Release Notes Requirement This patch introduces API changes (new macros, modified behavior of existing macros) and should include updates to `doc/guides/rel_notes/` for the current release noting: - New `RTE_INT_PTR_ADD`, `RTE_INT_PTR_SUB` macros - New `RTE_INT_PTR_ALIGN_*` macros - New `RTE_PTR_UNQUAL` macro (if defined in this patch) - Modified behavior of `RTE_PTR_ADD`/`RTE_PTR_SUB` to preserve qualifiers --- ## Overall Assessment The patch is well-structured and addresses real issues with pointer provenance and qualifier preservation. The test coverage is good, following DPDK test framework conventions. Main concerns are: 1. Missing definition/documentation for `RTE_PTR_UNQUAL` macro 2. FIXME comment should be resolved 3. Release notes should be updated for API changes 4. Consider adding fallback for `__rte_auto_type` for non-GCC/Clang compilers