Re: [PATCH 5/9] Introduce libio to common code for io read/write

[Christoffer Dall <christoffer.dall@linaro.org>]

On Thu, Jan 02, 2014 at 04:47:22PM +0100, Andrew Jones wrote:
> On Sat, Dec 28, 2013 at 10:30:35PM -0800, Christoffer Dall wrote:
> > > diff --git a/lib/libio.c b/lib/libio.c
> > > new file mode 100644
> > > index 0000000000000..e450e984fefb1
> > > --- /dev/null
> > > +++ b/lib/libio.c
> > > @@ -0,0 +1,67 @@
> > > +#include "libcflat.h"
> > > +#include "libio.h"
> > > +
> > > +void read_len(const volatile void *addr, void *buf, unsigned len)
> > > +{
> > > +	unsigned long val;
> > > +
> > > +	switch (len) {
> > > +	case 1:
> > > +		val = readb(addr);
> > > +		break;
> > > +	case 2:
> > > +		val = readw(addr);
> > > +		break;
> > > +	case 4:
> > > +		val = readl(addr);
> > > +		break;
> > > +#ifdef CONFIG_64BIT
> > > +	case 8:
> > > +		val = readq(addr);
> > > +		break;
> > > +#endif
> > > +	default:
> > > +	{
> > > +		u8 *p = buf;
> > > +		unsigned i;
> > > +
> > > +		for (i = 0; i < len; ++i)
> > > +			p[i] = readb(addr + i);
> > > +		return;
> > 
> > are you taking proper care of endianness in the common case?
> 
> Nope, and on second thought I'll change the default to be an error
> instead. Something like this doesn't really belong in an io lib.
> 
> > 
> > > +	}
> > > +	}
> > > +	memcpy(buf, &val, len);
> > > +}
> > > +
> > > +void write_len(volatile void *addr, const void *buf, unsigned len)
> > > +{
> > > +	unsigned long val;
> > > +
> > > +	if (len <= sizeof(unsigned long))
> > > +		memcpy(&val, buf, len);
> > 
> > Does this work with big-endian for sizes smaller than sizeof(unsigned
> > long)?
> 
> Dunno. I didn't test it. Looking at Marc's code makes me suspect
> is does not though...
> 
> > 
> > Take a look at arch/arm/kvm/mmio.c and see how Marc uses a union to
> > solve this issue.
> 
> I'll rework this with some shameless theft from Marc.
> 
> > > +/*
> > > + * Adapted from the Linux kernel's include/asm-generic/io.h and
> > > + * arch/arm/include/asm/io.h
> > > + */
> > > +#include "libcflat.h"
> > > +
> > > +typedef u32 compat_ptr_t;
> > 
> > How is this going to be used here?  The compat ptr stuff in the kernel
> > deals with user space pointer iirc, but I'm not an expert on the
> > details.  Does typedef'ing a pointer to a u32 in generic code work on a
> > 64-bit platform?
> 
> This is necessary to eliminate compiler warnings when casting 64-bit
> pointers to u32s.
> 

ah, ok, a comment to that effect would be nice for us neandertals still
mostly stuck in the 32-bit worlds.

> > 
> > > +
> > > +static inline void *compat_ptr(compat_ptr_t ptr)
> > > +{
> > > +	return (void *)(unsigned long)ptr;
> > > +}
> > > +
> > > +static inline compat_ptr_t ptr_to_compat(void *ptr)
> > > +{
> > > +	return (u32)(unsigned long)ptr;
> > > +}
> > > +
> > > +#ifndef __raw_readb
> > > +static inline u8 __raw_readb(const volatile void *addr)
> > > +{
> > > +	return *(const volatile u8 *)addr;
> > > +}
> > > +#endif
> > > +
> > > +#ifndef __raw_readw
> > > +static inline u16 __raw_readw(const volatile void *addr)
> > > +{
> > > +	return *(const volatile u16 *)addr;
> > > +}
> > > +#endif
> > > +
> > > +#ifndef __raw_readl
> > > +static inline u32 __raw_readl(const volatile void *addr)
> > > +{
> > > +	return *(const volatile u32 *)addr;
> > > +}
> > > +#endif
> > > +
> > > +#ifdef CONFIG_64BIT
> > > +#ifndef __raw_readq
> > > +static inline u64 __raw_readq(const volatile void *addr)
> > > +{
> > > +	return *(const volatile u64 *)addr;
> > > +}
> > > +#endif
> > > +#endif
> > > +
> > > +#ifndef __raw_writeb
> > > +static inline void __raw_writeb(u8 b, volatile void *addr)
> > > +{
> > > +	*(volatile u8 *)addr = b;
> > > +}
> > > +#endif
> > > +
> > > +#ifndef __raw_writew
> > > +static inline void __raw_writew(u16 b, volatile void *addr)
> > > +{
> > > +	*(volatile u16 *)addr = b;
> > > +}
> > > +#endif
> > > +
> > > +#ifndef __raw_writel
> > > +static inline void __raw_writel(u32 b, volatile void *addr)
> > > +{
> > > +	*(volatile u32 *)addr = b;
> > > +}
> > > +#endif
> > > +
> > > +#ifdef CONFIG_64BIT
> > > +#ifndef __raw_writeq
> > > +static inline void __raw_writeq(u64 b, volatile void *addr)
> > > +{
> > > +	*(volatile u64 *)addr = b;
> > > +}
> > > +#endif
> > > +#endif
> > > +
> > > +#ifndef __bswap16
> > > +static inline u16 __bswap16(u16 x)
> > > +{
> > > +	return ((x >> 8) & 0xff) | ((x & 0xff) << 8);
> > > +}
> > > +#endif
> > > +
> > > +#ifndef __bswap32
> > > +static inline u32 __bswap32(u32 x)
> > > +{
> > > +	return ((x & 0xff000000) >> 24) | ((x & 0x00ff0000) >>  8) |
> > > +	       ((x & 0x0000ff00) <<  8) | ((x & 0x000000ff) << 24);
> > > +}
> > > +#endif
> > > +
> > > +#ifdef CONFIG_64BIT
> > > +#ifndef __bswap64
> > > +static inline u64 __bswap64(u64 x)
> > > +{
> > > +	return ((x & 0x00000000000000ffULL) << 56) |
> > > +	       ((x & 0x000000000000ff00ULL) << 40) |
> > > +	       ((x & 0x0000000000ff0000ULL) << 24) |
> > > +	       ((x & 0x00000000ff000000ULL) <<  8) |
> > > +	       ((x & 0x000000ff00000000ULL) >>  8) |
> > > +	       ((x & 0x0000ff0000000000ULL) >> 24) |
> > > +	       ((x & 0x00ff000000000000ULL) >> 40) |
> > > +	       ((x & 0xff00000000000000ULL) >> 56);
> > > +}
> > > +#endif
> > > +#endif
> > > +
> > > +#ifndef cpu_is_be
> > > +#define cpu_is_be 0
> > > +#endif
> > > +
> > > +#define le16_to_cpu(x) \
> > > +	({ u16 __r = cpu_is_be ? __bswap16(x) : (x); __r; })
> > > +#define cpu_to_le16 le16_to_cpu
> > > +
> > > +#define le32_to_cpu(x) \
> > > +	({ u32 __r = cpu_is_be ? __bswap32(x) : (x); __r; })
> > > +#define cpu_to_le32 le32_to_cpu
> > > +
> > > +#ifdef CONFIG_64BIT
> > > +#define le64_to_cpu \
> > > +	({ u64 __r = cpu_is_be ? __bswap64(x) : (x); __r; })
> > > +#define cpu_to_le64 le64_to_cpu
> > > +#endif
> > > +
> > > +#define be16_to_cpu(x) \
> > > +	({ u16 __r = !cpu_is_be ? __bswap16(x) : (x); __r; })
> > > +#define cpu_to_be16 be16_to_cpu
> > > +
> > > +#define be32_to_cpu(x) \
> > > +	({ u32 __r = !cpu_is_be ? __bswap32(x) : (x); __r; })
> > > +#define cpu_to_be32 be32_to_cpu
> > > +
> > > +#ifdef CONFIG_64BIT
> > > +#define be64_to_cpu \
> > > +	({ u64 __r = !cpu_is_be ? __bswap64(x) : (x); __r; })
> > > +#define cpu_to_be64 be64_to_cpu
> > > +#endif
> > > +
> > > +#ifndef rmb
> > > +#define rmb() do { } while (0)
> > > +#endif
> > > +#ifndef wmb
> > > +#define wmb() do { } while (0)
> > > +#endif
> > > +
> > > +#define readb(addr) \
> > > +	({ u8 __r = __raw_readb(addr); rmb(); __r; })
> > > +#define readw(addr) \
> > > +	({ u16 __r = le16_to_cpu(__raw_readw(addr)); rmb(); __r; })
> > > +#define readl(addr) \
> > > +	({ u32 __r = le32_to_cpu(__raw_readl(addr)); rmb(); __r; })
> > > +#ifdef CONFIG_64BIT
> > > +#define readq(addr) \
> > > +	({ u64 __r = le64_to_cpu(__raw_readq(addr)); rmb(); __r; })
> > > +#endif
> > > +
> > > +#define writeb(b, addr) \
> > > +	({ wmb(); __raw_writeb(b, addr); })
> > > +#define writew(b, addr) \
> > > +	({ wmb(); __raw_writew(cpu_to_le16(b), addr); })
> > > +#define writel(b, addr) \
> > > +	({ wmb(); __raw_writel(cpu_to_le32(b), addr); })
> > > +#ifdef CONFIG_64BIT
> > > +#define writeq(b, addr) \
> > > +	({ wmb(); __raw_writeq(cpu_to_le64(b), addr); })
> > > +#endif
> > > +
> > > +extern void read_len(const volatile void *addr, void *buf, unsigned len);
> > > +extern void write_len(volatile void *addr, const void *buf, unsigned len);
> > > +
> > > +#endif
> > > -- 
> > > 1.8.1.4
> > 
> > I didn't review all these defines and raw_ functions carefully as I
> > assume the individual functions and defines are copied verbatim from the
> > kernel?
> >
> 
> You should take a look, as they're not verbatim - although almost. I can't
> recall where I might have diverged, I just know I didn't copy+paste :-)
> 

Wonderful, I'll take a look :)