Re: [PATCH] squashfs: Fix compilation on big endian systems

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3081 bytes --]

On Thu, Apr 07, 2022 at 11:58:55AM +0200, Pali Rohár wrote:
> On Thursday 07 April 2022 11:54:55 Miquel Raynal wrote:
> > Hi Pali,
> > 
> > pali@kernel.org wrote on Thu, 7 Apr 2022 11:41:59 +0200:
> > 
> > > On Thursday 07 April 2022 09:54:21 Miquel Raynal wrote:
> > > > Hi Pali,
> > > > 
> > > > pali@kernel.org wrote on Wed,  6 Apr 2022 23:31:53 +0200:
> > > > 
> > > > Would you mind explaining a little bit how this change fixes it? It
> > > > does not look straightforward to me.  
> > > 
> > > Yes! I though that it is straightforward this change...
> > > byteorder/little_endian.h defines cpu_to_le* macros for Little Endian
> > > systems and byteorder/big_endian.h for Big Endian systems.
> > > 
> > > File asm/byteorder.h is then ARCH-specific and implements macros for the
> > > current architecture (by including the correct header file).
> > > 
> > > So currently if you try to compile squashfs for big endian systems you
> > > get compile error:
> > > 
> > >   In file included from ./arch/powerpc/include/asm/byteorder.h:82,
> > >                    from include/linux/unaligned/access_ok.h:4,
> > >                    from ./arch/powerpc/include/asm/unaligned.h:9,
> > >                    from fs/squashfs/sqfs_filesystem.h:11,
> > >                    from fs/squashfs/sqfs_dir.c:16:
> > >   include/linux/byteorder/big_endian.h:34: warning: "__cpu_to_le32" redefined
> > >    #define __cpu_to_le32(x) ((__force __le32)__swab32((x)))
> > > 
> > >   In file included from fs/squashfs/sqfs_dir.c:10:
> > >   include/linux/byteorder/little_endian.h:34: note: this is the location of the previous definition
> > >    #define __cpu_to_le32(x) ((__force __le32)(__u32)(x))
> > > 
> > > Or:
> > > 
> > >   In file included from fs/squashfs/sqfs.c:14:
> > >   include/linux/byteorder/little_endian.h:89:21: error: redefinition of ‘__be16_to_cpup’
> > >    static inline __u16 __be16_to_cpup(const __be16 *p)
> > >                        ^~~~~~~~~~~~~~
> > >   In file included from ./arch/powerpc/include/asm/byteorder.h:82,
> > >                    from include/linux/unaligned/access_ok.h:4,
> > >                    from ./arch/powerpc/include/asm/unaligned.h:9,
> > >                    from fs/squashfs/sqfs.c:10:
> > >   include/linux/byteorder/big_endian.h:89:21: note: previous definition of ‘__be16_to_cpup’ was here
> > >    static inline __u16 __be16_to_cpup(const __be16 *p)
> > >                        ^~~~~~~~~~~~~~
> > > 
> > > As some header files include correct asm/byteorder.h file and this
> > > squashfs includes additional little_endian.h.
> > 
> > Great, thanks for the thorough explanation. Based on what you said,
> > wouldn't it be cleaner to just get rid of the little_endian.h include
> > rather than also use the ARCH specific byteorder.h header?
> 
> I think that this is not possible. squashfs code uses le16_to_cpu()
> macro and its correct definition is provided only by arch specific
> asm/byteorder.h.

Miquel, is this sufficient explanation?  Thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]