byteorder: cpu_to_le32_array vs cpu_to_be32_array function API differences

byteorder: cpu_to_le32_array vs cpu_to_be32_array function API differences

[Joe Perches]

There's an argument inconsistency between these 4 functions
in include/linux/byteorder/generic.h

It'd be more a consistent API with one form and not two.

   static inline void le32_to_cpu_array(u32 *buf, unsigned int words)
   {
   	while (words--) {
   		__le32_to_cpus(buf);
   		buf++;
   	}
   }

   static inline void cpu_to_le32_array(u32 *buf, unsigned int words)
   {
   	while (words--) {
   		__cpu_to_le32s(buf);
   		buf++;
   	}
   }

vs

   static inline void cpu_to_be32_array(__be32 *dst, const u32 *src, size_t len)
   {
   	int i;

   	for (i = 0; i < len; i++)
   		dst[i] = cpu_to_be32(src[i]);
   }

   static inline void be32_to_cpu_array(u32 *dst, const __be32 *src, size_t len)
   {
   	int i;

   	for (i = 0; i < len; i++)
   		dst[i] = be32_to_cpu(src[i]);
   }

Added via 2 different commits:

commit f2f2efb807d339513199b1bb771806c90cce83ae
Author: Mika Westerberg <mika.westerberg@linux.intel.com>
Date:   Mon Oct 2 13:38:28 2017 +0300

    byteorder: Move {cpu_to_be32, be32_to_cpu}_array() from Thunderbolt to core
    
    We will be using these when communicating XDomain discovery protocol
    over Thunderbolt link but they might be useful for other drivers as
    well.
    
    Make them available through byteorder/generic.h.
    
    Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

and

commit 9def051018c08e65c532822749e857eb4b2e12e7
Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date:   Wed Mar 21 19:01:40 2018 +0200

    crypto: Deduplicate le32_to_cpu_array() and cpu_to_le32_array()
    
    Deduplicate le32_to_cpu_array() and cpu_to_le32_array() by moving them
    to the generic header.
    
    No functional change implied.
    
    Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Re: byteorder: cpu_to_le32_array vs cpu_to_be32_array function API differences

[Anatol Belski]

Hi,

On Sun, 2019-10-20 at 12:02 -0700, Joe Perches wrote:
> There's an argument inconsistency between these 4 functions
> in include/linux/byteorder/generic.h
> 
> It'd be more a consistent API with one form and not two.
> 
>    static inline void le32_to_cpu_array(u32 *buf, unsigned int words)
>    {
>    	while (words--) {
>    		__le32_to_cpus(buf);
>    		buf++;
>    	}
>    }
> 
>    static inline void cpu_to_le32_array(u32 *buf, unsigned int words)
>    {
>    	while (words--) {
>    		__cpu_to_le32s(buf);
>    		buf++;
>    	}
>    }
> 
> vs
> 
>    static inline void cpu_to_be32_array(__be32 *dst, const u32 *src,
> size_t len)
>    {
>    	int i;
> 
>    	for (i = 0; i < len; i++)
>    		dst[i] = cpu_to_be32(src[i]);
>    }
> 
>    static inline void be32_to_cpu_array(u32 *dst, const __be32 *src,
> size_t len)
>    {
>    	int i;
> 
>    	for (i = 0; i < len; i++)
>    		dst[i] = be32_to_cpu(src[i]);
>    }
> 
> 

size_t is the right choice for this, as it'll generate more correct
binary depending on 32/64 bit. I've sent a patch in
'include/linux/byteorder/generic.h: fix signed/unsigned warnings'
before, but only touched the place where i've seen warnings. My very
bet is, that changing between size_t/unsigned, while it would be
consistent, wouldn't change the functionality. It'd probably make sense
to extend the aforementioned patch to move unsigned -> size_t.

Regards

Anatol

Re: byteorder: cpu_to_le32_array vs cpu_to_be32_array function API differences

[Joe Perches]

On Sun, 2019-10-20 at 19:28 +0000, Anatol Belski wrote:
> Hi,

Hello.

> On Sun, 2019-10-20 at 12:02 -0700, Joe Perches wrote:
> > There's an argument inconsistency between these 4 functions
> > in include/linux/byteorder/generic.h
> > 
> > It'd be more a consistent API with one form and not two.
> > 
> >    static inline void le32_to_cpu_array(u32 *buf, unsigned int words)
> >    {
> >    	while (words--) {
> >    		__le32_to_cpus(buf);
> >    		buf++;
> >    	}
> >    }
> > 
> >    static inline void cpu_to_le32_array(u32 *buf, unsigned int words)
> >    {
> >    	while (words--) {
> >    		__cpu_to_le32s(buf);
> >    		buf++;
> >    	}
> >    }
> > 
> > vs
> > 
> >    static inline void cpu_to_be32_array(__be32 *dst, const u32 *src,
> > size_t len)
> >    {
> >    	int i;
> > 
> >    	for (i = 0; i < len; i++)
> >    		dst[i] = cpu_to_be32(src[i]);
> >    }
> > 
> >    static inline void be32_to_cpu_array(u32 *dst, const __be32 *src,
> > size_t len)
> >    {
> >    	int i;
> > 
> >    	for (i = 0; i < len; i++)
> >    		dst[i] = be32_to_cpu(src[i]);
> >    }
> > 
> > 
> 
> size_t is the right choice for this, as it'll generate more correct
> binary depending on 32/64 bit. I've sent a patch in
> 'include/linux/byteorder/generic.h: fix signed/unsigned warnings'
> before, but only touched the place where i've seen warnings. My very
> bet is, that changing between size_t/unsigned, while it would be
> consistent, wouldn't change the functionality. It'd probably make sense
> to extend the aforementioned patch to move unsigned -> size_t.

True, but my point was the le versions have 2 arguments and
convert the buf input, and the be versions have 3 arguments
and convert src to dst.

cheers, Joe

Re: byteorder: cpu_to_le32_array vs cpu_to_be32_array function API differences

[Anatol Belski]

On Sun, 2019-10-20 at 12:40 -0700, Joe Perches wrote:
> On Sun, 2019-10-20 at 19:28 +0000, Anatol Belski wrote:
> > Hi,
> 
> Hello.
> 
> > On Sun, 2019-10-20 at 12:02 -0700, Joe Perches wrote:
> > > There's an argument inconsistency between these 4 functions
> > > in include/linux/byteorder/generic.h
> > > 
> > > It'd be more a consistent API with one form and not two.
> > > 
> > >    static inline void le32_to_cpu_array(u32 *buf, unsigned int
> > > words)
> > >    {
> > >    	while (words--) {
> > >    		__le32_to_cpus(buf);
> > >    		buf++;
> > >    	}
> > >    }
> > > 
> > >    static inline void cpu_to_le32_array(u32 *buf, unsigned int
> > > words)
> > >    {
> > >    	while (words--) {
> > >    		__cpu_to_le32s(buf);
> > >    		buf++;
> > >    	}
> > >    }
> > > 
> > > vs
> > > 
> > >    static inline void cpu_to_be32_array(__be32 *dst, const u32
> > > *src,
> > > size_t len)
> > >    {
> > >    	int i;
> > > 
> > >    	for (i = 0; i < len; i++)
> > >    		dst[i] = cpu_to_be32(src[i]);
> > >    }
> > > 
> > >    static inline void be32_to_cpu_array(u32 *dst, const __be32
> > > *src,
> > > size_t len)
> > >    {
> > >    	int i;
> > > 
> > >    	for (i = 0; i < len; i++)
> > >    		dst[i] = be32_to_cpu(src[i]);
> > >    }
> > > 
> > > 
> > 
> > size_t is the right choice for this, as it'll generate more correct
> > binary depending on 32/64 bit. I've sent a patch in
> > 'include/linux/byteorder/generic.h: fix signed/unsigned warnings'
> > before, but only touched the place where i've seen warnings. My
> > very
> > bet is, that changing between size_t/unsigned, while it would be
> > consistent, wouldn't change the functionality. It'd probably make
> > sense
> > to extend the aforementioned patch to move unsigned -> size_t.
> 
> True, but my point was the le versions have 2 arguments and
> convert the buf input, and the be versions have 3 arguments
> and convert src to dst.

Thanks for checking. Yes, that's another point of the inconsistency. I
could imagine fixing it by adapting all the signatures to be


static inline void _cpu_to_le32_array(__le32 *dst, const u32 *src,
size_t len)
static inline void _le32_to_cpu_array(u32 *dst, const __le32 *src,
size_t len)
static inline void cpu_to_be32_array(__be32 *dst, const u32 *src,
size_t len)
static inline void be32_to_cpu_array(u32 *dst, const __be32 *src,
size_t len)

and do the necessary implementation change in the le variants, plus
introduce some macros to ensure the backward compatibility.

#define le32_to_cpu_array(dst, len) _cpu_to_le32_array(dst, dst, len)
#define cpu_to_le32_array(dst, len) _le32_to_cpu_array(dst, dst, len)

But it is a bad situation for the backward compatibility in any case,
as the new API would have to be named somehow. That would be bad for
the backport. If breaching this is ok for master, then the fix would be
as easy as changing the signatures and adapting the implementation.


Thanks

Anatol

Re: byteorder: cpu_to_le32_array vs cpu_to_be32_array function API differences

[Andy Shevchenko]

On Sun, Oct 20, 2019 at 12:02:52PM -0700, Joe Perches wrote:
> There's an argument inconsistency between these 4 functions
> in include/linux/byteorder/generic.h
> 
> It'd be more a consistent API with one form and not two.

This is done in order to:
 - avoid changing existing code
 - start a very this discussion to see what we can do if it's even needed to be done

So, do we have other places in the kernel which can utilize either of these?

-- 
With Best Regards,
Andy Shevchenko