Should io(read|write)(8|16|32)_rep take (const|) volatile u(8|16|32) __iomem *addr?

Should io(read|write)(8|16|32)_rep take (const|) volatile u(8|16|32) __iomem *addr?

[Michael K. Edwards]

Sorry, I wrote this:
> (This is not an ARM-specific question, although the example is on ARM.)
and then sent it to linux-arm-kernel anyway.  :-P


I'm writing an ALSA driver for an ARM SoC whose PCM audio interface is
MMIO.  ALSA provides copy_from_user_toio and copy_to_user_fromio
routines for this purpose, but the results are not sane, perhaps
because STMIA to I/O addresses is bollixed on this core.  I suspect
that the right thing to do is to use iowrite32_rep and ioread32_rep,
approximately like so (slightly modified ALSA code):

/**
 * copy_u32s_to_user_fromio - copy data from mmio-space to user-space
 * @dst: the destination pointer on user-space
 * @src: the source pointer on mmio
 * @count: the data size to copy in u32s
 *
 * Copies the data from mmio-space to user-space.
 *
 * Returns zero if successful, or non-zero on failure.
 */
static int copy_u32s_to_user_fromio(void __user *dst, const volatile
void __iomem *src, size_t count)
{
        u32 buf[80];
        count <<= 2;
        while (count) {
                size_t c = count;
                if (c > sizeof(buf))
                        c = sizeof(buf);
                ioread32_rep(src, buf, c>>2);
                if (copy_to_user(dst, buf, c))
                        return -EFAULT;
                count -= c;
                dst += c;
                src += c;
        }
        return 0;
}

/**
 * copy_u32s_from_user_toio - copy data from user-space to mmio-space
 * @dst: the destination pointer on mmio-space
 * @src: the source pointer on user-space
 * @count: the data size to copy in u32s
 *
 * Copies the data from user-space to mmio-space.
 *
 * Returns zero if successful, or non-zero on failure.
 */
static int copy_u32s_from_user_toio(volatile void __iomem *dst, const
void __user *src, size_t count)
{
        u32 buf[80];
        count <<= 2;
        while (count) {
                size_t c = count;
                if (c > sizeof(buf))
                        c = sizeof(buf);
                if (copy_from_user(buf, src, c))
                        return -EFAULT;
                iowrite32_rep(dst, buf, c>>2);
                count -= c;
                dst += c;
                src += c;
        }
        return 0;
}

Compiling (with GCC 4.1.2 RC1) gives these warnings:

foo.c: In function 'copy_u32s_to_user_fromio':
foo.c:56: warning: passing argument 1 of '__raw_readsl' discards
qualifiers from pointer target type
foo.c: In function 'copy_u32s_from_user_toio':
foo.c:86: warning: passing argument 1 of '__raw_writesl' discards
qualifiers from pointer target type

The qualifiers being discarded are the "volatile"s, which are indeed
not present in the declarations of these functions (which happen to be
ARM's implementation of io(read|write)32_rep):

include/asm-arm/io.h:extern void __raw_writesb(void __iomem *addr,
const void *data, int bytelen);
include/asm-arm/io.h:extern void __raw_writesw(void __iomem *addr,
const void *data, int wordlen);
include/asm-arm/io.h:extern void __raw_writesl(void __iomem *addr,
const void *data, int longlen);
include/asm-arm/io.h:extern void __raw_readsb(const void __iomem
*addr, void *data, int bytelen);
include/asm-arm/io.h:extern void __raw_readsw(const void __iomem
*addr, void *data, int wordlen);
include/asm-arm/io.h:extern void __raw_readsl(const void __iomem
*addr, void *data, int longlen);

It looks to me, by comparison to memcpy_(from|to)io, as if the
volatiles ought to be there.  It also looks to me like the void *
parameters should be u(8|16|32) * instead, so the compiler knows what
alignment to expect (the implementations would generally fail or suck
on non-aligned arguments).  (That would also be more consistent with
the fact that the length parameters are in (8|16|32)-bit units, not
octets.)

Opinions?

Cheers,
- Michael

Re: Should io(read|write)(8|16|32)_rep take (const|) volatile u(8|16|32) __iomem *addr?

[H. Peter Anvin]

Michael K. Edwards wrote:
> 
> It looks to me, by comparison to memcpy_(from|to)io, as if the
> volatiles ought to be there.  It also looks to me like the void *
> parameters should be u(8|16|32) * instead, so the compiler knows what
> alignment to expect (the implementations would generally fail or suck
> on non-aligned arguments).  (That would also be more consistent with
> the fact that the length parameters are in (8|16|32)-bit units, not
> octets.)
> 
> Opinions?
> 

The real question is whether or not gcc does anything sane with "const 
volatile", which may incorrectly sound oxymoronic to some people (it's 
not, const means "this element must not be written to" and volatile 
means "reading or writing this element has side effects".)

I would argue the right thing to do is to do the patch assuming gcc is 
sane, and let it sit in -mm for a kernel cycle or two.

	-hpa

Re: Should io(read|write)(8|16|32)_rep take (const|) volatile u(8|16|32) __iomem *addr?

[Jeff Garzik]

Volatile is usually reserved for a specific need on a specific arch.  I 
doubt it is correct to force it on all arches.

	Jeff

Re: Should io(read|write)(8|16|32)_rep take (const|) volatile u(8|16|32) __iomem *addr?

[H. Peter Anvin]

Jeff Garzik wrote:
> Volatile is usually reserved for a specific need on a specific arch.  I 
> doubt it is correct to force it on all arches.

They already are volatile; the issue is adding "const".

All io(read|write)* pointers are volatile, IIRC.

	-hpa

Re: Should io(read|write)(8|16|32)_rep take (const|) volatile u(8|16|32) __iomem *addr?

[Jeff Garzik]

H. Peter Anvin wrote:
> Jeff Garzik wrote:
>> Volatile is usually reserved for a specific need on a specific arch.  
>> I doubt it is correct to force it on all arches.
> 
> They already are volatile; the issue is adding "const".
> 
> All io(read|write)* pointers are volatile, IIRC.

No, none are volatile, hence my comment.

> [jgarzik@pretzel linux-2.6]$ grep volatile lib/iomap.c include/asm-generic/iomap.h 
> [jgarzik@pretzel linux-2.6]$ 

Maybe you were thinking about writel() and friends, whose implementation 
(not prototype!) includes use of volatile.

Regards,

	Jeff

Re: Should io(read|write)(8|16|32)_rep take (const|) volatile u(8|16|32) __iomem *addr?

[Michael K. Edwards]

Preface: I am aware that other people here know far more that I do
about the details of C semantics.  If the inconsistency is deliberate,
please just point me to the previous LKML thread.

On 2/2/07, Jeff Garzik <jeff@garzik.org> wrote:
> H. Peter Anvin wrote:
> > Jeff Garzik wrote:
> >> Volatile is usually reserved for a specific need on a specific arch.
> >> I doubt it is correct to force it on all arches.
> >
> > They already are volatile; the issue is adding "const".

No, const is already there on the ioread.*_rep declarations.  But
since you mention it, it's missing from ioread(8|16|32) on some
(most?) arches (it's present at least on ixp4xx).  It would probably
be good to add it, so that a driver writer can declare that some
registers are read-only (ideally as const members of a struct; does
GCC support that?) and catch accidental iowrites to them at compile
time.

> > All io(read|write)* pointers are volatile, IIRC.
>
> No, none are volatile, hence my comment.

My concern is the lack of volatile's in the io(read|write).*_rep
declarations, by comparison to memcpy_(from|to)io.  I'm willing to be
convinced that the volatile should be in the implementation, not the
interface; and indeed, that's how the individual
io(read|write)(8|16|32) functions are declared (on arches where they
don't boil down to macros that cast with __force).  But that's not how
memcpy_(from|to)io or ALSA's copy_(from|to)_user_(to|from)io are
declared.  Which is right?

I also lean towards u8/u16/u32 for consistency with the normalization
of the length parameters (and possibly for catching alignment issues
at compile time), but that's separate.

> > [jgarzik@pretzel linux-2.6]$ grep volatile lib/iomap.c include/asm-generic/iomap.h
> > [jgarzik@pretzel linux-2.6]$
>
> Maybe you were thinking about writel() and friends, whose implementation
> (not prototype!) includes use of volatile.

Doubtless this is on purpose, but it's not clear to me why that should
be true for these particular functions/macros.  I shouldn't have to
cast away the volatile on a pointer to hardware registers in order to
pass it into writel(), should I?  And it shouldn't be forbidden for
the caller to declare the pointer volatile so that the compiler
catches attempts to pass it into non-volatile interfaces, should it?

My naive opinion is that the semantics of const and volatile ought to
be "you can add it but you can't take it away"; and IIRC this is their
actual semantics in ANSI C.  When writing a driver, I ought to be able
to say, "don't ever write to here (const), and don't ever reorder or
optimize away writes to or reads from there (volatile)".  (I am not
talking about reordering reads and writes to _different_ locations;
I'm aware that hardware support and memory barriers may be needed to
deal with that, and as far as I am concerned the compiler shouldn't
pretend to guarantee that reading *p will happen after writing *q.)

An interface that's declared without "volatile" on its pointers is
implicitly saying that it's not necessarily safe for use on pointers
to volatile data.  It could get inlined and then some access could get
optimized away based on the non-use of the result.  An interface
that's declared with "volatile" on its pointers is saying that it's
going to access them once for each time its source code says it
should, irrespective of inlining and optimization, and won't generate
extra accesses or reorder accesses through the _same_ pointer either.
(C semantics may be that it won't reorder accesses through different
volatile pointers either; I haven't checked, and wouldn't rely on that
because the hardware may do it anway.)

It seems to me that if you want a volatile implementation for volatile
arguments and an optimal implementation for non-volatile arguments you
need to implement as a macro, not an inline function.

Cheers,
- Michael

Re: Should io(read|write)(8|16|32)_rep take (const|) volatile u(8|16|32) __iomem *addr?

[Jeff Garzik]

Michael K. Edwards wrote:
> Doubtless this is on purpose, but it's not clear to me why that should
> be true for these particular functions/macros.  I shouldn't have to
> cast away the volatile on a pointer to hardware registers in order to
> pass it into writel(), should I?  And it shouldn't be forbidden for
> the caller to declare the pointer volatile so that the compiler
> catches attempts to pass it into non-volatile interfaces, should it?


Therein lies your bug:  you should not be annotating your __iomem 
pointers with volatile.

That's why volatile is found in the implementation and not the 
prototypes of the accessor functions.

	Jeff