Zynq driver: avoid race conditions (#1286)

* Zynq driver: avoid race conditions

* Corrected a comment

Author: htibosch

source/portable/NetworkInterface/Zynq/NetworkInterface.c

@@ -431,10 +431,20 @@ static BaseType_t xZynqNetworkInterfaceOutput( NetworkInterface_t * pxInterface,
         iptraceNETWORK_INTERFACE_TRANSMIT();
 
         /* emacps_send_message() will take ownership of pxBuffer, and
-         * make sure it will get release when bReleaseAfterSend is pdTRUE. */
-        emacps_send_message( &( xEMACpsifs[ xEMACIndex ] ), pxBuffer, bReleaseAfterSend );
+         * make sure it will get release when bReleaseAfterSend is pdTRUE.
+         * The calls to emacps_check_tx() and emacps_send_message()
+         * must be synchronised because they act on the same DMA descriptors.
+         */
+        if( xSemaphoreTake( xEMACpsifs[ xEMACIndex ].tx_mutex, 1000U ) == pdPASS )
+        {
+            emacps_send_message( &( xEMACpsifs[ xEMACIndex ] ), pxBuffer, bReleaseAfterSend );
+            xSemaphoreGive( xEMACpsifs[ xEMACIndex ].tx_mutex );
+            /* The function emacps_send_message() has released the packet. */
+            bReleaseAfterSend = pdFALSE;
+        }
     }
-    else if( bReleaseAfterSend != pdFALSE )
+
+    if( bReleaseAfterSend != pdFALSE )
     {
         /* No link. */
         vReleaseNetworkBufferAndDescriptor( pxBuffer );
@@ -598,9 +608,10 @@ static void prvEMACHandlerTask( void * pvParameters )
     TickType_t xPhyRemTime;
     BaseType_t xResult = 0;
     uint32_t xStatus;
-    const TickType_t ulMaxBlockTime = pdMS_TO_TICKS( 100UL );
+    const TickType_t ulMaxBlockTime = pdMS_TO_TICKS( 100U );
     BaseType_t xEMACIndex = ( BaseType_t ) pvParameters;
     xemacpsif_s * pxEMAC_PS;
+    uint32_t ulISREvents = 0U;
 
     configASSERT( xEMACIndex >= 0 );
     configASSERT( xEMACIndex < XPAR_XEMACPS_NUM_INSTANCES );
@@ -629,27 +640,27 @@ static void prvEMACHandlerTask( void * pvParameters )
         }
         #endif /* ( ipconfigHAS_PRINTF != 0 ) */
 
-        if( ( pxEMAC_PS->isr_events & EMAC_IF_ALL_EVENT ) == 0 )
-        {
-            /* No events to process now, wait for the next. */
-            ulTaskNotifyTake( pdFALSE, ulMaxBlockTime );
-        }
+        xTaskNotifyWait( 0U,                /* ulBitsToClearOnEntry */
+                         EMAC_IF_ALL_EVENT, /* ulBitsToClearOnExit */
+                         &( ulISREvents ),  /* pulNotificationValue */
+                         ulMaxBlockTime );
 
-        if( ( pxEMAC_PS->isr_events & EMAC_IF_RX_EVENT ) != 0 )
+        if( ( ulISREvents & EMAC_IF_RX_EVENT ) != 0 )
         {
-            pxEMAC_PS->isr_events &= ~EMAC_IF_RX_EVENT;
             xResult = emacps_check_rx( pxEMAC_PS, pxMyInterfaces[ xEMACIndex ] );
         }
 
-        if( ( pxEMAC_PS->isr_events & EMAC_IF_TX_EVENT ) != 0 )
+        if( ( ulISREvents & EMAC_IF_TX_EVENT ) != 0U )
         {
-            pxEMAC_PS->isr_events &= ~EMAC_IF_TX_EVENT;
-            emacps_check_tx( pxEMAC_PS );
+            if( xSemaphoreTake( pxEMAC_PS->tx_mutex, 1000U ) == pdPASS )
+            {
+                emacps_check_tx( pxEMAC_PS );
+                xSemaphoreGive( pxEMAC_PS->tx_mutex );
+            }
         }
 
-        if( ( pxEMAC_PS->isr_events & EMAC_IF_ERR_EVENT ) != 0 )
+        if( ( ulISREvents & EMAC_IF_ERR_EVENT ) != 0U )
         {
-            pxEMAC_PS->isr_events &= ~EMAC_IF_ERR_EVENT;
             emacps_check_errors( pxEMAC_PS );
         }
 

source/portable/NetworkInterface/Zynq/x_emacpsif.h

@@ -115,11 +115,14 @@
         volatile int rxHead, rxTail;
         volatile int txHead, txTail;
 
-        volatile int txBusy;
-
-        volatile uint32_t isr_events;
-
         unsigned int last_rx_frms_cntr;
+
+        /* Both the IP- and the EMAC task work on DMA descriptors
+         * for transmission. These function will be protected by 'tx_mutex':
+         *   emacps_send_message()
+         *   emacps_check_tx()
+         */
+        SemaphoreHandle_t tx_mutex;
     } xemacpsif_s;
 
 /*extern xemacpsif_s xemacpsif; */

source/portable/NetworkInterface/Zynq/x_emacpsif_dma.c

@@ -205,6 +205,8 @@ void emacps_check_tx( xemacpsif_s * xemacpsif )
 
 void emacps_send_handler( void * arg )
 {
+    /* This function is running in ISR mode,
+     * called to handle a TX interrupt. */
     xemacpsif_s * xemacpsif;
     BaseType_t xHigherPriorityTaskWoken = pdFALSE;
     BaseType_t xEMACIndex;
@@ -217,15 +219,9 @@ void emacps_send_handler( void * arg )
      * But it forgets to do a read-back. Do so now to avoid ever-returning ISR's. */
     ( void ) XEmacPs_ReadReg( xemacpsif->emacps.Config.BaseAddress, XEMACPS_TXSR_OFFSET );
 
-    /* In this port for FreeRTOS+TCP, the EMAC interrupts will only set a bit in
-     * "isr_events". The task in NetworkInterface will wake-up and do the necessary work.
-     */
-    xemacpsif->isr_events |= EMAC_IF_TX_EVENT;
-    xemacpsif->txBusy = pdFALSE;
-
     if( xEMACTaskHandles[ xEMACIndex ] != NULL )
     {
-        vTaskNotifyGiveFromISR( xEMACTaskHandles[ xEMACIndex ], &xHigherPriorityTaskWoken );
+        xTaskNotifyFromISR( xEMACTaskHandles[ xEMACIndex ], EMAC_IF_TX_EVENT, eSetBits, &( xHigherPriorityTaskWoken ) );
     }
 
     portYIELD_FROM_ISR( xHigherPriorityTaskWoken );
@@ -252,7 +248,6 @@ XStatus emacps_send_message( xemacpsif_s * xemacpsif,
                              int iReleaseAfterSend )
 {
     int txHead = xemacpsif->txHead;
-    int iHasSent = 0;
     uint32_t ulBaseAddress = xemacpsif->emacps.Config.BaseAddress;
     BaseType_t xEMACIndex = get_xEMACIndex( &xemacpsif->emacps );
     TickType_t xBlockTimeTicks = pdMS_TO_TICKS( 5000U );
@@ -315,8 +310,6 @@ XStatus emacps_send_message( xemacpsif_s * xemacpsif,
         {
         }
 
-        iHasSent = pdTRUE;
-
         txHead++;
 
         if( txHead == ipconfigNIC_N_TX_DESC )
@@ -331,13 +324,12 @@ XStatus emacps_send_message( xemacpsif_s * xemacpsif,
         /* Data Synchronization Barrier */
         dsb();
 
-        if( iHasSent == pdTRUE )
         {
             /* Make STARTTX high */
             uint32_t ulValue = XEmacPs_ReadReg( ulBaseAddress, XEMACPS_NWCTRL_OFFSET );
-            /* Start transmit */
-            xemacpsif->txBusy = pdTRUE;
-            XEmacPs_WriteReg( ulBaseAddress, XEMACPS_NWCTRL_OFFSET, ( ulValue | XEMACPS_NWCTRL_STARTTX_MASK ) );
+            /* Start transmit. */
+            ulValue |= XEMACPS_NWCTRL_STARTTX_MASK;
+            XEmacPs_WriteReg( ulBaseAddress, XEMACPS_NWCTRL_OFFSET, ulValue );
             /* Read back the register to make sure the data is flushed. */
             ( void ) XEmacPs_ReadReg( ulBaseAddress, XEMACPS_NWCTRL_OFFSET );
         }
@@ -356,12 +348,13 @@ XStatus emacps_send_message( xemacpsif_s * xemacpsif,
 
 void emacps_recv_handler( void * arg )
 {
+    /* This function is running in ISR mode,
+     * called to handle an RX interrupt. */
     xemacpsif_s * xemacpsif;
     BaseType_t xHigherPriorityTaskWoken = pdFALSE;
     BaseType_t xEMACIndex;
 
     xemacpsif = ( xemacpsif_s * ) arg;
-    xemacpsif->isr_events |= EMAC_IF_RX_EVENT;
     xEMACIndex = get_xEMACIndex( &xemacpsif->emacps );
 
     /* The driver has already cleared the FRAMERX, BUFFNA and error bits
@@ -371,7 +364,7 @@ void emacps_recv_handler( void * arg )
 
     if( xEMACTaskHandles[ xEMACIndex ] != NULL )
     {
-        vTaskNotifyGiveFromISR( xEMACTaskHandles[ xEMACIndex ], &xHigherPriorityTaskWoken );
+        xTaskNotifyFromISR( xEMACTaskHandles[ xEMACIndex ], EMAC_IF_RX_EVENT, eSetBits, &( xHigherPriorityTaskWoken ) );
     }
 
     portYIELD_FROM_ISR( xHigherPriorityTaskWoken );
@@ -685,6 +678,19 @@ XStatus init_dma( xemacpsif_s * xemacpsif )
         configASSERT( xTXDescriptorSemaphores[ xEMACIndex ] );
     }
 
+    if( xemacpsif->tx_mutex == NULL )
+    {
+        /* Both the IP- and the EMAC-task handle TX descriptors.
+         * The IP-task sends packets, and the EMAC task clear
+         * TX descriptors that are done.
+         * A mutex is used so that either emacps_send_message() or
+         * emacps_check_tx() can become active a a time.
+         */
+        xemacpsif->tx_mutex = xSemaphoreCreateBinary();
+        configASSERT( xemacpsif->tx_mutex != NULL );
+        xSemaphoreGive( xemacpsif->tx_mutex );
+    }
+
     /*
      * Allocate RX descriptors, 1 RxBD at a time.
      */

source/portable/NetworkInterface/Zynq/x_emacpsif_hw.c

@@ -107,14 +107,11 @@ void emacps_error_handler( void * arg,
             xErrorList[ xErrorHead ].ErrorWord = ErrorWord;
 
             xErrorHead = xNextHead;
-
-            xemacpsif = ( xemacpsif_s * ) ( arg );
-            xemacpsif->isr_events |= EMAC_IF_ERR_EVENT;
         }
 
         if( xEMACTaskHandles[ xEMACIndex ] != NULL )
         {
-            vTaskNotifyGiveFromISR( xEMACTaskHandles[ xEMACIndex ], &xHigherPriorityTaskWoken );
+            xTaskNotifyFromISR( xEMACTaskHandles[ xEMACIndex ], EMAC_IF_ERR_EVENT, eSetBits, &( xHigherPriorityTaskWoken ) );
         }
     }