Message ID | 20240226080328.334021-1-wangyuli@uniontech.com |
---|---|
State | Superseded |
Headers | show |
Series | loongarch/crypto: Clean up useless assignment operations | expand |
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>
That's why I didn't have sent a patch to MIPS group. WangYuli
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 --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)) {
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(-)