Re: [PATCH v2 00/14] common/sfc_efx/base: fix code analysis issues

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(
>
>