diff mbox series

loongarch/crypto: Clean up useless assignment operations

Message ID 20240226080328.334021-1-wangyuli@uniontech.com (mailing list archive)
State Superseded
Delegated to: Herbert Xu
Headers show
Series loongarch/crypto: Clean up useless assignment operations | expand

Commit Message

WangYuli Feb. 26, 2024, 8:03 a.m. UTC
Both crc32 and crc32c hw accelerated funcs will calculate the
remaining len. Those codes are derived from
arch/mips/crypto/crc32-mips.c and "len -= sizeof(u32)" is not
necessary for 64-bit CPUs.

Removing it can make context code style more unified and improve
code readability.

Suggested-by: Guan Wentao <guanwentao@uniontech.com>
Signed-off-by: WangYuli <wangyuli@uniontech.com>
---
 arch/loongarch/crypto/crc32-loongarch.c | 2 --
 1 file changed, 2 deletions(-)

Comments

WANG Xuerui Feb. 26, 2024, 8:15 a.m. UTC | #1
On 2/26/24 16:03, WangYuli wrote:
> Both crc32 and crc32c hw accelerated funcs will calculate the
> remaining len. Those codes are derived from
> arch/mips/crypto/crc32-mips.c and "len -= sizeof(u32)" is not
> necessary for 64-bit CPUs.
> 
> Removing it can make context code style more unified and improve
> code readability.
> 
> Suggested-by: Guan Wentao <guanwentao@uniontech.com>
> Signed-off-by: WangYuli <wangyuli@uniontech.com>
> ---
>   arch/loongarch/crypto/crc32-loongarch.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/arch/loongarch/crypto/crc32-loongarch.c b/arch/loongarch/crypto/crc32-loongarch.c
> index a49e507af38c..3eebea3a7b47 100644
> --- a/arch/loongarch/crypto/crc32-loongarch.c
> +++ b/arch/loongarch/crypto/crc32-loongarch.c
> @@ -44,7 +44,6 @@ static u32 crc32_loongarch_hw(u32 crc_, const u8 *p, unsigned int len)
>   
>   		CRC32(crc, value, w);
>   		p += sizeof(u32);
> -		len -= sizeof(u32);
>   	}
>   
>   	if (len & sizeof(u16)) {
> @@ -80,7 +79,6 @@ static u32 crc32c_loongarch_hw(u32 crc_, const u8 *p, unsigned int len)
>   
>   		CRC32C(crc, value, w);
>   		p += sizeof(u32);
> -		len -= sizeof(u32);
>   	}
>   
>   	if (len & sizeof(u16)) {

Sure, but IIRC Loongson still has hopes in having 32-bit-only models 
support upstream? The possibility cannot be ruled out because from 
public information (e.g. the 2023-11-28 news event), Loongson is known 
to be actively licensing their reduced 32-bit-only IP cores to third 
parties.

Ultimately whether we want to imply 64-bit operation with the crc32 
module is up to Loongson to decide, and I think Huacai may have 
something to add, but IMO at least we could gate the statement with 
#ifdef's so we don't outright lose 32-bit compatibility with this code.
Wentao Guan Feb. 26, 2024, 8:25 a.m. UTC | #2
Sure,but we not seem to have "crc.w.d.w" dword asm in loongarch 32bit mode,now the whole file can only run in 64bit mode?

Thanks.
WANG Xuerui Feb. 27, 2024, 7:42 a.m. UTC | #3
On 2/26/24 16:25, 关文涛 wrote:
> Sure,but we not seem to have "crc.w.d.w" dword asm in loongarch 32bit mode,now the whole file can only run in 64bit mode?
> 
> Thanks.

Sorry. I have checked the manual and it turns out you & Yuli are 
correct: even though the narrower CRC instructions doesn't require 
GRLEN=64, they still *aren't* part of LA32 (LoongArch reference manual 
v1.10, Volume 1, Table 2-1). Counter-intuitive as it is, the original 
patch is correct nevertheless.

I'm going to amend my review shortly.
WANG Xuerui Feb. 27, 2024, 7:52 a.m. UTC | #4
On 2/26/24 16:15, WANG Xuerui wrote:
> On 2/26/24 16:03, WangYuli wrote:
>> Both crc32 and crc32c hw accelerated funcs will calculate the
>> remaining len. Those codes are derived from
>> arch/mips/crypto/crc32-mips.c and "len -= sizeof(u32)" is not
>> necessary for 64-bit CPUs.

It's ultimately unrelated to the width of the running CPU, but rather 
the LoongArch ISA specification. You should mention that because the 
LoongArch ISA manual implies that CRC instructions are only available on 
LA64-compatible implementations, the code can assume 64-bit operation, 
so the 32-bit codepath can be guaranteed to execute only once.

>>
>> Removing it can make context code style more unified and improve
>> code readability.

"Remove it to improve readability." should do it?

>>
>> Suggested-by: Guan Wentao <guanwentao@uniontech.com>
>> Signed-off-by: WangYuli <wangyuli@uniontech.com>
>> ---
>>   arch/loongarch/crypto/crc32-loongarch.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/arch/loongarch/crypto/crc32-loongarch.c 
>> b/arch/loongarch/crypto/crc32-loongarch.c
>> index a49e507af38c..3eebea3a7b47 100644
>> --- a/arch/loongarch/crypto/crc32-loongarch.c
>> +++ b/arch/loongarch/crypto/crc32-loongarch.c
>> @@ -44,7 +44,6 @@ static u32 crc32_loongarch_hw(u32 crc_, const u8 *p, 
>> unsigned int len)
>>           CRC32(crc, value, w);
>>           p += sizeof(u32);
>> -        len -= sizeof(u32);
>>       }
>>       if (len & sizeof(u16)) {
>> @@ -80,7 +79,6 @@ static u32 crc32c_loongarch_hw(u32 crc_, const u8 
>> *p, unsigned int len)
>>           CRC32C(crc, value, w);
>>           p += sizeof(u32);
>> -        len -= sizeof(u32);
>>       }
>>       if (len & sizeof(u16)) {
> 
> Sure, but IIRC Loongson still has hopes in having 32-bit-only models 
> support upstream? The possibility cannot be ruled out because from 
> public information (e.g. the 2023-11-28 news event), Loongson is known 
> to be actively licensing their reduced 32-bit-only IP cores to third 
> parties.
> 
> Ultimately whether we want to imply 64-bit operation with the crc32 
> module is up to Loongson to decide, and I think Huacai may have 
> something to add, but IMO at least we could gate the statement with 
> #ifdef's so we don't outright lose 32-bit compatibility with this code.
> 

I was wrong in assuming that LA32 must contain CRC insns that operate on 
at most 32 bits. In fact this is wrong -- the LA32 subset omits a lot of 
32-bit operations for no apparent reason (perhaps implementation 
simplicity but outsiders cannot confirm). So by definition the code 
cannot be ported to LA32, and the changes are correct.

But for the same reason, the commit message is misleading. See above.
And with the nits addressed:

Reviewed-by: WANG Xuerui <git@xen0n.name>
Wentao Guan Feb. 27, 2024, 8:44 a.m. UTC | #5
Okay, somehow i think Yuli the origin commit message some misleading
things,the whole story expand that origin mips/crypto/crc32-mips.c code 
"len -= sizeof(u32);" necessary only in "!CONFIG_64BIT" macro define,
they are just a "binary bit" game as we tran the number to binary bit,
while less and less one in 64bit ,the less thing we do if >=32bit do once,
>=16bit do once ... etc... If we delete the useless code line, it is very cool,
and unrelated to specific ISA. We also think maybe we need to add 
"#ifnef CONFIG_64BIT"in origin mips/crypto/crc32-mips.c "len -= sizeof(u32);",
but it add code instead of delete code...

Thanks.
Guan Wentao
WangYuli Feb. 27, 2024, 9:14 a.m. UTC | #6
That's why I didn't have sent a patch to MIPS group. 

WangYuli
Xi Ruoyao Feb. 27, 2024, 9:22 a.m. UTC | #7
On Tue, 2024-02-27 at 15:42 +0800, WANG Xuerui wrote:
> Sorry. I have checked the manual and it turns out you & Yuli are 
> correct: even though the narrower CRC instructions doesn't require 
> GRLEN=64, they still *aren't* part of LA32 (LoongArch reference manual
> v1.10, Volume 1, Table 2-1). Counter-intuitive as it is, the original 
> patch is correct nevertheless.

But I don't think the table in the spec will 100% reflect the real
status of the LA32 hardware.  And we should use CPUCFG to detect it
anyway.

Based on my experience programming some aeronautic device I'd say having
some hardware CRC support should be useful even in embedded area.
Huacai Chen Feb. 29, 2024, 3:05 a.m. UTC | #8
Hi, Yuli,

The code is fine to me, please change loongarch to LoongArch in the
subject, and update your commit message with Xuerui's review in V2.

Huacai

On Mon, Feb 26, 2024 at 4:04 PM WangYuli <wangyuli@uniontech.com> wrote:
>
> Both crc32 and crc32c hw accelerated funcs will calculate the
> remaining len. Those codes are derived from
> arch/mips/crypto/crc32-mips.c and "len -= sizeof(u32)" is not
> necessary for 64-bit CPUs.
>
> Removing it can make context code style more unified and improve
> code readability.
>
> Suggested-by: Guan Wentao <guanwentao@uniontech.com>
> Signed-off-by: WangYuli <wangyuli@uniontech.com>
> ---
>  arch/loongarch/crypto/crc32-loongarch.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/arch/loongarch/crypto/crc32-loongarch.c b/arch/loongarch/crypto/crc32-loongarch.c
> index a49e507af38c..3eebea3a7b47 100644
> --- a/arch/loongarch/crypto/crc32-loongarch.c
> +++ b/arch/loongarch/crypto/crc32-loongarch.c
> @@ -44,7 +44,6 @@ static u32 crc32_loongarch_hw(u32 crc_, const u8 *p, unsigned int len)
>
>                 CRC32(crc, value, w);
>                 p += sizeof(u32);
> -               len -= sizeof(u32);
>         }
>
>         if (len & sizeof(u16)) {
> @@ -80,7 +79,6 @@ static u32 crc32c_loongarch_hw(u32 crc_, const u8 *p, unsigned int len)
>
>                 CRC32C(crc, value, w);
>                 p += sizeof(u32);
> -               len -= sizeof(u32);
>         }
>
>         if (len & sizeof(u16)) {
> --
> 2.33.1
>
diff mbox series

Patch

diff --git a/arch/loongarch/crypto/crc32-loongarch.c b/arch/loongarch/crypto/crc32-loongarch.c
index a49e507af38c..3eebea3a7b47 100644
--- a/arch/loongarch/crypto/crc32-loongarch.c
+++ b/arch/loongarch/crypto/crc32-loongarch.c
@@ -44,7 +44,6 @@  static u32 crc32_loongarch_hw(u32 crc_, const u8 *p, unsigned int len)
 
 		CRC32(crc, value, w);
 		p += sizeof(u32);
-		len -= sizeof(u32);
 	}
 
 	if (len & sizeof(u16)) {
@@ -80,7 +79,6 @@  static u32 crc32c_loongarch_hw(u32 crc_, const u8 *p, unsigned int len)
 
 		CRC32C(crc, value, w);
 		p += sizeof(u32);
-		len -= sizeof(u32);
 	}
 
 	if (len & sizeof(u16)) {