Re: [PATCH] net/crc: add 4x folding loop for aarch64 NEON implementation

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 16 Jun 2026 14:41:58 +0530
Shreesh Adiga <16567adigashreesh@gmail.com> wrote:

> Add a 64-byte loop that maintains 4 fold registers and processes
> 64 bytes at a time. The 4x fold registers is then reduced to 16 byte
> single fold, similar to x86 SSE implementation. This technique is
> described in the paper by Intel:
> "Fast CRC Computation for Generic Polynomials Using PCLMULQDQ Instruction"
> 
> This results in roughly 2x performance improvement due to better ILP
> for large input sizes like 1024 observed on Cortex-X925.
> 
> Signed-off-by: Shreesh Adiga <16567adigashreesh@gmail.com>
> ---

Detailed AI review (not the CI one), spotted a correctness issue.

On the 4x folding loop patch:

The 4x-fold port matches the x86 SSE implementation and the constant
tables are correct, but the patch introduces one correctness bug.

Error: lib/net/net_crc_neon.c, "17 to 31 bytes" path uses the wrong
fold constant.

This patch repurposes rk1_rk2 as the fold-by-4 (512-bit) constant and
moves the fold-by-1 (128-bit) constant into the new rk3_rk4, matching
the SSE layout. The main paths were updated to select rk3_rk4 before
falling into partial_bytes, and the partial_bytes comment was correctly
updated to "k = rk3 & rk4".

The 17-to-31 byte branch was missed. It still does:

	/* 17 to 31 bytes */
	fold = vld1q_u64((const uint64_t *)data);
	fold = veorq_u64(fold, temp);
	n = 16;
	k = params->rk1_rk2;      /* now the fold-by-4 constant */
	goto partial_bytes;

partial_bytes performs a single 128-bit fold and needs the fold-by-1
constant, but this path now feeds it rk1_rk2, which after the change
holds the 512-bit constant. CRC results are therefore wrong for every
input of length 17-31 bytes. This affects both crc32_eth and
crc16_ccitt, since they share this routine.

The fix is one line -- use rk3_rk4 here as the other paths do:

	k = params->rk3_rk4;

Verification: I cross-compiled the routine (armv8-a+crypto) and ran it
under qemu against a scalar reflected CRC-32 reference for lengths
1-256. As submitted it mismatches at exactly lengths 17-31 (15
lengths); with the one-line change above, all lengths pass. The >=64,
>=32, ==16, and <16 paths are already correct.

One suggestion for v2, not required: the SSE version does not carry a
separate 17-31 branch at all -- it handles everything below 64 through
the single_fold_loop plus partial_bytes with rk3_rk4. Collapsing the
NEON path the same way would remove this class of bug rather than just
this instance.