[PWCI] "[v2] devtools: check patch from stdin only if nothing else" #503
Conversation
Enhance the documentation by adding information about limitations specific to the synchronous API when used with hardware steering. This update provides users with a more comprehensive understanding of the constraints in this configuration. Signed-off-by: Maayan Kashani <mkashani@nvidia.com> Signed-off-by: Bing Zhao <bingz@nvidia.com>
Add tested, and hence recommended, firmware and other versions for i40e and ice drivers. In the process remove version inform for obsolete DPDK releases. Signed-off-by: Hailin Xu <hailinx.xu@intel.com> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
There is the updated firmware providing the new capability bit "header_length_field_offset_mode". If this bit is set the length field offset in flex parser configuration should not be adjusted by the supported field mask left margin, and the bit "header_length_field_offset_mode" should be set in configuration command on the flex parser creation firmware call. Fixes: b04b06f ("net/mlx5: fix flex item header length field translation") Cc: stable@dpdk.org Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
The update of flow engine devargs introduced a regression with untrusted VFs/SFs. Using HWS engine caused a degradation in probing 1K SFs, because memzone segments exceeds maximum 2560. Fixes: 53fdc23 ("common/mlx5: read SW steering capability bits") Fixes: d1ac7b6 ("net/mlx5: update flow devargs handling for future HW") Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
Count flow action is supported on root table with HW Steering flow engine with upstream drivers/libraries and drivers/libraries packaged with recently released DOCA. This patch updates relevant docs with minimum DOCA version. Signed-off-by: Dariusz Sosnowski <dsosnowski@nvidia.com> Acked-by: Bing Zhao <bingz@nvidia.com>
Previously, the Tx burst size in l3fwd was fixed at 256, which could lead to suboptimal performance in certain scenarios. This patch introduces separate --rx-burst and --tx-burst options to explicitly configure Rx and Tx burst sizes. By default, the Tx burst size now matches the Rx burst size for better efficiency and pipeline balance. Fixes: d5c4897 ("examples/l3fwd: add option to set Rx burst size") Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com> Tested-by: Venkat Kumar Ande <venkatkumar.ande@amd.com> Tested-by: Dengdui Huang <huangdengdui@huawei.com> Tested-by: Pavan Nikhilesh <pbhagavatula@marvell.com> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
Currently, l3fwd starts in auto-negotiation mode, but it may fail to link up when auto-negotiation is not supported. Therefore, it is necessary to support starting with a specified speed for port. Additionally, this patch does not support changing the duplex mode. So speeds like 10M, 100M are not configurable using this method. Signed-off-by: Dengdui Huang <huangdengdui@huawei.com> Reviewed-by: Chengwen Feng <fengchengwen@huawei.com>
Currently, l3fwd-power starts in auto-negotiation mode, but it may fail to link up when auto-negotiation is not supported. Therefore, it is necessary to support starting with a specified speed for port. Additionally, this patch does not support changing the duplex mode. So speeds like 10M, 100M are not configurable using this method. Signed-off-by: Dengdui Huang <huangdengdui@huawei.com> Reviewed-by: Chengwen Feng <fengchengwen@huawei.com>
If format-truncation is enabled, the compiler detects a string overflow
since the snprintf() is putting ethernet address and new line
in buffer only sized for the address.
Since get_rx_queue_name() is used to create a ring name.
The buffer should be sized to be a valid ring name.
This fixes some format-overflow warnings and also corrects
for future errors.
In file included from ../examples/server_node_efd/efd_server/main.c:31:
In function ‘get_printable_mac_addr’,
inlined from ‘do_stats_display’ at ../examples/server_node_efd/efd_server/main.c:136:3:
../lib/net/rte_ether.h:248:36: warning: ‘snprintf’ output truncated before the last format character [-Wformat-truncation=]
248 | #define RTE_ETHER_ADDR_PRT_FMT "%02X:%02X:%02X:%02X:%02X:%02X"
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../examples/server_node_efd/efd_server/main.c:86:33: note: in expansion of macro ‘RTE_ETHER_ADDR_PRT_FMT’
86 | RTE_ETHER_ADDR_PRT_FMT "\n",
| ^~~~~~~~~~~~~~~~~~~~~~
../examples/server_node_efd/efd_server/main.c: In function ‘do_stats_display’:
../examples/server_node_efd/efd_server/main.c:86:59: note: format string is defined here
86 | RTE_ETHER_ADDR_PRT_FMT "\n",
|
Fixes: 39aad0e ("examples/flow_distributor: new example to demonstrate EFD")
Cc: stable@dpdk.org
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
The ifname is limited to 128 characters, but it would allow up
to 128 characters as prefix then could overflow creating ifname.
Change to limit path prefix (iface) to 123 (128 - sizeof("1024"))
to avoid possible format overflow
../examples/vdpa/main.c:501:76: warning: ‘snprintf’ output may be truncated before the last format character [-Wformat-truncation=]
501 | snprintf(vports[devcnt].ifname, MAX_PATH_LEN, "%s%d",
| ^
../examples/vdpa/main.c:501:25: note: ‘snprintf’ output between 2 and 139 bytes into a destination of size 128
501 | snprintf(vports[devcnt].ifname, MAX_PATH_LEN, "%s%d",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
502 | iface, devcnt);
|
Fixes: 38f8ab0 ("vhost: make vDPA framework bus agnostic")
Cc: stable@dpdk.org
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
In theory, lcore and queue could be so large that mbuf pool name could overflow. But that can never happen since lcore and queue will be in range. Add a check so that static tools know that. Format overflow warnings seen on older versions of Gcc only. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
There were several hidden bugs in examples because format truncation warning was disabled. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
After removing callback checks from ethdev fast path, some drivers crashed because tx_pkt_prepare was set to NULL. Some drivers (hns3, ngbe, txgbe) need to use rte_eth_tx_pkt_prepare_dummy when configuring queues. Other drivers (ntnic, softnic) does not need to set tx_pkt_prepare as it was set by eth_dev_set_dummy_fops() called by rte_eth_dev_allocate(). Bugzilla ID: 1834 Fixes: 066f3d9 ("ethdev: remove callback checks from fast path") Reported-by: Jiawen Wu <jiawenwu@trustnetic.com> Signed-off-by: Thomas Monjalon <thomas@monjalon.net> Reviewed-by: Morten Brørup <mb@smartsharesystems.com> Tested-by: Jiawen Wu <jiawenwu@trustnetic.com>
The Pollara 400 AI NIC is supported without any PMD changes. Add a link to the product brief. Cc: stable@dpdk.org Signed-off-by: Andrew Boyer <andrew.boyer@amd.com>
changes: 1. fix spelling from `SIENNA` to `SIENA` 2. add tuning guide for zen5 AMD Epyc 9005 3. add uncore power details 4. update on using AMD Solarflare X4 and X2 Signed-off-by: Vipin Varghese <vipin.varghese@amd.com> Tested-by: Thiyagarajan P <thiyagarajan.p@amd.com>
Updated some external links and removed a note which does not apply to MSVC builds anymore. Also reworded a sentence clarifying how to open a Visual Studio Developer Command Prompt from CMD and Powershell, as the original text could induce people to run VsDevCmd.bat from a Powershell prompt, which does not work. Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
The note about the prefix of the package pyelftools was not showed in the documentation output because the syntax was missing a colon, so it was considered as a simple comment in the source file. Fixes: 9e6f75e ("doc: update build section of FreeBSD guide") Cc: stable@dpdk.org Signed-off-by: Thomas Monjalon <thomas@monjalon.net> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
The version python used in current FreeBSD stable release 14.3 is now 3.11. Update documentation accordingly. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
KNI was removed from DPDK but the diagrams in the documentation were left behind. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Acked-by: Chengwen Feng <fengchengwen@huawei.com> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
This allows building and installing only the documentation, without recompiling the whole project, using: meson build -Denable_docs=true meson compile -C build doc meson install -C build --no-rebuild --tags doc In Debian/Ubuntu the documentation is built separately from the binaries, in a separate architecture-independent build job, so that it has to be done only once, instead of once per supported architecture. Signed-off-by: Luca Boccassi <bluca@debian.org> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Add tested platforms with NVIDIA NICs to the 25.11 release notes. Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
Add tested Intel platforms with Intel NICs to v25.11 release note. Signed-off-by: Yu Jiang <yux.jiang@intel.com>
Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Implement the vlan_offload_set device operation to allow enabling or disabling VLAN offloads through the API. rte_eth_dev_set_vlan_offload() calls the driver to update the hardware configuration. Because this driver performs VLAN stripping entirely in software, only a dummy implementation is needed to avoid returning an error. Validation of unsupported or invalid flags is already handled in ethdev. Fixes: 9d7757d ("net/nbl: simulate VLAN offload") Signed-off-by: Dimon Zhao <dimon.zhao@nebula-matrix.com>
Queue statistics are being continuously updated in Rx/Tx burst routines while handling traffic. In addition to that, statistics can be reset (written with zeroes) on statistics reset in other threads, causing a race condition, which in turn could result in wrong stats. The patch provides an approach with reference values, allowing the actual counters to be writable within Rx/Tx burst threads only, and updating reference values on stats reset. Fixes: 661c0cc ("net/nbl: support statistics") Signed-off-by: Dimon Zhao <dimon.zhao@nebula-matrix.com>
Fix grammar, spelling and formatting of DPDK 25.11 release notes. Signed-off-by: John McNamara <john.mcnamara@intel.com>
If the packet is ether+ipv6+vxlan+ether+ipv4+udp, the parse_l4_proto() function will treat the inner IPv4 header as an IPv6 header. As a result, it cannot identify the inner L4 protocol type, leading to the failure of the inner UDP checksum offload. This commit changes the handling of the inner layer to match that of the outer layer, allowing for the reuse of subsequent processes. It also fixes the issue where the L4 could not be identified in the case of an ether+ipv4+vxlan+ether+ipv6+udp encapsulation. Bugzilla ID: 1813 Fixes: 4961596 ("app/testpmd: fix L4 protocol retrieval from L3 header") Fixes: 76730c7 ("app/testpmd: use packet type parsing API") Cc: stable@dpdk.org Signed-off-by: Chengwen Feng <fengchengwen@huawei.com> Tested-by: Shani Peretz <shperetz@nvidia.com> Tested-by: Mingjin Ye <mingjinx.ye@intel.com>
1001s Running binary with argv[]:'/tmp/autopkgtest.LvdXhc/build.6vT/src/build/app/dpdk-test' '--file-prefix=debug' '-l' '0' '--no-huge' '-m' '2048' <...> 1001s EAL: Cannot use IOVA as 'PA' since physical addresses are not available This is similar to the commit 6b07762 ("test: force IOVA mode on PPC64 without huge pages") on ppc64 iova-mode=pa is the default, but it doesn't work with --no-huge, so add --iova-mode=va to the test arguments. Fixes: 2b403dd ("test/debug: fix crash with mlx5 devices") Cc: stable@dpdk.org Signed-off-by: Luca Boccassi <bluca@debian.org> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
With latest glibc (Fedora Rawhide):
drivers/bus/fslmc/fslmc_bus.c: In function ‘rte_fslmc_parse’:
drivers/bus/fslmc/fslmc_bus.c:298:15: error:
assignment discards ‘const’ qualifier from pointer target type
298 | t_ptr = strchr(sep, '.');
| ^
Fixes: e67a616 ("bus/fslmc: support device iteration")
Cc: stable@dpdk.org
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
Acked-by: David Marchand <david.marchand@redhat.com>
Use device configured Rx offloads instead of hardcoded values while draining work. Fixes: e8594de ("event/cnxk: implement event port quiesce function") Cc: stable@dpdk.org Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com> Acked-by: Jerin Jacob <jerinj@marvell.com>
For different devices, the mailbox flow between software and firmware is different. There are several mailbox commands in the txgbe driver, but only txgbe_hic_sr_read() was changed to use the new flow. It leads to other mailbox commands timeout for Amber-Lite devices, which is required to use the new flow. So this patch fills in the missing part. For the sake of code tidy, txgbe_hic_sr_read() should change to use the locked function txgbe_host_interface_command(), and this function could be change to pointer in struct txgbe_mbx_info for different devices. Fixes: 6a139ad ("net/txgbe: add new SW-FW mailbox interface") Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
This reverts commit d21c2fe. While fixing an issue with enabling Rx timestamping on PFs which don't report Rx timestamping, the offending commit broke PFs which do support Rx timestamping. Fixes: d21c2fe ("net/iavf: fix check for PF Rx timestamp support") Suggested-by: Jacob Keller <jacob.e.keller@intel.com> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> Acked-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
The checkpatches.sh script was checking if stdin is a terminal before honoring the -n flag, causing it to incorrectly attempt to read patches from stdin when run without a TTY (e.g., in Jenkins/CI pipelines). Reorder the conditionals to check for the -n and -r flags before checking stdin state. This ensures the -n/-r flags takes precedence and the script checks git commits as intended. Signed-off-by: Ali Alnubani <alialnu@nvidia.com> Signed-off-by: Thomas Monjalon <thomas@monjalon.net> Signed-off-by: 0-day Robot <robot@bytheb.org>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
WalkthroughThis pull request updates version to 25.11.0-rc4, adds maintainer file pattern for BPF testing, refactors example application burst parameter handling, updates driver implementations across multiple vendors (MLX5, CNXK, TXGBE, NBL, and others), clarifies MLX5 documentation on flow API behavior, and adds platform/NIC support details to release notes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
examples/l3fwd/l3fwd_em.c (1)
643-648: Samerx_burst_sizebound concern as in l3fwd_lpmThis loop now also uses
rx_burst_sizewith aMAX_PKT_BURST-sized array; the same requirement thatrx_burst_sizenever exceedMAX_PKT_BURSTapplies here.See the comment on
l3fwd_lpm.c(Line 204–209) about clampingrx_burst_size; the resolution there should be applied consistently to this path as well.examples/l3fwd/l3fwd_fib.c (1)
238-243:rx_burst_sizeusage mirrors other loops; same bounding requirement appliesAs with the LPM/EM paths, this FIB loop now uses
rx_burst_sizefor RX bursts with aMAX_PKT_BURST-sized buffer.Please ensure whatever guard you add for
rx_burst_size(see comment inl3fwd_lpm.cLines 204–209) also protects this loop from values larger thanMAX_PKT_BURST.examples/l3fwd/l3fwd_acl.c (1)
1136-1140: ACL RX loop also depends onrx_burst_sizebeing boundedThis ACL main loop now uses
rx_burst_sizewhen filling aMAX_PKT_BURST‑sizedpkts_burstarray, so it shares the same constraints as the other l3fwd variants.After you ensure
rx_burst_sizeis clamped appropriately (seel3fwd_lpm.cLines 204–209), confirm this loop can’t request more packets thanMAX_PKT_BURST.
🧹 Nitpick comments (11)
examples/server_node_efd/shared/common.h (1)
58-66: Update comment to match new buffer sizing logicThe buffer now uses
RTE_RING_NAMESIZE, but the preceding comment still says the size is calculated from the%uexpansion and extra byte. That’s misleading after this change.I’d suggest rewriting the whole block to describe the new contract instead of layering an extra line on top of outdated text:
- /* - * Buffer for return value. Size calculated by %u being replaced - * by maximum 3 digits (plus an extra byte for safety) - * Used as ring name, so upper limit is ring name size. - */ - static char buffer[RTE_RING_NAMESIZE]; + /* + * Buffer for return value used as ring name. + * Size is limited to RTE_RING_NAMESIZE (maximum ring name length). + */ + static char buffer[RTE_RING_NAMESIZE];Functionally the change to
RTE_RING_NAMESIZElooks good; this is just to keep the documentation accurate.drivers/common/mlx5/mlx5_prm.h (1)
1600-1602: Consider type safety implications of enum to macro conversion.Converting from an enum to macro defines removes compile-time type checking. While this is a common pattern for PRM definitions and the values remain identical, consider whether type safety is beneficial for this steering format identifier.
examples/l3fwd/l3fwd.h (1)
115-115: Guard rx_burst_size against exceeding MAX_PKT_BURST
send_single_packet()now usesrx_burst_sizeboth as the flush threshold and as thenpassed tosend_burst, whilem_tableis still sizedMAX_PKT_BURST. Ifrx_burst_sizecan be configured larger thanMAX_PKT_BURST, this would lead to out-of-bounds writes and an oversized TX burst.If this is not already enforced in the CLI / initialization logic, please either:
- Clamp
rx_burst_sizetoMAX_PKT_BURST(or a smaller safe limit) when parsing options, and/or- Add a defensive check or
RTE_ASSERT(rx_burst_size <= MAX_PKT_BURST)near the declaration or here.This keeps the new flexibility without risking buffer overruns on misconfiguration.
Also applies to: 151-153
doc/guides/windows_gsg/build_dpdk.rst (1)
28-33: Docs update reads well; optionally switch Meson URL to HTTPSThe Windows build instructions and updated LLVM/MinGW guidance look good, and the new note about the Developer Command Prompt / PowerShell is clear.
For consistency with the other HTTPS links you’ve introduced, you might optionally update the Meson MSI installer URL to use
https://mesonbuild.com/...instead ofhttp://.Also applies to: 48-52, 65-70, 134-147
examples/l3fwd/l3fwd_lpm.c (1)
204-209: Ensurerx_burst_sizeis always ≤MAX_PKT_BURSTSwitching to
rx_burst_sizekeeps semantics flexible, butpkts_burstis statically sized toMAX_PKT_BURST. Ifrx_burst_sizecan exceed that (e.g., from CLI),rte_eth_rx_burst()could overrun the array.Please confirm that
rx_burst_sizeis centrally clamped toMAX_PKT_BURST(or smaller) in the common config/CLI parsing. If not, either clamp it there or cap the argument here to avoid potential buffer overflow.examples/l3fwd-power/main.c (3)
1504-1504: Enhance usage text with expected format and value range.The help text for
--eth-link-speedis minimal. Consider adding details about the expected input format (e.g., speed in Mbps) and mentioning which speeds are unsupported (10M/100M are rejected in the parsing logic).Apply this diff to improve the help text:
- " --eth-link-speed: force link speed\n" + " --eth-link-speed SPEED: force link speed in Mbps\n" + " Unsupported: 10M, 100M\n"
1746-1746: Consider consistent naming convention for option macros.The macro
CMD_LINK_OPT_ETH_LINK_SPEEDuses a different prefix (CMD_LINK_OPT) compared to other command-line options which useCMD_LINE_OPT(e.g.,CMD_LINE_OPT_MAX_PKT_LEN). For consistency, consider renaming toCMD_LINE_OPT_ETH_LINK_SPEED.
1753-1753: Consider narrowing variable scope.The variable
speed_numis only used within theCMD_LINK_OPT_ETH_LINK_SPEED_NUMcase block (lines 1958-1970). Declaring it at the top of the function increases its scope unnecessarily. Consider declaring it within the case block where it's used.Apply this diff:
int opt, ret; - int speed_num; char **argvopt; int option_index;And declare it in the case block:
if (!strncmp(lgopts[option_index].name, CMD_LINK_OPT_ETH_LINK_SPEED, sizeof(CMD_LINK_OPT_ETH_LINK_SPEED))) { + int speed_num; speed_num = atoi(optarg);examples/l3fwd/main.c (2)
428-428: Enhance usage text with expected format and value range.Similar to the l3fwd-power implementation, the help text for
--eth-link-speedis minimal. Consider adding details about the expected input format and mentioning unsupported speeds (10M/100M are rejected in the parsing logic at lines 932-937).Apply this diff:
- " --eth-link-speed: force link speed\n" + " --eth-link-speed SPEED: force link speed in Mbps\n" + " Unsupported: 10M, 100M\n"
881-881: Consider narrowing variable scope.The variable
speed_numis only used within theCMD_LINK_OPT_ETH_LINK_SPEED_NUMcase block (lines 930-940). Declaring it at the top of the function increases its scope unnecessarily. Consider declaring it within the case block where it's used.drivers/net/cnxk/cn20k_rx.h (1)
846-852: Vector RX: zero-initialized meta/LMT locals; consider also initializingd_offInitializing
lbase,laddr,buf_sz, andlmt_idto 0 incn20k_nix_recv_pkts_vector()is good hygiene and matches the CN10K vector path.d_offis always assigned inside theNIX_RX_OFFLOAD_SECURITY_Fbranch before use, so behavior is correct, but giving it an explicit 0 default would fully eliminate any static “may be uninitialized” noise.- uint64_t lbase = 0, laddr = 0, buf_sz = 0; - uint8_t loff = 0, lnum = 0, shft = 0; - uint16_t lmt_id = 0, d_off; + uint64_t lbase = 0, laddr = 0, buf_sz = 0; + uint8_t loff = 0, lnum = 0, shft = 0; + uint16_t lmt_id = 0, d_off = 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
doc/guides/prog_guide/img/kni_traffic_flow.pngis excluded by!**/*.pngdoc/guides/prog_guide/img/pkt_flow_kni.pngis excluded by!**/*.png
📒 Files selected for processing (60)
MAINTAINERS(1 hunks)VERSION(1 hunks)app/test-pmd/csumonly.c(1 hunks)app/test/test_debug.c(2 hunks)devtools/checkpatches.sh(2 hunks)doc/api/meson.build(3 hunks)doc/guides/cryptodevs/ionic.rst(2 hunks)doc/guides/freebsd_gsg/build_dpdk.rst(1 hunks)doc/guides/linux_gsg/amd_platform.rst(2 hunks)doc/guides/meson.build(1 hunks)doc/guides/nics/i40e.rst(2 hunks)doc/guides/nics/ice.rst(2 hunks)doc/guides/nics/ionic.rst(2 hunks)doc/guides/nics/mlx5.rst(11 hunks)doc/guides/rel_notes/release_25_11.rst(6 hunks)doc/guides/sample_app_ug/l3_forward.rst(2 hunks)doc/guides/windows_gsg/build_dpdk.rst(4 hunks)drivers/bus/fslmc/fslmc_bus.c(1 hunks)drivers/common/mlx5/mlx5_devx_cmds.c(2 hunks)drivers/common/mlx5/mlx5_devx_cmds.h(2 hunks)drivers/common/mlx5/mlx5_prm.h(4 hunks)drivers/event/cnxk/cn10k_eventdev.c(3 hunks)drivers/event/cnxk/cn10k_worker.h(1 hunks)drivers/event/cnxk/cn20k_eventdev.c(3 hunks)drivers/event/cnxk/cn20k_worker.h(1 hunks)drivers/net/cnxk/cn10k_rx.h(2 hunks)drivers/net/cnxk/cn20k_rx.h(2 hunks)drivers/net/hns3/hns3_rxtx.c(2 hunks)drivers/net/intel/iavf/iavf_ethdev.c(1 hunks)drivers/net/mlx5/mlx5.c(2 hunks)drivers/net/mlx5/mlx5_flow_flex.c(2 hunks)drivers/net/nbl/nbl_dev/nbl_dev.c(1 hunks)drivers/net/nbl/nbl_dev/nbl_dev.h(1 hunks)drivers/net/nbl/nbl_ethdev.c(1 hunks)drivers/net/nbl/nbl_hw/nbl_resource.h(2 hunks)drivers/net/nbl/nbl_hw/nbl_txrx.c(3 hunks)drivers/net/ngbe/ngbe_rxtx.c(1 hunks)drivers/net/ntnic/ntnic_ethdev.c(0 hunks)drivers/net/softnic/rte_eth_softnic.c(0 hunks)drivers/net/txgbe/base/txgbe_aml.c(2 hunks)drivers/net/txgbe/base/txgbe_aml40.c(2 hunks)drivers/net/txgbe/base/txgbe_eeprom.c(8 hunks)drivers/net/txgbe/base/txgbe_hw.c(2 hunks)drivers/net/txgbe/base/txgbe_mng.c(11 hunks)drivers/net/txgbe/base/txgbe_mng.h(1 hunks)drivers/net/txgbe/base/txgbe_type.h(1 hunks)drivers/net/txgbe/txgbe_rxtx.c(1 hunks)examples/ip_reassembly/main.c(1 hunks)examples/l3fwd-power/main.c(4 hunks)examples/l3fwd/l3fwd.h(2 hunks)examples/l3fwd/l3fwd_acl.c(1 hunks)examples/l3fwd/l3fwd_common.h(2 hunks)examples/l3fwd/l3fwd_em.c(1 hunks)examples/l3fwd/l3fwd_fib.c(1 hunks)examples/l3fwd/l3fwd_lpm.c(1 hunks)examples/l3fwd/main.c(15 hunks)examples/meson.build(1 hunks)examples/server_node_efd/efd_server/main.c(1 hunks)examples/server_node_efd/shared/common.h(1 hunks)examples/vdpa/main.c(3 hunks)
💤 Files with no reviewable changes (2)
- drivers/net/softnic/rte_eth_softnic.c
- drivers/net/ntnic/ntnic_ethdev.c
🧰 Additional context used
🧬 Code graph analysis (6)
drivers/net/txgbe/base/txgbe_hw.c (1)
drivers/net/txgbe/base/txgbe_mng.c (1)
txgbe_host_interface_command_sp(99-161)
drivers/net/txgbe/base/txgbe_aml.c (1)
drivers/net/txgbe/base/txgbe_mng.c (1)
txgbe_host_interface_command_aml(163-272)
drivers/net/txgbe/base/txgbe_mng.h (1)
drivers/net/txgbe/base/txgbe_mng.c (2)
txgbe_host_interface_command_sp(99-161)txgbe_host_interface_command_aml(163-272)
drivers/net/nbl/nbl_dev/nbl_dev.h (1)
drivers/net/nbl/nbl_dev/nbl_dev.c (1)
nbl_vlan_offload_set(761-768)
app/test-pmd/csumonly.c (1)
lib/mbuf/rte_mbuf.h (1)
rte_pktmbuf_read(1837-1844)
drivers/net/nbl/nbl_ethdev.c (1)
drivers/net/nbl/nbl_dev/nbl_dev.c (1)
nbl_vlan_offload_set(761-768)
🔇 Additional comments (77)
doc/guides/nics/i40e.rst (1)
102-108: Documentation update for DPDK 25.11 looks good.The new entries are properly formatted, correctly positioned chronologically, and align with the existing table structure. Both X710/XL710/XXV710 and X722 sections have been consistently updated with the DPDK 25.11 kernel driver and firmware versions.
Please confirm that kernel driver version 2.28.13 and the firmware versions (9.55 for X710/XL710/XXV710 and 6.51 for X722) are accurate for DPDK 25.11.0-rc4.
Also applies to: 135-141
drivers/bus/fslmc/fslmc_bus.c (1)
241-311: Const‑correctness fort_ptris appropriate and behavior‑preservingChanging
t_ptrtoconst char *matches its read‑only use (strchr,sscanfoffset) and avoids implying writable storage; no call sites or operations require a non‑const pointer here, so this is a safe, behavior‑neutral cleanup.drivers/net/nbl/nbl_ethdev.c (1)
48-48: LGTM: VLAN offload callback properly registered.The addition of the
.vlan_offload_setcallback to the device operations structure is correct. The implementation innbl_dev.cappropriately handles VLAN offload configuration as a no-op since VLAN processing is done in software based onrxmode.offloadflags.drivers/net/nbl/nbl_dev/nbl_dev.h (1)
98-98: LGTM: Function prototype correctly declared.The function prototype is properly defined with appropriate
__rte_unusedannotations that match the implementation. The signature is consistent with DPDK ethdev API requirements for VLAN offload callbacks.drivers/net/nbl/nbl_dev/nbl_dev.c (1)
761-768: LGTM: No-op implementation is appropriate for software-based VLAN handling.The implementation correctly returns success without hardware configuration, as documented in the comment. This is consistent with the driver's design where VLAN capabilities are advertised in
nbl_dev_infos_get(lines 420-423), but the actual processing is performed in software by checkingrxmode.offloadflags in the receive path. This pattern is standard for DPDK PMDs that handle offloads in software.examples/ip_reassembly/main.c (1)
887-892: I'll verify the concerns raised in this review comment by examining the actual code and checking the related constants and patterns.
<function_calls>
#!/bin/bashRead the specific lines in question (887-892) and surrounding context
sed -n '880,900p' examples/ip_reassembly/main.c | cat -n
</function_calls>Now let me verify the array declaration and loop mentioned:
<function_calls>
#!/bin/bashCheck the lcore_queue_conf array declaration around line 159
sed -n '155,165p' examples/ip_reassembly/main.c | cat -n
</function_calls>Let me also check the loop at line 920:
<function_calls>
#!/bin/bashCheck the loop around line 920
sed -n '915,925p' examples/ip_reassembly/main.c | cat -n
</function_calls>Now let me search for RTE_MAX_QUEUES_PER_PORT definition and usage:
<function_calls>
#!/bin/bashSearch for RTE_MAX_QUEUES_PER_PORT in the codebase
rg -n "RTE_MAX_QUEUES_PER_PORT" --type c --type h -C2
</function_calls>Let me check other RTE_LOG calls in the file to verify the newline pattern:
<function_calls>
#!/bin/bashCheck RTE_LOG calls at lines 780, 804, 865, 900
sed -n '778,782p' examples/ip_reassembly/main.c | cat -n
echo "---"
sed -n '802,806p' examples/ip_reassembly/main.c | cat -n
echo "---"
sed -n '863,867p' examples/ip_reassembly/main.c | cat -n
echo "---"
sed -n '898,902p' examples/ip_reassembly/main.c | cat -n
</function_calls>Let me also check what constraints exist for the queue parameter:
<function_calls>
#!/bin/bashSearch for MAX_RX_QUEUE and MAX_TX_QUEUE constants mentioned in observations
rg -n "MAX_RX_QUEUE|MAX_TX_QUEUE" examples/ip_reassembly/main.c
</function_calls>Let me check how the queue parameter is used after this validation:
<function_calls>
#!/bin/bashRead lines around 894 where the pool name is created
sed -n '890,910p' examples/ip_reassembly/main.c | cat -n
</function_calls>Let me search for where queue is actually constrained in the function:
<function_calls>
#!/bin/bashFind the setup_queue_tbl function to understand the full context
rg -n "setup_queue_tbl" examples/ip_reassembly/main.c -A 100 | head -120
</function_calls>Let me get a better view of the function signature and the beginning:
<function_calls>
#!/bin/bashFind function start - looking for setup_queue_tbl
sed -n '850,900p' examples/ip_reassembly/main.c | cat -n
</function_calls>Now let me verify if there's documentation about these DPDK constants:
<function_calls>
RTE_MAX_QUEUES_PER_PORT DPDK constant definition
</function_calls>drivers/net/intel/iavf/iavf_ethdev.c (1)
1180-1181: The code change is intentional and correct; no issue exists.The simplified capability check at line 1180 was restored after being temporarily reverted. Here's what happened:
Commit d21c2fe (Jacob Keller, Nov 13): Added the stricter check requiring both
VIRTCHNL_VF_CAP_PTPandvf->ptp_caps & VIRTCHNL_1588_PTP_CAP_RX_TSTAMPto prevent advertising RX timestamp capability on PFs that don't support it.Commit 07f93e7 (Bruce Richardson, Nov 26): Reverted the stricter check because it broke PFs that DO support RX timestamping. The issue was that
vf->ptp_capsmight not be properly initialized whendev_info_get()is called early in the initialization sequence.The current simplified check is safe because:
- Most PFs supporting
VIRTCHNL_VF_CAP_PTPdo support RX timestamping- The actual queue setup at
iavf_vchnl.c:1276-1278still performs a triple verification before enabling RX timestamps (requires user request + both capability flags)- This defensive check at queue setup prevents runtime failures even if capability is overly advertised
drivers/net/hns3/hns3_rxtx.c (2)
4547-4565: Non-started/reset path now uses dummy tx-prep, which is appropriateUsing
rte_eth_tx_pkt_prepare_dummywhen the device is not started (or resetting) keepstx_pkt_preparenon-NULL while still being a no-op. This is consistent with the started path (which installs the real prep function) and removes the need for external NULL checks without changing datapath behavior.
4917-4921: Stopping TX datapath consistently switches both burst and prep to dummyAssigning
dev->tx_pkt_prepare = rte_eth_tx_pkt_prepare_dummyalongsidetx_pkt_burst = rte_eth_pkt_burst_dummyensures callers never see a NULL prep hook while TX is stopped. This matcheshns3_set_rxtx_function’s non-started behavior and avoids subtle differences between “stopped” and “never-started” states.drivers/net/ngbe/ngbe_rxtx.c (1)
1918-1945: Simple TX path: using dummy tx-prep instead of NULL is safe and clearerIn the simple, no-offload TX path, wiring
dev->tx_pkt_preparetorte_eth_tx_pkt_prepare_dummykeeps the function pointer non-NULL without introducing extra work. Callers can uniformly calltx_pkt_prepareregardless of path, while full-featured TX still usesngbe_prep_pktswhen offloads are enabled. This is a clean, behavior-preserving improvement.drivers/net/txgbe/txgbe_rxtx.c (1)
2347-2347: LGTM: Improved safety with dummy prepare function.Assigning
rte_eth_tx_pkt_prepare_dummyinstead of NULL for the simple TX path is a good improvement. This is a standard DPDK ethdev API callback that serves as a no-op implementation, allowing the framework to call the callback unconditionally without NULL checks. The change follows the established pattern of other drivers switching from NULL to this dummy function for safer and more consistent interfaces.drivers/common/mlx5/mlx5_prm.h (3)
2118-2120: LGTM!The bit field restructuring correctly preserves the total bit count (3 bits = 1 + 1 + 1) while introducing the
header_length_field_offset_modefield. This aligns with the related changes in the DevX structures.
4990-4992: LGTM!The bit field restructuring mirrors the changes in
mlx5_ifc_parse_graph_node_cap_bitsand correctly preserves the total bit count while introducing theheader_length_field_offset_modefield.
2351-2351: Verify the reserved field expansion matches the latest MLX5 hardware PRM specification.The reserved field at offset 0 in
mlx5_ifc_flow_table_esw_cap_bitsis set to0x800(2048 bits). This change was introduced in commit ba4a55c as part of a large repository initialization. While the code accesses this structure using named fields (preserving ABI compatibility), the correctness of the0x800value depends on the current MLX5 hardware PRM specification, which cannot be verified from the repository alone.Confirm with the MLX5 hardware team or PRM documentation that
0x800is the correct reserved space size for the ESW flow table capabilities structure.drivers/common/mlx5/mlx5_devx_cmds.h (2)
652-652: LGTM!The
header_length_field_offset_modefield addition aligns with the corresponding PRM structure changes and is properly utilized inmlx5_flow_flex.cline 575.
122-122: Theheader_length_field_mode_wafield is properly initialized from HCA capabilities.The field is populated in
mlx5_devx_cmd_query_hca_parse_graph_node_cap()(drivers/common/mlx5/mlx5_devx_cmds.c) via:attr->header_length_field_mode_wa = !MLX5_GET(parse_graph_node_cap, hcattr, header_length_field_offset_mode);This extracts the capability from the HCA response and converts it to the workaround flag using inverted logic. Every instantiation of
mlx5_hca_flex_attrgoes through this query function, ensuring the field is always initialized.drivers/net/mlx5/mlx5_flow_flex.c (2)
557-570: LGTM!The addition of the
header_length_field_mode_waguard correctly gates the hardware-specific offset adjustment logic. This workaround flag conditionally applies compatibility handling for header length field offsets based on hardware capabilities.
575-575: Verify the inversion logic for offset_mode.The assignment
node->header_length_field_offset_mode = !attr->header_length_field_mode_wainverts the workaround flag. This means:
- When WA is needed (wa=1) → offset_mode=0
- When WA is not needed (wa=0) → offset_mode=1
This suggests two different hardware offset calculation modes. Please confirm this inversion is correct according to the hardware specification.
drivers/net/mlx5/mlx5.c (2)
1083-1091: Verify the header length offset calculation for legacy firmware.The conditional logic handles different firmware versions, but the offset adjustment arithmetic for legacy firmware deserves verification:
node.header_length_field_offset = 8; if (attr->header_length_mask_width < 8) node.header_length_field_offset += 8 - attr->header_length_mask_width;This means the offset increases as
header_length_mask_widthdecreases (e.g., width=6 → offset=10). Please confirm this calculation is correct and aligns with the firmware workaround requirements.
1494-1496: Verify hardcoded DV flow defaults are safe for all hardware.The code now forces
dv_flow_en = 1andallow_duplicate_pattern = 1without the previous dynamic adjustment based on hardware/software capability detection. While lines 1512-1524 still perform some capability validation, please confirm:
- All supported hardware configurations can handle
dv_flow_en = 1- The removed capability-detection helpers and warnings are no longer necessary
- This simplification won't break existing deployments with older hardware/firmware
drivers/common/mlx5/mlx5_devx_cmds.c (2)
787-788: LGTM: Header length offset mode field properly wired.The new
header_length_field_offset_modefield is correctly integrated into the flex parser creation path using the standardMLX5_SETmacro, consistent with other field assignments in this function.
916-917: LGTM: Correct workaround flag derivation.The
header_length_field_mode_waflag is correctly derived by negating the firmware capability bit. This properly indicates when the legacy firmware workaround is needed (capability absent) vs. when the new firmware behavior is available (capability present).examples/vdpa/main.c (3)
25-25: Good defensive programming.The macro correctly calculates the space needed for the stringified device count. This is used at line 40 to ensure the concatenated result at line 501-502 never overflows the destination buffer.
40-40: Excellent buffer overflow prevention.By reducing the
ifacebuffer size to reserve space for the device count digits, the code ensures that the concatenation at line 501-502 will never overflow or truncate the result. The math is sound: 123-byte buffer allows a 122-character string, which with a 4-digit device count (max 1023) and null terminator requires exactly 127 bytes, fitting safely within the 128-byte destination.
78-79: Correct fixes for both string operations.Line 78: Replacing
strncmpwithstrcmpis the right fix—usingstrncmpwithMAX_PATH_LENto compare against the short string "iface" was incorrect.Line 79: Using
sizeof(iface)instead ofMAX_PATH_LENis now correct since theifacebuffer size has been reduced to 123 bytes. This ensuresrte_strscpyrespects the actual buffer bounds.doc/guides/cryptodevs/ionic.rst (1)
2-2: Update Ionic crypto docs for 2025 and Pollara 400The extended copyright year and added Pollara 400 model entry (with link and formatting) are consistent with the existing list.
Also applies to: 19-20
MAINTAINERS (1)
1905-1912: Extend BPF maintainer scope to app/test/bpfIncluding
app/test/bpf/under the BPF section ensures BPF tests are routed to the right maintainer set.VERSION (1)
1-1: Bump VERSION to 25.11.0-rc4The version string is updated to the new RC; just ensure other version references (e.g., project version, release notes) in the series are kept in sync.
examples/meson.build (1)
82-85: Align example default_cflags with global machine_argsSetting
default_cflags = machine_argsmakes examples build with the same baseline flags as the rest of the project, without extra per-example warning suppression. This is fine as long as all examples compile cleanly with your supported compilers.examples/server_node_efd/efd_server/main.c (1)
70-88: Fix MAC cache buffer size for formatted address + newlineUsing
RTE_ETHER_ADDR_FMT_SIZE + 1ensures each cached MAC string has enough space for the formatted address, the explicit newline, and the terminating NUL, sosnprintfno longer truncates the cached value.doc/api/meson.build (1)
24-32: Tag API documentation install targets as 'doc'Adding
install_tag: 'doc'to the examples and Doxygen custom targets cleanly groups all API documentation under a common install tag, consistent with other doc targets.Please confirm that your minimum Meson version in this tree supports
install_tagoncustom_targetacross all supported build environments.Also applies to: 71-82, 86-96
doc/guides/nics/ionic.rst (1)
2-2: Refresh Ionic NIC docs for 2025 and Pollara 400The updated copyright year and added Pollara 400 entry (with matching PDF link and bullet formatting) are consistent with the rest of the Ionic NIC documentation.
Also applies to: 18-19
doc/guides/sample_app_ug/l3_forward.rst (1)
80-81: Document per-direction Rx/Tx burst size optionsThe new
--rx-burstand--tx-burstoptions are clearly integrated into the usage synopsis and described (including the default of 32), keeping the sample app guide in sync with the updated l3fwd behavior.Also applies to: 118-121
doc/guides/meson.build (1)
25-25: Doc install tagging looks goodAdding
install_tag: 'doc'tohtml_guidesis consistent with Meson tagging and won’t affect build behavior; useful for packaging docs separately.examples/l3fwd/l3fwd_common.h (1)
28-29: Configurable TX burst threshold integration looks correctExporting
rx_burst_size/tx_burst_sizeand usingtx_burst_sizein thesend_packetsx4fast path cleanly ties TX bursting to the new configurable knobs, while the rest of the function still enforcesMAX_PKT_BURSTon the buffered path.Also applies to: 77-78
app/test/test_debug.c (1)
49-49: Expanded test_args and PPC64 iova-mode handling are consistentIncreasing
test_argsto 8 and wiring slot 7 for--iova-mode=vaonRTE_ARCH_PPC_64(while keeping it""elsewhere) aligns with the existingprocess_dup(test_args, RTE_DIM(test_args), ...)usage and fixes the--no-huge+ default PA issue without impacting other platforms.Also applies to: 161-171
drivers/net/nbl/nbl_hw/nbl_resource.h (1)
144-145: Reset stats fields align with per-queue reset behaviorAdding
txq_stats_reset/rxq_stats_resetalongside the existing stats structs matches the delta-based stats approach described fornbl_txrx. Please just ensure these new fields are explicitly zeroed (or memcpy’d from current stats) in the queue setup / reset paths so that reported deltas are correct from the first read after a reset.Also applies to: 185-187
drivers/net/nbl/nbl_hw/nbl_txrx.c (2)
792-792: LGTM! Clean reset implementation.The snapshot-based reset approach correctly preserves hardware counters while allowing user-visible stats to be reset. This is the recommended pattern for statistics reset in DPDK drivers.
Also applies to: 800-800
730-732: Reset counters are correctly initialized via rte_zmalloc zero-initialization.The review comment's concern about uninitialized memory is incorrect. The
rxq_stats_resetandtxq_stats_resetfields are struct members (not separately allocated pointers) embedded innbl_res_rx_ringandnbl_res_tx_ringstructures. Both structures are allocated exclusively withrte_zmallocorrte_zmalloc_socket(lines 111 and 251 respectively), which guarantee zero-initialization. This means:
- First stats read:
current_stats - 0 = current_stats(correct initial state)- After
reset_stats()is called: reset point is updated to current stats- No explicit initialization is needed
The code correctly implements delta statistics tracking and requires no changes.
Likely an incorrect or invalid review comment.
doc/guides/rel_notes/release_25_11.rst (2)
29-29: LGTM! Clear wording improvements.The editorial changes improve clarity:
- Line 29: "Added 800G speed" is concise and direct
- Lines 36-40: Clear description of mbuf tracking feature with explicit default state
- Line 62: More precise language about driver specification
Also applies to: 36-40, 62-62
237-491: Excellent platform coverage documentation.The extensive tested platforms section provides valuable information for users about supported hardware configurations, including CPUs, operating systems, NICs, DOCA versions, and firmware details. This level of detail significantly improves the release notes quality.
doc/guides/nics/ice.rst (2)
90-91: LGTM! Version table updated correctly.The 25.11 entries are properly added to both E810 and E830 tables with the latest driver, DDP, and firmware versions. This maintains consistency with the established table format.
Also applies to: 100-101
632-876: Excellent raw pattern documentation.The new raw pattern sections provide comprehensive guidance including:
- Clear explanation of spec and mask components
- Step-by-step generation instructions
- Concrete examples with both CLI and C API usage
- Appropriate limitations and dependency notes
This significantly enhances the usability of the protocol-agnostic filtering feature.
devtools/checkpatches.sh (2)
424-425: LGTM! Improved default parameter handling.Changing from
number=0and unsetrangeto both being empty strings makes the presence-based checks in the patch selection logic more explicit and readable.
561-575: Clearer patch selection logic.The refactored logic is more explicit and maintainable:
- Uses
-n "$number"and-n "$range"for presence checks- Adds default range assignment (
origin/main..) when needed- Stdin handling moved to explicit else branch
-t 0correctly checks if stdin is a terminalThe new structure is easier to understand and less error-prone than the previous implicit numeric-based approach.
app/test-pmd/csumonly.c (3)
529-531: LGTM! Better parameter naming.Renaming
parse_innertoin_tunneland introducingalign_ptypeimproves code clarity. The new variable name more accurately describes the boolean condition (whether we're inside a tunnel).
534-546: Correct tunnel ptype alignment logic.The new mapping logic properly translates inner tunnel packet types to their corresponding outer L3 types:
INNER_L3_IPV4→L3_IPV4INNER_L3_IPV4_EXT→L3_IPV4_EXTINNER_L3_IPV6→L3_IPV6INNER_L3_IPV6_EXT→L3_IPV6_EXTThis allows the subsequent header checks to work correctly for tunneled packets by using a consistent ptype representation.
548-571: Consistent use of align_ptype for header parsing.The updated logic correctly uses
align_ptypethroughout the IPv4 and IPv6 header parsing paths. This ensures that tunneled packets are processed with the properly aligned packet type, fixing the L4 protocol extraction for inner headers.doc/guides/nics/mlx5.rst (11)
648-651: Clarifies tunnel-offload limitation with sync API + HWSThe added note that tunnel offload is unsupported for the synchronous flow API when using HW steering is clear and consistent with the surrounding
dv_xmeta_enmode description.Please double‑check against the current mlx5 HWS implementation that no sync‑HWS tunnel modes slipped in since this text was written.
689-701: Documentingdv_flow_endefault behavior looks correct but relies on driver configStating that
dv_flow_endefaults to 1 (DV flow steering) when supported and 0 otherwise makes the implicit behavior explicit and matches typical mlx5 defaults.Confirm that the actual probe/devargs logic still aligns with this description on all supported platforms, especially where rdma-core < 24 or HWS-only configurations may alter defaults.
824-837:allow_duplicate_patterndefault = 1 is useful to documentRecording that the default is 1 (allow duplicate patterns) is helpful for users debugging rule insertion semantics; the behavior description above remains accurate.
Ensure the driver’s default for
allow_duplicate_patternreally is 1 across NIC/E-Switch domains so the doc doesn’t diverge from the implementation.
1360-1372: Good to mandaterte_flow_configure()for async API, recommendation for sync is reasonableThe note clearly states that
rte_flow_configure()must be called before any async flow creation under HWS and recommends using it for the sync API as well, which matches how port/action/queue resources are managed later in the doc.Please verify that all HWS async paths in mlx5 actually enforce
rte_flow_configure()having been called, and that the sync path behaves gracefully if applications still rely on implicit defaults.
1410-1443: Sync API + HWS limitations are well scoped and worth calling outThe bullets about:
- match-field size limits vs tree-based engines,
- rule update semantics (new rule taking effect before old one in non‑zero groups),
- default
rte_flow_port_attr/ action allocations and memory usage,- and partial‑mask
specbehaviorare concrete and match known HWS characteristics. The warning about
spec & maskis especially important.Consider cross‑checking these bullets against current HWS behavior for all match engines (esp. IPv6/flex) and adding a brief version note if some limitations only apply from a given FW/rdma-core level onward.
2390-2403: Representor/transfer limitations for sync API + HWS are clearly describedThe added points that synchronous HWS:
- doesn’t support
RTE_FLOW_ITEM_TYPE_PORT_REPRESENTOR,- doesn’t support transfer rules on representor ports,
- and that proxy-port rules without explicit represented-port matching hit all ports
give important constraints for users migrating from SWS.
It may be worth validating that these three bullets still hold for the latest ConnectX‑8/BF‑3+ stacks (no new support added) and, if any exceptions exist, reflecting them here.
2593-2596: Mark-item limitation for sync API + HWS is preciseDocumenting that
RTE_FLOW_ITEM_TYPE_MARK(16‑bit) is unsupported under sync HWS and thatMETAshould be used instead is helpful and avoids silent misconfigs.Confirm that there is no partial support (e.g., specific domains or FW versions) where MARK item matching works under sync HWS; if there are, consider qualifying the statement.
2752-2773: Root-table count action requirements are important for deployment planningThe note that COUNT on root tables with HWS requires:
- Linux ≥ 6.4 and rdma-core ≥ 60.0, or
- DOCA ≥ 3.2.0
adds crucial environment constraints for users expecting per‑root‑table counters.
Please verify these minimum versions against the current upstream/NVIDIA stacks and consider explicitly stating whether both E‑Switch and NIC domains share the same requirements.
2797-2814: One-shot reporting of aged flows under sync API + HWS is a key behavioral nuanceHighlighting that aged flows are only reported once in sync‑HWS avoids surprises for applications that expect repeated age notifications.
It might be worth confirming whether this behavior is stable across FW/rdma-core versions and whether any future plans exist to align it with async behavior, then referencing that here if so.
2947-2952: Clarifying that onlyMETER_MARKis supported with HWS avoids API misuseThe statement that HWS supports
RTE_FLOW_ACTION_TYPE_METER_MARKbut notRTE_FLOW_ACTION_TYPE_METERis clear and aligns with the later emphasis on HW quota/meter object limits.Double-check examples and guides to ensure none still show plain
METERwith HWS; if they do, update them for consistency.
3816-3821: GREc_rsvd0_verlimitation on group 0 under sync API + HWS is clearly scopedThe new GRE limitations section narrowly documents that matching
c_rsvd0_veris unsupported on group 0 when using the synchronous flow API with HWS, which is the kind of subtle constraint operators need.Confirm that this limitation only affects group 0 and that non‑root groups behave as described; if HW/FW revisions change that, consider adding a brief version note.
examples/l3fwd/main.c (3)
60-62: LGTM!The per-direction burst size variables are properly declared and initialized with appropriate defaults. The split from a single
nb_pkt_per_burstto separate RX and TX burst sizes provides better configurability.
949-955: LGTM!The burst parsing calls correctly pass direction flags and variable addresses to the refactored
parse_pkt_burst()function. The separation of RX and TX burst configuration is properly implemented.
1690-1691: LGTM!The runtime log provides clear feedback about the configured burst sizes. The placement after argument parsing and before the main logic is appropriate, and the log level (INFO) is suitable for configuration information.
drivers/net/cnxk/cn10k_rx.h (2)
572-578: Defensive init ofinb_privis fine and consistentInitializing
inb_privto NULL avoids any chance of using an indeterminate pointer if the SA lookup path is skipped, and remains consistent with the laterif (inb_sa)guard around its use. No functional issues.
1465-1470: Zero-initializing LMT-related locals in vector RX improves safetySetting
lmt_id,d_off,lbase, andladdrto 0 is a clean way to avoid uninitialized values on non-security paths; they are still properly assigned before use whenNIX_RX_OFFLOAD_SECURITY_Fis set. This matches the CN20K RX style and should also keep static analyzers quiet.drivers/event/cnxk/cn10k_eventdev.c (1)
122-145: Usingdev->rx_offloadsin flush/quiesce paths keeps behavior consistentSwitching
cn10k_sso_hws_get_work_empty()to usedev->rx_offloads(instead of a hardcoded mask) ensures event flush and port-quiesce paths decode/convert RX work with the same offload configuration as the steady-state dequeue path (e.g., timestamps, reassembly, multi-seg). This reduces the risk of mismatched handling without changing control flow.Also applies to: 472-506
drivers/event/cnxk/cn20k_worker.h (1)
53-61: Vector RX: initializing meta/LMT locals removes any uninitialized-riskInitializing
meta_aura,laddr,lmt_id, andd_offto 0 incn20k_process_vwqe()is a straightforward safety improvement: they are still assigned inside theNIX_RX_OFFLOAD_SECURITY_F/non_vecpath before use, but now have deterministic values otherwise, which is friendlier to analyzers and future refactors.drivers/event/cnxk/cn10k_worker.h (1)
52-60: cn10k VWQE path locals now safely initializedSetting
meta_aura,laddr,lmt_id,d_offto 0 andmpto NULL incn10k_process_vwqe()removes any chance of reading indeterminate values on non-security paths, while the security path still assigns them before use. This mirrors the CN20K worker implementation and is a solid defensive cleanup.drivers/event/cnxk/cn20k_eventdev.c (1)
188-210: Flush/quiesce now honor per-device RX offload configurationPassing
dev->rx_offloadsintocn20k_sso_hws_get_work_empty()(instead of a fixed mask) for bothcn20k_sso_hws_flush_eventsandcn20k_sso_port_quiescemakes the clean-up paths decode RX work exactly as the runtime dequeue path does. This avoids assuming features like reassembly or multi-seg are enabled when they are not and is a clear correctness improvement.Also applies to: 520-552
drivers/net/cnxk/cn20k_rx.h (1)
307-313: Initializingfsz_w1/cq_w5clarifies control flowSetting
fsz_w1andcq_w5to 0 at declaration innix_cqe_xtract_mseg()is a harmless clarity win: both are assigned before use on the security-reassembly path, and remain unused on the non-security path. This just removes any doubt about their initial state.drivers/net/txgbe/base/txgbe_hw.c (1)
3001-3015: LGTM - Clean mailbox command path initialization.The addition of the mailbox pointer and initialization of the SP-specific host interface command function pointer follows a clear pattern and enables proper runtime dispatch of mailbox commands based on hardware type.
drivers/net/txgbe/base/txgbe_aml40.c (1)
17-35: LGTM - Consistent mailbox initialization for AML40.The AML40 variant correctly initializes the mailbox command path using the AML implementation, which appears intentional as these hardware variants share the same mailbox communication protocol.
drivers/net/txgbe/base/txgbe_aml.c (1)
16-34: LGTM - Proper AML mailbox initialization.The mailbox command path is correctly initialized for the AML variant, following the established pattern and enabling proper hardware-specific mailbox communication.
drivers/net/txgbe/base/txgbe_mng.h (1)
211-214: LGTM - Clean API extension for hardware-specific mailbox commands.The addition of SP and AML variant declarations provides a clear public API for hardware-specific mailbox communication. The signatures are consistent and properly match the function pointer interface.
drivers/net/txgbe/base/txgbe_mng.c (3)
82-101: Function rename enables hardware-specific dispatch.The rename from
txgbe_host_interface_commandtotxgbe_host_interface_command_spclearly identifies this as the SP-specific implementation. The function logic remains unchanged, which is appropriate for this refactoring.
163-272: AML variant implementation follows correct protocol.The AML-specific implementation correctly uses:
- AML mailbox registers (TXGBE_AML_MNG_MBOX_SW2FW/FW2SW)
- Atomic lock mechanism via
rte_atomic32_test_and_set(&hw->swfw_busy)- Sequence indexing for message tracking (hw->swfw_index)
- Proper polling and timeout handling
This implementation is distinct from the SP variant due to different hardware mailbox architecture.
306-307: Call sites correctly updated to use function pointer dispatch.All call sites have been consistently updated from direct function calls to use
hw->mbx.host_interface_command(...), enabling proper runtime dispatch based on hardware variant. The function signatures and error handling remain unchanged, ensuring backward compatibility.Also applies to: 380-382, 410-412, 465-468, 510-513, 567-568, 598-599, 618-620
drivers/net/txgbe/base/txgbe_type.h (1)
789-790: No initialization order issues exist—the function pointer is always assigned before use.The
host_interface_commandfunction pointer is initialized intxgbe_init_shared_code()(which dispatches totxgbe_init_ops_sp(),txgbe_init_ops_aml(), ortxgbe_init_ops_aml40()depending on hardware type) beforetxgbe_init_hw()is called. All call sites are in management functions that are invoked only after initialization completes, guaranteeing the function pointer is set. No code path can invokehw->mbx.host_interface_commandprior to the init_ops functions assigning it.
| * py311-pyelftools | ||
|
|
||
| .. note: | ||
| .. note:: | ||
|
|
||
| The specific package for pyelftools is dependent on the version of python in use, | ||
| Python 3.8 being the version at type of writing, hence the ``py38`` prefix. | ||
| The specific package for pyelftools is dependent on the version of python in use, | ||
| Python 3.11 being the version at type of writing, hence the ``py311`` prefix. | ||
|
|
||
| These can be installed using (as root):: | ||
|
|
||
| pkg install meson pkgconf py38-pyelftools | ||
| pkg install meson pkgconf py311-pyelftools | ||
|
|
There was a problem hiding this comment.
Minor wording fix in pyelftools note
In the note, “Python 3.11 being the version at type of writing” should be “version at time of writing”.
🤖 Prompt for AI Agents
In doc/guides/freebsd_gsg/build_dpdk.rst around lines 19 to 29, the note uses
the incorrect word "type" in the phrase "version at type of writing"; replace
"type" with "time" so the sentence reads "Python 3.11 being the version at time
of writing" to correct the wording.
| AMD EPYC SoC supports CPU power functions via ``rte_power`` API from `23.11 LTS` onwards. | ||
| These are tested and validated on MILAN, GENOA, BERGAMO and SIENA | ||
| using Linux kernel `6.4` and above with ``amd_pstate`` power driver. | ||
| These are tested and validated on MILAN, GENOA, BERGAMO, SIENA and TURIN | ||
| using Linux kernel `6.4` and above with ``amd_pstate`` power driver in ``passive`` mode. | ||
|
|
||
| .. note:: | ||
|
|
||
| * Power libraries are supported on Linux only. | ||
| * DPDK uncore support on Linux is work in progress. | ||
|
|
||
| Uncore | ||
| ~~~~~~ | ||
|
|
||
| AMD EPYC SoC support UNCORE power functions via ``rte_power_uncore`` from `25.03` onwards. | ||
| These are tested and validated on GENOA, SIENA and TURIN. | ||
| Please refer the tuning guides to enable ``HSMP`` and DPDK power management guide. | ||
|
|
There was a problem hiding this comment.
Clarify Linux uncore support status between note and new section
The note still says “DPDK uncore support on Linux is work in progress”, but the new Uncore section immediately below states that rte_power_uncore is available from 25.03 and “tested and validated” on specific SoCs. Consider rewording the note to match the current status (e.g., mark it as available but limited to certain platforms / still evolving) so the guidance is self-consistent.
| u16 *data) | ||
| { | ||
| const u32 mask = TXGBE_MNGSEM_SWMBX | TXGBE_MNGSEM_SWFLASH; | ||
| const u32 mask = TXGBE_MNGSEM_SWFLASH; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== SWMBX semaphore usage in entire codebase ==="
rg -n "TXGBE_MNGSEM_SWMBX" --type c -C 2
echo ""
echo "=== acquire_swfw_sync function calls with SWMBX ==="
rg -n "acquire_swfw_sync" --type c -A 3 -B 1
echo ""
echo "=== host_interface_command function implementations ==="
rg -n "host_interface_command" --type c -A 5 -B 1
echo ""
echo "=== Checking the specific file for context ==="
head -n 350 drivers/net/txgbe/base/txgbe_eeprom.c | tail -n 220Repository: ovsrobot/dpdk
Length of output: 50369
🏁 Script executed:
#!/bin/bash
echo "=== Check EEPROM functions to see what they call ==="
head -n 350 drivers/net/txgbe/base/txgbe_eeprom.c | tail -n 220
echo ""
echo "=== Check for txgbe_hic_pcie or management calls in eeprom ==="
rg -n "txgbe_hic|txgbe_mng|host_interface" drivers/net/txgbe/base/txgbe_eeprom.c -A 2 -B 2
echo ""
echo "=== Check the management interface function signature ==="
rg -n "int txgbe_hic_pcie_read" drivers/net/txgbe/base/txgbe_mng.c -A 10
echo ""
echo "=== Check if new mbx.host_interface_command is used in eeprom ==="
rg -n "host_interface_command\|mbx\.host" drivers/net/txgbe/base/txgbe_eeprom.c -B 2 -A 2Repository: ovsrobot/dpdk
Length of output: 6263
🏁 Script executed:
#!/bin/bash
echo "=== Find txgbe_hic_sr_read and txgbe_hic_sr_write definitions ==="
rg -n "txgbe_hic_sr_read|txgbe_hic_sr_write" drivers/net/txgbe/base/ -A 15 | head -n 100
echo ""
echo "=== Check if these functions are in txgbe_mng.c ==="
grep -n "txgbe_hic_sr_read\|txgbe_hic_sr_write" drivers/net/txgbe/base/txgbe_mng.c
echo ""
echo "=== Find all txgbe_hic functions ==="
rg -n "^s32 txgbe_hic_|^int txgbe_hic_" drivers/net/txgbe/base/txgbe_mng.c -A 3
echo ""
echo "=== Look at txgbe_mng.c for the management command flow ==="
head -n 200 drivers/net/txgbe/base/txgbe_mng.c | tail -n 150Repository: ovsrobot/dpdk
Length of output: 12453
🏁 Script executed:
#!/bin/bash
echo "=== Full txgbe_hic_sr_read implementation ==="
sed -n '275,323p' drivers/net/txgbe/base/txgbe_mng.c
echo ""
echo "=== Full txgbe_hic_sr_write implementation ==="
sed -n '325,375p' drivers/net/txgbe/base/txgbe_mng.c
echo ""
echo "=== Check what semaphore the old code used before this change ==="
git log --oneline --all -- drivers/net/txgbe/base/txgbe_eeprom.c | head -5
echo ""
echo "=== Check the PR context to see what changed ==="
git show HEAD:drivers/net/txgbe/base/txgbe_eeprom.c | head -n 350 | tail -n 220Repository: ovsrobot/dpdk
Length of output: 8095
🏁 Script executed:
#!/bin/bash
echo "=== Confirm host_interface_command includes SWMBX acquisition ==="
rg -n "host_interface_command" drivers/net/txgbe/base/txgbe_mng.c -B 5 -A 15 | head -n 80
echo ""
echo "=== Verify txgbe_hic_sr_write writes directly to mailbox ==="
sed -n '332,375p' drivers/net/txgbe/base/txgbe_mng.c
echo ""
echo "=== Check if there are other concurrent mailbox accesses possible ==="
rg -n "TXGBE_MNGMBX" drivers/net/txgbe/base/ --type c | head -20Repository: ovsrobot/dpdk
Length of output: 5780
🏁 Script executed:
#!/bin/bash
echo "=== Check git commit message for this change ==="
git log --format="%H %s" --all -- drivers/net/txgbe/base/txgbe_eeprom.c | head -3
echo ""
echo "=== Get the actual commit that changed this ==="
git log -p --all -S "TXGBE_MNGSEM_SWFLASH" -- drivers/net/txgbe/base/txgbe_eeprom.c | head -150
echo ""
echo "=== Check if there's a related commit for txgbe_mng.c ==="
git log --oneline --all -- drivers/net/txgbe/base/txgbe_mng.c | head -5Repository: ovsrobot/dpdk
Length of output: 750
Based on my thorough verification, I have identified a critical race condition introduced by this change. Here's what I found:
The Problem:
txgbe_hic_sr_read()(line 283) callshw->mbx.host_interface_command()which internally acquires the SWMBX semaphoretxgbe_hic_sr_write()(line 332) directly writes toTXGBE_MNGMBXregisters WITHOUT callinghost_interface_command()and WITHOUT acquiring SWMBX- EEPROM functions now only acquire
SWFLASH, notSWMBX - The old code acquired both
(SWMBX | SWFLASH)together
The Race Condition:
Multiple threads can concurrently access the same mailbox (TXGBE_MNGMBX) registers without synchronization:
- EEPROM write holds only
SWFLASHand writes directly to mailbox - Management command holds
SWMBXand also accesses mailbox - Result: Unprotected concurrent writes to the same hardware registers = data corruption
Rewritten review comment:
Critical race condition on EEPROM write operations due to asymmetric semaphore handling.
The EEPROM functions now acquire only SWFLASH semaphore instead of (SWMBX | SWFLASH). However, txgbe_hic_sr_write() directly accesses the mailbox (TXGBE_MNGMBX) registers without SWMBX protection, while txgbe_hic_sr_read() calls host_interface_command() which does acquire SWMBX. This asymmetry creates a race condition: concurrent EEPROM writes and management commands can both access the same mailbox registers without synchronization, risking mailbox state corruption.
Fix: Either restore SWMBX acquisition in EEPROM operations or have txgbe_hic_sr_write() also call host_interface_command() like txgbe_hic_sr_read() does.
🤖 Prompt for AI Agents
In drivers/net/txgbe/base/txgbe_eeprom.c around line 140, the change that only
acquires TXGBE_MNGSEM_SWFLASH creates a race because txgbe_hic_sr_read() uses
hw->mbx.host_interface_command() which takes SWMBX while txgbe_hic_sr_write()
writes TXGBE_MNGMBX directly without SWMBX; fix by restoring acquisition of both
semaphores (SWMBX | SWFLASH) for EEPROM operations or modify
txgbe_hic_sr_write() to invoke hw->mbx.host_interface_command() (or equivalent)
so SWMBX is held before touching TXGBE_MNGMBX, ensuring symmetric protection and
preventing concurrent mailbox access.
| if (!strncmp(lgopts[option_index].name, | ||
| CMD_LINK_OPT_ETH_LINK_SPEED, | ||
| sizeof(CMD_LINK_OPT_ETH_LINK_SPEED))) { | ||
| speed_num = atoi(optarg); | ||
| if ((speed_num == RTE_ETH_SPEED_NUM_10M) || | ||
| (speed_num == RTE_ETH_SPEED_NUM_100M)) { | ||
| fprintf(stderr, "Unsupported fixed speed\n"); | ||
| print_usage(prgname); | ||
| return -1; | ||
| } | ||
| if (speed_num >= 0 && rte_eth_speed_bitflag(speed_num, 0) > 0) | ||
| port_conf.link_speeds = rte_eth_speed_bitflag(speed_num, 0); | ||
| } |
There was a problem hiding this comment.
Improve input validation for eth-link-speed option.
The parsing logic has several validation issues:
-
atoi()doesn't detect parse errors:atoi()returns 0 for invalid input, which cannot be distinguished from a valid speed of 0. Usestrtol()with proper error checking instead. -
Redundant check: The condition
speed_num >= 0is always true foratoi()results since it returnsintand negative speeds are not meaningful. -
Silent failure: If
rte_eth_speed_bitflag()returns 0 (invalid speed), the code silently skips settingport_conf.link_speedswithout informing the user.
Apply this diff to add proper validation:
if (!strncmp(lgopts[option_index].name,
CMD_LINK_OPT_ETH_LINK_SPEED,
sizeof(CMD_LINK_OPT_ETH_LINK_SPEED))) {
- speed_num = atoi(optarg);
+ char *end = NULL;
+ long speed_val = strtol(optarg, &end, 10);
+
+ if (optarg[0] == '\0' || end == NULL || *end != '\0' ||
+ speed_val < 0 || speed_val > INT_MAX) {
+ fprintf(stderr, "Invalid eth-link-speed value\n");
+ print_usage(prgname);
+ return -1;
+ }
+ speed_num = (int)speed_val;
+
if ((speed_num == RTE_ETH_SPEED_NUM_10M) ||
(speed_num == RTE_ETH_SPEED_NUM_100M)) {
fprintf(stderr, "Unsupported fixed speed\n");
print_usage(prgname);
return -1;
}
- if (speed_num >= 0 && rte_eth_speed_bitflag(speed_num, 0) > 0)
+ uint32_t speed_bitflag = rte_eth_speed_bitflag(speed_num, 0);
+ if (speed_bitflag > 0) {
- port_conf.link_speeds = rte_eth_speed_bitflag(speed_num, 0);
+ port_conf.link_speeds = speed_bitflag;
+ } else {
+ fprintf(stderr, "Invalid eth-link-speed: %d\n", speed_num);
+ print_usage(prgname);
+ return -1;
+ }
}🤖 Prompt for AI Agents
In examples/l3fwd-power/main.c around lines 1958 to 1970, replace the fragile
atoi-based parsing with strtol-based parsing and robust validation: parse optarg
with strtol(base 10), check errno and that endptr points to the end of the
string to detect invalid input, reject negative or out-of-range values; keep the
existing checks for unsupported fixed speeds (RTE_ETH_SPEED_NUM_10M and
RTE_ETH_SPEED_NUM_100M) and return -1 on those, call rte_eth_speed_bitflag only
after successful parse and if it returns 0 print a user-facing error and return
-1 instead of silently continuing, and otherwise assign port_conf.link_speeds to
the returned bitflag.
| if (is_rx_burst) { | ||
| /* If user gives a value of zero, query the PMD for its recommended | ||
| * Rx burst size. | ||
| */ | ||
| ret = rte_eth_dev_info_get(0, &dev_info); | ||
| if (ret != 0) | ||
| return; | ||
| burst_size = dev_info.default_rxportconf.burst_size; | ||
| if (burst_size == 0) { | ||
| RTE_LOG(INFO, L3FWD, "PMD does not recommend a burst size. Using default value %d. " | ||
| "User provided value must be in [1, %d]\n", | ||
| rx_burst_size, MAX_PKT_BURST); | ||
| return; | ||
| } else if (burst_size > MAX_PKT_BURST) { | ||
| RTE_LOG(INFO, L3FWD, "PMD recommended burst size %d exceeds maximum value %d. " | ||
| "Using default value %d\n", | ||
| burst_size, MAX_PKT_BURST, rx_burst_size); | ||
| return; | ||
| } | ||
| *burst_sz = burst_size; | ||
| RTE_LOG(INFO, L3FWD, "Using PMD-provided RX burst value %d\n", burst_size); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find where parse_pkt_burst is called and understand the context
rg -n "parse_pkt_burst" examples/l3fwd/main.c -A 3 -B 3Repository: ovsrobot/dpdk
Length of output: 532
🏁 Script executed:
#!/bin/bash
# Also check for port initialization and enabled_port_mask usage
rg -n "enabled_port_mask|check_port|init.*port" examples/l3fwd/main.c | head -30Repository: ovsrobot/dpdk
Length of output: 1045
🏁 Script executed:
#!/bin/bash
# Get the overall structure - check function definitions and main flow
rg -n "^(int main|void parse_pkt_burst|static void|void.*port)" examples/l3fwd/main.c | head -40Repository: ovsrobot/dpdk
Length of output: 285
🏁 Script executed:
#!/bin/bash
# Read the full parse_pkt_burst function
sed -n '702,750p' examples/l3fwd/main.cRepository: ovsrobot/dpdk
Length of output: 1518
🏁 Script executed:
#!/bin/bash
# Check the main function to see the exact execution flow
sed -n '1333,1380p' examples/l3fwd/main.cRepository: ovsrobot/dpdk
Length of output: 1320
🏁 Script executed:
#!/bin/bash
# Check if port 0 is specially handled or if enabled_port_mask is validated at start
sed -n '880,920p' examples/l3fwd/main.cRepository: ovsrobot/dpdk
Length of output: 911
🏁 Script executed:
#!/bin/bash
# Check if there's any validation before parse_pkt_burst is called
sed -n '880,960p' examples/l3fwd/main.c | grep -A 20 -B 20 "parse_pkt_burst"Repository: ovsrobot/dpdk
Length of output: 919
🏁 Script executed:
#!/bin/bash
# Search for other uses of rte_eth_dev_info_get in the file to see patterns
rg -n "rte_eth_dev_info_get" examples/l3fwd/main.c -B 2 -A 5Repository: ovsrobot/dpdk
Length of output: 859
🏁 Script executed:
#!/bin/bash
# Check the check_port_config function to see what it validates
sed -n '326,360p' examples/l3fwd/main.cRepository: ovsrobot/dpdk
Length of output: 868
Based on my verification of the codebase, I can now provide the rewritten review comment:
Hardcoded port 0 may not be appropriate for all configurations.
The RX burst size query uses hardcoded port 0 to retrieve PMD recommendations (line 729):
ret = rte_eth_dev_info_get(0, &dev_info);Unlike other rte_eth_dev_info_get() calls in the code (lines 1382, 1529) that operate on specific ports within validation loops, this query happens during argument parsing before any port validation. If port 0 is not in enabled_port_mask or doesn't exist on the board, the query may fail silently or return unrepresentative values that are then applied globally to all ports.
In multi-port setups with heterogeneous NICs or when port 0 is disabled, the RX burst recommendation from port 0 may not be optimal for other ports.
Consider one of these approaches:
- Query the first enabled port from
enabled_port_maskinstead of assuming port 0 - Defer the PMD query until port configuration time when the specific port is known
- Document that RX burst size applies globally based on port 0's capabilities and add validation that port 0 is available
🤖 Prompt for AI Agents
In examples/l3fwd/main.c around lines 725-745, the code queries PMD RX burst
size using a hardcoded port 0 which may be absent or unrepresentative; change
the logic to pick the first enabled port from enabled_port_mask (or return/error
if enabled_port_mask is empty) and call rte_eth_dev_info_get(first_enabled_port,
&dev_info) instead of using 0, then use that port's dev_info for burst_size
checks; ensure you validate ret for errors, handle the case where no ports are
enabled by emitting a clear error/usage message, and keep the existing bounds
checks and log messages but reference the chosen port where appropriate.
| ; | ||
|
|
||
| #define CMD_LINE_OPT_CONFIG "config" | ||
| #define CMD_LINK_OPT_ETH_LINK_SPEED "eth-link-speed" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use consistent naming convention for option macros.
The macro CMD_LINK_OPT_ETH_LINK_SPEED uses a different prefix (CMD_LINK_OPT) compared to all other command-line options which use CMD_LINE_OPT (lines 761-786). This inconsistency appears in both l3fwd and l3fwd-power. Rename to CMD_LINE_OPT_ETH_LINK_SPEED for consistency.
Apply this diff:
-#define CMD_LINK_OPT_ETH_LINK_SPEED "eth-link-speed"
+#define CMD_LINE_OPT_ETH_LINK_SPEED "eth-link-speed"And update the enum name at line 795:
- CMD_LINK_OPT_ETH_LINK_SPEED_NUM,
+ CMD_LINE_OPT_ETH_LINK_SPEED_NUM,And the lgopts array at line 824:
- {CMD_LINK_OPT_ETH_LINK_SPEED, 1, 0, CMD_LINK_OPT_ETH_LINK_SPEED_NUM},
+ {CMD_LINE_OPT_ETH_LINK_SPEED, 1, 0, CMD_LINE_OPT_ETH_LINK_SPEED_NUM},And the case statement at line 930:
- case CMD_LINK_OPT_ETH_LINK_SPEED_NUM:
+ case CMD_LINE_OPT_ETH_LINK_SPEED_NUM:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #define CMD_LINK_OPT_ETH_LINK_SPEED "eth-link-speed" | |
| #define CMD_LINE_OPT_ETH_LINK_SPEED "eth-link-speed" |
🤖 Prompt for AI Agents
In examples/l3fwd/main.c around lines 762, 795, 824 and 930, rename the
inconsistent macro and corresponding identifiers from
CMD_LINK_OPT_ETH_LINK_SPEED to CMD_LINE_OPT_ETH_LINK_SPEED: update the #define
at line 762, rename the enum member referenced at line 795, update the entry key
in the lgopts array at line 824 to use the new macro, and adjust the switch/case
label at line 930 to the new enum name; ensure all occurrences in this file are
changed consistently to the CMD_LINE_OPT_* naming to match other options.
| case CMD_LINK_OPT_ETH_LINK_SPEED_NUM: | ||
| speed_num = atoi(optarg); | ||
| if ((speed_num == RTE_ETH_SPEED_NUM_10M) || | ||
| (speed_num == RTE_ETH_SPEED_NUM_100M)) { | ||
| fprintf(stderr, "Unsupported fixed speed\n"); | ||
| print_usage(prgname); | ||
| return -1; | ||
| } | ||
| if (speed_num >= 0 && rte_eth_speed_bitflag(speed_num, 0) > 0) | ||
| port_conf.link_speeds = rte_eth_speed_bitflag(speed_num, 0); | ||
| break; |
There was a problem hiding this comment.
Improve input validation for eth-link-speed option.
This implementation has the same validation issues as the l3fwd-power variant:
-
atoi()doesn't detect parse errors:atoi()returns 0 for invalid input, which cannot be distinguished from a valid speed of 0. Usestrtol()with proper error checking. -
Redundant check: The condition
speed_num >= 0is always true foratoi()results. -
Silent failure: If
rte_eth_speed_bitflag()returns 0 (invalid speed), the code silently skips settingport_conf.link_speedswithout informing the user.
Apply this diff to add proper validation:
- case CMD_LINK_OPT_ETH_LINK_SPEED_NUM:
+ case CMD_LINE_OPT_ETH_LINK_SPEED_NUM: {
+ char *end = NULL;
+ long speed_val = strtol(optarg, &end, 10);
+
+ if (optarg[0] == '\0' || end == NULL || *end != '\0' ||
+ speed_val < 0 || speed_val > INT_MAX) {
+ fprintf(stderr, "Invalid eth-link-speed value\n");
+ print_usage(prgname);
+ return -1;
+ }
- speed_num = atoi(optarg);
+ int speed_num = (int)speed_val;
+
if ((speed_num == RTE_ETH_SPEED_NUM_10M) ||
(speed_num == RTE_ETH_SPEED_NUM_100M)) {
fprintf(stderr, "Unsupported fixed speed\n");
print_usage(prgname);
return -1;
}
- if (speed_num >= 0 && rte_eth_speed_bitflag(speed_num, 0) > 0)
+ uint32_t speed_bitflag = rte_eth_speed_bitflag(speed_num, 0);
+ if (speed_bitflag > 0) {
- port_conf.link_speeds = rte_eth_speed_bitflag(speed_num, 0);
+ port_conf.link_speeds = speed_bitflag;
+ } else {
+ fprintf(stderr, "Invalid eth-link-speed: %d\n", speed_num);
+ print_usage(prgname);
+ return -1;
+ }
break;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case CMD_LINK_OPT_ETH_LINK_SPEED_NUM: | |
| speed_num = atoi(optarg); | |
| if ((speed_num == RTE_ETH_SPEED_NUM_10M) || | |
| (speed_num == RTE_ETH_SPEED_NUM_100M)) { | |
| fprintf(stderr, "Unsupported fixed speed\n"); | |
| print_usage(prgname); | |
| return -1; | |
| } | |
| if (speed_num >= 0 && rte_eth_speed_bitflag(speed_num, 0) > 0) | |
| port_conf.link_speeds = rte_eth_speed_bitflag(speed_num, 0); | |
| break; | |
| case CMD_LINE_OPT_ETH_LINK_SPEED_NUM: { | |
| char *end = NULL; | |
| long speed_val = strtol(optarg, &end, 10); | |
| if (optarg[0] == '\0' || end == NULL || *end != '\0' || | |
| speed_val < 0 || speed_val > INT_MAX) { | |
| fprintf(stderr, "Invalid eth-link-speed value\n"); | |
| print_usage(prgname); | |
| return -1; | |
| } | |
| int speed_num = (int)speed_val; | |
| if ((speed_num == RTE_ETH_SPEED_NUM_10M) || | |
| (speed_num == RTE_ETH_SPEED_NUM_100M)) { | |
| fprintf(stderr, "Unsupported fixed speed\n"); | |
| print_usage(prgname); | |
| return -1; | |
| } | |
| uint32_t speed_bitflag = rte_eth_speed_bitflag(speed_num, 0); | |
| if (speed_bitflag > 0) { | |
| port_conf.link_speeds = speed_bitflag; | |
| } else { | |
| fprintf(stderr, "Invalid eth-link-speed: %d\n", speed_num); | |
| print_usage(prgname); | |
| return -1; | |
| } | |
| break; | |
| } |
🤖 Prompt for AI Agents
In examples/l3fwd/main.c around lines 930 to 940, the eth-link-speed parsing
uses atoi and has redundant/insufficient checks; replace atoi with strtol using
endptr and errno to detect parse errors (handle empty string, non-numeric chars,
overflow/underflow), validate the parsed value is within acceptable integer
range (non-negative and fits expected enum), call rte_eth_speed_bitflag and if
it returns 0 print an informative error and return -1 instead of silently
continuing, and remove the redundant speed_num >= 0 check; ensure all error
paths call print_usage(prgname) where appropriate.
NOTE: This is an auto submission for "[v2] devtools: check patch from stdin only if nothing else".
See "http://patchwork.dpdk.org/project/dpdk/list/?series=36808" for details.
Summary by CodeRabbit
Release Notes
New Features
--rx-burst,--tx-burst, and--eth-link-speedfor flexible traffic handling configuration.Documentation
Bug Fixes & Improvements
✏️ Tip: You can customize this high-level summary in your review settings.