On 03/09/15 12:29, Ananyev, Konstantin wrote: > Hi Vlad, > >> -----Original Message----- >> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Vlad Zolotarov >> Sent: Monday, March 09, 2015 10:13 AM >> To: dev at dpdk.org >> Subject: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the >> rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when reading/setting HW ring descriptor >> fields >> >> Fixed the above in ixgbe_rx_alloc_bufs() and in ixgbe_recv_scattered_pkts(). >> >> Signed-off-by: Vlad Zolotarov <vladz at cloudius-systems.com> >> --- >> lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c >> b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c >> index 9ecf3e5..b033e04 100644 >> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c >> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c >> @@ -1028,7 +1028,7 @@ ixgbe_rx_alloc_bufs(struct igb_rx_queue *rxq) >> struct igb_rx_entry *rxep; >> struct rte_mbuf *mb; >> uint16_t alloc_idx; >> - uint64_t dma_addr; >> + __le64 dma_addr; > Wonder Why you changed from uint64_t to __le64 here? > Effectively __le64 is the same a uint64_t,
I'm afraid the above it's not completely correct. See below. > and I think it is better to use always the same type across all PMD code for > consistency. Pls., note that "dma_addr" is only used (see below)... > Konstantin > > >> int diag, i; >> >> /* allocate buffers in bulk directly into the S/W ring */ >> @@ -1051,7 +1051,7 @@ ixgbe_rx_alloc_bufs(struct igb_rx_queue *rxq) >> mb->port = rxq->port_id; >> >> /* populate the descriptors */ >> - dma_addr = (uint64_t)mb->buf_physaddr + RTE_PKTMBUF_HEADROOM; >> + dma_addr = rte_cpu_to_le_64(RTE_MBUF_DATA_DMA_ADDR_DEFAULT(mb)); >> rxdp[i].read.hdr_addr = dma_addr; >> rxdp[i].read.pkt_addr = dma_addr; here. ;) And the type of both hdr_addr and pkt_addr is __le64. I don't exactly understand what do u mean by "use the same type across all PMD code for consistency" - there are a lot of types used in the PMD code and __le64 is one of them... ;) Now more seriously, let's recall what is the semantics of the __leXX types - they represent the integer in the "little endian" format. Here, NIC expects the physical address in a little endian format, thus the descriptor is defined the way it is defined - using __le64. The same relates to dma_addr local variable in this patch - it contains the physical (more correctly "DMA-able") address of the Rx buffer in the form NIC expects it to be written in the descriptor. So, why to use __leXX anyway? - Debugging the (invalid) endianess is a real headache. Therefore there are a few static code analysis tools like "sparse" that allow to detect such inconsistencies (see here http://en.wikipedia.org/wiki/Sparse) and __leXX is a helper to allow tools like sparse to detect such problems. In addition after spending some time writing patches for Linux netdev list u develop a strong habit for such stuff - Dave and others are very strict about such things... ;) So, is it the same as uint64_t? I guess now it's clear why it is now... ;) >> } >> @@ -1559,13 +1559,14 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct >> rte_mbuf **rx_pkts, >> first_seg->ol_flags = pkt_flags; >> >> if (likely(pkt_flags & PKT_RX_RSS_HASH)) >> - first_seg->hash.rss = rxd.wb.lower.hi_dword.rss; >> + first_seg->hash.rss = >> + rte_le_to_cpu_32(rxd.wb.lower.hi_dword.rss); >> else if (pkt_flags & PKT_RX_FDIR) { >> first_seg->hash.fdir.hash = >> - (uint16_t)((rxd.wb.lower.hi_dword.csum_ip.csum) >> - & IXGBE_ATR_HASH_MASK); >> + rte_le_to_cpu_16(rxd.wb.lower.hi_dword.csum_ip.csum) >> + & IXGBE_ATR_HASH_MASK; >> first_seg->hash.fdir.id = >> - rxd.wb.lower.hi_dword.csum_ip.ip_id; >> + rte_le_to_cpu_16(rxd.wb.lower.hi_dword.csum_ip.ip_id); >> } >> >> /* Prefetch data of first segment, if configured to do so. */ >> -- >> 2.1.0
