Re: [PATCH net-next v5 10/10] net: axienet: Introduce dmaengine support
From: Jakub Kicinski <kuba@kernel.org>
Date: 2023-08-08 22:49:18
Also in:
dmaengine, linux-devicetree, lkml, netdev
On Mon, 7 Aug 2023 11:21:49 +0530 Radhey Shyam Pandey wrote:
+struct axi_skbuff {
+ struct scatterlist sgl[MAX_SKB_FRAGS + 1];
+ struct dma_async_tx_descriptor *desc;
+ dma_addr_t dma_address;
+ struct sk_buff *skb;
+ struct list_head lh;
+ int sg_len;
+} __packed;Why __packed?
+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);
+ u32 app[DMA_NUM_APP_WORDS] = {0};
+ struct axi_skbuff *axi_skb;
+ u32 csum_start_off;
+ u32 csum_index_off;
+ int sg_len;
+ int ret;
+
+ sg_len = skb_shinfo(skb)->nr_frags + 1;
+ axi_skb = kmem_cache_zalloc(lp->skb_cache, GFP_KERNEL);
+ if (!axi_skb)
+ return NETDEV_TX_BUSY;Drop on error, you're not stopping the queue correctly, just drop, return OK and avoid bugs.
+static inline int axienet_init_dmaengine(struct net_device *ndev)
Why inline? Please remove the spurious inline annotations.
+{
+ struct axienet_local *lp = netdev_priv(ndev);
+ 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 found\n");
+ }
+
+ lp->rx_chan = dma_request_chan(lp->dev, "rx_chan0");
+ if (IS_ERR(lp->rx_chan)) {
+ ret = PTR_ERR(lp->rx_chan);
+ dev_err_probe(lp->dev, ret, "No Ethernet DMA (RX) channel found\n");
+ goto err_dma_request_rx;name labels after the target, not the source. Makes it a ton easier to maintain sanity when changing the code later.
+ }
+
+ lp->skb_cache = kmem_cache_create("ethernet", sizeof(struct axi_skbuff),
+ 0, 0, NULL);Why create a cache ? Isn't it cleaner to create a fake ring buffer of sgl? Most packets will not have MAX_SKB_FRAGS of memory. On a ring buffer you can use only as many sg entries as the packet requires. Also no need to alloc/free.
+ if (!lp->skb_cache) {
+ ret = -ENOMEM;double space, please fix everywhere
+ goto err_kmem;
quoted hunk ↗ jump to hunk
@@ -2140,6 +2432,31 @@ static int axienet_probe(struct platform_device *pdev) } netif_napi_add(ndev, &lp->napi_rx, axienet_rx_poll); netif_napi_add(ndev, &lp->napi_tx, axienet_tx_poll); + } else { + struct xilinx_vdma_config cfg; + struct dma_chan *tx_chan; + + lp->eth_irq = platform_get_irq_optional(pdev, 0);
This can't fail?
+ tx_chan = dma_request_chan(lp->dev, "tx_chan0");
+
+ if (IS_ERR(tx_chan)) {no empty lines between call and its error check, please fix thru out
+ ret = PTR_ERR(tx_chan); + dev_err_probe(lp->dev, ret, "No Ethernet DMA (TX) channel found\n"); + goto cleanup_clk; + }
-- pw-bot: cr _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel