RE: [PATCH net-next v7 3/3] net: axienet: Introduce dmaengine support

-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: Thursday, October 5, 2023 2:23 AM
To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
Cc: davem@davemloft.net; edumazet@google.com; pabeni@redhat.com;
robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
conor+dt@kernel.org; Simek, Michal [off-list ref];
linux@armlinux.org.uk; f.fainelli@gmail.com; netdev@vger.kernel.org;
devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
kernel@lists.infradead.org; git (AMD-Xilinx) [off-list ref]
Subject: Re: [PATCH net-next v7 3/3] net: axienet: Introduce dmaengine
support

On Thu, 28 Sep 2023 01:02:31 +0530 Radhey Shyam Pandey wrote:

quoted

+static int axienet_rx_submit_desc(struct net_device *ndev);

can the forward declaration be avoided?

axienet_dma_rx_cb()- make call to axienet_rx_submit_desc(). and 
axienet_rx_submit_desc() registers axienet_dma_rx_cb as callback 
func. There seems to be circular dependency.

quoted

+/**
+ * axienet_dma_tx_cb - DMA engine callback for TX channel.
+ * @data:       Pointer to the axienet_local structure.
+ * @result:     error reporting through dmaengine_result.
+ * This function is called by dmaengine driver for TX channel to
+notify
+ * that the transmit is done.
+ */
+static void axienet_dma_tx_cb(void *data, const struct
+dmaengine_result *result) {
+	struct axienet_local *lp = data;
+	struct skbuf_dma_descriptor *skbuf_dma;
+
+	skbuf_dma = axienet_get_tx_desc(lp, lp->tx_ring_tail++);
+	u64_stats_update_begin(&lp->tx_stat_sync);
+	u64_stats_add(&lp->tx_bytes, skbuf_dma->skb->len);
+	u64_stats_add(&lp->tx_packets, 1);
+	u64_stats_update_end(&lp->tx_stat_sync);
+	dma_unmap_sg(lp->dev, skbuf_dma->sgl, skbuf_dma->sg_len,

DMA_TO_DEVICE);

quoted

+	dev_kfree_skb_any(skbuf_dma->skb);

dev_consume_skb_any(), kfree is for drops

Thanks, will use dev_consume_skb_any in next version.

quoted

+/**
+ * axienet_start_xmit_dmaengine - Starts the transmission.
+ * @skb:        sk_buff pointer that contains data to be Txed.
+ * @ndev:       Pointer to net_device structure.
+ *
+ * Return: NETDEV_TX_OK on success or any non space errors.
+ *         NETDEV_TX_BUSY when free element in TX skb ring buffer
+ *         is not available.
+ *
+ * This function is invoked from xmit to initiate transmission. The
+ * function sets the skbs, register dma call back API and submit
+ * the dma transaction.
+ * Additionally if checksum offloading is supported,
+ * it populates AXI Stream Control fields with appropriate values.
+ */
+static netdev_tx_t
+axienet_start_xmit_dmaengine(struct sk_buff *skb, struct net_device
+*ndev) {
+	struct dma_async_tx_descriptor *dma_tx_desc = NULL;
+	struct axienet_local *lp = netdev_priv(ndev);
+	struct skbuf_dma_descriptor *skbuf_dma;
+	u32 app_metadata[DMA_NUM_APP_WORDS] = {0};
+	u32 csum_start_off;
+	u32 csum_index_off;
+	int sg_len;
+	int ret;
+
+	sg_len = skb_shinfo(skb)->nr_frags + 1;
+	if (!CIRC_SPACE(lp->tx_ring_head, lp->tx_ring_tail,

TX_BD_NUM_MAX)) {

quoted

+		netif_stop_queue(ndev);
+		if (net_ratelimit())
+			netdev_warn(ndev, "TX ring unexpectedly full\n");

I don't see you stopping the queue when the ring gets full, am I not looking
into the right place? Otherwise this is sort of expected to occasionally hapen

We are calling stop_queue if TX ring is full. Is that what you meant?
netif_stop_queue(ndev);

quoted

+		return NETDEV_TX_BUSY;
+	}
+
+	skbuf_dma = axienet_get_tx_desc(lp, lp->tx_ring_head);
+	if (!skbuf_dma)
+		return NETDEV_TX_OK;

Leaks the skb?

Accepted. Will add dev_kfree_skb in this and below TX_OK error path and
also increment stats.tx_errors++.

quoted

+	lp->tx_ring_head++;
+	sg_init_table(skbuf_dma->sgl, sg_len);
+	ret = skb_to_sgvec(skb, skbuf_dma->sgl, 0, skb->len);
+	if (ret < 0)
+		return NETDEV_TX_OK;
+
+	ret = dma_map_sg(lp->dev, skbuf_dma->sgl, sg_len,

DMA_TO_DEVICE);

quoted

+	if (!ret)
+		return NETDEV_TX_OK;
+
+	/* Fill up app fields for checksum */
+	if (skb->ip_summed == CHECKSUM_PARTIAL) {
+		if (lp->features & XAE_FEATURE_FULL_TX_CSUM) {
+			/* Tx Full Checksum Offload Enabled */
+			app_metadata[0] |= 2;
+		} else if (lp->features & XAE_FEATURE_PARTIAL_TX_CSUM) {
+			csum_start_off = skb_transport_offset(skb);
+			csum_index_off = csum_start_off + skb-
csum_offset;
+			/* Tx Partial Checksum Offload Enabled */
+			app_metadata[0] |= 1;
+			app_metadata[1] = (csum_start_off << 16) |

csum_index_off;

quoted

+		}
+	} else if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
+		app_metadata[0] |= 2; /* Tx Full Checksum Offload Enabled

*/

quoted

+	}
+
+	dma_tx_desc = lp->tx_chan->device->device_prep_slave_sg(lp-
tx_chan,
+skbuf_dma->sgl,

Possibly store the device_prep_slave_sg pointer to a temporary variable to
make this line less painfully long?

Will modify in next version.

quoted

+	skbuf_dma = axienet_get_rx_desc(lp, lp->rx_ring_head);
+	if (!skbuf_dma)
+		return -ENOMEM;
+	lp->rx_ring_head++;
+	skb = netdev_alloc_skb(ndev, lp->max_frm_size);
+	if (!skb)
+		return -ENOMEM;
+
+	sg_init_table(skbuf_dma->sgl, 1);
+	addr = dma_map_single(lp->dev, skb->data, lp->max_frm_size,

DMA_FROM_DEVICE);

quoted

+	if (unlikely(dma_mapping_error(lp->dev, addr))) {
+		netdev_err(ndev, "DMA mapping error\n");

needs rate limiting

Agree. Will change it to rate limit call.

quoted

+		ret = -ENOMEM;
+		goto rx_submit_err_free_skb;

is it legal to unmap dma error ?

Will fix this error handling in next version.

quoted

+	}
+	sg_dma_address(skbuf_dma->sgl) = addr;
+	sg_dma_len(skbuf_dma->sgl) = lp->max_frm_size;
+	dma_rx_desc = dmaengine_prep_slave_sg(lp->rx_chan, skbuf_dma-
sgl,
+					      1, DMA_DEV_TO_MEM,
+					      DMA_PREP_INTERRUPT);

quoted

+/**
+ * axienet_init_dmaengine - init the dmaengine code.
+ * @ndev:       Pointer to net_device structure
+ *
+ * Return: 0, on success.
+ *          non-zero error value on failure
+ *
+ * This is the dmaengine initialization code.
+ */
+static int axienet_init_dmaengine(struct net_device *ndev) {
+	struct axienet_local *lp = netdev_priv(ndev);
+	struct skbuf_dma_descriptor *skbuf_dma;
+	int i, ret;
+
+	lp->tx_chan = dma_request_chan(lp->dev, "tx_chan0");
+	if (IS_ERR(lp->tx_chan)) {
+		ret = PTR_ERR(lp->tx_chan);
+		return dev_err_probe(lp->dev, ret, "No Ethernet DMA (TX)

channel

quoted

+found\n");

Why use dev_err_probe()? This is not on the probe path. If ndo_open fails it
fails, it won't get retried later by itself, right?

I see , will fix it, and remove _probe variant.

quoted

@@ -1238,7 +1540,24 @@ static int axienet_open(struct net_device

*ndev)

 	phylink_start(lp->phylink);

-	if (!lp->use_dmaengine) {
+	if (lp->use_dmaengine) {
+		/* Enable interrupts for Axi Ethernet core (if defined) */
+		if (lp->eth_irq > 0) {
+			ret = request_irq(lp->eth_irq, axienet_eth_irq,

IRQF_SHARED,

quoted

+					  ndev->name, ndev);
+			if (ret)
+				goto error_code;
+		}
+
+		ret = axienet_init_dmaengine(ndev);
+

pointless new line

Will fix in next version.

quoted

+		if (ret < 0) {
+			if (lp->eth_irq > 0)
+				free_irq(lp->eth_irq, ndev);

can't this be on the error path?

It's just single instance so didn't introduce error label for it.
But I can move to error path.

quoted

+			goto error_code;
+		}
+

pointless new line

Will fix it in next version.

quoted

+	} else {

--
pw-bot: cr