drivers: eth_nxp_enet_qos: Refactor RX code

Refactor the RX code to be more cognitively coherent, by splitting up
into smaller functions with readable names, instead of a giant code
block with lots of nesting conditionals and loops and complication.

The only intended logical change is that, instead of dropping a packet
if a new RX buf is not available, still receive the packet but do not
give the descriptor back to the DMA. This means that the DMA will likely
get suspended upon reaching this descriptor which could not have it's
buffer replaced, and have another chance on the next work run.

Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

Author: decsny

drivers/ethernet/eth_nxp_enet_qos/eth_nxp_enet_qos_mac.c

@@ -185,6 +185,16 @@ static bool software_owns_descriptor(volatile union nxp_enet_qos_rx_desc *desc)
 	return (desc->write.control3 & OWN_FLAG) != OWN_FLAG;
 }
 
+static bool frame_is_first(volatile union nxp_enet_qos_rx_desc *desc)
+{
+	return (desc->write.control3 & FIRST_DESCRIPTOR_FLAG) == FIRST_DESCRIPTOR_FLAG;
+}
+
+static bool frame_is_last(volatile union nxp_enet_qos_rx_desc *desc)
+{
+	return (desc->write.control3 & LAST_DESCRIPTOR_FLAG) == LAST_DESCRIPTOR_FLAG;
+}
+
 /* this function resumes the rx dma in case of underrun */
 static void enet_qos_dma_rx_resume(const struct device *dev)
 {
@@ -208,6 +218,209 @@ static void enet_qos_dma_rx_resume(const struct device *dev)
 		ENET_QOS_ALIGN_ADDR_SHIFT((uint32_t)&rx_data->descriptors[NUM_RX_BUFDESC]));
 }
 
+/* treats the frame as a potential start of a new packet.
+ * returns new net_pkt pointer if successfully received or null otherwise.
+ */
+static struct net_pkt *enet_qos_start_new_packet(const struct device *dev,
+						 volatile union nxp_enet_qos_rx_desc *desc)
+{
+	struct nxp_enet_qos_mac_data *data = dev->data;
+	struct nxp_enet_qos_rx_data *rx_data = &data->rx;
+	struct net_pkt *pkt;
+
+	/* If it's not the start of a packet, ignore this frame */
+	if (!frame_is_first(desc)) {
+		/* since this frame is not the start of a packet, we don't log new packet
+		 * drop error, since the starting frame must have been dropped and logged
+		 * earlier. ie. we only log dropping first descriptor flag frames.
+		 */
+		goto skip;
+	}
+
+	pkt = net_pkt_rx_alloc(K_NO_WAIT);
+
+	if (!pkt) {
+		LOG_ERR("Could not alloc new RX pkt");
+		goto skip;
+	}
+
+	rx_data->processed_pkt_len = 0;
+
+	LOG_DBG("New RX pkt %p", pkt);
+
+	return pkt;
+skip:
+	return NULL;
+}
+
+static void enet_qos_drop_rx_packet(const struct device *dev,
+				    struct net_pkt *pkt)
+{
+	struct nxp_enet_qos_mac_data *data = dev->data;
+	struct nxp_enet_qos_rx_data *rx_data = &data->rx;
+
+	eth_stats_update_errors_rx(data->iface);
+	if (pkt != NULL) {
+		LOG_WRN("Dropped packet %p", pkt);
+		net_pkt_unref(pkt);
+	}
+
+	rx_data->processed_pkt_len = 0;
+}
+
+static void enet_qos_drop_rx_frame(const struct device *dev,
+				   struct net_pkt *pkt,
+				   volatile union nxp_enet_qos_rx_desc *desc)
+{
+	struct nxp_enet_qos_mac_data *data = dev->data;
+	struct nxp_enet_qos_rx_data *rx_data = &data->rx;
+
+	if (rx_data->pkt != NULL) {
+		/* Only drop packet if we didn't before */
+		enet_qos_drop_rx_packet(dev);
+	} else {
+		LOG_DBG("Dropping frame from error packet");
+	}
+
+	desc->read.control = rx_desc_refresh_flags;
+}
+
+static int enet_qos_get_frame_len(const struct device *dev,
+				  volatile union nxp_enet_qos_rx_desc *desc)
+{
+	struct nxp_enet_qos_mac_data *data = dev->data;
+	struct nxp_enet_qos_rx_data *rx_data = &data->rx;
+
+	/* The descriptor has the cumulative length of the packet so far, including FCS */
+	size_t pkt_len = desc->write.control3 & DESC_RX_PKT_LEN;
+
+	if (rx_data->processed_pkt_len >= pkt_len) {
+		LOG_ERR("Packet length glitched, previous %d > new %d",
+				rx_data->processed_pkt_len, pkt_len);
+		return -EINVAL;
+	}
+
+	size_t frame_len = pkt_len - rx_data->processed_pkt_len;
+
+	if (frame_len > ENET_QOS_RX_BUFFER_SIZE) {
+		LOG_ERR("Frame len %d too big for max buffer len %d",
+				frame_len, ENET_QOS_RX_BUFFER_SIZE);
+		return -E2BIG;
+	}
+
+	return frame_len;
+}
+
+static int enet_qos_append_frame_to_packet(const struct device *dev,
+					   struct net_pkt *pkt,
+					   volatile union nxp_enet_qos_rx_desc *desc,
+					   size_t frame_len)
+{
+	struct nxp_enet_qos_mac_data *data = dev->data;
+	struct nxp_enet_qos_rx_data *rx_data = &data->rx;
+	volatile union nxp_enet_qos_rx_desc *desc_arr = rx_data->descriptors;
+	int desc_idx = ARRAY_INDEX(desc_arr, desc);
+	struct net_buf *frame_buf = data->rx.reserved_bufs[desc_idx];
+
+	net_buf_add(frame_buf, frame_len);
+	net_pkt_frag_add(pkt, frame_buf);
+
+	return 0;
+}
+
+static int enet_qos_pass_up_rx(const struct device *dev, struct net_pkt **pkt)
+{
+	struct nxp_enet_qos_mac_data *data = dev->data;
+	struct nxp_enet_qos_rx_data *rx_data = &data->rx;
+	int ret = 0;
+
+	LOG_DBG("Receiving RX packet");
+
+	ret = net_recv_data(data->iface, *pkt);
+	if (ret != 0) {
+		LOG_ERR("RECV failed for pkt %p", *pkt);
+		enet_qos_drop_rx_packet(dev, *pkt);
+		goto done;
+	}
+
+	eth_stats_update_pkts_rx(data->iface);
+
+done:
+	rx_data->processed_pkt_len = 0;
+	*pkt = NULL;
+	return ret;
+}
+
+static void enet_qos_swap_rx_desc_buf(const struct device *dev,
+				     volatile union nxp_enet_qos_rx_desc *desc)
+{
+	struct nxp_enet_qos_mac_data *data = dev->data;
+	struct nxp_enet_qos_rx_data *rx_data = &data->rx;
+	volatile union nxp_enet_qos_rx_desc *desc_arr = rx_data->descriptors;
+	int desc_idx = ARRAY_INDEX(desc_arr, desc);
+	struct net_buf *new_buf;
+
+	new_buf = net_pkt_get_reserve_rx_data(CONFIG_NET_BUF_DATA_SIZE, K_NO_WAIT);
+	if (new_buf == NULL) {
+		LOG_WRN("No new RX buf available");
+		return;
+	}
+
+	data->rx.reserved_bufs[desc_idx] = new_buf;
+	desc->read.buf1_addr = (uint32_t)new_buf->data;
+
+	/* notice we only refresh (give back to DMA if we got a buffer */
+	desc->read.control = rx_desc_refresh_flags;
+}
+
+static void eth_nxp_enet_qos_process_rx_frame(const struct device *dev,
+					     struct net_pkt **pkt,
+					     volatile union nxp_enet_qos_rx_desc *desc)
+{
+	struct nxp_enet_qos_mac_data *data = dev->data;
+	struct nxp_enet_qos_rx_data *rx_data = &data->rx;
+	int frame_len = 0;
+	int ret = 0;
+
+	if (*pkt == NULL) {
+		/* Trying to start a new packet if none is provided */
+		*pkt = enet_qos_start_new_packet(dev, desc);
+	}
+	if (*pkt == NULL) {
+		/* Still no packet so drop the frame */
+		goto drop;
+	}
+
+	frame_len = enet_qos_get_frame_len(dev, desc);
+	if (frame_len < 0) {
+		goto drop;
+	}
+
+	/* Take the received data and add to the packet */
+	enet_qos_append_frame_to_packet(dev, *pkt, desc, frame_len);
+	rx_data->processed_pkt_len += frame_len;
+
+	if (!frame_is_last(desc)) {
+		goto done;
+	}
+
+	/* If this is the last frame of the packet, send it up the stack */
+	ret = enet_qos_pass_up_rx(dev, pkt);
+	if (ret != 0) {
+		goto drop;
+	}
+
+	goto done;
+drop:
+	enet_qos_drop_rx_frame(dev, *pkt, desc);
+	*pkt = NULL;
+	return;
+done:
+	/* last thing to do is switch the buf so the DMA doesn't overwrite
+	 * the net buf that the net stack is processing.
+	 */
+	enet_qos_swap_rx_desc_buf(dev, desc);
+}
 
 static void eth_nxp_enet_qos_rx(struct k_work *work)
 {
@@ -220,10 +433,6 @@ static void eth_nxp_enet_qos_rx(struct k_work *work)
 	uint32_t desc_idx = rx_data->next_desc_idx;
 	volatile union nxp_enet_qos_rx_desc *desc = &desc_arr[desc_idx];
 	struct net_pkt *pkt = NULL;
-	struct net_buf *new_buf;
-	struct net_buf *buf;
-	size_t pkt_len;
-	size_t processed_len;
 
 	LOG_DBG("RX work start: %p", work);
 
@@ -233,99 +442,8 @@ static void eth_nxp_enet_qos_rx(struct k_work *work)
 	while (software_owns_descriptor(desc)) {
 		rx_data->next_desc_idx = (desc_idx + 1U) % NUM_RX_BUFDESC;
 
-		if (pkt == NULL) {
-			if ((desc->write.control3 & FIRST_DESCRIPTOR_FLAG) !=
-				FIRST_DESCRIPTOR_FLAG) {
-				LOG_DBG("receive packet mask %X ",
-					(desc->write.control3 >> 28) & 0x0f);
-				LOG_ERR("Rx descriptor does not have first descriptor flag, drop");
-				desc->read.control = rx_desc_refresh_flags;
-				/* Error statistics for this packet already updated earlier */
-				goto next;
-			}
-
-			/* Otherwise, we found a packet that we need to process */
-			pkt = net_pkt_rx_alloc(K_NO_WAIT);
-
-			if (!pkt) {
-				LOG_ERR("Could not alloc new RX pkt");
-				/* error: no new buffer, reuse previous immediately */
-				desc->read.control = rx_desc_refresh_flags;
-				eth_stats_update_errors_rx(data->iface);
-				goto next;
-			}
-
-			processed_len = 0U;
+		eth_nxp_enet_qos_process_rx_frame(dev, &pkt, desc);
 
-			LOG_DBG("Created new RX pkt %u of %d: %p",
-				desc_idx + 1U, NUM_RX_BUFDESC, pkt);
-		}
-
-		/* Read the cumulative length of data in this buffer and previous buffers (if any).
-		 * The complete length is in a descriptor with the last descriptor flag set
-		 * (note that it includes four byte FCS as well). This length will be validated
-		 * against processed_len to ensure it's within expected bounds.
-		 */
-		pkt_len = desc->write.control3 & DESC_RX_PKT_LEN;
-		if ((pkt_len < processed_len) ||
-			((pkt_len - processed_len) > ENET_QOS_RX_BUFFER_SIZE)) {
-			LOG_ERR("Invalid packet length in descriptor: pkt_len=%u, processed_len=%u",
-				pkt_len, processed_len);
-			net_pkt_unref(pkt);
-			pkt = NULL;
-			desc->read.control = rx_desc_refresh_flags;
-			eth_stats_update_errors_rx(data->iface);
-			goto next;
-		}
-
-		/* We need to know if we can replace the reserved fragment in advance.
-		 * At no point can we allow the driver to have less the amount of reserved
-		 * buffers it needs to function, so we will not give up our previous buffer
-		 * unless we know we can get a new one.
-		 */
-		new_buf = net_pkt_get_frag(pkt, CONFIG_NET_BUF_DATA_SIZE, K_NO_WAIT);
-		if (new_buf == NULL) {
-			/* We have no choice but to lose the previous packet,
-			 * as the buffer is more important. If we recv this packet,
-			 * we don't know what the upper layer will do to our poor buffer.
-			 * drop this buffer, reuse allocated DMA buffer
-			 */
-			LOG_ERR("No new RX buf available");
-			net_pkt_unref(pkt);
-			pkt = NULL;
-			desc->read.control = rx_desc_refresh_flags;
-			eth_stats_update_errors_rx(data->iface);
-			goto next;
-		}
-
-		/* Append buffer to a packet */
-		buf = data->rx.reserved_bufs[desc_idx];
-		net_buf_add(buf, pkt_len - processed_len);
-		net_pkt_frag_add(pkt, buf);
-		processed_len = pkt_len;
-
-		if ((desc->write.control3 & LAST_DESCRIPTOR_FLAG) == LAST_DESCRIPTOR_FLAG) {
-			/* Propagate completed packet to network stack */
-			LOG_DBG("Receiving RX packet");
-			if (net_recv_data(data->iface, pkt)) {
-				LOG_ERR("RECV failed");
-				/* Error during processing, we continue with new buffer */
-				net_pkt_unref(pkt);
-				eth_stats_update_errors_rx(data->iface);
-			} else {
-				/* Record successfully received packet */
-				eth_stats_update_pkts_rx(data->iface);
-			}
-			pkt = NULL;
-		}
-
-		LOG_DBG("Swap RX buf");
-		/* Allow receive into a new buffer */
-		data->rx.reserved_bufs[desc_idx] = new_buf;
-		desc->read.buf1_addr = (uint32_t)new_buf->data;
-		desc->read.control = rx_desc_refresh_flags;
-
-next:
 		desc_idx = rx_data->next_desc_idx;
 		desc = &desc_arr[desc_idx];
 	}
@@ -335,15 +453,13 @@ static void eth_nxp_enet_qos_rx(struct k_work *work)
 		 * fragment of the packet, deallocate the incomplete one
 		 */
 		LOG_DBG("Incomplete packet received, cleaning up");
-		net_pkt_unref(pkt);
-		pkt = NULL;
-		eth_stats_update_errors_rx(data->iface);
+		enet_qos_drop_rx_packet(dev, pkt);
 	}
 
 	/* now that we updated the descriptors, resume in case we are suspended */
 	enet_qos_dma_rx_resume(dev);
 
-	LOG_DBG("End RX work normally");
+	LOG_DBG("End RX work");
 	return;
 }
 
@@ -748,8 +864,6 @@ static const struct device *eth_nxp_enet_qos_get_phy(const struct device *dev)
 	return config->phy_dev;
 }
 
-
-
 static int eth_nxp_enet_qos_set_config(const struct device *dev,
 			       enum ethernet_config_type type,
 			       const struct ethernet_config *cfg)

drivers/ethernet/eth_nxp_enet_qos/nxp_enet_qos_priv.h

@@ -116,6 +116,7 @@ struct nxp_enet_qos_rx_data {
 	uint32_t next_desc_idx;
 	volatile union nxp_enet_qos_rx_desc descriptors[NUM_RX_BUFDESC];
 	struct net_buf *reserved_bufs[NUM_RX_BUFDESC];
+	size_t processed_pkt_len;
 };
 
 struct nxp_enet_qos_mac_data {