Re: [PATCH] parisc: More csum_ipv6_magic fixes

[Helge Deller <deller@gmx.de>]

On 2/27/24 19:53, Guenter Roeck wrote:
> IPv6 checksum tests with unaligned addresses resulted in unexpected
> failures.
>
> Expected expected == csum_result, but
>      expected == 46591 (0xb5ff)
>      csum_result == 46381 (0xb52d)
> with alignment offset 1
>
> Oddly enough, the problem disappeared after adding test code into
> the beginning of csum_ipv6_magic().
>
> As it turns out, the 'sum' parameter of csum_ipv6_magic() is declared as
> __wsum, which is a 32-bit variable type. However, it is treated as 64-bit
> variable in the assembler code.

Nice catch!
That kind of bugs is actually the reason why I start to prefer
C-code over inline assembly, even if C might perform slower.

I've applied that patch to the parisc git tree, but do you think
you can come up with a better patch title, e.g.
"strip upper 32bits of sum in csum_ipv6_magic()" ?

Other than that you may add
Acked-by: Helge Deller <deller@gmx.de>

Helge

> Tests showed that the upper 32 bit of
> the register used to pass the variable are _not_ cleared when entering
> the function. This can result in checksum calculation errors.
>
> Clearing the upper 32 bit of 'sum' as first operation in the assembler
> code fixes the problem.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> Maybe there is a way to do this without additional instruction, but if so
> I have not been able to find it.
>
>   arch/parisc/include/asm/checksum.h | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/arch/parisc/include/asm/checksum.h b/arch/parisc/include/asm/checksum.h
> index c949aa20fa16..2aceebcd695c 100644
> --- a/arch/parisc/include/asm/checksum.h
> +++ b/arch/parisc/include/asm/checksum.h
> @@ -126,6 +126,7 @@ static __inline__ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
>   	** Try to keep 4 registers with "live" values ahead of the ALU.
>   	*/
>
> +"	depdi		0, 31, 32, %0\n"/* clear upper half of incoming checksum */
>   "	ldd,ma		8(%1), %4\n"	/* get 1st saddr word */
>   "	ldd,ma		8(%2), %5\n"	/* get 1st daddr word */
>   "	add		%4, %0, %0\n"