From: Andrew May <acmay@acmay.org>
To: David Daney <ddaney@caviumnetworks.com>
Cc: ralf@linux-mips.org, linux-mips@linux-mips.org,
netdev@vger.kernel.org, gregkh@suse.de
Subject: Re: [PATCH 7/7] Staging: Octeon Ethernet: Use constants from in.h
Date: Fri, 15 Jan 2010 21:44:19 -0800 [thread overview]
Message-ID: <20100115214419.39357506@mud> (raw)
In-Reply-To: <4B50C4F4.60903@caviumnetworks.com>
On Fri, 15 Jan 2010 11:41:40 -0800
David Daney <ddaney@caviumnetworks.com> wrote:
> Andrew May wrote:
> > On Thu, 7 Jan 2010 11:05:06 -0800
> > David Daney <ddaney@caviumnetworks.com> wrote:
> >
> >> Signed-off-by: David Daney <ddaney@caviumnetworks.com>
> >> ---
> >> drivers/staging/octeon/ethernet-defines.h | 3 ---
> >> drivers/staging/octeon/ethernet-tx.c | 8 ++++----
> >> 2 files changed, 4 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/staging/octeon/ethernet-defines.h
> >> b/drivers/staging/octeon/ethernet-defines.h index 9c4910e..00a8561
> >> 100644 --- a/drivers/staging/octeon/ethernet-defines.h
> >> +++ b/drivers/staging/octeon/ethernet-defines.h
> >> @@ -98,9 +98,6 @@
> >> #define MAX_SKB_TO_FREE 10
> >> #define MAX_OUT_QUEUE_DEPTH 1000
> >>
> >> -#define IP_PROTOCOL_TCP 6
> >> -#define IP_PROTOCOL_UDP 0x11
> >> -
> >> #define FAU_NUM_PACKET_BUFFERS_TO_FREE (CVMX_FAU_REG_END -
> >> sizeof(uint32_t)) #define TOTAL_NUMBER_OF_PORTS
> >> (CVMX_PIP_NUM_INPUT_PORTS+1)
> >> diff --git a/drivers/staging/octeon/ethernet-tx.c
> >> b/drivers/staging/octeon/ethernet-tx.c index bc67e41..62258bd
> >> 100644 --- a/drivers/staging/octeon/ethernet-tx.c
> >> +++ b/drivers/staging/octeon/ethernet-tx.c
> >> @@ -359,8 +359,8 @@ dont_put_skbuff_in_hw:
> >> if (USE_HW_TCPUDP_CHECKSUM && (skb->protocol ==
> >> htons(ETH_P_IP)) && (ip_hdr(skb)->version == 4) &&
> >> (ip_hdr(skb)->ihl == 5) && ((ip_hdr(skb)->frag_off == 0) ||
> >> (ip_hdr(skb)->frag_off == 1 << 14))
> >> - && ((ip_hdr(skb)->protocol == IP_PROTOCOL_TCP)
> >> - || (ip_hdr(skb)->protocol == IP_PROTOCOL_UDP))) {
> >> + && ((ip_hdr(skb)->protocol == IPPROTO_TCP)
> >> + || (ip_hdr(skb)->protocol == IPPROTO_UDP))) {
> >> /* Use hardware checksum calc */
> >> pko_command.s.ipoffp1 = sizeof(struct ethhdr) + 1;
> >> }
> >
> > Why isn't skb->ip_summed checked here instead?
>
> That may indeed be the correct thing to do, but the main point of
> this particular patch is to remove local definitions that mirror
> definitions provided by core include files.
>
> So in order to not let 'the perfect be the enemy of the good' I think
> this patch should be applied.
I am not against the patch being applied. I also don't think I have
any sort of influence to get it rejected. But in seeing the code it is
just something that jumps out at me, and I wanted to make sure I wasn't
missing something else.
> Indeed it does. Actually it writes a good checksum on *all* IP
> packets without regard to their source.
That seems like a very bad thing. Maybe the entire section of code
should be removed along with the defines for now.
I don't actually have one of these chips at home to play with and test.
prev parent reply other threads:[~2010-01-16 5:44 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-01-07 19:03 [PATCH 0/7] Staging: Improvments to Octeon Ethernet driver (second attempt) David Daney
2010-01-07 19:05 ` [PATCH 1/7] MIPS: Octeon: Fix EIO handling David Daney
2010-01-07 20:37 ` Sergei Shtylyov
2010-01-07 20:52 ` David Daney
2010-01-07 21:59 ` Greg KH
2010-01-12 11:36 ` Ralf Baechle
2010-01-12 13:43 ` Greg KH
2010-01-07 19:05 ` [PATCH 2/7] Staging: Octeon Ethernet: Remove unused code David Daney
2010-01-07 19:05 ` [PATCH 3/7] Staging: Octeon Ethernet: Fix memory allocation David Daney
2010-01-07 19:05 ` [PATCH 4/7] Staging: Octeon Ethernet: Rewrite transmit code David Daney
2010-01-07 19:05 ` [PATCH 5/7] Staging: Octeon Ethernet: Convert to NAPI David Daney
2010-01-07 19:05 ` [PATCH 6/7] Staging: Octeon Ethernet: Enable scatter-gather David Daney
2010-01-07 19:05 ` [PATCH 7/7] Staging: Octeon Ethernet: Use constants from in.h David Daney
2010-01-15 7:29 ` Andrew May
2010-01-15 19:41 ` David Daney
2010-01-16 5:44 ` Andrew May [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20100115214419.39357506@mud \
--to=acmay@acmay.org \
--cc=ddaney@caviumnetworks.com \
--cc=gregkh@suse.de \
--cc=linux-mips@linux-mips.org \
--cc=netdev@vger.kernel.org \
--cc=ralf@linux-mips.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).