[PATCH] parisc: More csum_ipv6_magic fixes

[PATCH] parisc: More csum_ipv6_magic fixes

[Guenter Roeck]

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. 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"
-- 
2.39.2

Re: [PATCH] parisc: More csum_ipv6_magic fixes

[Helge Deller]

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"

Re: [PATCH] parisc: More csum_ipv6_magic fixes

[Guenter Roeck]

On 2/27/24 12:14, Helge Deller wrote:
> 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()" ?
> 

Sure. My initial fix was way more complicated, and I didn't update
the description after I figured out what was actually going on and
found the simpler fix. I'll resend.

Thanks,
Guenter