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