[Prism54-devel] On the use of "volatile"

[Jens Maurer]

Hello!

There are some places where the current driver source uses "volatile",
notably "struct islpci_mib" and pointers to the device's control block.

I believe that both uses of "volatile" are not required, or even
misleading, and thus the "volatile" should be removed.

"volatile" is a means to tell the compiler that the variable being
accessed may change in ways the compiler cannot fathom.  For example,
in a C application program, any variable changed from a signal
handler and also accessed by the main thread of control should be
marked "volatile" so that the compiler does not presume the
variable remains unchanged (and thereby might decide to cache it
in a register) when it actually might be changed on the
(unforeseeable) invocation of the signal handler.

For device driver programming, the situation is different.

First, any locking primitive such as a semaphore or a spinlock will
implicitly tell the compiler to consider *any* variable changed,
and thus not maintain any register caches across a lock or unlock.
(More precisely, it is only logically required to consider changed
any variable that is accessed in any critical section governed by
the lock, but we don't have a way to tell the compiler that.)
If you look at asm/spinlock.h, you'll see that the gcc __asm__
statements there include "memory" in the list of "registers"
changed by the assembly statement, similar for semaphores.  All
access to "struct islpci_mib" is protected by its semaphore,
therefore "volatile" is not required there.

Regarding the control block, "volatile" is not only unnecessary,
but even misleading.  Remember that "volatile" is a hint to the
compiler, and nothing more.  However, the control block is a
structure shared between the device and the host, residing in
host memory.  So, whenever modifying the control block, the driver
has to ensure that the modification becomes visisble to the device,
and in the correct order.  For CPUs with so-called "out-of-order
stores" (early Pentium Pros with a bug, P4s, and some non-x86 CPUs),
store instructions may not hit the main memory in the same order
than they showed up in the program.  Since "volatile" is just a
compiler hint, "volatile" helps nothing for this issue.  The
correct remedy is to use a memory barrier, as demonstrated in
my islpci_mgt.c rework: The full fragment descriptor must hit
the device-visible control block in main memory before we dare
increment the driver_curr_frag.

I therefore suggest that we remove "volatile" for those two cases,
and audit any other place where we still have "volatile" in use.

Jens Maurer