Fix volatileness of variables

Quite often, variables that need to be read/written using volatile
accesses are only read using volatile accesses (with ACCESS macro). They
need to be written using volatile writes as well to guarantee correct
ordering with locking functions as they do not introduce a compiler
memory barrier.

All the following statement is valid if considering a CPU that does not
need runtime barriers (DMB, exclusive atomic instructions, etc.) to have
predictable ordering between reads and writes on different threads
(multicore configuration). On the ARM architecture, these runtime
barriers are needed in addition to what the C standard guarantees. These
runtime barriers are typically provided by locking functions, but this
is not sufficient when synchronizing more than just memory accesses (for
example system register access as it is the case in the TF and TFTF).

Locking functions are implemented either as volatile inline assembly, or
can be considered as complete "volatile" blackboxes by the compiler as it
is implemented in pure assembly.

Volatile access only gives ordering guarantees between volatile accesses
in a specific thread, and does not influence the ordering of the
non-volatile accesses. It also gives no guarantee about the ordering
between volatile accesses in different threads. Only a compiler memory
barrier can guarantee an ordering between all kind of access.

Even though GCC currently seems to be quite conservative in its memory
analysis, it could very well determine that a non-volatile variable
local to the compilation unit never sees its address taken and that no
return value of any function depends on this variable, and therefore
that the locking function has no way of seeing it. This allows the
compiler to move the non-volatile write to this variable accross a
function call like spin_lock(). All these behaviors have been tested and
GCC even completely removes the storage space used by the variable if no
volatile access is made or if the variable is not "exposed" in any way.

As these variables are usually only read and written once or in polling
loops, all there accesses are required to be volatile, and using an
ACCESS macro brings no optimization compared to making the variables
volatile themselves.

The Linux kernels' document
Documentation/volatile-considered-harmful.txt does makes the assumption
that all volatile variables should be accessed within locked sections,
and that lock functions act as compiler memory barrier. This is not the
case in our code-base so volatile is still needed.

This patch makes this variables volatile and remove ACCESS macro usage
that becomes useless.

Also, remove the ACCESS macro as it is not needed anymore.

Change-Id: I69c48e28116118e6ff8e089ae8e1fe22c484f3c0
Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>

4 files changed, 22 insertions, 21 deletions

diff --git a/tests/runtime_services/standard_service/psci/api_tests/cpu_suspend/test_suspend.c b/tests/runtime_services/standard_service/psci/api_tests/cpu_suspend/test_suspend.c
index d9b0633..f16eee3 100644
--- a/tests/runtime_services/standard_service/psci/api_tests/cpu_suspend/test_suspend.c
+++ b/tests/runtime_services/standard_service/psci/api_tests/cpu_suspend/test_suspend.c
@@ -60,7 +60,7 @@ static event_t cpu_ready[PLATFORM_CORE_COUNT];
 static event_t event_received_wake_irq[PLATFORM_CORE_COUNT];
 
 /* Variable used to confirm the CPU is woken up by IRQ_WAKE_SGI or Timer IRQ */
-static int requested_irq_received[PLATFORM_CORE_COUNT];
+static volatile int requested_irq_received[PLATFORM_CORE_COUNT];
 
 static int requested_irq_handler(void *data)
 {
@@ -128,7 +128,7 @@ static test_result_t suspend_non_lead_cpu(void)
 	isb();
 
 	/* Wait until the IRQ wake interrupt is received */
-	while (!ACCESS(requested_irq_received[core_pos]))
+	while (!requested_irq_received[core_pos])
 		;
 
 	tftf_send_event(&event_received_wake_irq[core_pos]);
diff --git a/tests/runtime_services/standard_service/psci/api_tests/system_suspend/test_psci_system_suspend.c b/tests/runtime_services/standard_service/psci/api_tests/system_suspend/test_psci_system_suspend.c
index e7190b1..1f440fb 100644
--- a/tests/runtime_services/standard_service/psci/api_tests/system_suspend/test_psci_system_suspend.c
+++ b/tests/runtime_services/standard_service/psci/api_tests/system_suspend/test_psci_system_suspend.c
@@ -62,10 +62,10 @@ static event_t cpu_ready[PLATFORM_CORE_COUNT];
 static event_t sgi_received[PLATFORM_CORE_COUNT];
 static event_t waitq[PLATFORM_CORE_COUNT];
 
-static int wakeup_irq_rcvd[PLATFORM_CORE_COUNT];
-static unsigned int sgi_handled[PLATFORM_CORE_COUNT];
+static volatile int wakeup_irq_rcvd[PLATFORM_CORE_COUNT];
+static volatile unsigned int sgi_handled[PLATFORM_CORE_COUNT];
 static sgi_data_t sgi_data;
-static int cpu_ref_count;
+static volatile int cpu_ref_count;
 
 extern unsigned long __RO_START__;
 #define TFTF_RO_START (unsigned long)(&__RO_START__)
@@ -121,7 +121,7 @@ static test_result_t sys_suspend_from_all_cores(void)
 								NULL, NULL);
 
 	/* Wait until the IRQ wake interrupt is received */
-	while (!ACCESS(wakeup_irq_rcvd[core_pos]))
+	while (!wakeup_irq_rcvd[core_pos])
 		;
 
 	if (ret) {
@@ -155,7 +155,7 @@ static test_result_t sys_suspend_from_all_cores(void)
 		 * requires more than one CPU to be in the test to detect that the
 		 * test has not finished.
 		 */
-		while (!ACCESS(cpu_ref_count))
+		while (!cpu_ref_count)
 			;
 	}
 
@@ -207,7 +207,7 @@ test_result_t test_system_suspend_from_all_cores(void)
 	 * requires more than one CPU to be in the test to detect that the
 	 * test has not finished.
 	 */
-	while (!ACCESS(cpu_ref_count))
+	while (!cpu_ref_count)
 		;
 
 	return TEST_RESULT_SUCCESS;
@@ -275,7 +275,7 @@ static test_result_t invalid_entrypoint_for_sys_suspend(void)
 		 * requires more than one CPU to be in the test to detect that the
 		 * test has not finished.
 		 */
-		while (!ACCESS(cpu_ref_count))
+		while (!cpu_ref_count)
 			;
 	}
 
@@ -326,7 +326,7 @@ test_result_t test_system_suspend_invalid_entrypoint(void)
 	 * requires more than one CPU to be in the test to detect that the
 	 * test has not finished.
 	 */
-	while (!ACCESS(cpu_ref_count))
+	while (!cpu_ref_count)
 		;
 
 	return TEST_RESULT_SUCCESS;
@@ -357,7 +357,7 @@ static test_result_t non_lead_cpu_sgi_test(void)
 	tftf_send_event(&cpu_ready[core_pos]);
 
 	/* Wait for SGI */
-	while (ACCESS(sgi_handled[core_pos]) == 0)
+	while (sgi_handled[core_pos] == 0)
 		;
 	/* Send event to indicate reception of SGI */
 	tftf_send_event(&sgi_received[core_pos]);
@@ -411,7 +411,7 @@ test_result_t test_psci_sys_susp_multiple_iteration(void)
 		tftf_program_timer_and_sys_suspend(
 				PLAT_SUSPEND_ENTRY_TIME, &timer_ret, &psci_ret);
 
-		while (!ACCESS(wakeup_irq_rcvd[core_pos]))
+		while (!wakeup_irq_rcvd[core_pos])
 			;
 
 		if (psci_ret != PSCI_E_SUCCESS) {
@@ -530,13 +530,13 @@ test_result_t test_psci_sys_susp_pending_irq(void)
 	 * test case failed.
 	 *
 	 */
-	if (ACCESS(wakeup_irq_rcvd[core_pos])) {
+	if (wakeup_irq_rcvd[core_pos]) {
 		tftf_testcase_printf("Timer irq received\n");
 		ret = TEST_RESULT_FAIL;
 	}
 
 	/* Wait for the SGI to be handled */
-	while (ACCESS(sgi_handled[core_pos]) == 0)
+	while (sgi_handled[core_pos] == 0)
 		;
 
 	/* Verify the sgi data received by the SGI handler */
@@ -612,7 +612,7 @@ test_result_t test_psci_sys_susp_validate_ram(void)
 	tftf_program_timer_and_sys_suspend(SUSPEND_TIME_10_SECS,
 						     &timer_ret, &psci_ret);
 
-	while (!ACCESS(wakeup_irq_rcvd[core_pos]))
+	while (!wakeup_irq_rcvd[core_pos])
 		;
 	if (psci_ret == PSCI_E_SUCCESS) {
 		/*
diff --git a/tests/runtime_services/standard_service/psci/api_tests/validate_power_state/test_validate_power_state.c b/tests/runtime_services/standard_service/psci/api_tests/validate_power_state/test_validate_power_state.c
index 5116668..25febde 100644
--- a/tests/runtime_services/standard_service/psci/api_tests/validate_power_state/test_validate_power_state.c
+++ b/tests/runtime_services/standard_service/psci/api_tests/validate_power_state/test_validate_power_state.c
@@ -42,7 +42,7 @@
 #include <tftf_lib.h>
 
 static event_t cpu_ready[PLATFORM_CORE_COUNT];
-static unsigned int sgi_received[PLATFORM_CORE_COUNT];
+static volatile unsigned int sgi_received[PLATFORM_CORE_COUNT];
 
 static test_result_t (*psci_validate_test_function)(void);
 
@@ -451,7 +451,7 @@ static test_result_t test_execute_test_function(void)
 	enable_irq();
 	isb();
 
-	while (!ACCESS(sgi_received[core_pos]))
+	while (!sgi_received[core_pos])
 		;
 
 	tftf_irq_disable(IRQ_NS_SGI_0);
diff --git a/tests/runtime_services/standard_service/psci/system_tests/test_psci_on_off_suspend_stress.c b/tests/runtime_services/standard_service/psci/system_tests/test_psci_on_off_suspend_stress.c
index 5fa6af8..07359a0 100644
--- a/tests/runtime_services/standard_service/psci/system_tests/test_psci_on_off_suspend_stress.c
+++ b/tests/runtime_services/standard_service/psci/system_tests/test_psci_on_off_suspend_stress.c
@@ -57,10 +57,11 @@ static cpu_pm_ops_desc_t device_pm_ops_desc
 static cpu_pm_ops_desc_t normal_pm_ops_desc;
 
 static event_t cpu_booted[PLATFORM_CORE_COUNT];
-static volatile unsigned int start_test, exit_test;
+static volatile unsigned int start_test;
+static unsigned int exit_test;
 static unsigned int power_state;
 /* The target for CPU ON requests */
-static unsigned long long target_mpid;
+static volatile unsigned long long target_mpid;
 
 static spinlock_t counter_lock;
 static volatile unsigned int cpu_on_count;
@@ -355,7 +356,7 @@ static test_result_t secondary_cpu_on_race_test(void)
 	tftf_send_event(&cpu_booted[core_pos]);
 
 	/* Wait for start flag */
-	while (ACCESS(start_test) == 0)
+	while (start_test == 0)
 		;
 
 	do {
@@ -364,7 +365,7 @@ static test_result_t secondary_cpu_on_race_test(void)
 		 * The target mpid will be target for CPU ON requests by other
 		 * cores.
 		 */
-		if (mpid == ACCESS(target_mpid))
+		if (mpid == target_mpid)
 			return TEST_RESULT_SUCCESS;
 
 		ret = test_cpu_on_race();