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

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

[INAGAKI Hiroshi]

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@gmail.com>
---
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;
@@ -152,11 +152,11 @@ static int u_boot_env_parse(struct u_boot_env *priv)
 		data_offset = offsetof(struct u_boot_env_image_broadcom, data);
 		break;
 	}
-	crc32 = le32_to_cpu(*(__le32 *)(buf + crc32_offset));
+	crc32 = cpu_to_le32(*(uint32_t *)(buf + crc32_offset));
 	crc32_data_len = priv->mtd->size - crc32_data_offset;
 	data_len = priv->mtd->size - data_offset;
 
-	calc = crc32(~0, buf + crc32_data_offset, crc32_data_len) ^ ~0L;
+	calc = cpu_to_le32(crc32(~0, buf + crc32_data_offset, crc32_data_len) ^ ~0L);
 	if (calc != crc32) {
 		dev_err(dev, "Invalid calculated CRC32: 0x%08x (expected: 0x%08x)\n", calc, crc32);
 		err = -EINVAL;
-- 
2.25.1

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

[Rafał Miłecki]

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@gmail.com>
> ---
> 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().

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

[Christian Lamparter]

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@gmail.com>
>> ---
>> 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?

Regards,
Christian

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@gmail.com>
>>> ---
>>> 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

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

[Christian Lamparter]

Hi Hiroshi,
On 2/19/23 05:45,  Hiroshi wrote:
> 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@gmail.com>
>>>> ---
>> 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.

Oh, yes. This could be why! I was wondering why any additional endian related
changes would be necessary. But no, they are not. This is the patch from
October/November last year.

I now remember '[Patch v2] nvmem: u-boot-env: align endianness of crc32 values' too
<https://lore.kernel.org/lkml/e7ebabf9-a7b2-6155-cdd0-5dafb6bb2a7a@isd.uni-stuttgart.de/T/>
and back then I provided a tested-by tag as well. So there's no difference between
the  APRESIA ApresiaLightGS120GT-SS and WD MyBook Live. Both will need this patch.

Cheers,
Christian

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

[Rafał Miłecki]

On 13.02.2023 14:23, INAGAKI Hiroshi wrote:
> @@ -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;
> @@ -152,11 +152,11 @@ static int u_boot_env_parse(struct u_boot_env *priv)
>   		data_offset = offsetof(struct u_boot_env_image_broadcom, data);
>   		break;
>   	}
> -	crc32 = le32_to_cpu(*(__le32 *)(buf + crc32_offset));
> +	crc32 = cpu_to_le32(*(uint32_t *)(buf + crc32_offset));
>   	crc32_data_len = priv->mtd->size - crc32_data_offset;
>   	data_len = priv->mtd->size - data_offset;
>   
> -	calc = crc32(~0, buf + crc32_data_offset, crc32_data_len) ^ ~0L;
> +	calc = cpu_to_le32(crc32(~0, buf + crc32_data_offset, crc32_data_len) ^ ~0L);

Can you see what happens on BE device if instead of this whole patch you
just replace crc32() in above line with crc32_le()?


>   	if (calc != crc32) {
>   		dev_err(dev, "Invalid calculated CRC32: 0x%08x (expected: 0x%08x)\n", calc, crc32);
>   		err = -EINVAL;

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

[INAGAKI Hiroshi]

Hi Rafał,

On 2023/02/20 17:01, Rafał Miłecki wrote:
> On 13.02.2023 14:23, INAGAKI Hiroshi wrote:
>> @@ -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;
>> @@ -152,11 +152,11 @@ static int u_boot_env_parse(struct u_boot_env 
>> *priv)
>>           data_offset = offsetof(struct u_boot_env_image_broadcom, 
>> data);
>>           break;
>>       }
>> -    crc32 = le32_to_cpu(*(__le32 *)(buf + crc32_offset));
>> +    crc32 = cpu_to_le32(*(uint32_t *)(buf + crc32_offset));
>>       crc32_data_len = priv->mtd->size - crc32_data_offset;
>>       data_len = priv->mtd->size - data_offset;
>>   -    calc = crc32(~0, buf + crc32_data_offset, crc32_data_len) ^ 
>> ~0L;
>> +    calc = cpu_to_le32(crc32(~0, buf + crc32_data_offset, 
>> crc32_data_len) ^ ~0L);
>
> Can you see what happens on BE device if instead of this whole patch 
> you
> just replace crc32() in above line with crc32_le()?

Thank you for your suggestion.
I tried it, but "calc" still has big-endianness value and needs to be 
converted to little-endianness value.

log:

[    8.725024] u_boot_env 
18001200.spi:flash@0:partitions:partition@c0000: Invalid calculated 
CRC32: 0x88cd6f09 (expected: 0x096fcd88)

"crc32()" seems to be the same as "crc32_le()" [0].

[0]: 
https://elixir.bootlin.com/linux/v5.15.94/source/include/linux/crc32.h#L66

>
>
>>       if (calc != crc32) {
>>           dev_err(dev, "Invalid calculated CRC32: 0x%08x (expected: 
>> 0x%08x)\n", calc, crc32);
>>           err = -EINVAL;

Thanks,
Hiroshi