Dear Stephen,
If I may, I should like to point out the following:
- Patch 11/14:
The classification of the issue as an 'error' does not hold water. First of
all, no real operability issue is observed in practice; hence, this warrants,
at most, the status of a warning, not an error. Secondly, the 'count' and
'stride' are fields of the firmware's own response. A successful MCDI response
is self-consistent and shall not be treated as adversarial. Furthermore, the
MCDI layer explicitly clamps 'emr_out_length_used' to 'emr_out_length', the
allocated output buffer size, so a buffer overrun should not be possible. The
note thus does not meet the threshold of an actual defect.
- Patch 13/14:
The default of 'TECH_AUTO' when 'flags_seen == 0' is deliberate: it is the
correct instruction to the firmware when the capability map yields no
technology preference, and 'TECH_NONE' would be semantically incorrect in a
fixed-link context. The commit message describes the refactoring; exhaustive
documentation of an edge-case path does not belong in such changes. Therefore,
the review note does not meet the threshold of an actual defect.
- Patches 01–04:
The comment on 'EFX_MCDI_BUF_SIZE' [1] explains in no uncertain terms that
the rounding requirement exists to accommodate Siena on-chip buffers. The note
does not apply to the modern adapters currently supported by the DPDK driver.
No actual defect.
- Patch 05/14:
In production builds, 'EFSYS_ASSERT' is elided. A NULL check is the correct
defensive posture for upstream code and accurately reflects the '__in_opt'
semantics at the call site.
- Patches 09/14 and 13/14:
The convention cited applies to 'boolean_t'-returning functions carrying
'__checkReturn', where '__success', '__checkReturn', and the return type all
annotate the return value and naturally share a line. The functions in
question, however, return 'void', carry no '__checkReturn', and express the
success condition on an output parameter. The note is thus not valid at all;
the placement stands.
On these premises, I respectfully suggest that the series be put forward for
reconsideration and integration.
[1]
https://github.com/DPDK/dpdk/blob/c1a46b9d9243e922428e8a5f87fa3c6ac177dc5a/drivers/common/sfc_efx/base/efx_mcdi.h#L582
Thank you.
On Wed, 12 Aug 2026, Stephen Hemminger wrote:
On Wed, 12 Aug 2026 21:08:20 +0400
Ivan Malov <[email protected]> wrote:
This series addresses code analysis defects in the
common/sfc_efx/base library.
The first four patches fix excessive stack consumption in
MCDI helper functions, each exceeding 1 KB on-stack, by
switching to heap-allocated payload buffers.
The remaining ten patches correct SAL annotations, add NULL
checks across netport and filter helpers, resolving
uninitialised memory, buffer overrun, and potential
dereference issues. The final patch widens loop
variable types to address a CodeQL warning.
v2:
- note for the future AI reviews: apply this on top of
the 'next-net-main' branch
Still has AI review issues.
Reviewed v2 applied on c1a46b9 ("doc: remove unreferenced KNI and
examples figures"). All 14 apply cleanly. Comparing commit contents
against v1, only patches 11 and 13 have real changes; 12 differs only
in hunk offsets.
Addressed since v1, all correct as far as I can tell:
- 11/14 now reads MORE_ENTRIES with MCDI_OUT_DWORD_FIELD against
MAC_STATISTICS_DESCRIPTOR_OUT_FLAGS, so only LBN 0 is tested.
- 13/14 adds matched_mask so *enum_hwp is written only on the first
match per SW flag. That restores the original selection order that
the removed "mask_sw &= ~(flag_sw)" used to provide, including the
case where several distinct SW flags are set.
- 13/14 passes cap_enum_hw rather than MC_CMD_FEC_AUTO as the default,
so a request with no FEC bits keeps MC_CMD_FEC_NONE.
- __success() placement is now consistent between 09/14 and 13/14.
Patch 11/14: common/sfc_efx/base: fix flex array in netport stat describe
Error: count and stride are still used to index the response buffer with
no bound derived from the response length. This was the main finding on
v1 and is unchanged:
stride = MCDI_OUT_DWORD(req, MAC_STATISTICS_DESCRIPTOR_OUT_ENTRY_SIZE);
count = MCDI_OUT_DWORD(req, MAC_STATISTICS_DESCRIPTOR_OUT_ENTRY_COUNT);
...
for (i = 0; i < count; ++i) {
efx_np_stat_describe(entries + i * stride,
Both fields come from firmware. entries points at payload + 20 in a
1020-byte allocation and efx_np_stat_describe() reads 8 bytes per entry,
so any count above (out_sz - 20) / stride reads bytes that were never
written, and count * stride above 1000 reads past the end of the
allocation. The old ENTRIES_NUM(out_sz) expression was wrong for
stride > 8, as the commit message says, but it did bound the loop by the
data actually received; nothing replaces that bound.
if (stride < MC_CMD_STAT_DESC_LEN ||
count > (out_sz -
MC_CMD_MAC_STATISTICS_DESCRIPTOR_OUT_ENTRIES_OFST) /
stride) {
rc = EMSGSIZE;
goto fail4;
}
The stride test has to come first, otherwise stride == 0 divides by
zero.
Patch 13/14: common/sfc_efx/base: rework SW mask to HW enum converter
Warning: In the fixed-link branch the programmed technology still
changes. link_tech is initialised to MC_CMD_ETH_TECH_NONE and previously
stayed NONE when no requested tech bit was present in the map; passing
MC_CMD_ETH_TECH_AUTO as enum_hw_def now overwrites it. Unlike the FEC
call, which v2 changed to pass the pre-computed value, this one keeps
the hardcoded default. That may well be the intent, but the commit
message is unchanged from v1 and still describes the patch only as a
refactor plus annotation fix; it does not mention the new enum_hw_def
parameter or this behaviour change. Please say so in the commit message,
or pass link_tech to keep the old value.
Patch 01-04: common/sfc_efx/base: reduce stack in ...
Info: Unchanged from v1, repeating for the record. The four conversions
open-code MAX(IN_LEN, OUT_LEN) where EFX_MCDI_BUF_SIZE() exists and also
rounds up to a dword multiple and enforces a two-dword minimum. The
rounding matters because ef10_mcdi_send_request() reads the payload a
full dword at a time. All four current lengths are dword multiples so
there is no defect today, but the property is lost for future length
changes.
Patch 05/14: common/sfc_efx/base: fix filter saved spec handling
Info: Unchanged from v1. Both added NULL checks are unreachable:
saved_spec == NULL forces EF10_FILTER_ADD_NEW in
ef10_filter_add_select_action(), so ADD_STORE and ADD_REPLACE both imply
a non-NULL saved_spec. The __in_opt annotations are right; the STORE
branch already asserts its sibling invariant one line above, so
EFSYS_ASSERT(saved_spec != NULL) would match local style rather than
silently skipping the efs_overridden_spec assignment.
Patch 09/14 and 13/14
Info: The two are consistent with each other now, but both put
__success() on its own line above "static". Existing uses in the tree
put it on the return type line, e.g. ef10_nvram.c:941
__checkReturn __success(return != B_FALSE) boolean_t
ef10_nvram_buffer_find_item(
