Re: [PATCH v3] nvmem: u-boot-env: align endianness of crc32 values

[INAGAKI Hiroshi]

Hi Christian,

On 2023/02/18 2:30, Christian Lamparter wrote:
> On 2/13/23 14:37, Rafał Miłecki wrote:
> > On 2023-02-13 14:23, INAGAKI Hiroshi wrote:
> > > This patch fixes crc32 error on Big-Endianness system by 
> > > conversion of
> > > calculated crc32 value.
> > >
> > > Little-Endianness system:
> > >
> > >   obtained crc32: Little
> > > calculated crc32: Little
> > >
> > > Big-Endianness system:
> > >
> > >   obtained crc32: Little
> > > calculated crc32: Big
> > >
> > > log (APRESIA ApresiaLightGS120GT-SS, RTL8382M, Big-Endianness):
> > >
> > > [    8.570000] u_boot_env
> > > 18001200.spi:flash@0:partitions:partition@c0000: Invalid calculated
> > > CRC32: 0x88cd6f09 (expected: 0x096fcd88)
> > > [    8.580000] u_boot_env: probe of
> > > 18001200.spi:flash@0:partitions:partition@c0000 failed with error -22
> > >
> > > Fixes: f955dc144506 ("nvmem: add driver handling U-Boot 
> > > environment variables")
> > >
> > > Signed-off-by: INAGAKI Hiroshi <musashino.open@xxxxxxxxx>
> > > ---
> > > v2 -> v3
> > >
> > > - fix sparse warning by using __le32 type and cpu_to_le32
> > > - fix character length of the short commit hash in "Fixes:" tag
> > >
> > > v1 -> v2
> > >
> > > - wrong fix for sparse warning due to misunderstanding
> > > - add missing "Fixes:" tag
> > >
> > >  drivers/nvmem/u-boot-env.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/nvmem/u-boot-env.c b/drivers/nvmem/u-boot-env.c
> > > index 29b1d87a3c51..164bb04dfc3b 100644
> > > --- a/drivers/nvmem/u-boot-env.c
> > > +++ b/drivers/nvmem/u-boot-env.c
> > > @@ -117,8 +117,8 @@ static int u_boot_env_parse(struct u_boot_env 
> > > *priv)
> > >      size_t crc32_offset;
> > >      size_t data_offset;
> > >      size_t data_len;
> > > -    uint32_t crc32;
> > > -    uint32_t calc;
> > > +    __le32 crc32;
> > > +    __le32 calc;
> > >      size_t bytes;
> > >      uint8_t *buf;
> > >      int err;
> >
> > This looks counter-intuitive to me, to store values on host system in
> > specified endianness. I'd say we should use __le32 type only to
> > represent numbers in device stored data (e.g. structs as processed by
> > device).
> >
> > My suggesion: leave uint32_t for local variables and use 
> > le32_to_cpu().
>
> Hmm, this is strange. The kernel's u-boot-env driver works without any
> additional changes in the le<->be department on the Big-Endian
> PowerPC APM82181 WD MyBook Live NAS.
>
> Is there something odd going on with the WD MyBook Live, or is it
> the APRESIA ApresiaLightGS120GT-SS that is special?

This additional changes are for resolving sparse warnings. Of course 
it's working fine on my device with the previous changes, but due to 
the warning it wasn't merged into the mainline and needs to be resolved.

> Regards,
> Christian

Thanks,
Hiroshi