Re: [PATCH 6.6 111/111] nvmem: layouts: u-boot-env: remove crc32 endianness conversion

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Wed, Jul 23, 2025 at 06:33:10AM +0000, Michael Pratt wrote:
> Hi,
> 
> I don't mean to be nitpicking too hard
> but the manual edit description below would  read better as:
> 
> On 7/22/25 9:56 AM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> >  6.6-stable review patch.  If anyone has any objections, please let me know.
> >  
> >  ------------------
> >  
> >  From: Michael C. Pratt <mcpratt@pm.me>
> >  
> >  commit 2d7521aa26ec2dc8b877bb2d1f2611a2df49a3cf upstream.
> >  
> >  On 11 Oct 2022, it was reported that the crc32 verification
> >  of the u-boot environment failed only on big-endian systems
> >  for the u-boot-env nvmem layout driver with the following error.
> >  
> >    Invalid calculated CRC32: 0x88cd6f09 (expected: 0x096fcd88)
> >  
> >  This problem has been present since the driver was introduced,
> >  and before it was made into a layout driver.
> >  
> >  The suggested fix at the time was to use further endianness
> >  conversion macros in order to have both the stored and calculated
> >  crc32 values to compare always represented in the system's endianness.
> >  This was not accepted due to sparse warnings
> >  and some disagreement on how to handle the situation.
> >  Later on in a newer revision of the patch, it was proposed to use
> >  cpu_to_le32() for both values to compare instead of le32_to_cpu()
> >  and store the values as __le32 type to remove compilation errors.
> >  
> >  The necessity of this is based on the assumption that the use of crc32()
> >  requires endianness conversion because the algorithm uses little-endian,
> >  however, this does not prove to be the case and the issue is unrelated.
> >  
> >  Upon inspecting the current kernel code,
> >  there already is an existing use of le32_to_cpu() in this driver,
> >  which suggests there already is special handling for big-endian systems,
> >  however, it is big-endian systems that have the problem.
> >  
> >  This, being the only functional difference between architectures
> >  in the driver combined with the fact that the suggested fix
> >  was to use the exact same endianness conversion for the values
> >  brings up the possibility that it was not necessary to begin with,
> >  as the same endianness conversion for two values expected to be the same
> >  is expected to be equivalent to no conversion at all.
> >  
> >  After inspecting the u-boot environment of devices of both endianness
> >  and trying to remove the existing endianness conversion,
> >  the problem is resolved in an equivalent way as the other suggested fixes.
> >  
> >  Ultimately, it seems that u-boot is agnostic to endianness
> >  at least for the purpose of environment variables.
> >  In other words, u-boot reads and writes the stored crc32 value
> >  with the same endianness that the crc32 value is calculated with
> >  in whichever endianness a certain architecture runs on.
> >  
> >  Therefore, the u-boot-env driver does not need to convert endianness.
> >  Remove the usage of endianness macros in the u-boot-env driver,
> >  and change the type of local variables to maintain the same return type.
> >  
> >  If there is a special situation in the case of endianness,
> >  it would be a corner case and should be handled by a unique "compatible".
> >  
> >  Even though it is not necessary to use endianness conversion macros here,
> >  it may be useful to use them in the future for consistent error printing.
> >  
> >  Fixes: d5542923f200 ("nvmem: add driver handling U-Boot environment variables")
> >  Reported-by: INAGAKI Hiroshi <musashino.open@gmail.com>
> >  Link: https://lore.kernel.org/all/20221011024928.1807-1-musashino.open@gmail.com
> >  Cc: stable@vger.kernel.org
> >  Signed-off-by: "Michael C. Pratt" <mcpratt@pm.me>
> >  Signed-off-by: Srinivas Kandagatla <srini@kernel.org>
> >  Link: https://lore.kernel.org/r/20250716144210.4804-1-srini@kernel.org
> >  [ applied changes to drivers/nvmem/u-boot-env.c after code was moved from drivers/nvmem/layouts/u-boot-env.c ]
> 
> [ applied changes to drivers/nvmem/u-boot-env.c before code was moved to drivers
> drivers/nvmem/layouts/u-boot-env.c ]

Now fixed, thanks.

greg k-h