FIXED: Race conditions in FIQ synchronization

Affected using FIQ together with EnterCritical() or class CSpinLock.

Issue rsta2#87
Reported by @stephanedamo

CREDITS

@@ -46,3 +46,6 @@ USB HID class gamepad driver
 
 @Matheus-Garbelini
 Adding bootloader support and tools
+
+Stephane Damo
+Helping to fix FIQ synchronization issue

include/circle/spinlock.h

@@ -2,7 +2,7 @@
 // spinlock.h
 //
 // Circle - A C++ bare metal environment for Raspberry Pi
-// Copyright (C) 2015-2017  R. Stange <[email protected]>
+// Copyright (C) 2015-2018  R. Stange <[email protected]>
 // 
 // This program is free software: you can redistribute it and/or modify
 // it under the terms of the GNU General Public License as published by
@@ -43,7 +43,6 @@ class CSpinLock
 
 private:
 	unsigned m_nTargetLevel;
-	u32 m_nCPSR[CORES];
 
 	u32 m_nLocked;
 

lib/spinlock.cpp

@@ -2,7 +2,7 @@
 // spinlock.cpp
 //
 // Circle - A C++ bare metal environment for Raspberry Pi
-// Copyright (C) 2015-2017  R. Stange <[email protected]>
+// Copyright (C) 2015-2018  R. Stange <[email protected]>
 // 
 // This program is free software: you can redistribute it and/or modify
 // it under the terms of the GNU General Public License as published by
@@ -44,18 +44,7 @@ void CSpinLock::Acquire (void)
 {
 	if (m_nTargetLevel >= IRQ_LEVEL)
 	{
-		asm volatile
-		(
-			"mrs %0, cpsr\n"
-			"cpsid i\n"
-
-			: "=r" (m_nCPSR[CMultiCoreSupport::ThisCore ()])
-		);
-
-		if (m_nTargetLevel == FIQ_LEVEL)
-		{
-			DisableFIQs ();
-		}
+		EnterCritical (m_nTargetLevel);
 	}
 
 	if (s_bEnabled)
@@ -102,13 +91,8 @@ void CSpinLock::Release (void)
 
 	if (m_nTargetLevel >= IRQ_LEVEL)
 	{
-		asm volatile
-		(
-			"msr cpsr_c, %0\n"
-
-			: : "r" (m_nCPSR[CMultiCoreSupport::ThisCore ()])
-		);
-	};
+		LeaveCritical ();
+	}
 }
 
 void CSpinLock::Enable (void)

lib/synchronize.cpp

@@ -2,7 +2,7 @@
 // synchronize.cpp
 //
 // Circle - A C++ bare metal environment for Raspberry Pi
-// Copyright (C) 2014-2017  R. Stange <[email protected]>
+// Copyright (C) 2014-2018  R. Stange <[email protected]>
 // 
 // This program is free software: you can redistribute it and/or modify
 // it under the terms of the GNU General Public License as published by
@@ -61,15 +61,16 @@ void EnterCritical (unsigned nTargetLevel)
 	// if we are already on FIQ_LEVEL, we must not go back to IRQ_LEVEL here
 	assert (nTargetLevel == FIQ_LEVEL || !(nCPSR & 0x40));
 
-	DisableIRQs ();
-	if (nTargetLevel == FIQ_LEVEL)
-	{
-		DisableFIQs ();
-	}
+	asm volatile ("cpsid if");	// disable both IRQ and FIQ
 
 	assert (s_nCriticalLevel[nCore] < MAX_CRITICAL_LEVEL);
 	s_nCPSR[nCore][s_nCriticalLevel[nCore]++] = nCPSR;
 
+	if (nTargetLevel == IRQ_LEVEL)
+	{
+		EnableFIQs ();
+	}
+
 	DataMemBarrier ();
 }
 
@@ -81,6 +82,8 @@ void LeaveCritical (void)
 
 	DataMemBarrier ();
 
+	DisableFIQs ();
+
 	assert (s_nCriticalLevel[nCore] > 0);
 	u32 nCPSR = s_nCPSR[nCore][--s_nCriticalLevel[nCore]];
 
@@ -102,22 +105,25 @@ void EnterCritical (unsigned nTargetLevel)
 	// if we are already on FIQ_LEVEL, we must not go back to IRQ_LEVEL here
 	assert (nTargetLevel == FIQ_LEVEL || !(nCPSR & 0x40));
 
-	DisableIRQs ();
-	if (nTargetLevel == FIQ_LEVEL)
-	{
-		DisableFIQs ();
-	}
+	asm volatile ("cpsid if");	// disable both IRQ and FIQ
 
 	assert (s_nCriticalLevel < MAX_CRITICAL_LEVEL);
 	s_nCPSR[s_nCriticalLevel++] = nCPSR;
 
+	if (nTargetLevel == IRQ_LEVEL)
+	{
+		EnableFIQs ();
+	}
+
 	DataMemBarrier ();
 }
 
 void LeaveCritical (void)
 {
 	DataMemBarrier ();
 
+	DisableFIQs ();
+
 	assert (s_nCriticalLevel > 0);
 	u32 nCPSR = s_nCPSR[--s_nCriticalLevel];