Reformat, fix STM32 Eth v2 potential to have issues when the Tx ring gets full

Author: multiplemonomials

connectivity/drivers/emac/TARGET_STM/STM32EthMACv2.h

@@ -48,6 +48,7 @@ namespace mbed {
             void giveToDMA(size_t descIdx, uint8_t const * buffer, size_t len, bool firstDesc, bool lastDesc) override;
         public:
             explicit TxDMA(ETH_TypeDef * const base):
+            GenericTxDMARing(1), // Request that 1 Tx descriptor is always left unfilled
             base(base)
             {}
         };

connectivity/drivers/emac/include/GenericEthDMA.h

@@ -39,6 +39,10 @@ namespace mbed {
         /// Number of entries in the Tx descriptor ring
         static constexpr size_t TX_NUM_DESCS = MBED_CONF_NSAPI_EMAC_TX_NUM_DESCS;
 
+        /// Extra, unfilled Tx descs to leave in the DMA ring at all times.
+        /// This is used to support Eth MACs that don't allow enqueuing every single descriptor at a time.
+        const size_t extraTxDescsToLeave;
+
         /// Pointer to first memory buffer in the chain associated with descriptor n.
         /// The buffer address shall only be set for the *last* descriptor, so that the entire chain is freed
         /// when the last descriptor is returned.
@@ -55,6 +59,11 @@ namespace mbed {
         std::atomic<size_t> txDescsOwnedByApplication; ///< Number of Tx descriptors owned by the application. Decremented by txPacket() and incremented by reclaimTxDescs()
         size_t txReclaimIndex; ///< Index of the next Tx descriptor that will be reclaimed by the mac thread calling reclaimTxDescs().
 
+        /// Construct, passing a value for extraTxDescsToLeave
+        GenericTxDMARing(size_t extraTxDescsToLeave = 0):
+        extraTxDescsToLeave(extraTxDescsToLeave)
+        {}
+
         /// Configure DMA registers to point to the DMA ring,
         /// and enable DMA. This is done before the MAC itself is enabled.
         virtual void startDMA() = 0;
@@ -181,17 +190,18 @@ namespace mbed {
 
         CompositeEMAC::ErrCode txPacket(net_stack_mem_buf_t * buf) {
             // Step 1: Figure out if we can send this zero-copy, or if we need to copy it.
-            size_t neededDescs = memory_manager->count_buffers(buf);
+            size_t packetDescsUsed = memory_manager->count_buffers(buf);
+            size_t neededFreeDescs = packetDescsUsed + extraTxDescsToLeave;
             bool needToCopy = false;
-            if(neededDescs >= TX_NUM_DESCS)
+            if(neededFreeDescs >= TX_NUM_DESCS)
             {
                 // Packet uses too many buffers, we have to copy it into a continuous buffer.
                 // Note: Some Eth DMAs (e.g. STM32 v2) cannot enqueue all the descs in the ring at the same time
                 // so we can't use every single descriptor to send the packet.
                 needToCopy = true;
             }
 
-            if(!needToCopy && (neededDescs > txDescsOwnedByApplication && txDescsOwnedByApplication > 0)) {
+            if(!needToCopy && (neededFreeDescs > txDescsOwnedByApplication && txDescsOwnedByApplication > extraTxDescsToLeave)) {
                 // Packet uses more buffers than we have descriptors, but we can send it immediately if we copy
                 // it into a single buffer.
                 needToCopy = true;
@@ -233,7 +243,8 @@ namespace mbed {
 
                 // We should have gotten just one contiguous buffer
                 MBED_ASSERT(memory_manager->get_next(newBuf) == nullptr);
-                neededDescs = 1;
+                packetDescsUsed = 1;
+                neededFreeDescs = packetDescsUsed + extraTxDescsToLeave;
 
                 // Copy data over
                 memory_manager->copy_from_buf(static_cast<uint8_t *>(memory_manager->get_ptr(newBuf)), memory_manager->get_len(newBuf), buf);
@@ -245,14 +256,14 @@ namespace mbed {
             // Note that, in my experience, it's better to block here, as dropping the packet
             // due to not having enough buffers can create weird effects when the application sends
             // lots of packets at once.
-            while(txDescsOwnedByApplication < neededDescs)
+            while(txDescsOwnedByApplication < neededFreeDescs)
             {
                 txDescAvailFlag.wait_any_for(1, rtos::Kernel::wait_for_u32_forever);
             }
 
             // Step 4: Load buffer into descriptors and send
             net_stack_mem_buf_t * currBuf = buf;
-            for(size_t descCount = 0; descCount < neededDescs; descCount++)
+            for(size_t descCount = 0; descCount < packetDescsUsed; descCount++)
             {
 #if __DCACHE_PRESENT
                 // Write buffer back to main memory
@@ -453,6 +464,8 @@ namespace mbed {
             // This could potentially produce a false positive if we do this in the middle of receiving
             // an existing packet, but that is unlikely and will not cause anything bad to happen if it does.
 
+            bool seenFirstDesc = false;
+
             for(size_t descCount = 0; descCount < RX_NUM_DESCS; descCount++)
             {
                 size_t descIdx = (rxNextIndex + descCount) % RX_NUM_DESCS;
@@ -466,6 +479,20 @@ namespace mbed {
                     // Descriptor owned by DMA.  We are out of descriptors to process.
                     return false;
                 }
+                if(isFirstDesc(descIdx))
+                {
+                    if(seenFirstDesc)
+                    {
+                        // First desc seen after another first desc.
+                        // Some MACs do this if they run out of Rx descs when halfway through a packet.
+                        // dequeuePacket() can clean this up and reclaim the partial packet desc(s).
+                        return true;
+                    }
+                    else
+                    {
+                        seenFirstDesc = true;
+                    }
+                }
                 if(isErrorDesc(descIdx) || isLastDesc(descIdx))
                 {
                     // Reclaimable descriptor or complete packet detected.

connectivity/netsocket/tests/TESTS/network/emac/emac_test_memory.cpp

@@ -49,12 +49,10 @@ void test_emac_memory_cb(int opt)
         // Verify that, if this was a test step where allocations were supposed to fail, they actually did fail.
         // We can accept a couple of successful packets due to buffers that had been allocated earlier, but the
         // *majority* of the packets should have failed.
-        if(test_step > 0 && !echo_may_succeed)
-        {
-            if(successful_pkts_this_step >= failed_pkts_this_step)
-            {
+        if (test_step > 0 && !echo_may_succeed) {
+            if (successful_pkts_this_step >= failed_pkts_this_step) {
                 printf("Too many successful packets (%d) vs failed packets (%d). This was supposed to be a failure test!\r\n",
-                    successful_pkts_this_step, failed_pkts_this_step);
+                       successful_pkts_this_step, failed_pkts_this_step);
                 SET_ERROR_FLAGS(TEST_FAILED);
                 END_TEST_LOOP;
             }
@@ -164,8 +162,7 @@ void test_emac_memory_cb(int opt)
         }
     }
 
-    if(opt == INPUT)
-    {
+    if (opt == INPUT) {
         successful_pkts_this_step += 1;
     }
 

connectivity/netsocket/tests/emac_test_utils/emac_util.h

@@ -114,7 +114,7 @@ void emac_if_add_multicast_group(uint8_t *address);
 void emac_if_remove_multicast_group(uint8_t *address);
 
 /// If called with false as the arg, disables memory allocation
-/// (a) from the pool at all times, and 
+/// (a) from the pool at all times, and
 /// (b) from the heap when not actively transmitting a packet
 void emac_if_set_output_memory(bool memory);