From d32c6f8294f5c581ec1078a13d083e63e4db5a56 Mon Sep 17 00:00:00 2001 From: Hein Tibosch Date: Mon, 1 Sep 2025 13:19:04 +0800 Subject: [PATCH 1/2] Zynq driver: avoid race conditions --- .../NetworkInterface/Zynq/NetworkInterface.c | 43 ++++++++++++------- .../NetworkInterface/Zynq/x_emacpsif.h | 11 +++-- .../NetworkInterface/Zynq/x_emacpsif_dma.c | 38 +++++++++------- .../NetworkInterface/Zynq/x_emacpsif_hw.c | 5 +-- 4 files changed, 57 insertions(+), 40 deletions(-) diff --git a/source/portable/NetworkInterface/Zynq/NetworkInterface.c b/source/portable/NetworkInterface/Zynq/NetworkInterface.c index ba4cdc3f8d..38de594a9d 100644 --- a/source/portable/NetworkInterface/Zynq/NetworkInterface.c +++ b/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 ); } diff --git a/source/portable/NetworkInterface/Zynq/x_emacpsif.h b/source/portable/NetworkInterface/Zynq/x_emacpsif.h index cd18fd997e..ef6e794d47 100644 --- a/source/portable/NetworkInterface/Zynq/x_emacpsif.h +++ b/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; */ diff --git a/source/portable/NetworkInterface/Zynq/x_emacpsif_dma.c b/source/portable/NetworkInterface/Zynq/x_emacpsif_dma.c index a9b9e2b03d..b3d3e3fe94 100644 --- a/source/portable/NetworkInterface/Zynq/x_emacpsif_dma.c +++ b/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. */ diff --git a/source/portable/NetworkInterface/Zynq/x_emacpsif_hw.c b/source/portable/NetworkInterface/Zynq/x_emacpsif_hw.c index 34a564e3a4..3477050b20 100644 --- a/source/portable/NetworkInterface/Zynq/x_emacpsif_hw.c +++ b/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 ) ); } } From 001251565b8c4090f450c01f695b93fc13be0458 Mon Sep 17 00:00:00 2001 From: Hein Tibosch Date: Mon, 1 Sep 2025 13:28:32 +0800 Subject: [PATCH 2/2] Corrected a comment --- source/portable/NetworkInterface/Zynq/NetworkInterface.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/portable/NetworkInterface/Zynq/NetworkInterface.c b/source/portable/NetworkInterface/Zynq/NetworkInterface.c index 38de594a9d..6901a50d88 100644 --- a/source/portable/NetworkInterface/Zynq/NetworkInterface.c +++ b/source/portable/NetworkInterface/Zynq/NetworkInterface.c @@ -439,7 +439,7 @@ static BaseType_t xZynqNetworkInterfaceOutput( NetworkInterface_t * pxInterface, { emacps_send_message( &( xEMACpsifs[ xEMACIndex ] ), pxBuffer, bReleaseAfterSend ); xSemaphoreGive( xEMACpsifs[ xEMACIndex ].tx_mutex ); - *The function emacps_send_message() has released the packet. * / + /* The function emacps_send_message() has released the packet. */ bReleaseAfterSend = pdFALSE; } }