I missed your earlier review comments, and I apologize for the oversight. Regarding the AI review, I assumed the result from the initial patchset, which passed cleanly (https://mails.dpdk.org/archives/test-report/2026-October/1060187.html), would still apply, since the code logic has not changed between versions. However, the latest run does report new issues that also need to be addressed. I will address all of the points raised and aim to send a v4 before the code freeze tomorrow.
Sorry again, Shai > -----Original Message----- > From: Stephen Hemminger <[email protected]> > Sent: Monday, October 19, 2026 7:34 PM > To: Brandes, Shai <[email protected]> > Cc: [email protected] > Subject: RE: [EXTERNAL] [PATCH v3 00/13] ENA release 2.15.0 > > CAUTION: This email originated from outside of the organization. Do not click > links or open attachments unless you can confirm the sender and know the > content is safe. > > > > On Sun, 18 Oct 2026 12:45:41 +0300 > Shai Brandes <[email protected]> wrote: > > > Hello all, > > This series upgrades HAL and fixes two PHC correctness issues, an LLQ > > feature version not communicated to the device, and a failed LLQ > > configuration zeroing the negotiated tx_max_header_size. > > Additionally it adds n-tuple Rx flow steering support through the rte_flow > API. > > Review and comments are welcome > > > > Best regards, > > Shai Brandes > > > I am disappointed that you did not address the AI identified problems in last > version. > > Review of [PATCH v3 00/13] net/ena: 2.15.0 (bundle 2204) > > Applied to main at 9a45341f18 (26.11-rc1); every commit built with - > Dwerror=true (gcc, x86-64). > > Series level > ------------ > > There is no changelog. Against the previous posting, v3 only rebases, keeps > ena_com_execute_admin_command() exported in 01, and fixes a typo in 11. > None of the earlier review comments are addressed; they are repeated below. > > Error: patch 04 still breaks the build. > > Info: the stable fixes (02, 03, 06, 10) sit behind refactors, and > 06 and 10 depend on context from 01, 05 and 09. Put the fixes first so they > backport without rework. > > > [PATCH v3 01/13] net/ena: avoid using un-needed admin response buffers > ---------------------------------------------------------------------- > > Warning: undocumented behaviour change. Besides the move, the missing- > interrupt fallback in > ena_com_wait_and_process_admin_cq_interrupts() changes from > > admin_queue->polling = true; > > to > > ena_com_set_admin_polling_mode(admin_queue->ena_dev, true); > > which also writes ENA_REGS_ADMIN_INTR_MASK to the device. Say so in the > commit message, or split the wait path rework into its own patch. > > > [PATCH v3 03/13] net/ena/base: fix missing barriers in PHC timestamp read > ------------------------------------------------------------------------ > > Warning: wrong Fixes tag. The req_id poll loop without a barrier comes from > f73f53f7dc7a (v23.11); a14be4dd8693 (v25.11) only added the statistics > block. As tagged, 23.11 and 24.11 will not pick this up. Use: > > Fixes: f73f53f7dc7a ("net/ena: upgrade HAL") > > > [PATCH v3 04/13] net/ena/base: rename bounce buffer references > ------------------------------------------------------------- > > Error: build break. The member is renamed in ena_com.h but ena_eth_com.c > still uses the old name until patch 05: > > ena_eth_com.c:160: error: 'struct ena_com_llq_pkt_ctrl' has no > member named 'curr_bounce_buf' > > Move the ena_eth_com.c renames (curr_bounce_buf, plus > write_header_to_bounce and close_bounce_buffer, which 05 also renames > without mentioning it) into this patch. 05 is then only the > ena_com_sq_is_llq() > helper, matching its subject. > > > [PATCH v3 08/13] net/ena/base: deduplicate meta caching config flag > ------------------------------------------------------------------ > > Info: removing the field leaves two consecutive blank lines in struct > ena_com_io_sq after mem_queue_type. Drop one. > > > [PATCH v3 09/13] net/ena/base: split LLQ info into datapath and config > --------------------------------------------------------------------- > > Warning: this is not "No functional change". The burst limit divisor changes: > > - llq_default_cfg->llq_ring_entry_size_value; > + llq_info->common.desc_list_entry_size; > > They differ when the device does not support the default entry size and the > fallback above picks another one. With a 128 byte default and a 256 byte > fallback, the old code allows twice as many entries per burst as the device > accepts. That is a real bug, present since the LIMIT_TX_BURST support went in. > Split it out: > > net/ena/base: fix Tx burst limit with fallback entry size > > Fixes: b68309be44c0 ("net/ena/base: update communication layer > for the ENAv2") > Cc: [email protected] > > and keep 09 a pure refactor. > > Info: continuation lines of the descs_num_before_header assignments are not > indented: > > llq_info->common.descs_num_before_header = > llq_default_cfg->llq_num_decs_before_header; > > Indent the second line one more tab, or keep it on one line. > > > [PATCH v3 11/13] net/ena: support flow API for n-tuple Rx steering > ----------------------------------------------------------------- > > Warning: an item without spec is rejected for IPV4, IPV6, TCP and UDP ("IPv4 > item requires spec"). In rte_flow a NULL spec means "match any packet of this > protocol". testpmd passes spec == NULL for items given without fields, so the > obvious rule > > flow create 0 ingress pattern eth / ipv4 / tcp dst is 80 / end > actions queue index 1 / end > > fails. Treat NULL spec as protocol presence with zero masks, e.g.: > > case RTE_FLOW_ITEM_TYPE_IPV4: > ... > has_ipv4 = true; > if (spec == NULL) > break; > if (mask == NULL) > mask = &rte_flow_item_ipv4_mask; > > Same for IPV6, TCP and UDP. > > Warning: secondary process. All other admin commands (MTU, RSS, > stats) go through ENA_PROXY because only the primary owns the admin > queue. The flow ops call ena_com_flow_steering_*() directly, so rte_flow from > a secondary submits admin commands from the wrong process. Proxy them, > or simply refuse in ena_flow_ops_get(): > > if (rte_eal_process_type() != RTE_PROC_PRIMARY) > return -E_RTE_SECONDARY; > > Warning: features/ena.ini adds the [rte_flow items] and [rte_flow actions] > tables but not "Flow API = Y" under [Features]. > > Info: > - ena_infos_get(): the two "&= ~" lines are no-ops (ENA never sets > RTE_ETH_DEV_CAPA_FLOW_RULE_KEEP or > RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE) > and dev_infos_get should not write dev->data. Drop them and the > comments. > - ena_flow_create() parses the pattern twice, once inside > ena_flow_validate(). Split the attr/started checks into a helper > and call ena_flow_parse() once. > - adapter->flow_rule_count duplicates > ena_dev->flow_steering.active_rules_cnt. Use the base counter. > - ena_flow_ops_get() logs WARNING when steering is unsupported. > testpmd calls rte_flow_flush() on port close, so this fires on > every close. Use DEBUG or no log. > - ena_flow_query() only returns ENOTSUP; leave .query NULL and > ethdev returns ENOSYS. > - ena_com_flow_steering_remove_rule() and _remove_all_rules() pass > a response buffer they never read. Use > ena_com_execute_admin_command_no_resp() from patch 01. > - ena_com_flow_steering_destroy() leaves dangling pointers. End it > with memset(flow_steering, 0, sizeof(*flow_steering)) like > ena_com_rss_destroy(). > > > [PATCH v3 12/13] net/ena/base: remove x86 Tx LLQ write barrier > ------------------------------------------------------------- > > Warning: arm_wmb() is a no-op on every non-ARM architecture, not just x86. > ppc64le, riscv and loongarch are weakly ordered and the driver still builds > there. The argument for dropping the barrier (UC doorbell store flushes WC > buffers) is x86 specific, so make the test x86 specific: > > #if defined(RTE_ARCH_X86) > #define ena_llq_wmb() do { } while (0) > #else > #define ena_llq_wmb() rte_wmb() > #endif > > (rte_io_wmb() everywhere is the other option, if Graviton testing shows dmb > oshst is enough; on x86 it is a compiler barrier.) > > Info: the new comment in ena_com_write_bounce_buffer_to_dev() is > 20 lines, half of it history of the old wmb(). History belongs in the commit > message; keep the comment to what the barrier orders and why x86 does not > need it.
