[dpdk-dev] [PATCH v4] enforce rules of the cpu and ixgbe exchange data.

[Xuelin Shi]

Hi Thomas & Konstantin,

Thanks for the review and the comments are addressed by 
http://www.dpdk.org/dev/patchwork/patch/6653/

Best Regards,
Xuelin Shi

> -Origina Konstantin l Message-
> From: Ananyev, Konstantin [mailto:konstantin.ananyev at intel.com]
> Sent: Monday, July 27, 2015 22:43
> To: Thomas Monjalon
> Cc: Shi Xuelin-B29237; dev at dpdk.org
> Subject: RE: [PATCH v4] enforce rules of the cpu and ixgbe exchange data.
> 
> 
> 
> > -Original Message-
> > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> > Sent: Monday, July 27, 2015 3:18 PM
> > To: Ananyev, Konstantin
> > Cc: xuelin.shi at freescale.com; dev at dpdk.org
> > Subject: Re: [PATCH v4] enforce rules of the cpu and ixgbe exchange
> data.
> >
> > A quick review of this long pending patch would be great.
> > Thanks
> 
> Well, it doesn't compile:
> 
> /local/kananye1/dpdk.org-ixgbevfix2-tst1/drivers/net/ixgbe/ixgbe_rxtx.c:
> In function ?ixgbe_rx_scan_hw_ring?:
> /local/kananye1/dpdk.org-ixgbevfix2-
> tst1/drivers/net/ixgbe/ixgbe_rxtx.c:1114:4: error: implicit declaration
> of function ?rte_le_to_cpu16? [-Werror=implicit-function-declaration]
> pkt_len = rte_le_to_cpu16(rxdp[j].wb.upper.length) -
> ^
> /local/kananye1/dpdk.org-ixgbevfix2-
> tst1/drivers/net/ixgbe/ixgbe_rxtx.c:1114:4: error: nested extern
> declaration of ?rte_le_to_cpu16? [-Werror=nested-externs]
> 
> 
> Should be rte_le_to_cpu_16(), I believe.
> 
> And checkpatch.pl complains on it:
> 
> WARNING: line over 80 characters
> #151: FILE: drivers/net/ixgbe/ixgbe_rxtx.c:1133:
> +
> + rte_le_to_cpu_32(rxdp[j].wb.lower.lo_dword.data));
> 
> ERROR: code indent should use tabs where possible
> #170: FILE: drivers/net/ixgbe/ixgbe_rxtx.c:1147:
> +^I^I^I ^Irxdp[j].wb.lower.hi_dword.csum_ip.csum) &$
> 
> WARNING: please, no space before tabs
> #170: FILE: drivers/net/ixgbe/ixgbe_rxtx.c:1147:
> +^I^I^I ^Irxdp[j].wb.lower.hi_dword.csum_ip.csum) &$
> 
> total: 1 errors, 2 warnings, 192 lines checked
> 
> Apart from that, looks harmless :)
> 
> Konstantin
> 
> >
> > 2015-07-16 14:45, xuelin.shi at freescale.com:
> > > From: Xuelin Shi 
> > >
> > > 1. cpu use data owned by ixgbe must use rte_le_to_cpu_xx(...) 2. cpu
> > > fill data to ixgbe must use rte_cpu_to_le_xx(...) 3. checking pci
> > > status with converted constant.
> > >
> > > Signed-off-by: Xuelin Shi 

[dpdk-dev] [PATCH v4] enforce rules of the cpu and ixgbe exchange data.

[Thomas Monjalon]

A quick review of this long pending patch would be great.
Thanks

2015-07-16 14:45, xuelin.shi at freescale.com:
> From: Xuelin Shi 
> 
> 1. cpu use data owned by ixgbe must use rte_le_to_cpu_xx(...)
> 2. cpu fill data to ixgbe must use rte_cpu_to_le_xx(...)
> 3. checking pci status with converted constant.
> 
> Signed-off-by: Xuelin Shi 

[dpdk-dev] [PATCH v4] enforce rules of the cpu and ixgbe exchange data.

[Ananyev, Konstantin]

> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Monday, July 27, 2015 3:18 PM
> To: Ananyev, Konstantin
> Cc: xuelin.shi at freescale.com; dev at dpdk.org
> Subject: Re: [PATCH v4] enforce rules of the cpu and ixgbe exchange data.
> 
> A quick review of this long pending patch would be great.
> Thanks

Well, it doesn't compile:

/local/kananye1/dpdk.org-ixgbevfix2-tst1/drivers/net/ixgbe/ixgbe_rxtx.c: In 
function ?ixgbe_rx_scan_hw_ring?:
/local/kananye1/dpdk.org-ixgbevfix2-tst1/drivers/net/ixgbe/ixgbe_rxtx.c:1114:4: 
error: implicit declaration of function ?rte_le_to_cpu16? 
[-Werror=implicit-function-declaration]
pkt_len = rte_le_to_cpu16(rxdp[j].wb.upper.length) -
^
/local/kananye1/dpdk.org-ixgbevfix2-tst1/drivers/net/ixgbe/ixgbe_rxtx.c:1114:4: 
error: nested extern declaration of ?rte_le_to_cpu16? [-Werror=nested-externs]


Should be rte_le_to_cpu_16(), I believe.

And checkpatch.pl complains on it:

WARNING: line over 80 characters
#151: FILE: drivers/net/ixgbe/ixgbe_rxtx.c:1133:
+   
rte_le_to_cpu_32(rxdp[j].wb.lower.lo_dword.data));

ERROR: code indent should use tabs where possible
#170: FILE: drivers/net/ixgbe/ixgbe_rxtx.c:1147:
+^I^I^I ^Irxdp[j].wb.lower.hi_dword.csum_ip.csum) &$

WARNING: please, no space before tabs
#170: FILE: drivers/net/ixgbe/ixgbe_rxtx.c:1147:
+^I^I^I ^Irxdp[j].wb.lower.hi_dword.csum_ip.csum) &$

total: 1 errors, 2 warnings, 192 lines checked

Apart from that, looks harmless :)

Konstantin

> 
> 2015-07-16 14:45, xuelin.shi at freescale.com:
> > From: Xuelin Shi 
> >
> > 1. cpu use data owned by ixgbe must use rte_le_to_cpu_xx(...)
> > 2. cpu fill data to ixgbe must use rte_cpu_to_le_xx(...)
> > 3. checking pci status with converted constant.
> >
> > Signed-off-by: Xuelin Shi 

[dpdk-dev] [PATCH v4] enforce rules of the cpu and ixgbe exchange data.

[xuelin....@freescale.com]

From: Xuelin Shi 

1. cpu use data owned by ixgbe must use rte_le_to_cpu_xx(...)
2. cpu fill data to ixgbe must use rte_cpu_to_le_xx(...)
3. checking pci status with converted constant.

Signed-off-by: Xuelin Shi 
---
 drivers/net/ixgbe/ixgbe_rxtx.c | 77 --
 1 file changed, 51 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index a7c94a9..5a6cc1c 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -130,7 +130,7 @@ ixgbe_tx_free_bufs(struct ixgbe_tx_queue *txq)

/* check DD bit on threshold descriptor */
status = txq->tx_ring[txq->tx_next_dd].wb.status;
-   if (! (status & IXGBE_ADVTXD_STAT_DD))
+   if (!(status & rte_cpu_to_le_32(IXGBE_ADVTXD_STAT_DD)))
return 0;

/*
@@ -175,11 +175,14 @@ tx4(volatile union ixgbe_adv_tx_desc *txdp, struct 
rte_mbuf **pkts)
pkt_len = (*pkts)->data_len;

/* write data to descriptor */
-   txdp->read.buffer_addr = buf_dma_addr;
+   txdp->read.buffer_addr = rte_cpu_to_le_64(buf_dma_addr);
+
txdp->read.cmd_type_len =
-   ((uint32_t)DCMD_DTYP_FLAGS | pkt_len);
+   rte_cpu_to_le_32((uint32_t)DCMD_DTYP_FLAGS | pkt_len);
+
txdp->read.olinfo_status =
-   (pkt_len << IXGBE_ADVTXD_PAYLEN_SHIFT);
+   rte_cpu_to_le_32(pkt_len << IXGBE_ADVTXD_PAYLEN_SHIFT);
+
rte_prefetch0(&(*pkts)->pool);
}
 }
@@ -195,11 +198,14 @@ tx1(volatile union ixgbe_adv_tx_desc *txdp, struct 
rte_mbuf **pkts)
pkt_len = (*pkts)->data_len;

/* write data to descriptor */
-   txdp->read.buffer_addr = buf_dma_addr;
+   txdp->read.buffer_addr = rte_cpu_to_le_64(buf_dma_addr);
+
txdp->read.cmd_type_len =
-   ((uint32_t)DCMD_DTYP_FLAGS | pkt_len);
+   rte_cpu_to_le_32((uint32_t)DCMD_DTYP_FLAGS | pkt_len);
+
txdp->read.olinfo_status =
-   (pkt_len << IXGBE_ADVTXD_PAYLEN_SHIFT);
+   rte_cpu_to_le_32(pkt_len << IXGBE_ADVTXD_PAYLEN_SHIFT);
+
rte_prefetch0(&(*pkts)->pool);
 }

@@ -511,6 +517,7 @@ ixgbe_xmit_cleanup(struct ixgbe_tx_queue *txq)
uint16_t nb_tx_desc = txq->nb_tx_desc;
uint16_t desc_to_clean_to;
uint16_t nb_tx_to_clean;
+   uint32_t stat;

/* Determine the last descriptor needing to be cleaned */
desc_to_clean_to = (uint16_t)(last_desc_cleaned + txq->tx_rs_thresh);
@@ -519,7 +526,9 @@ ixgbe_xmit_cleanup(struct ixgbe_tx_queue *txq)

/* Check to make sure the last descriptor to clean is done */
desc_to_clean_to = sw_ring[desc_to_clean_to].last_id;
-   if (! (txr[desc_to_clean_to].wb.status & IXGBE_TXD_STAT_DD))
+
+   stat = txr[desc_to_clean_to].wb.status;
+   if (!(stat & rte_cpu_to_le_32(IXGBE_TXD_STAT_DD)))
{
PMD_TX_FREE_LOG(DEBUG,
"TX descriptor %4u is not done"
@@ -806,12 +815,14 @@ ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
 */
slen = m_seg->data_len;
buf_dma_addr = RTE_MBUF_DATA_DMA_ADDR(m_seg);
+
txd->read.buffer_addr =
rte_cpu_to_le_64(buf_dma_addr);
txd->read.cmd_type_len =
rte_cpu_to_le_32(cmd_type_len | slen);
txd->read.olinfo_status =
rte_cpu_to_le_32(olinfo_status);
+
txe->last_id = tx_last;
tx_id = txe->next_id;
txe = txn;
@@ -1062,14 +1073,16 @@ ixgbe_rx_scan_hw_ring(struct ixgbe_rx_queue *rxq)
int s[LOOK_AHEAD], nb_dd;
 #endif /* RTE_NEXT_ABI */
int i, j, nb_rx = 0;
+   uint32_t stat;


/* get references to current descriptor and S/W ring entry */
rxdp = >rx_ring[rxq->rx_tail];
rxep = >sw_ring[rxq->rx_tail];

+   stat = rxdp->wb.upper.status_error;
/* check to make sure there is at least 1 packet to receive */
-   if (! (rxdp->wb.upper.status_error & IXGBE_RXDADV_STAT_DD))
+   if (!(stat & rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD)))
return 0;

/*
@@ -1081,7 +1094,7 @@ ixgbe_rx_scan_hw_ring(struct ixgbe_rx_queue *rxq)
{
/* Read desc statuses backwards to avoid race condition */
for (j = LOOK_AHEAD-1; j >= 0; --j)
-   s[j] = rxdp[j].wb.upper.status_error;
+   s[j] = rte_le_to_cpu_32(rxdp[j].wb.upper.status_error);

 #ifdef RTE_NEXT_ABI
for (j = LOOK_AHEAD - 1; j >= 0; --j)
@@ -1099,7 +1112,9 @@ ixgbe_rx_scan_hw_ring(struct ixgbe_rx_queue