Fix race condition in DMA

The high-priority bridge ISR modifies CHID, and if that preempts these critical sections, they would accidentally configure the bridge DMA channel instead of their own.
tessel · Aug 26, 2016 · 120ad3c · 120ad3c
1 parent fc59f05
commit 120ad3c
Show file tree

Hide file tree

Showing 4 changed files with 26 additions and 12 deletions.
diff --git a/common/dma.c b/common/dma.c
@@ -21,8 +21,24 @@ void dma_init() {
 }
 
 void dma_abort(DmaChan chan) {
+    __disable_irq();
     DMAC->CHID.reg = chan;
     DMAC->CHCTRLA.reg = 0;
+    __enable_irq();
+}
+
+void dma_start(DmaChan chan) {
+    __disable_irq();
+    DMAC->CHID.reg = chan;
+    DMAC->CHCTRLA.reg = DMAC_CHCTRLA_ENABLE;
+    __enable_irq();
+}
+
+void dma_enable_interrupt(DmaChan chan) {
+    __disable_irq();
+    DMAC->CHID.reg = chan;
+    DMAC->CHINTENSET.reg = DMAC_CHINTENSET_TCMPL | DMAC_CHINTENSET_TERR;
+    __enable_irq();
 }
 
 u32 dma_remaining(DmaChan chan) {
@@ -78,24 +94,21 @@ void dma_link_chain(DmacDescriptor* chain, u32 count) {
 }
 
 void dma_start_descriptor(DmaChan chan, DmacDescriptor* chain) {
-    DMAC->CHID.reg = chan;
-    DMAC->CHCTRLA.reg = 0;
+    dma_abort(chan);
     memcpy(&dma_descriptors[chan], &chain[0], sizeof(DmacDescriptor));
-    DMAC->CHCTRLA.reg = DMAC_CHCTRLA_ENABLE;
+    dma_start(chan);
 }
 
 void dma_sercom_start_tx(DmaChan chan, SercomId id, u8* src, unsigned size) {
-    DMAC->CHID.reg = chan;
-    DMAC->CHCTRLA.reg = 0;
+    dma_abort(chan);
     dma_fill_sercom_tx(&dma_descriptors[chan], id, src, size);
     dma_descriptors[chan].DESCADDR.reg = 0;
-    DMAC->CHCTRLA.reg = DMAC_CHCTRLA_ENABLE;
+    dma_start(chan);
 }
 
 void dma_sercom_start_rx(DmaChan chan, SercomId id, u8* dst, unsigned size) {
-    DMAC->CHID.reg = chan;
-    DMAC->CHCTRLA.reg = 0;
+    dma_abort(chan);
     dma_fill_sercom_rx(&dma_descriptors[chan], id, dst, size);
     dma_descriptors[chan].DESCADDR.reg = 0;
-    DMAC->CHCTRLA.reg = DMAC_CHCTRLA_ENABLE;
+    dma_start(chan);
 }
diff --git a/common/hw.h b/common/hw.h
@@ -187,6 +187,7 @@ void dma_init();
 void dma_sercom_start_tx(DmaChan chan, SercomId id, u8* src, unsigned size);
 void dma_sercom_start_rx(DmaChan chan, SercomId id, u8* dst, unsigned size);
 void dma_abort(DmaChan chan);
+void dma_enable_interrupt(DmaChan chan);
 void dma_fill_sercom_tx(DmacDescriptor* desc, SercomId id, u8 *src, unsigned size);
 void dma_fill_sercom_rx(DmacDescriptor* desc, SercomId id, u8 *dst, unsigned size);
 void dma_sercom_configure_tx(DmaChan chan, SercomId id);

diff --git a/firmware/flash.c b/firmware/flash.c
@@ -59,7 +59,7 @@ void flash_init() {
     sercom_spi_master_init(SERCOM_BRIDGE, FLASH_DIPO, FLASH_DOPO, 0, 0, SERCOM_SPI_BAUD_12MHZ);
     dma_sercom_configure_tx(DMA_FLASH_TX, SERCOM_BRIDGE);
     dma_sercom_configure_rx(DMA_FLASH_RX, SERCOM_BRIDGE);
-    DMAC->CHINTENSET.reg = DMAC_CHINTENSET_TCMPL | DMAC_CHINTENSET_TERR; // ID depends on prev call
+    dma_enable_interrupt(DMA_FLASH_RX);
 
     pin_low(PIN_SOC_RST);
     pin_out(PIN_SOC_RST);

diff --git a/firmware/port.c b/firmware/port.c
@@ -425,7 +425,7 @@ ExecStatus port_begin_cmd(PortData *p) {
                 !!(p->arg[0] & FLAG_SPI_CPOL), !!(p->arg[0] & FLAG_SPI_CPHA), p->arg[1]);
             dma_sercom_configure_tx(p->dma_tx, p->port->spi);
             dma_sercom_configure_rx(p->dma_rx, p->port->spi);
-            DMAC->CHINTENSET.reg = DMAC_CHINTENSET_TCMPL | DMAC_CHINTENSET_TERR; // ID depends on prev call
+            dma_enable_interrupt(p->dma_rx);
             pin_mux(p->port->mosi);
             pin_mux(p->port->miso);
             pin_mux(p->port->sck);
@@ -477,7 +477,7 @@ ExecStatus port_begin_cmd(PortData *p) {
             sercom_uart_init(p->port->uart_i2c, p->port->uart_dipo,
                 p->port->uart_dopo, (p->arg[0] << 8) + p->arg[1]); // 63019
             dma_sercom_configure_tx(p->dma_tx, p->port->uart_i2c);
-            DMAC->CHINTENSET.reg = DMAC_CHINTENSET_TCMPL | DMAC_CHINTENSET_TERR;
+            dma_enable_interrupt(p->dma_tx);
 
             p->mode = MODE_UART;