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@xxxxxxxxxx> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/