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

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Florian Fainelli <f.fainelli@xxxxxxxxx>
> Sent: Tuesday, September 19, 2023 7:53 AM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@xxxxxxx>;
> davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; Simek, Michal
> <michal.simek@xxxxxxx>; linux@xxxxxxxxxxxxxxx
> Cc: netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; git (AMD-
> Xilinx) <git@xxxxxxx>
> Subject: Re: [PATCH net-next v6 3/3] net: axienet: Introduce dmaengine
> support
> 
> 
> 
> On 9/18/2023 12:16 PM, Radhey Shyam Pandey wrote:
> > Add dmaengine framework to communicate with the xilinx DMAengine
> > driver(AXIDMA).
> >
> > Axi ethernet driver uses separate channels for transmit and receive.
> > Add support for these channels to handle TX and RX with skb and
> > appropriate callbacks. Also add axi ethernet core interrupt for
> > dmaengine framework support.
> >
> > The dmaengine framework was extended for metadata API support.
> > However it still needs further enhancements to make it well suited for
> > ethernet usecases. The ethernet features i.e ethtool set/get of DMA IP
> > properties, ndo_poll_controller,(mentioned in TODO) are not supported
> > and it requires follow-up discussions.
> >
> > dmaengine support has a dependency on xilinx_dma as it uses
> > xilinx_vdma_channel_set_config() API to reset the DMA IP which
> > internally reset MAC prior to accessing MDIO.
> >
> > Benchmark with netperf:
> >
> > xilinx-zcu102-20232:~$ netperf -H 192.168.10.20 -t TCP_STREAM MIGRATED
> > TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.10.20
> > () port 0 AF_INET
> > Recv   Send    Send
> > Socket Socket  Message  Elapsed
> > Size   Size    Size     Time     Throughput
> > bytes  bytes   bytes    secs.    10^6bits/sec
> >
> > 131072  16384  16384    10.03     915.55
> >
> > xilinx-zcu102-20232:~$ netperf -H 192.168.10.20 -t UDP_STREAM
> MIGRATED
> > UDP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.10.20
> > () port 0 AF_INET
> > Socket  Message  Elapsed      Messages
> > Size    Size     Time         Okay Errors   Throughput
> > bytes   bytes    secs            #      #   10^6bits/sec
> >
> > 212992   65507   10.00       18192      0     953.35
> > 212992           10.00       18192            953.35
> >
> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xxxxxxx>
> > ---
> [snip]
> 
> Nice example of how to integrate an Ethernet driver with dmaengine, BTW.

Thanks!
> 
> 
> > +
> > +	/*Fill up app fields for checksum */
> 
> Missing space between * and Fill up, there are a few comments where it is
> the opposite where you don't add a space at the end before the *.

Will fix the comment space in next version.

> 
> > +	if (skb->ip_summed == CHECKSUM_PARTIAL) {
> > +		if (lp->features & XAE_FEATURE_FULL_TX_CSUM) {
> > +			/* Tx Full Checksum Offload Enabled */
> > +			app[0] |= 2;
> > +		} else if (lp->features & XAE_FEATURE_PARTIAL_RX_CSUM) {
> 
> This is the transmit path here, is this path even taken?

This fragment is c&p from non dmaengine flow which used PARTIAL_RX.
Will fix it to use partial TX csum define.
> 
> > +			csum_start_off = skb_transport_offset(skb);
> > +			csum_index_off = csum_start_off + skb-
> >csum_offset;
> > +			/* Tx Partial Checksum Offload Enabled */
> > +			app[0] |= 1;
> > +			app[1] = (csum_start_off << 16) | csum_index_off;
> > +		}
> > +	} else if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
> > +		app[0] |= 2; /* Tx Full Checksum Offload Enabled */
> > +	}
> > +
> > +	dma_tx_desc = lp->tx_chan->device->device_prep_slave_sg(lp-
> >tx_chan, skbuf_dma->sgl,
> > +			sg_len, DMA_MEM_TO_DEV,
> > +			DMA_PREP_INTERRUPT, (void *)app);
> > +	if (!dma_tx_desc)
> > +		goto xmit_error_unmap_sg;
> > +
> > +	skbuf_dma->skb = skb;
> > +	skbuf_dma->sg_len = sg_len;
> > +	dma_tx_desc->callback_param = lp;
> > +	dma_tx_desc->callback_result = axienet_dma_tx_cb;
> > +	dmaengine_submit(dma_tx_desc);
> > +	dma_async_issue_pending(lp->tx_chan);
> > +
> > +	return NETDEV_TX_OK;
> > +
> > +xmit_error_unmap_sg:
> > +	dma_unmap_sg(lp->dev, skbuf_dma->sgl, sg_len, DMA_TO_DEVICE);
> > +	return NETDEV_TX_OK;
> > +}
> > +
> >   /**
> >    * axienet_tx_poll - Invoked once a transmit is completed by the
> >    * Axi DMA Tx channel.
> > @@ -911,7 +1036,42 @@ axienet_start_xmit(struct sk_buff *skb, struct
> net_device *ndev)
> >   	if (!lp->use_dmaengine)
> >   		return axienet_start_xmit_legacy(skb, ndev);
> >   	else
> > -		return NETDEV_TX_BUSY;
> > +		return axienet_start_xmit_dmaengine(skb, ndev); }
> > +
> > +/**
> > + * axienet_dma_rx_cb - DMA engine callback for RX channel.
> > + * @data:       Pointer to the skbuf_dma_descriptor structure.
> > + * @result:     error reporting through dmaengine_result.
> > + * This function is called by dmaengine driver for RX channel to
> > +notify
> > + * that the packet is received.
> > + */
> > +static void axienet_dma_rx_cb(void *data, const struct
> > +dmaengine_result *result) {
> > +	struct axienet_local *lp = data;
> > +	struct skbuf_dma_descriptor *skbuf_dma;
> > +	size_t meta_len, meta_max_len, rx_len;
> > +	struct sk_buff *skb;
> > +	u32 *app;
> > +
> > +	skbuf_dma = axienet_get_rx_desc(lp, lp->rx_ring_tail++);
> > +	skb = skbuf_dma->skb;
> > +	app = dmaengine_desc_get_metadata_ptr(skbuf_dma->desc,
> &meta_len,
> > +&meta_max_len);
> 
> app might not be the best name, I understand this refers to a DMA
> application, in a broad sense that it is not specific, maybe cookie, or
> opaque_ptr would work better?

Yes, app fields are specific to DMA. In Descriptor Fields we have 
User Application Field 0-4 (APP0-4).

The AXI4 Control stream allows user application metadata associated with 
the MM2S channel to be transmitted to a target IP. User application fields 0 
through 4 of an MM2S Scatter / Gather Start Of Frame (SOF) descriptor Transmit 
Start Of Frame (TXSOF =1) are transmitted on the m_axis_mm2s_cntrl stream 
interface along with an associated packet being transmitted on the m_axis_mm2s 
stream interface.

Is user_app_metadata/app_metadata looking more meaningful?


> > +	dma_unmap_single(lp->dev, skbuf_dma->dma_address, lp-
> >max_frm_size,
> > +			 DMA_FROM_DEVICE);
> > +	/* TODO: Derive app word index programmatically */
> > +	rx_len = (app[LEN_APP] & 0xFFFF);
> > +	skb_put(skb, rx_len);
> > +	skb->protocol = eth_type_trans(skb, lp->ndev);
> > +	skb->ip_summed = CHECKSUM_NONE;
> > +
> > +	netif_rx(skb);
> 
> Could save some cycles and call __netif_rx() here since AFAIR dmaengine
> uses a tasklet? netif_rx() is fine, just would have to compute need_bh_off.

Good point - will switch to __netif_rx().
> 
> > +	u64_stats_update_begin(&lp->rx_stat_sync);
> > +	u64_stats_add(&lp->rx_packets, 1);
> > +	u64_stats_add(&lp->rx_bytes, rx_len);
> > +	u64_stats_update_end(&lp->rx_stat_sync);
> > +	axienet_rx_submit_desc(lp->ndev);
> > +	dma_async_issue_pending(lp->rx_chan);
> >   }
> >
> >   /**
> > @@ -1147,6 +1307,142 @@ static irqreturn_t axienet_eth_irq(int irq,
> > void *_ndev)
> >
> >   static void axienet_dma_err_handler(struct work_struct *work);
> >
> > +/**
> > + * axienet_rx_submit_desc - Submit the descriptors with required data
> > + * like call backup API, skb buffer.. etc to dmaengine.
> > + *
> > + * @ndev:	net_device pointer
> > + *
> > + *Return: 0, on success.
> > + *          non-zero error value on failure
> > + */
> > +static int axienet_rx_submit_desc(struct net_device *ndev) {
> > +	struct dma_async_tx_descriptor *dma_rx_desc = NULL;
> > +	struct axienet_local *lp = netdev_priv(ndev);
> > +	struct skbuf_dma_descriptor *skbuf_dma;
> > +	struct sk_buff *skb;
> > +	dma_addr_t addr;
> > +	int ret;
> > +
> > +	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);
> 
> Need to check with dma_mapping_error() that the mapping succeeded.

Will add mapping_error check in next version. 
> 
> Thanks!
> 
> pw-bot: cr
> --
> Florian