| Message ID | 1446571576-28399-1-git-send-email-3chas3@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 88216919F; Tue, 3 Nov 2015 18:26:23 +0100 (CET) Received: from mail-qk0-f171.google.com (mail-qk0-f171.google.com [209.85.220.171]) by dpdk.org (Postfix) with ESMTP id D83D99198 for <dev@dpdk.org>; Tue, 3 Nov 2015 18:26:22 +0100 (CET) Received: by qkcl124 with SMTP id l124so9362768qkc.3 for <dev@dpdk.org>; Tue, 03 Nov 2015 09:26:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:date:message-id; bh=q/spc/jf4/zC5VX3OkOI3ejbTPYHr/dQ+P72+spV/lI=; b=JjMa3D6vVF+Otw1r7KAkPDf13viC/2p/nz6nWvNXiZdvh0W9ssDO1kSxaYItnfeymy bfsngUk7hBH9v7ne/aHPkNTlL0EEM0f6oPXYfd3mjT3hK3lMUNDWNefPbnmd0FYnXAWA H3P78TpP3hemUoJLrGFpsa+TK2tz0czFzTIr+w4ZsOGf5PEUwU/YF2WJP8Y7IT6vJL/T pyh5Bmr0iphdOZtQJ6rS4+uC8IpPBd7tJpjXfx0xslJyUynuK+QqpDy4mTgODS2Ewz2+ FNsUx9i3WDiDro2nbHP5ql92Ll73IBbHi4MU+r0M7yHir0vp7aXZN7pimmpG4dOFl2qM 6RhQ== X-Received: by 10.55.77.201 with SMTP id a192mr38819502qkb.48.1446571582329; Tue, 03 Nov 2015 09:26:22 -0800 (PST) Received: from foobar.home (pool-71-163-182-126.washdc.fios.verizon.net. [71.163.182.126]) by smtp.gmail.com with ESMTPSA id w50sm10045263qge.45.2015.11.03.09.26.21 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 03 Nov 2015 09:26:21 -0800 (PST) From: Chas Williams <3chas3@gmail.com> To: dev@dpdk.org Date: Tue, 3 Nov 2015 12:26:16 -0500 Message-Id: <1446571576-28399-1-git-send-email-3chas3@gmail.com> X-Mailer: git-send-email 2.1.0 Subject: [dpdk-dev] [PATCH] bnx2x: tx_start_bd->vlan_or_ethertype is le16 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Chas Williams
Nov. 3, 2015, 5:26 p.m. UTC
Signed-off-by: Chas Williams <3chas3@gmail.com>
---
drivers/net/bnx2x/bnx2x.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Comments
Anyone to review please? 2015-11-03 12:26, Chas Williams: > Signed-off-by: Chas Williams <3chas3@gmail.com> > --- > drivers/net/bnx2x/bnx2x.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/bnx2x/bnx2x.c b/drivers/net/bnx2x/bnx2x.c > index fed7a06..76444eb 100644 > --- a/drivers/net/bnx2x/bnx2x.c > +++ b/drivers/net/bnx2x/bnx2x.c > @@ -2169,7 +2169,8 @@ int bnx2x_tx_encap(struct bnx2x_tx_queue *txq, struct rte_mbuf **m_head, int m_p > struct ether_hdr *eh > = rte_pktmbuf_mtod(m0, struct ether_hdr *); > > - tx_start_bd->vlan_or_ethertype = eh->ether_type; > + tx_start_bd->vlan_or_ethertype > + = rte_cpu_to_le_16(rte_be_to_cpu_16(eh->ether_type)); > } > }
> >Anyone to review please? > >2015-11-03 12:26, Chas Williams: >> Signed-off-by: Chas Williams <3chas3@gmail.com> >> --- >> drivers/net/bnx2x/bnx2x.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/bnx2x/bnx2x.c b/drivers/net/bnx2x/bnx2x.c >> index fed7a06..76444eb 100644 >> --- a/drivers/net/bnx2x/bnx2x.c >> +++ b/drivers/net/bnx2x/bnx2x.c >> @@ -2169,7 +2169,8 @@ int bnx2x_tx_encap(struct bnx2x_tx_queue *txq, >>struct rte_mbuf **m_head, int m_p >> struct ether_hdr *eh >> = rte_pktmbuf_mtod(m0, struct ether_hdr *); >> >> - tx_start_bd->vlan_or_ethertype = eh->ether_type; >> + tx_start_bd->vlan_or_ethertype >> + = rte_cpu_to_le_16(rte_be_to_cpu_16(eh->ether_type)); >> } >> } > > Acked-by: Harish Patil <harish.patil@qlogic.com> Minor question - any specific reason to use rte_be_to_cpu_16() on ether_type alone before converting from native order to le16? Thanks, Harish ________________________________ This message and any attached documents contain information from the sending company or its parent company(s), subsidiaries, divisions or branch offices that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.
On Tue, 1 Dec 2015 21:53:59 +0000 Harish Patil <harish.patil@qlogic.com> wrote: > > > >Anyone to review please? > > > >2015-11-03 12:26, Chas Williams: > >> Signed-off-by: Chas Williams <3chas3@gmail.com> > >> --- > >> drivers/net/bnx2x/bnx2x.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/net/bnx2x/bnx2x.c b/drivers/net/bnx2x/bnx2x.c > >> index fed7a06..76444eb 100644 > >> --- a/drivers/net/bnx2x/bnx2x.c > >> +++ b/drivers/net/bnx2x/bnx2x.c > >> @@ -2169,7 +2169,8 @@ int bnx2x_tx_encap(struct bnx2x_tx_queue *txq, > >>struct rte_mbuf **m_head, int m_p > >> struct ether_hdr *eh > >> = rte_pktmbuf_mtod(m0, struct ether_hdr *); > >> > >> - tx_start_bd->vlan_or_ethertype = eh->ether_type; > >> + tx_start_bd->vlan_or_ethertype > >> + = rte_cpu_to_le_16(rte_be_to_cpu_16(eh->ether_type)); > >> } > >> } > > > > > > Acked-by: Harish Patil <harish.patil@qlogic.com> > > > Minor question - any specific reason to use rte_be_to_cpu_16() on > ether_type alone before converting from native order to le16? ether_type is in network byte order (big endian) and hardware wants little endian. On x86 the second step is a nop.
> >On Tue, 1 Dec 2015 21:53:59 +0000 >Harish Patil <harish.patil@qlogic.com> wrote: > >> > >> >Anyone to review please? >> > >> >2015-11-03 12:26, Chas Williams: >> >> Signed-off-by: Chas Williams <3chas3@gmail.com> >> >> --- >> >> drivers/net/bnx2x/bnx2x.c | 3 ++- >> >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/net/bnx2x/bnx2x.c b/drivers/net/bnx2x/bnx2x.c >> >> index fed7a06..76444eb 100644 >> >> --- a/drivers/net/bnx2x/bnx2x.c >> >> +++ b/drivers/net/bnx2x/bnx2x.c >> >> @@ -2169,7 +2169,8 @@ int bnx2x_tx_encap(struct bnx2x_tx_queue *txq, >> >>struct rte_mbuf **m_head, int m_p >> >> struct ether_hdr *eh >> >> = rte_pktmbuf_mtod(m0, struct >>ether_hdr *); >> >> >> >> - tx_start_bd->vlan_or_ethertype = >>eh->ether_type; >> >> + tx_start_bd->vlan_or_ethertype >> >> + = >>rte_cpu_to_le_16(rte_be_to_cpu_16(eh->ether_type)); >> >> } >> >> } >> > >> > >> >> Acked-by: Harish Patil <harish.patil@qlogic.com> >> >> >> Minor question - any specific reason to use rte_be_to_cpu_16() on >> ether_type alone before converting from native order to le16? > >ether_type is in network byte order (big endian) >and hardware wants little endian. On x86 the second step is a nop. > Thanks. Yes the question was for second step, second step would be a no-op on x86 - thanks for clarifying. ________________________________ This message and any attached documents contain information from the sending company or its parent company(s), subsidiaries, divisions or branch offices that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.
2015-12-01 14:37, Stephen Hemminger: > Harish Patil <harish.patil@qlogic.com> wrote: > > >2015-11-03 12:26, Chas Williams: > > >> --- a/drivers/net/bnx2x/bnx2x.c > > >> +++ b/drivers/net/bnx2x/bnx2x.c > > >> - tx_start_bd->vlan_or_ethertype = eh->ether_type; > > >> + tx_start_bd->vlan_or_ethertype > > >> + = rte_cpu_to_le_16(rte_be_to_cpu_16(eh->ether_type)); > > > > Minor question - any specific reason to use rte_be_to_cpu_16() on > > ether_type alone before converting from native order to le16? > > ether_type is in network byte order (big endian) > and hardware wants little endian. On x86 the second step is a nop. Doesn't it deserve a macro rte_ntole16()? It may be in lib/librte_eal/common/include/generic/rte_byteorder.h.
On Wed, 2015-12-02 at 00:34 +0100, Thomas Monjalon wrote: > 2015-12-01 14:37, Stephen Hemminger: > > Harish Patil <harish.patil@qlogic.com> wrote: > > > >2015-11-03 12:26, Chas Williams: > > > >> --- a/drivers/net/bnx2x/bnx2x.c > > > >> +++ b/drivers/net/bnx2x/bnx2x.c > > > >> - tx_start_bd->vlan_or_ethertype = eh->ether_type; > > > >> + tx_start_bd->vlan_or_ethertype > > > >> + = rte_cpu_to_le_16(rte_be_to_cpu_16(eh->ether_type)); > > > > > > Minor question - any specific reason to use rte_be_to_cpu_16() on > > > ether_type alone before converting from native order to le16? > > > > ether_type is in network byte order (big endian) > > and hardware wants little endian. On x86 the second step is a nop. > > Doesn't it deserve a macro rte_ntole16()? > It may be in lib/librte_eal/common/include/generic/rte_byteorder.h. I looked I didn't see anything. This value, according to the linux driver, wants to be little endian regardless of the host endian.
2015-12-01 18:58, Charles Williams: > On Wed, 2015-12-02 at 00:34 +0100, Thomas Monjalon wrote: > > 2015-12-01 14:37, Stephen Hemminger: > > > Harish Patil <harish.patil@qlogic.com> wrote: > > > > >2015-11-03 12:26, Chas Williams: > > > > >> --- a/drivers/net/bnx2x/bnx2x.c > > > > >> +++ b/drivers/net/bnx2x/bnx2x.c > > > > >> - tx_start_bd->vlan_or_ethertype = eh->ether_type; > > > > >> + tx_start_bd->vlan_or_ethertype > > > > >> + = rte_cpu_to_le_16(rte_be_to_cpu_16(eh->ether_type)); > > > > > > > > Minor question - any specific reason to use rte_be_to_cpu_16() on > > > > ether_type alone before converting from native order to le16? > > > > > > ether_type is in network byte order (big endian) > > > and hardware wants little endian. On x86 the second step is a nop. > > > > Doesn't it deserve a macro rte_ntole16()? > > It may be in lib/librte_eal/common/include/generic/rte_byteorder.h. > > I looked I didn't see anything. This value, according to the linux > driver, wants to be little endian regardless of the host endian. Yes, that's why I suggest to create some macros to do this kind of conversion. Example: rte_ntole16 means "network to little endian 16-bit". Do you think it would be clearer to use?
On Wed, 2015-12-02 at 02:04 +0100, Thomas Monjalon wrote: > 2015-12-01 18:58, Charles Williams: > > On Wed, 2015-12-02 at 00:34 +0100, Thomas Monjalon wrote: > > > 2015-12-01 14:37, Stephen Hemminger: > > > > Harish Patil <harish.patil@qlogic.com> wrote: > > > > > >2015-11-03 12:26, Chas Williams: > > > > > >> --- a/drivers/net/bnx2x/bnx2x.c > > > > > >> +++ b/drivers/net/bnx2x/bnx2x.c > > > > > >> - tx_start_bd->vlan_or_ethertype = eh->ether_type; > > > > > >> + tx_start_bd->vlan_or_ethertype > > > > > >> + = rte_cpu_to_le_16(rte_be_to_cpu_16(eh->ether_type)); > > > > > > > > > > Minor question - any specific reason to use rte_be_to_cpu_16() on > > > > > ether_type alone before converting from native order to le16? > > > > > > > > ether_type is in network byte order (big endian) > > > > and hardware wants little endian. On x86 the second step is a nop. > > > > > > Doesn't it deserve a macro rte_ntole16()? > > > It may be in lib/librte_eal/common/include/generic/rte_byteorder.h. > > > > I looked I didn't see anything. This value, according to the linux > > driver, wants to be little endian regardless of the host endian. > > Yes, that's why I suggest to create some macros to do this kind of conversion. > Example: rte_ntole16 means "network to little endian 16-bit". > Do you think it would be clearer to use? This is the only example of this kind of conversion in the source code so it would be a macro for one user. If you create rte_ntole16() you might feel obligated to create the various permutations for which there are no consumers.
2015-12-02 05:18, Charles Williams: > On Wed, 2015-12-02 at 02:04 +0100, Thomas Monjalon wrote: > > 2015-12-01 18:58, Charles Williams: > > > On Wed, 2015-12-02 at 00:34 +0100, Thomas Monjalon wrote: > > > > 2015-12-01 14:37, Stephen Hemminger: > > > > > Harish Patil <harish.patil@qlogic.com> wrote: > > > > > > >2015-11-03 12:26, Chas Williams: > > > > > > >> --- a/drivers/net/bnx2x/bnx2x.c > > > > > > >> +++ b/drivers/net/bnx2x/bnx2x.c > > > > > > >> - tx_start_bd->vlan_or_ethertype = eh->ether_type; > > > > > > >> + tx_start_bd->vlan_or_ethertype > > > > > > >> + = rte_cpu_to_le_16(rte_be_to_cpu_16(eh->ether_type)); > > > > > > > > > > > > Minor question - any specific reason to use rte_be_to_cpu_16() on > > > > > > ether_type alone before converting from native order to le16? > > > > > > > > > > ether_type is in network byte order (big endian) > > > > > and hardware wants little endian. On x86 the second step is a nop. > > > > > > > > Doesn't it deserve a macro rte_ntole16()? > > > > It may be in lib/librte_eal/common/include/generic/rte_byteorder.h. > > > > > > I looked I didn't see anything. This value, according to the linux > > > driver, wants to be little endian regardless of the host endian. > > > > Yes, that's why I suggest to create some macros to do this kind of conversion. > > Example: rte_ntole16 means "network to little endian 16-bit". > > Do you think it would be clearer to use? > > This is the only example of this kind of conversion in the source code > so it would be a macro for one user. If you create rte_ntole16() you > might feel obligated to create the various permutations for which there > are no consumers. Yes, that's why I was not sure of the interest.
> >> Signed-off-by: Chas Williams <3chas3@gmail.com> > > Acked-by: Harish Patil <harish.patil@qlogic.com> Applied, thanks
diff --git a/drivers/net/bnx2x/bnx2x.c b/drivers/net/bnx2x/bnx2x.c index fed7a06..76444eb 100644 --- a/drivers/net/bnx2x/bnx2x.c +++ b/drivers/net/bnx2x/bnx2x.c @@ -2169,7 +2169,8 @@ int bnx2x_tx_encap(struct bnx2x_tx_queue *txq, struct rte_mbuf **m_head, int m_p struct ether_hdr *eh = rte_pktmbuf_mtod(m0, struct ether_hdr *); - tx_start_bd->vlan_or_ethertype = eh->ether_type; + tx_start_bd->vlan_or_ethertype + = rte_cpu_to_le_16(rte_be_to_cpu_16(eh->ether_type)); } }