Fix gpio reinit race condition (#2098)

* fix unregister api for gpio

* consumers

* guard with a critical section

* index

* guard less

* unregister under lock

* fix reinit

* dead flag

* fix most tests

* initialize properly

* initialize properly

* assertions in tests

* fix message

* we must lock earlier to be truly safe

* this was using huge memory

* devirtualize

firmware/controllers/actuators/aux_pid.cpp

@@ -132,11 +132,9 @@ void startAuxPins() {
 }
 
 void stopAuxPins() {
-#if EFI_PROD_CODE
 	for (int i = 0;i < AUX_PID_COUNT;i++) {
-		efiSetPadUnused(activeConfiguration.auxPidPins[i]);
+		instances[i].auxOutputPin.deInit();
 	}
-#endif /* EFI_PROD_CODE */
 }
 
 void initAuxPid(Logging *sharedLogger) {

firmware/controllers/algo/engine.h

@@ -195,12 +195,6 @@ public:
 	 */
 	efitimems64_t callFromPitStopEndTime = 0;
 
-	/**
-	 * This flag indicated a big enough problem that engine control would be
-	 * prohibited if this flag is set to true.
-	 */
-	bool withError = false;
-
 	RpmCalculator rpmCalculator;
 	persistent_config_s *config = nullptr;
 	/**

firmware/controllers/system/efi_gpio.cpp

@@ -9,6 +9,7 @@
 #include "global.h"
 #include "engine.h"
 #include "efi_gpio.h"
+#include "os_access.h"
 #include "drivers/gpio/gpio_ext.h"
 #include "perf_trace.h"
 #include "engine_controller.h"
@@ -90,12 +91,9 @@ void RegisteredOutputPin::init(DECLARE_ENGINE_PARAMETER_SIGNATURE) {
 }
 
 void RegisteredOutputPin::unregister() {
-#if EFI_PROD_CODE
-	brain_pin_e        curPin = *(brain_pin_e       *) ((void *) (&((char*)&activeConfiguration)[pinOffset]));
-    if (isPinConfigurationChanged()) {
-    	unregisterOutput(curPin);
-    }
-#endif // EFI_PROD_CODE
+	if (isPinConfigurationChanged()) {
+		OutputPin::deInit();
+	}
 }
 
 #define CONFIG_OFFSET(x) x##_offset
@@ -150,13 +148,13 @@ EnginePins::EnginePins() :
 #if EFI_PROD_CODE
 #define unregisterOutputIfPinChanged(output, pin) {                                \
 	if (isConfigurationChanged(pin)) {                                             \
-		(output).unregisterOutput(activeConfiguration.pin);                        \
+		(output).deInit();                        \
 	}                                                                              \
 }
 
 #define unregisterOutputIfPinOrModeChanged(output, pin, mode) {                    \
 	if (isPinOrModeChanged(pin, mode)) {                                           \
-		(output).unregisterOutput(activeConfiguration.pin);                        \
+		(output).deInit();                        \
 	}                                                                              \
 }
 
@@ -191,15 +189,13 @@ void EnginePins::unregisterPins() {
 	for (int i = 0;i < FSIO_COMMAND_COUNT;i++) {
 		unregisterOutputIfPinChanged(fsioOutputs[i], fsioOutputPins[i]);
 	}
-
+#endif /* EFI_PROD_CODE */
 
 	RegisteredOutputPin * pin = registeredOutputHead;
 	while (pin != nullptr) {
 		pin->unregister();
 		pin = pin->next;
 	}
-
-#endif /* EFI_PROD_CODE */
 }
 
 void EnginePins::debug() {
@@ -387,7 +383,6 @@ bool OutputPin::getAndSet(int logicValue) {
 #if EFI_PROD_CODE
 void OutputPin::setOnchipValue(int electricalValue) {
 	palWritePad(port, pin, electricalValue);
-
 }
 #endif // EFI_PROD_CODE
 
@@ -397,12 +392,21 @@ void OutputPin::setValue(int logicValue) {
 //	ScopePerf perf(PE::OutputPinSetValue);
 #endif // ENABLE_PERF_TRACE
 
-#if EFI_PROD_CODE
+	// Always store the current logical value of the pin (so it can be
+	// used internally even if not connected to a real hardware pin)
+	currentLogicValue = logicValue;
+
+	// Nothing else to do if not configured
+	if (brainPin == GPIO_UNASSIGNED) {
+		return;
+	}
+
 	efiAssertVoid(CUSTOM_ERR_6621, modePtr!=NULL, "pin mode not initialized");
 	pin_output_mode_e mode = *modePtr;
 	efiAssertVoid(CUSTOM_ERR_6622, mode <= OM_OPENDRAIN_INVERTED, "invalid pin_output_mode_e");
 	int electricalValue = getElectricalValue(logicValue, mode);
 
+#if EFI_PROD_CODE
 	#if (BOARD_EXT_GPIOCHIPS > 0)
 		if (!this->ext) {
 			setOnchipValue(electricalValue);
@@ -414,13 +418,9 @@ void OutputPin::setValue(int logicValue) {
 	#else
 		setOnchipValue(electricalValue);
 	#endif
-
 #else /* EFI_PROD_CODE */
-	setMockState(brainPin, logicValue);
+	setMockState(brainPin, electricalValue);
 #endif /* EFI_PROD_CODE */
-
-	// Lastly store the current logical value of the pin
-	currentLogicValue = logicValue;
 }
 
 bool OutputPin::getLogicValue() const {
@@ -464,13 +464,19 @@ void OutputPin::initPin(const char *msg, brain_pin_e brainPin) {
 }
 
 void OutputPin::initPin(const char *msg, brain_pin_e brainPin, const pin_output_mode_e *outputMode) {
-#if EFI_UNIT_TEST
-	this->brainPin = brainPin;
-#endif
-
-#if EFI_GPIO_HARDWARE && EFI_PROD_CODE
-	if (brainPin == GPIO_UNASSIGNED)
+	if (brainPin == GPIO_UNASSIGNED) {
 		return;
+	}
+
+	// Enter a critical section so that other threads can't change the pin state out from underneath us
+	chibios_rt::CriticalSectionLocker csl;
+
+	// Check that this OutputPin isn't already assigned to another pin (reinit is allowed to change mode)
+	// To avoid this error, call deInit() first
+	if (this->brainPin != GPIO_UNASSIGNED && this->brainPin != brainPin) {
+		firmwareError(CUSTOM_OBD_PIN_CONFLICT, "outputPin [%s] already assigned, cannot reassign without unregister first", msg);
+		return;
+	}
 
 	if (*outputMode > OM_OPENDRAIN_INVERTED) {
 		firmwareError(CUSTOM_INVALID_MODE_SETTING, "%s invalid pin_output_mode_e %d %s",
@@ -480,6 +486,8 @@ void OutputPin::initPin(const char *msg, brain_pin_e brainPin, const pin_output_
 				);
 		return;
 	}
+
+#if EFI_GPIO_HARDWARE && EFI_PROD_CODE
 	iomode_t mode = (*outputMode == OM_DEFAULT || *outputMode == OM_INVERTED) ?
 		PAL_MODE_OUTPUT_PUSHPULL : PAL_MODE_OUTPUT_OPENDRAIN;
 
@@ -490,39 +498,24 @@ void OutputPin::initPin(const char *msg, brain_pin_e brainPin, const pin_output_
 		ioportid_t port = getHwPort(msg, brainPin);
 		int pin = getHwPin(msg, brainPin);
 
-		/**
-		 * This method is used for digital GPIO pins only, for peripheral pins see mySetPadMode
-		 */
+		// Validate port
 		if (port == GPIO_NULL) {
-			// that's for GRIO_NONE
-			this->port = port;
+			firmwareError(OBD_PCM_Processor_Fault, "OutputPin::initPin got invalid port for pin idx %d", static_cast<int>(brainPin));
 			return;
 		}
 
-		/**
-		 * @brief Initialize the hardware output pin while also assigning it a logical name
-		 */
-		if (this->port != NULL && (this->port != port || this->pin != pin)) {
-			/**
-			 * here we check if another physical pin is already assigned to this logical output
-			 */
-		// todo: need to clear '&outputs' in io_pins.c
-			warning(CUSTOM_OBD_PIN_CONFLICT, "outputPin [%s] already assigned to %x%d", msg, this->port, this->pin);
-			engine->withError = true;
-			return;
-		}
 		this->port = port;
 		this->pin = pin;
 	}
 	#if (BOARD_EXT_GPIOCHIPS > 0)
 		else {
 			this->ext = true;
-			this->brainPin = brainPin;
 		}
 	#endif
-
 #endif // briefly leave the include guard because we need to set default state in tests
 
+	this->brainPin = brainPin;
+
 	// The order of the next two calls may look strange, which is a good observation.
 	// We call them in this order so that the pin is set to a known state BEFORE
 	// it's enabled.  Enabling the pin then setting it could result in a (brief)
@@ -544,22 +537,35 @@ void OutputPin::initPin(const char *msg, brain_pin_e brainPin, const pin_output_
 
 			// if the pin was set to logical 1, then set an error and disable the pin so that things don't catch fire
 			if (logicalValue) {
-				efiSetPadUnused(brainPin);
 				firmwareError(OBD_PCM_Processor_Fault, "%s: startup pin state %s actual value=%d logical value=%d mode=%s", msg, hwPortname(brainPin), actualValue, logicalValue, getPin_output_mode_e(*outputMode));
+				OutputPin::deInit();
 			}
 		}
 	}
 #endif /* EFI_GPIO_HARDWARE */
 }
 
-void OutputPin::unregisterOutput(brain_pin_e oldPin) {
-	if (oldPin != GPIO_UNASSIGNED) {
-		scheduleMsg(logger, "unregistering %s", hwPortname(oldPin));
-#if EFI_GPIO_HARDWARE && EFI_PROD_CODE
-		efiSetPadUnused(oldPin);
-		port = nullptr;
-#endif /* EFI_GPIO_HARDWARE */
+void OutputPin::deInit() {
+	// Unregister under lock - we don't want other threads mucking with the pin while we're trying to turn it off
+	chibios_rt::CriticalSectionLocker csl;
+
+	// nothing to do if not registered in the first place
+	if (brainPin == GPIO_UNASSIGNED) {
+		return;
 	}
+
+#if (BOARD_EXT_GPIOCHIPS > 0)
+	ext = false;
+#endif // (BOARD_EXT_GPIOCHIPS > 0)
+
+	scheduleMsg(logger, "unregistering %s", hwPortname(brainPin));
+
+#if EFI_GPIO_HARDWARE && EFI_PROD_CODE
+	efiSetPadUnused(brainPin);
+#endif /* EFI_GPIO_HARDWARE */
+
+	// Clear the pin so that it won't get set any more
+	brainPin = GPIO_UNASSIGNED;
 }
 
 #if EFI_GPIO_HARDWARE

firmware/controllers/system/efi_gpio.h

@@ -47,10 +47,11 @@ public:
 	 * same as above, with DEFAULT_OUTPUT mode
 	 */
 	void initPin(const char *msg, brain_pin_e brainPin);
+
 	/**
 	 * dissociates pin from this output and un-registers it in pin repository
 	 */
-	void unregisterOutput(brain_pin_e oldPin);
+	void deInit();
 
 	bool isInitialized();
 
@@ -59,19 +60,16 @@ public:
 	void toggle();
 	bool getLogicValue() const;
 
-
 #if EFI_GPIO_HARDWARE
 	ioportid_t port = 0;
 	uint8_t pin = 0;
 #endif /* EFI_GPIO_HARDWARE */
 
+	brain_pin_e brainPin = GPIO_UNASSIGNED;
+
 #if (EFI_GPIO_HARDWARE && (BOARD_EXT_GPIOCHIPS > 0))
 	/* used for external pins */
-	brain_pin_e brainPin;
-	bool ext;
-#elif EFI_SIMULATOR || EFI_UNIT_TEST
-	// used for setMockState
-	brain_pin_e brainPin;
+	bool ext = false;
 #endif /* EFI_GPIO_HARDWARE */
 
 	int8_t currentLogicValue = INITIAL_PIN_STATE;
@@ -85,7 +83,7 @@ private:
 	void setOnchipValue(int electricalValue);
 
 	// 4 byte pointer is a bit of a memory waste here
-	const pin_output_mode_e *modePtr;
+	const pin_output_mode_e *modePtr = nullptr;
 };
 
 /**

firmware/controllers/trigger/trigger_emulator_algo.cpp

@@ -222,12 +222,7 @@ void startTriggerEmulatorPins() {
 
 void stopTriggerEmulatorPins() {
 	for (size_t i = 0; i < efi::size(emulatorOutputs); i++) {
-		brain_pin_e brainPin = activeConfiguration.triggerSimulatorPins[i];
-		if (brainPin != GPIO_UNASSIGNED) {
-#if EFI_PROD_CODE
-			efiSetPadUnused(brainPin);
-#endif // EFI_PROD_CODE
-		}
+		triggerSignal.outputPins[i]->deInit();
 	}
 }
 

firmware/hw_layer/sensors/hip9011.cpp

@@ -385,10 +385,8 @@ static msg_t hipThread(void *arg) {
 }
 
 void stopHip9001_pins() {
-#if EFI_PROD_CODE
-	efiSetPadUnused(activeConfiguration.hip9011IntHoldPin);
-	efiSetPadUnused(activeConfiguration.hip9011CsPin);
-#endif /* EFI_PROD_CODE */
+	intHold.deInit();
+	enginePins.hipCs.deInit();
 }
 
 void startHip9001_pins() {

unit_tests/engine_test_helper.cpp

@@ -51,6 +51,7 @@ EngineTestHelper::EngineTestHelper(engine_type_e engineType, configuration_callb
 	memset(&activeConfiguration, 0, sizeof(activeConfiguration));
 
 	enginePins.reset();
+	enginePins.unregisterPins();
 
 	persistent_config_s *config = &persistentConfig;
 	Engine *engine = &this->engine;
@@ -99,6 +100,8 @@ EngineTestHelper::~EngineTestHelper() {
 	writeEvents(filePath.str().c_str());
 
 	// Cleanup
+	enginePins.reset();
+	enginePins.unregisterPins();
 	Sensor::resetRegistry();
 	memset(mockPinStates, 0, sizeof(mockPinStates));
 }