On 11/17/2022 8:33 AM, Chaoyong He wrote:
> From: Long Wu <long...@corigine.com>
>
> If dma_len is larger than "NFDK_DESC_TX_DMA_LEN_HEAD", the value of
> dma_len bitwise and NFDK_DESC_TX_DMA_LEN_HEAD maybe less than packet
> head length. Fill maximum dma_len in first tx descriptor to make
> sure the whole head is included in the first descriptor.
I understand the problem, but impact is not clear. I assume this cause
failure on Tx of the package or Tx a corrupted package maybe but can you
please explain in the commit log what observed problem is fixed?
> In addition,
> add some explanation for NFDK code more readable.
>
> Fixes: c73dced48c8c ("net/nfp: add NFDk Tx")
> Cc: jin....@corigine.com
> Cc: sta...@dpdk.org
>
> Signed-off-by: Long Wu <long...@corigine.com>
> Reviewed-by: Niklas Söderlund <niklas.soderl...@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong...@corigine.com>
> ---
> drivers/net/nfp/nfp_rxtx.c | 27 ++++++++++++++++++++++++++-
> 1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/nfp/nfp_rxtx.c b/drivers/net/nfp/nfp_rxtx.c
> index b8c874d315..ed88d740fa 100644
> --- a/drivers/net/nfp/nfp_rxtx.c
> +++ b/drivers/net/nfp/nfp_rxtx.c
> @@ -1064,6 +1064,7 @@ nfp_net_nfdk_tx_maybe_close_block(struct nfp_net_txq
> *txq, struct rte_mbuf *pkt)
> if (unlikely(n_descs > NFDK_TX_DESC_GATHER_MAX))
> return -EINVAL;
>
> + /* Under count by 1 (don't count meta) for the round down to work out */
> n_descs += !!(pkt->ol_flags & RTE_MBUF_F_TX_TCP_SEG);
>
> if (round_down(txq->wr_p, NFDK_TX_DESC_BLOCK_CNT) !=
> @@ -1180,6 +1181,7 @@ nfp_net_nfdk_xmit_pkts(void *tx_queue, struct rte_mbuf
> **tx_pkts, uint16_t nb_pk
> /* Sending packets */
> while ((npkts < nb_pkts) && free_descs) {
> uint32_t type, dma_len, dlen_type, tmp_dlen;
> + uint32_t tmp_hlen;
> int nop_descs, used_descs;
>
> pkt = *(tx_pkts + npkts);
> @@ -1218,8 +1220,23 @@ nfp_net_nfdk_xmit_pkts(void *tx_queue, struct rte_mbuf
> **tx_pkts, uint16_t nb_pk
> } else {
> type = NFDK_DESC_TX_TYPE_GATHER;
> }
> +
> + /* Implicitly truncates to chunk in below logic */
> dma_len -= 1;
> - dlen_type = (NFDK_DESC_TX_DMA_LEN_HEAD & dma_len) |
> +
> + /*
> + * We will do our best to pass as much data as we can in
> descriptor
> + * and we need to make sure the first descriptor includes whole
> + * head since there is limitation in firmware side. Sometimes
> the
> + * value of dma_len bitwise and NFDK_DESC_TX_DMA_LEN_HEAD will
> less
"bitwise and" is confusing while reading, because of 'and', can you
please use '&' instead, I think it is easier to understand that way.
> + * than packet head len.
> + */
> + if (dma_len > NFDK_DESC_TX_DMA_LEN_HEAD)
> + tmp_hlen = NFDK_DESC_TX_DMA_LEN_HEAD;
> + else
> + tmp_hlen = dma_len;
> +
What is the point of masking if you already have above check?
Why don't you use 'tmp_hlen' directly, instead of
"(NFDK_DESC_TX_DMA_LEN_HEAD & tmp_hlen)" after above check?
> + dlen_type = (NFDK_DESC_TX_DMA_LEN_HEAD & tmp_hlen) |
Since 'tmp_hlen' is only used one, you may prefer ternary operator to
get rid of temporary variable, but it is up to you based on readability:
dlen_type = (dma_len > NFDK_DESC_TX_DMA_LEN_HEAD ?
NFDK_DESC_TX_DMA_LEN_HEAD : dma_len) |
> (NFDK_DESC_TX_TYPE_HEAD & (type << 12));
> ktxds->dma_len_type = rte_cpu_to_le_16(dlen_type);
> dma_addr = rte_mbuf_data_iova(pkt);
> @@ -1229,10 +1246,18 @@ nfp_net_nfdk_xmit_pkts(void *tx_queue, struct
> rte_mbuf **tx_pkts, uint16_t nb_pk
> ktxds->dma_addr_lo = rte_cpu_to_le_32(dma_addr & 0xffffffff);
> ktxds++;
>
> + /*
> + * Preserve the original dlen_type, this way below the EOP logic
> + * can use dlen_type.
> + */
> tmp_dlen = dlen_type & NFDK_DESC_TX_DMA_LEN_HEAD;
> dma_len -= tmp_dlen;
> dma_addr += tmp_dlen + 1;
>
> + /*
> + * The rest of the data (if any) will be in larger dma
> descritors
> + * and is handled with the dma_len loop.
> + */
> while (pkt) {
> if (*lmbuf)
> rte_pktmbuf_free_seg(*lmbuf);
