From 2fd15f4f4451943e7d315ec9c3bb811aa4b009f3 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Tue, 25 Oct 2016 13:22:17 -0500 Subject: [PATCH 1/2] lwip - Fixed memory leak in k64f cyclic-buffer overflow This was actually several bugs colluding together. 1. Confusion on the buffer-semaphore paradigm used led to misuse of the tx semaphore and potential for odd behaviour. 2. Equality tests on tx_consume_index and tx_produce_index did not handle overflow correctly. This would allow tx_consume_index to catch up to tx_produce_index and trick the k64f_rx_reclaim function into forgetting about a whole buffer of pbufs. 3. On top of all of that, the ENET_BUFFDESCRIPTOR_TX_READ_MASK was not correctly read immediately after being set due to either a compiler optimization or hardware delays. This caused k64f_low_level_output to eagerly overrun existing buff-descriptors before they had been completely sent. Adopting the counting-semaphore paradigm for 1 avoided this concern. As pointed out by @infinnovation, the overflow only occurs in the rare case that the 120MHz CPU can actually generate packets faster than the ENET hardware can transmit on a 100Mbps link. --- .../arch/TARGET_Freescale/k64f_emac.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/features/FEATURE_LWIP/lwip-interface/lwip-eth/arch/TARGET_Freescale/k64f_emac.c b/features/FEATURE_LWIP/lwip-interface/lwip-eth/arch/TARGET_Freescale/k64f_emac.c index 7fb4dcf7e6d..66a0b587b02 100644 --- a/features/FEATURE_LWIP/lwip-interface/lwip-eth/arch/TARGET_Freescale/k64f_emac.c +++ b/features/FEATURE_LWIP/lwip-interface/lwip-eth/arch/TARGET_Freescale/k64f_emac.c @@ -102,24 +102,22 @@ static void update_read_buffer(uint8_t *buf) */ static void k64f_tx_reclaim(struct k64f_enetdata *k64f_enet) { - uint8_t i = 0 ; - /* Get exclusive access */ sys_mutex_lock(&k64f_enet->TXLockMutex); - i = k64f_enet->tx_consume_index; // Traverse all descriptors, looking for the ones modified by the uDMA - while((i != k64f_enet->tx_produce_index) && (!(g_handle.txBdDirty->control & ENET_BUFFDESCRIPTOR_TX_READY_MASK))) { - pbuf_free(tx_buff[i]); + while((k64f_enet->tx_consume_index != k64f_enet->tx_produce_index) && + (!(g_handle.txBdDirty->control & ENET_BUFFDESCRIPTOR_TX_READY_MASK))) { + pbuf_free(tx_buff[k64f_enet->tx_consume_index % ENET_TX_RING_LEN]); if (g_handle.txBdDirty->control & ENET_BUFFDESCRIPTOR_TX_WRAP_MASK) g_handle.txBdDirty = g_handle.txBdBase; else g_handle.txBdDirty++; - i = (i + 1) % ENET_TX_RING_LEN; + k64f_enet->tx_consume_index += 1; + osSemaphoreRelease(k64f_enet->xTXDCountSem.id); } - k64f_enet->tx_consume_index = i; /* Restore access */ sys_mutex_unlock(&k64f_enet->TXLockMutex); } @@ -526,15 +524,14 @@ static err_t k64f_low_level_output(struct netif *netif, struct pbuf *p) /* Wait until a descriptor is available for the transfer. */ /* THIS WILL BLOCK UNTIL THERE ARE A DESCRIPTOR AVAILABLE */ - while (g_handle.txBdCurrent->control & ENET_BUFFDESCRIPTOR_TX_READY_MASK) - osSemaphoreWait(k64f_enet->xTXDCountSem.id, osWaitForever); + osSemaphoreWait(k64f_enet->xTXDCountSem.id, osWaitForever); /* Get exclusive access */ sys_mutex_lock(&k64f_enet->TXLockMutex); /* Save the buffer so that it can be freed when transmit is done */ - tx_buff[k64f_enet->tx_produce_index] = temp_pbuf; - k64f_enet->tx_produce_index = (k64f_enet->tx_produce_index + 1) % ENET_TX_RING_LEN; + tx_buff[k64f_enet->tx_produce_index % ENET_TX_RING_LEN] = temp_pbuf; + k64f_enet->tx_produce_index += 1; /* Setup transfers */ g_handle.txBdCurrent->buffer = psend; From 8b7c051d03b9d68a5f2f452e60d1e67d4121acdb Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Fri, 28 Oct 2016 15:22:08 -0500 Subject: [PATCH 2/2] lwip - Change k64f emac layer to drop frames on buffer exhaustion Previously, exhausting hardware buffers would begin blocking the lwip thread. This patch changes the emac layer to simply drop ethernet frames, leaving recovery up to a higher level protocol. This is consistent with the behaviour of the emac layer when unable to allocate dynamic memory. --- .../lwip-eth/arch/TARGET_Freescale/k64f_emac.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/features/FEATURE_LWIP/lwip-interface/lwip-eth/arch/TARGET_Freescale/k64f_emac.c b/features/FEATURE_LWIP/lwip-interface/lwip-eth/arch/TARGET_Freescale/k64f_emac.c index 66a0b587b02..0710450ffce 100644 --- a/features/FEATURE_LWIP/lwip-interface/lwip-eth/arch/TARGET_Freescale/k64f_emac.c +++ b/features/FEATURE_LWIP/lwip-interface/lwip-eth/arch/TARGET_Freescale/k64f_emac.c @@ -522,9 +522,10 @@ static err_t k64f_low_level_output(struct netif *netif, struct pbuf *p) dst += q->len; } - /* Wait until a descriptor is available for the transfer. */ - /* THIS WILL BLOCK UNTIL THERE ARE A DESCRIPTOR AVAILABLE */ - osSemaphoreWait(k64f_enet->xTXDCountSem.id, osWaitForever); + /* Check if a descriptor is available for the transfer. */ + int32_t count = osSemaphoreWait(k64f_enet->xTXDCountSem.id, 0); + if (count < 1) + return ERR_BUF; /* Get exclusive access */ sys_mutex_lock(&k64f_enet->TXLockMutex);