* Re: [net-next-2.6 PATCH v4 2/3] TCPCT part 1b: sysctl_tcp_cookie_size, socket option TCP_COOKIE_TRANSACTIONS, functions
From: David Miller @ 2009-11-02 10:51 UTC (permalink / raw)
To: william.allen.simpson; +Cc: eric.dumazet, netdev
In-Reply-To: <4AEEB842.1070705@gmail.com>
From: William Allen Simpson <william.allen.simpson@gmail.com>
Date: Mon, 02 Nov 2009 05:45:22 -0500
> Is that a problem?
Yes, it is a problem.
You can allocate the bit when you add the feature.
^ permalink raw reply
* Re: [net-next-2.6 PATCH RFC] TCPCT part 1d: generate Responder Cookie
From: Eric Dumazet @ 2009-11-02 10:56 UTC (permalink / raw)
To: William Allen Simpson
Cc: Linux Kernel Developers, Linux Kernel Network Developers
In-Reply-To: <4AEEB6D2.7090505@gmail.com>
William Allen Simpson a écrit :
>
> The (buggy?) syncookie code that I'm avoiding/replacing is:
>
> static DEFINE_PER_CPU(__u32 [16 + 5 + SHA_WORKSPACE_WORDS],
> ipv4_cookie_scratch);
>
> static u32 cookie_hash(__be32 saddr, __be32 daddr, __be16 sport, __be16
> dport,
> u32 count, int c)
> {
> __u32 *tmp = __get_cpu_var(ipv4_cookie_scratch);
>
> memcpy(tmp + 4, syncookie_secret[c], sizeof(syncookie_secret[c]));
> tmp[0] = (__force u32)saddr;
> tmp[1] = (__force u32)daddr;
> tmp[2] = ((__force u32)sport << 16) + (__force u32)dport;
> tmp[3] = count;
> sha_transform(tmp + 16, (__u8 *)tmp, tmp + 16 + 5);
>
> return tmp[17];
> }
>
> It appears to me that the operations could be interrupted at any time, and
> the fairly expensive sha transform (or probably any of the assignments)
> could be corrupted?
>
> That is, cpu0 grabs a scratch area, copies some data into it, interrupts,
> cpu0 comes along again with another packet, points into the same workspace,
> mashes the data, while cpu1 completes the previous operation with the
> old tmp pointer on the stack.
>
> Worse, this is called twice, each time getting tmp, and mashing the data,
> and at the same time others are calling it twice more for verification.
>
> Since syncookies only occur under stress, the code isn't well tested, and
> the only symptom would be the returned packet would be dropped after
> failing to verify. Since this only happens when lots of packets are
> arriving, dropped packets probably aren't noticed.
>
> However, that would be unacceptable for my code.
>
cookie_hash() runs in a non preemptable context. CPU cannot change under us.
(or else, we would not use __get_cpu_var(ipv4_cookie_scratch); )
And of course, each cpu gets its own scratch area, thanks to __get_cpu_var()
^ permalink raw reply
* [PATCH net-next-2.6] ipv6: no more dev_put() in inet6_bind()
From: Eric Dumazet @ 2009-11-02 11:10 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
Avoids touching device refcount in inet6_bind(), thanks to RCU
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/ipv6/af_inet6.c | 17 +++++++++--------
1 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index b6d0588..9105b25 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -314,6 +314,7 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
if (addr_type != IPV6_ADDR_ANY) {
struct net_device *dev = NULL;
+ rcu_read_lock();
if (addr_type & IPV6_ADDR_LINKLOCAL) {
if (addr_len >= sizeof(struct sockaddr_in6) &&
addr->sin6_scope_id) {
@@ -326,12 +327,12 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
/* Binding to link-local address requires an interface */
if (!sk->sk_bound_dev_if) {
err = -EINVAL;
- goto out;
+ goto out_unlock;
}
- dev = dev_get_by_index(net, sk->sk_bound_dev_if);
+ dev = dev_get_by_index_rcu(net, sk->sk_bound_dev_if);
if (!dev) {
err = -ENODEV;
- goto out;
+ goto out_unlock;
}
}
@@ -342,14 +343,11 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
if (!(addr_type & IPV6_ADDR_MULTICAST)) {
if (!ipv6_chk_addr(net, &addr->sin6_addr,
dev, 0)) {
- if (dev)
- dev_put(dev);
err = -EADDRNOTAVAIL;
- goto out;
+ goto out_unlock;
}
}
- if (dev)
- dev_put(dev);
+ rcu_read_unlock();
}
}
@@ -381,6 +379,9 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
out:
release_sock(sk);
return err;
+out_unlock:
+ rcu_read_unlock();
+ goto out;
}
EXPORT_SYMBOL(inet6_bind);
^ permalink raw reply related
* RE: Patch: Fix userland build failure in linux/if_tunnel.h
From: Sledz, Steffen @ 2009-11-02 11:15 UTC (permalink / raw)
To: Sergei Poselenov; +Cc: wd, linux-kernel, netdev
In-Reply-To: <20091102135750.436a7b54@emcraft.com>
Hello Sergei, hello other maintainers,
> > i'm working for the OpenEmbedded project and we hit the same busybox
> > problem you mentioned in <http://patchwork.kernel.org/patch/31164/>.
> >
> > It doesn't look like you got much feedback on you patch suggestion.
> > Did it made it's way to the kernel sources? Or where there any
> > objections on it?
> Hm, I received a confirmation, that my fix was applied
> http://www.spinics.net/lists/netdev/msg103860.html
No, i think it didn't. This confirmation is related to a patch from
Joe Perches to the MAINTAINERS file.
> However, it indeed didn't find their way into the mainline.
> I have no idea why, received no objections.
What are the maintainers saying?
Steffen
^ permalink raw reply
* Re: Patch: Fix userland build failure in linux/if_tunnel.h
From: Sergei Poselenov @ 2009-11-02 10:57 UTC (permalink / raw)
To: Steffen Sledz, Steffen Sledz; +Cc: sledz, wd, linux-kernel, netdev
In-Reply-To: <4AEDCBA6.7040206@dresearch.de>
Hello Steffen,
Hm, I received a confirmation, that my fix was applied
http://www.spinics.net/lists/netdev/msg103860.html
However, it indeed didn't find their way into the mainline.
I have no idea why, received no objections.
I'm sending the reply to the resp. mailing lists too; hope you don't
mind.
Regards,
Sergei
On Sun, 01 Nov 2009 18:55:50 +0100
Steffen Sledz <sledz@dresearch.de> wrote:
> Hello Sergei,
>
> i'm working for the OpenEmbedded project and we hit the same busybox
> problem you mentioned in <http://patchwork.kernel.org/patch/31164/>.
>
> It doesn't look like you got much feedback on you patch suggestion.
> Did it made it's way to the kernel sources? Or where there any
> objections on it?
>
> Steffen
^ permalink raw reply
* [PATCH net-next-2.6] ipv6: no more dev_put() in datagram_send_ctl()
From: Eric Dumazet @ 2009-11-02 11:21 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
Avoids touching device refcount in datagram_send_ctl(), thanks to RCU
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/ipv6/datagram.c | 14 +++++++++-----
1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 9f70452..e6f9cdf 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -537,12 +537,17 @@ int datagram_send_ctl(struct net *net,
addr_type = __ipv6_addr_type(&src_info->ipi6_addr);
+ rcu_read_lock();
if (fl->oif) {
- dev = dev_get_by_index(net, fl->oif);
- if (!dev)
+ dev = dev_get_by_index_rcu(net, fl->oif);
+ if (!dev) {
+ rcu_read_unlock();
return -ENODEV;
- } else if (addr_type & IPV6_ADDR_LINKLOCAL)
+ }
+ } else if (addr_type & IPV6_ADDR_LINKLOCAL) {
+ rcu_read_unlock();
return -EINVAL;
+ }
if (addr_type != IPV6_ADDR_ANY) {
int strict = __ipv6_addr_src_scope(addr_type) <= IPV6_ADDR_SCOPE_LINKLOCAL;
@@ -553,8 +558,7 @@ int datagram_send_ctl(struct net *net,
ipv6_addr_copy(&fl->fl6_src, &src_info->ipi6_addr);
}
- if (dev)
- dev_put(dev);
+ rcu_read_unlock();
if (err)
goto exit_f;
^ permalink raw reply related
* Re: Patch: Fix userland build failure in linux/if_tunnel.h
From: David Miller @ 2009-11-02 11:30 UTC (permalink / raw)
To: sposelenov; +Cc: sledz, sledz, wd, linux-kernel, netdev
In-Reply-To: <20091102135750.436a7b54@emcraft.com>
From: Sergei Poselenov <sposelenov@emcraft.com>
Date: Mon, 2 Nov 2009 13:57:50 +0300
> Hm, I received a confirmation, that my fix was applied
> http://www.spinics.net/lists/netdev/msg103860.html
>
> However, it indeed didn't find their way into the mainline.
> I have no idea why, received no objections.
That posting doesn't say that your patch got applied, it says
that Joe Perches's MAINTAINERS file patch got applied.
Look at the context of what I'm replying to, rather than the
fact that you see "Applied." and it happens to be in a thread
you started with another patch.
Your patch is not necessary, current iproute2 sources compile
just fine without your change. iproute2 got changed to include
a local copy of linux/ip_tunnel.h which is modified so that it
compiles properly in userspace.
^ permalink raw reply
* Re: [RFC] multiqueue changes
From: David Miller @ 2009-11-02 11:35 UTC (permalink / raw)
To: jarkao2; +Cc: mchan, kaber, eric.dumazet, netdev
In-Reply-To: <20091101132017.GA2598@ami.dom.local>
From: Jarek Poplawski <jarkao2@gmail.com>
Date: Sun, 1 Nov 2009 14:20:17 +0100
> There is a question if we can predict in ->probe() MSI-X should be
> successfully enabled in ->ndo_open() for probed hardware. If so,
> then it could go e.g. like this:
We never can know this.
Another device driver can eat up all the MSI-X vectors in the PCI
domain before we make the request_irq() calls in ->open().
^ permalink raw reply
* Re: [PATCH net-next-2.6] packet: less dev_put() calls
From: David Miller @ 2009-11-02 11:42 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <4AEEA9C4.6040305@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Nov 2009 10:43:32 +0100
> - packet_sendmsg_spkt() can use dev_get_by_name_rcu() to avoid touching device refcount.
>
> - packet_getname_spkt() & packet_getname() can use dev_get_by_index_rcu() to
> avoid touching device refcount too.
>
> tpacket_snd() & packet_snd() can not use RCU yet because they can sleep when
> allocating skb.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
You're unstoppable :-)
Applied.
^ permalink raw reply
* Re: [PATCH net-next-2.6] ip6tnl: less dev_put() calls
From: David Miller @ 2009-11-02 11:43 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <4AEEB2B1.9070302@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Nov 2009 11:21:37 +0100
> Using dev_get_by_index_rcu() in ip6_tnl_rcv_ctl() & ip6_tnl_xmit_ctl()
> avoids touching device refcount.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next-2.6] ipv6: no more dev_put() in inet6_bind()
From: David Miller @ 2009-11-02 11:43 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <4AEEBE2F.4020005@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Nov 2009 12:10:39 +0100
> Avoids touching device refcount in inet6_bind(), thanks to RCU
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next-2.6] ipv6: no more dev_put() in datagram_send_ctl()
From: David Miller @ 2009-11-02 11:43 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <4AEEC0A2.1070502@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Nov 2009 12:21:06 +0100
> Avoids touching device refcount in datagram_send_ctl(), thanks to RCU
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply
* Re: [net-next-2.6 PATCH v4 3/3] TCPCT part 1c: initial SYN exchange with SYNACK data
From: William Allen Simpson @ 2009-11-02 12:25 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Linux Kernel Network Developers
In-Reply-To: <4AEDDF33.9030205@gmail.com>
Eric Dumazet wrote:
> This part is really hard to review, and might be splitted ?
>
> cleanups could be done in a cleanup patch only
>
> Examples:
>
> - tmp_opt.mss_clamp = 536;
> - tmp_opt.user_mss = tcp_sk(sk)->rx_opt.user_mss;
> + tmp_opt.mss_clamp = TCP_MIN_RCVMSS;
> + tmp_opt.user_mss = tp->rx_opt.user_mss;
>
>
> - tp->mss_cache = 536;
> + tp->mss_cache = TCP_MIN_RCVMSS;
>
Often hard to decide what's "cleanup" and what's essential. I'll look at
that again for the next round, but I've already split the original single
patch into multiple parts.
> Also your tests are reversed, if you look at the existing coding style.
>
I checked Documentation/CodingStyle, and that's not specified. I've seen
plenty of examples of modern security coding style around here.
As a long-time (25+ years) consultant and 30 years C programmer, I'm
heedful of the project coding style, and had to endure many variants.
Where I'm working with others' code, you'll note that I keep the same
style, no matter how ugly, as that makes patches easier to read.
> Example :
>
> + /* TCP Cookie Transactions */
> + if (0 < sysctl_tcp_cookie_size) {
> + /* Default, cookies without s_data. */
> + tp->cookie_values =
> + kzalloc(sizeof(*tp->cookie_values),
> + sk->sk_allocation);
> + if (NULL != tp->cookie_values)
> + kref_init(&tp->cookie_values->kref);
> + }
>
> should be ->
>
> + /* TCP Cookie Transactions */
> + if (sysctl_tcp_cookie_size > 0) {
> + /* Default, cookies without s_data. */
> + tp->cookie_values =
> + kzalloc(sizeof(*tp->cookie_values),
> + sk->sk_allocation);
> + if (tp->cookie_values != NULL)
> + kref_init(&tp->cookie_values->kref);
> + }
>
And "tp->cookie_values != NULL" is egregiously poor C practice. It's very
hard for code review to ensure that didn't get truncated to "= NULL". The
important visual element is the NULL, not the variable name.
Also, avoid "!tp->cookie_values", as this is *not* a boolean.
When I'm adding new code, I use constant-to-the-left security coding style,
as they teach in modern universities (lately also for PHP). And this is a
security extension, so a security style is particularly appropriate.
As in switch statements, constant-to-the-left makes the value obvious,
especially in a series (and assists transforming if series into a switch).
For complex tests, this makes the code much more readable and easier to
visually verify on code walk-through:
+ if (0 < tmp_opt.cookie_plus
+ && tmp_opt.saw_tstamp
+ && !tp->cookie_out_never
+ && (0 < sysctl_tcp_cookie_size
+ || (NULL != tp->cookie_values
+ && 0 < tp->cookie_values->cookie_desired))) {
Consistent use of security style would have obviated a lot of foolish >= 0
tests that seem to be constantly in need of fixing. It's a bad idea to
depend on the compiler to catch non-executable code.
^ permalink raw reply
* Re: [net-next PATCH 0/4] qlge: Add ethtool self-test.
From: David Miller @ 2009-11-02 12:26 UTC (permalink / raw)
To: ron.mercer; +Cc: netdev
In-Reply-To: <1256940816-27540-1-git-send-email-ron.mercer@qlogic.com>
From: Ron Mercer <ron.mercer@qlogic.com>
Date: Fri, 30 Oct 2009 15:13:32 -0700
> Changes for QLGE.
>
> 1) Add ethtool selftest.
> 2) Reduce debug print output.
> 3) Generic cleanup.
I've applied patches #1 and #2
Patch #3 needs to be looked into based upon the feedback we
gave you.
Patch #4 needs to be respun based upon whatever happens to
patch #3.
Thanks Ron.
^ permalink raw reply
* Re: [RFC] multiqueue changes
From: Jarek Poplawski @ 2009-11-02 12:30 UTC (permalink / raw)
To: David Miller; +Cc: mchan, kaber, eric.dumazet, netdev
In-Reply-To: <20091102.033533.08766686.davem@davemloft.net>
On Mon, Nov 02, 2009 at 03:35:33AM -0800, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Sun, 1 Nov 2009 14:20:17 +0100
>
> > There is a question if we can predict in ->probe() MSI-X should be
> > successfully enabled in ->ndo_open() for probed hardware. If so,
> > then it could go e.g. like this:
>
> We never can know this.
>
> Another device driver can eat up all the MSI-X vectors in the PCI
> domain before we make the request_irq() calls in ->open().
Right, but it's not a 50% chance, I guess? A user most of the time
gets consistently multiqueue or non-multiqueue behavior after open,
unless I miss something. Then such an exceptional state could be
handled by real_num_tx_queues (just like in case of powered of cpus).
The main difference is to hold in num_tx_queues something that is
really available vs max possible value for all configs.
Jarek P.
^ permalink raw reply
* Re: [net-next-2.6 PATCH RFC] TCPCT part 1d: generate Responder Cookie
From: William Allen Simpson @ 2009-11-02 12:36 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Linux Kernel Developers, Linux Kernel Network Developers
In-Reply-To: <4AEEBAC6.7020308@gmail.com>
Eric Dumazet wrote:
> cookie_hash() runs in a non preemptable context. CPU cannot change under us.
>
> (or else, we would not use __get_cpu_var(ipv4_cookie_scratch); )
>
> And of course, each cpu gets its own scratch area, thanks to __get_cpu_var()
>
Interesting. I'm not sure that running CPU intensive functions like SHA1 in
a non-preemptable context is a good idea. I'd assumed it wasn't!
Perhaps you could point at the documentation in the code that explains this?
Perhaps a function header comment that mentions it?
All I know is (from testing) that the tcp_minisockets.c caller is sometimes
called in a fashion that requires atomic allocation, and other times does not!
See my "Subject: query: tcpdump versus atomic?" thread from Oct 14th.
^ permalink raw reply
* Re: [RFC] multiqueue changes
From: David Miller @ 2009-11-02 12:39 UTC (permalink / raw)
To: jarkao2; +Cc: mchan, kaber, eric.dumazet, netdev
In-Reply-To: <20091102123029.GA7790@ff.dom.local>
From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 2 Nov 2009 12:30:29 +0000
> Right, but it's not a 50% chance, I guess? A user most of the time
> gets consistently multiqueue or non-multiqueue behavior after open,
> unless I miss something. Then such an exceptional state could be
> handled by real_num_tx_queues (just like in case of powered of cpus).
> The main difference is to hold in num_tx_queues something that is
> really available vs max possible value for all configs.
I see your point, yes this would seem to be a reasonable way
to start handling num_tx_queues and real_num_tx_queues.
^ permalink raw reply
* Re: [net-next-2.6 PATCH v4 3/3] TCPCT part 1c: initial SYN exchange with SYNACK data
From: Ilpo Järvinen @ 2009-11-02 12:57 UTC (permalink / raw)
To: William Allen Simpson; +Cc: Eric Dumazet, Linux Kernel Network Developers
In-Reply-To: <4AEECFA8.1080306@gmail.com>
On Mon, 2 Nov 2009, William Allen Simpson wrote:
> Eric Dumazet wrote:
> > This part is really hard to review, and might be splitted ?
> >
> > cleanups could be done in a cleanup patch only
> >
> > Examples:
> >
> > - tmp_opt.mss_clamp = 536;
> > - tmp_opt.user_mss = tcp_sk(sk)->rx_opt.user_mss;
> > + tmp_opt.mss_clamp = TCP_MIN_RCVMSS;
> > + tmp_opt.user_mss = tp->rx_opt.user_mss;
> >
> >
> > - tp->mss_cache = 536;
> > + tp->mss_cache = TCP_MIN_RCVMSS;
> >
> Often hard to decide what's "cleanup" and what's essential. I'll look at
> that again for the next round, but I've already split the original single
> patch into multiple parts.
Are you talking about particular case?!? ...You can safely split into even
more parts if there are cleanups which is essential. ...We'll not stop you
from doing that nor be angry if do that.
> > Also your tests are reversed, if you look at the existing coding style.
> >
> I checked Documentation/CodingStyle, and that's not specified. I've seen
> plenty of examples of modern security coding style around here.
>
> As a long-time (25+ years) consultant and 30 years C programmer, I'm
> heedful of the project coding style, and had to endure many variants.
>
> Where I'm working with others' code, you'll note that I keep the same
> style, no matter how ugly, as that makes patches easier to read.
>
>
> > Example :
> >
> > + /* TCP Cookie Transactions */
> > + if (0 < sysctl_tcp_cookie_size) {
> > + /* Default, cookies without s_data. */
> > + tp->cookie_values =
> > + kzalloc(sizeof(*tp->cookie_values),
> > + sk->sk_allocation);
> > + if (NULL != tp->cookie_values)
> > + kref_init(&tp->cookie_values->kref);
> > + }
> >
> > should be ->
> >
> > + /* TCP Cookie Transactions */
> > + if (sysctl_tcp_cookie_size > 0) {
> > + /* Default, cookies without s_data. */
> > + tp->cookie_values =
> > + kzalloc(sizeof(*tp->cookie_values),
> > + sk->sk_allocation);
> > + if (tp->cookie_values != NULL)
> > + kref_init(&tp->cookie_values->kref);
> > + }
> >
> And "tp->cookie_values != NULL" is egregiously poor C practice. It's very
> hard for code review to ensure that didn't get truncated to "= NULL". The
> important visual element is the NULL, not the variable name.
>
> Also, avoid "!tp->cookie_values", as this is *not* a boolean.
>
> When I'm adding new code, I use constant-to-the-left security coding style,
> as they teach in modern universities (lately also for PHP). And this is a
> security extension, so a security style is particularly appropriate.
>
> As in switch statements, constant-to-the-left makes the value obvious,
> especially in a series (and assists transforming if series into a switch).
>
> For complex tests, this makes the code much more readable and easier to
> visually verify on code walk-through:
>
> + if (0 < tmp_opt.cookie_plus
> + && tmp_opt.saw_tstamp
> + && !tp->cookie_out_never
> + && (0 < sysctl_tcp_cookie_size
> + || (NULL != tp->cookie_values
> + && 0 < tp->cookie_values->cookie_desired))) {
>
> Consistent use of security style would have obviated a lot of foolish >= 0
> tests that seem to be constantly in need of fixing. It's a bad idea to
> depend on the compiler to catch non-executable code.
That kind of response certainly won't help you any. ...First, you said you
adapt the current style but for some reason immediately start to say why
you would careless about that principle. ...Also, telling that you have
lots of experience here and there will not get you there either ;-).
--
i.
^ permalink raw reply
* Re: [RFC] multiqueue changes
From: Jarek Poplawski @ 2009-11-02 13:02 UTC (permalink / raw)
To: David Miller; +Cc: mchan, kaber, eric.dumazet, netdev
In-Reply-To: <20091102.043907.236634594.davem@davemloft.net>
On Mon, Nov 02, 2009 at 04:39:07AM -0800, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Mon, 2 Nov 2009 12:30:29 +0000
>
> > Right, but it's not a 50% chance, I guess? A user most of the time
> > gets consistently multiqueue or non-multiqueue behavior after open,
> > unless I miss something. Then such an exceptional state could be
> > handled by real_num_tx_queues (just like in case of powered of cpus).
> > The main difference is to hold in num_tx_queues something that is
> > really available vs max possible value for all configs.
>
> I see your point, yes this would seem to be a reasonable way
> to start handling num_tx_queues and real_num_tx_queues.
Very nice! So, I hope Eric should be satisfied with these requested
comments already :-)
Thanks everybody,
Jarek P.
^ permalink raw reply
* Re: [RFC] multiqueue changes
From: Eric Dumazet @ 2009-11-02 13:03 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: David Miller, mchan, kaber, netdev
In-Reply-To: <20091102130223.GB7790@ff.dom.local>
Jarek Poplawski a écrit :
> On Mon, Nov 02, 2009 at 04:39:07AM -0800, David Miller wrote:
>> From: Jarek Poplawski <jarkao2@gmail.com>
>> Date: Mon, 2 Nov 2009 12:30:29 +0000
>>
>>> Right, but it's not a 50% chance, I guess? A user most of the time
>>> gets consistently multiqueue or non-multiqueue behavior after open,
>>> unless I miss something. Then such an exceptional state could be
>>> handled by real_num_tx_queues (just like in case of powered of cpus).
>>> The main difference is to hold in num_tx_queues something that is
>>> really available vs max possible value for all configs.
>> I see your point, yes this would seem to be a reasonable way
>> to start handling num_tx_queues and real_num_tx_queues.
>
> Very nice! So, I hope Eric should be satisfied with these requested
> comments already :-)
>
Sure, but I prefer a patch from you ;)
^ permalink raw reply
* Re: [RFC] multiqueue changes
From: Jarek Poplawski @ 2009-11-02 13:09 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, mchan, kaber, netdev
In-Reply-To: <4AEED899.9040106@gmail.com>
On Mon, Nov 02, 2009 at 02:03:21PM +0100, Eric Dumazet wrote:
...
> Sure, but I prefer a patch from you ;)
>
Wrong code! (RFP? ;-)
Jarek P.
^ permalink raw reply
* Re: [net-next-2.6 PATCH RFC] TCPCT part 1d: generate Responder Cookie
From: Eric Dumazet @ 2009-11-02 13:16 UTC (permalink / raw)
To: William Allen Simpson
Cc: Linux Kernel Developers, Linux Kernel Network Developers
In-Reply-To: <4AEED23A.7070009@gmail.com>
William Allen Simpson a écrit :
> Eric Dumazet wrote:
>> cookie_hash() runs in a non preemptable context. CPU cannot change
>> under us.
>>
>> (or else, we would not use __get_cpu_var(ipv4_cookie_scratch); )
>>
>> And of course, each cpu gets its own scratch area, thanks to
>> __get_cpu_var()
>>
> Interesting. I'm not sure that running CPU intensive functions like
> SHA1 in
> a non-preemptable context is a good idea. I'd assumed it wasn't!
>
> Perhaps you could point at the documentation in the code that explains
> this?
I suggest you read Documentations/ files about softirq
http://docs.blackfin.uclinux.org/kernel/generated/kernel-hacking.xml
Large part of network code is run by softirq handler, and a softirq handler
is not preemptable with another softirq (including itself).
> Perhaps a function header comment that mentions it?
So we are going to add a header to thousand of functions repeating this prereq ?
>
> All I know is (from testing) that the tcp_minisockets.c caller is sometimes
> called in a fashion that requires atomic allocation, and other times
> does not!
Maybe callers have different contexts (running from softirq handler or
from process context). Atomic ops are expensive and we try to avoid them
if/when possible.
>
> See my "Subject: query: tcpdump versus atomic?" thread from Oct 14th.
You probably add a bug in your kernel, leaving a function with unpaired lock/unlock
of notallow_something/allow_something
There are books about linux internals that you could read if you want some extra
documentation. Dont ask me details, I never read them :)
^ permalink raw reply
* Re: [net-next-2.6 PATCH v4 3/3] TCPCT part 1c: initial SYN exchange with SYNACK data
From: Eric Dumazet @ 2009-11-02 13:31 UTC (permalink / raw)
To: William Allen Simpson; +Cc: Linux Kernel Network Developers
In-Reply-To: <4AEECFA8.1080306@gmail.com>
William Allen Simpson a écrit :
> Eric Dumazet wrote:
>> This part is really hard to review, and might be splitted ?
>>
>> cleanups could be done in a cleanup patch only
>>
>> Examples:
>>
>> - tmp_opt.mss_clamp = 536;
>> - tmp_opt.user_mss = tcp_sk(sk)->rx_opt.user_mss;
>> + tmp_opt.mss_clamp = TCP_MIN_RCVMSS;
>> + tmp_opt.user_mss = tp->rx_opt.user_mss;
>>
>>
>> - tp->mss_cache = 536;
>> + tp->mss_cache = TCP_MIN_RCVMSS;
>>
> Often hard to decide what's "cleanup" and what's essential. I'll look at
> that again for the next round, but I've already split the original single
> patch into multiple parts.
cleanups are trivial, and should be separated from functionnal changes.
>
>
>> Also your tests are reversed, if you look at the existing coding style.
>>
> I checked Documentation/CodingStyle, and that's not specified. I've seen
> plenty of examples of modern security coding style around here.
>
> As a long-time (25+ years) consultant and 30 years C programmer, I'm
> heedful of the project coding style, and had to endure many variants.
>
> Where I'm working with others' code, you'll note that I keep the same
> style, no matter how ugly, as that makes patches easier to read.
>
>
>> Example :
>>
>> + /* TCP Cookie Transactions */
>> + if (0 < sysctl_tcp_cookie_size) {
>> + /* Default, cookies without s_data. */
>> + tp->cookie_values =
>> + kzalloc(sizeof(*tp->cookie_values),
>> + sk->sk_allocation);
>> + if (NULL != tp->cookie_values)
>> + kref_init(&tp->cookie_values->kref);
>> + }
>>
>> should be ->
>>
>> + /* TCP Cookie Transactions */
>> + if (sysctl_tcp_cookie_size > 0) {
>> + /* Default, cookies without s_data. */
>> + tp->cookie_values =
>> + kzalloc(sizeof(*tp->cookie_values),
>> + sk->sk_allocation);
>> + if (tp->cookie_values != NULL)
>> + kref_init(&tp->cookie_values->kref);
>> + }
>>
> And "tp->cookie_values != NULL" is egregiously poor C practice. It's very
> hard for code review to ensure that didn't get truncated to "= NULL". The
> important visual element is the NULL, not the variable name.
Maybe, but check in linux source code and you'll see this poor pratice is the facto.
Dont try to change our minds, because it wont happen.
>
> Also, avoid "!tp->cookie_values", as this is *not* a boolean.
Oh, good to learn that ! operator only applies to boolean. I didnt know that.
>
> When I'm adding new code, I use constant-to-the-left security coding style,
> as they teach in modern universities (lately also for PHP). And this is a
> security extension, so a security style is particularly appropriate.
>
> As in switch statements, constant-to-the-left makes the value obvious,
> especially in a series (and assists transforming if series into a switch).
>
> For complex tests, this makes the code much more readable and easier to
> visually verify on code walk-through:
>
> + if (0 < tmp_opt.cookie_plus
> + && tmp_opt.saw_tstamp
> + && !tp->cookie_out_never
> + && (0 < sysctl_tcp_cookie_size
> + || (NULL != tp->cookie_values
> + && 0 < tp->cookie_values->cookie_desired))) {
>
> Consistent use of security style would have obviated a lot of foolish >= 0
> tests that seem to be constantly in need of fixing. It's a bad idea to
> depend on the compiler to catch non-executable code.
You can _talk_, I can stop reviewing your patches, and wait another gentle guy do the job,
because I am 30 years experimented (and tired ?) programmer, and dont want to
lose my time to discuss Coding-Style with you ?
Cooking patches to linux is not only matter of good ideas and programming (and Dropping
patches for the masses).
Its also a matter of convincing _people_ that your additions will be maintainable
when you leave kernel programming and let people like us correct bugs.
For the moment, I am not convinced at all. I prefer to talk now.
Note: I did read your TCPCT 25 pages documentation and very am interested by this
improvement, but its _also_ important to implement it in the normal way.
(I wish this document could be public in a RFC form)
^ permalink raw reply
* [PATCH 1/2] tc35815: Kill non-napi code
From: Atsushi Nemoto @ 2009-11-02 14:34 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Ralf Roesch
Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
---
drivers/net/tc35815.c | 68 +------------------------------------------------
1 files changed, 1 insertions(+), 67 deletions(-)
diff --git a/drivers/net/tc35815.c b/drivers/net/tc35815.c
index 0d621ca..803eb64 100644
--- a/drivers/net/tc35815.c
+++ b/drivers/net/tc35815.c
@@ -22,12 +22,7 @@
* All Rights Reserved.
*/
-#define TC35815_NAPI
-#ifdef TC35815_NAPI
-#define DRV_VERSION "1.38-NAPI"
-#else
-#define DRV_VERSION "1.38"
-#endif
+#define DRV_VERSION "1.39"
static const char *version = "tc35815.c:v" DRV_VERSION "\n";
#define MODNAME "tc35815"
@@ -563,12 +558,8 @@ static void free_rxbuf_skb(struct pci_dev *hwdev, struct sk_buff *skb, dma_addr_
static int tc35815_open(struct net_device *dev);
static int tc35815_send_packet(struct sk_buff *skb, struct net_device *dev);
static irqreturn_t tc35815_interrupt(int irq, void *dev_id);
-#ifdef TC35815_NAPI
static int tc35815_rx(struct net_device *dev, int limit);
static int tc35815_poll(struct napi_struct *napi, int budget);
-#else
-static void tc35815_rx(struct net_device *dev);
-#endif
static void tc35815_txdone(struct net_device *dev);
static int tc35815_close(struct net_device *dev);
static struct net_device_stats *tc35815_get_stats(struct net_device *dev);
@@ -924,9 +915,7 @@ static int __devinit tc35815_init_one(struct pci_dev *pdev,
dev->netdev_ops = &tc35815_netdev_ops;
dev->ethtool_ops = &tc35815_ethtool_ops;
dev->watchdog_timeo = TC35815_TX_TIMEOUT;
-#ifdef TC35815_NAPI
netif_napi_add(dev, &lp->napi, tc35815_poll, NAPI_WEIGHT);
-#endif
dev->irq = pdev->irq;
dev->base_addr = (unsigned long)ioaddr;
@@ -1401,9 +1390,7 @@ tc35815_open(struct net_device *dev)
return -EAGAIN;
}
-#ifdef TC35815_NAPI
napi_enable(&lp->napi);
-#endif
/* Reset the hardware here. Don't forget to set the station address. */
spin_lock_irq(&lp->lock);
@@ -1537,11 +1524,7 @@ static void tc35815_fatal_error_interrupt(struct net_device *dev, u32 status)
tc35815_schedule_restart(dev);
}
-#ifdef TC35815_NAPI
static int tc35815_do_interrupt(struct net_device *dev, u32 status, int limit)
-#else
-static int tc35815_do_interrupt(struct net_device *dev, u32 status)
-#endif
{
struct tc35815_local *lp = netdev_priv(dev);
int ret = -1;
@@ -1580,12 +1563,7 @@ static int tc35815_do_interrupt(struct net_device *dev, u32 status)
/* normal notification */
if (status & Int_IntMacRx) {
/* Got a packet(s). */
-#ifdef TC35815_NAPI
ret = tc35815_rx(dev, limit);
-#else
- tc35815_rx(dev);
- ret = 0;
-#endif
lp->lstats.rx_ints++;
}
if (status & Int_IntMacTx) {
@@ -1593,12 +1571,8 @@ static int tc35815_do_interrupt(struct net_device *dev, u32 status)
lp->lstats.tx_ints++;
tc35815_txdone(dev);
netif_wake_queue(dev);
-#ifdef TC35815_NAPI
if (ret < 0)
ret = 0;
-#else
- ret = 0;
-#endif
}
return ret;
}
@@ -1613,7 +1587,6 @@ static irqreturn_t tc35815_interrupt(int irq, void *dev_id)
struct tc35815_local *lp = netdev_priv(dev);
struct tc35815_regs __iomem *tr =
(struct tc35815_regs __iomem *)dev->base_addr;
-#ifdef TC35815_NAPI
u32 dmactl = tc_readl(&tr->DMA_Ctl);
if (!(dmactl & DMA_IntMask)) {
@@ -1630,22 +1603,6 @@ static irqreturn_t tc35815_interrupt(int irq, void *dev_id)
return IRQ_HANDLED;
}
return IRQ_NONE;
-#else
- int handled;
- u32 status;
-
- spin_lock(&lp->lock);
- status = tc_readl(&tr->Int_Src);
- /* BLEx, FDAEx will be cleared later */
- tc_writel(status & ~(Int_BLEx | Int_FDAEx),
- &tr->Int_Src); /* write to clear */
- handled = tc35815_do_interrupt(dev, status);
- if (status & (Int_BLEx | Int_FDAEx))
- tc_writel(status & (Int_BLEx | Int_FDAEx), &tr->Int_Src);
- (void)tc_readl(&tr->Int_Src); /* flush */
- spin_unlock(&lp->lock);
- return IRQ_RETVAL(handled >= 0);
-#endif /* TC35815_NAPI */
}
#ifdef CONFIG_NET_POLL_CONTROLLER
@@ -1658,20 +1615,13 @@ static void tc35815_poll_controller(struct net_device *dev)
#endif
/* We have a good packet(s), get it/them out of the buffers. */
-#ifdef TC35815_NAPI
static int
tc35815_rx(struct net_device *dev, int limit)
-#else
-static void
-tc35815_rx(struct net_device *dev)
-#endif
{
struct tc35815_local *lp = netdev_priv(dev);
unsigned int fdctl;
int i;
-#ifdef TC35815_NAPI
int received = 0;
-#endif
while (!((fdctl = le32_to_cpu(lp->rfd_cur->fd.FDCtl)) & FD_CownsFD)) {
int status = le32_to_cpu(lp->rfd_cur->fd.FDStat);
@@ -1694,10 +1644,8 @@ tc35815_rx(struct net_device *dev)
int offset;
#endif
-#ifdef TC35815_NAPI
if (--limit < 0)
break;
-#endif
#ifdef TC35815_USE_PACKEDBUFFER
BUG_ON(bd_count > 2);
skb = dev_alloc_skb(pkt_len + NET_IP_ALIGN);
@@ -1767,12 +1715,8 @@ tc35815_rx(struct net_device *dev)
if (netif_msg_pktdata(lp))
print_eth(data);
skb->protocol = eth_type_trans(skb, dev);
-#ifdef TC35815_NAPI
netif_receive_skb(skb);
received++;
-#else
- netif_rx(skb);
-#endif
dev->stats.rx_packets++;
dev->stats.rx_bytes += pkt_len;
} else {
@@ -1888,12 +1832,9 @@ tc35815_rx(struct net_device *dev)
#endif
}
-#ifdef TC35815_NAPI
return received;
-#endif
}
-#ifdef TC35815_NAPI
static int tc35815_poll(struct napi_struct *napi, int budget)
{
struct tc35815_local *lp = container_of(napi, struct tc35815_local, napi);
@@ -1930,7 +1871,6 @@ static int tc35815_poll(struct napi_struct *napi, int budget)
}
return received;
}
-#endif
#ifdef NO_CHECK_CARRIER
#define TX_STA_ERR (Tx_ExColl|Tx_Under|Tx_Defer|Tx_LateColl|Tx_TxPar|Tx_SQErr)
@@ -2050,11 +1990,7 @@ tc35815_txdone(struct net_device *dev)
pci_unmap_single(lp->pci_dev, lp->tx_skbs[lp->tfd_end].skb_dma, skb->len, PCI_DMA_TODEVICE);
lp->tx_skbs[lp->tfd_end].skb = NULL;
lp->tx_skbs[lp->tfd_end].skb_dma = 0;
-#ifdef TC35815_NAPI
dev_kfree_skb_any(skb);
-#else
- dev_kfree_skb_irq(skb);
-#endif
}
txfd->fd.FDSystem = cpu_to_le32(0xffffffff);
@@ -2118,9 +2054,7 @@ tc35815_close(struct net_device *dev)
struct tc35815_local *lp = netdev_priv(dev);
netif_stop_queue(dev);
-#ifdef TC35815_NAPI
napi_disable(&lp->napi);
-#endif
if (lp->phy_dev)
phy_stop(lp->phy_dev);
cancel_work_sync(&lp->restart_work);
--
1.5.6.5
^ permalink raw reply related
* [PATCH 2/2] tc35815: Kill unused code
From: Atsushi Nemoto @ 2009-11-02 14:34 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Ralf Roesch
- TC35815_DMA_SYNC_ONDEMAND is always enabled.
- WORKAROUND_LOSTCAR is always enabled.
- WORKAROUND_100HALF_PROMISC is always enabled.
- GATHER_TXINT is always enabled.
- TC35815_USE_PACKEDBUFFER is always disabled.
- NO_CHECK_CARRIER is always disabled.
---
drivers/net/tc35815.c | 224 +------------------------------------------------
1 files changed, 3 insertions(+), 221 deletions(-)
diff --git a/drivers/net/tc35815.c b/drivers/net/tc35815.c
index 803eb64..6572e8a 100644
--- a/drivers/net/tc35815.c
+++ b/drivers/net/tc35815.c
@@ -50,13 +50,6 @@ static const char *version = "tc35815.c:v" DRV_VERSION "\n";
#include <asm/io.h>
#include <asm/byteorder.h>
-/* First, a few definitions that the brave might change. */
-
-#define GATHER_TXINT /* On-Demand Tx Interrupt */
-#define WORKAROUND_LOSTCAR
-#define WORKAROUND_100HALF_PROMISC
-/* #define TC35815_USE_PACKEDBUFFER */
-
enum tc35815_chiptype {
TC35815CF = 0,
TC35815_NWU,
@@ -326,17 +319,10 @@ struct BDesc {
/* Some useful constants. */
-#undef NO_CHECK_CARRIER /* Does not check No-Carrier with TP */
-#ifdef NO_CHECK_CARRIER
-#define TX_CTL_CMD (Tx_EnComp | Tx_EnTxPar | Tx_EnLateColl | \
- Tx_EnExColl | Tx_EnExDefer | Tx_EnUnder | \
- Tx_En) /* maybe 0x7b01 */
-#else
-#define TX_CTL_CMD (Tx_EnComp | Tx_EnTxPar | Tx_EnLateColl | \
+#define TX_CTL_CMD (Tx_EnTxPar | Tx_EnLateColl | \
Tx_EnExColl | Tx_EnLCarr | Tx_EnExDefer | Tx_EnUnder | \
Tx_En) /* maybe 0x7b01 */
-#endif
/* Do not use Rx_StripCRC -- it causes trouble on BLEx/FDAEx condition */
#define RX_CTL_CMD (Rx_EnGood | Rx_EnRxPar | Rx_EnLongErr | Rx_EnOver \
| Rx_EnCRCErr | Rx_EnAlign | Rx_RxEn) /* maybe 0x6f01 */
@@ -357,13 +343,6 @@ struct BDesc {
#define TX_THRESHOLD_KEEP_LIMIT 10
/* 16 + RX_BUF_NUM * 8 + RX_FD_NUM * 16 + TX_FD_NUM * 32 <= PAGE_SIZE*FD_PAGE_NUM */
-#ifdef TC35815_USE_PACKEDBUFFER
-#define FD_PAGE_NUM 2
-#define RX_BUF_NUM 8 /* >= 2 */
-#define RX_FD_NUM 250 /* >= 32 */
-#define TX_FD_NUM 128
-#define RX_BUF_SIZE PAGE_SIZE
-#else /* TC35815_USE_PACKEDBUFFER */
#define FD_PAGE_NUM 4
#define RX_BUF_NUM 128 /* < 256 */
#define RX_FD_NUM 256 /* >= 32 */
@@ -377,7 +356,6 @@ struct BDesc {
#define RX_BUF_SIZE \
L1_CACHE_ALIGN(ETH_FRAME_LEN + VLAN_HLEN + ETH_FCS_LEN + NET_IP_ALIGN)
#endif
-#endif /* TC35815_USE_PACKEDBUFFER */
#define RX_FD_RESERVE (2 / 2) /* max 2 BD per RxFD */
#define NAPI_WEIGHT 16
@@ -435,11 +413,7 @@ struct tc35815_local {
/*
* Transmitting: Batch Mode.
* 1 BD in 1 TxFD.
- * Receiving: Packing Mode. (TC35815_USE_PACKEDBUFFER)
- * 1 circular FD for Free Buffer List.
- * RX_BUF_NUM BD in Free Buffer FD.
- * One Free Buffer BD has PAGE_SIZE data buffer.
- * Or Non-Packing Mode.
+ * Receiving: Non-Packing Mode.
* 1 circular FD for Free Buffer List.
* RX_BUF_NUM BD in Free Buffer FD.
* One Free Buffer BD has ETH_FRAME_LEN data buffer.
@@ -453,21 +427,11 @@ struct tc35815_local {
struct RxFD *rfd_limit;
struct RxFD *rfd_cur;
struct FrFD *fbl_ptr;
-#ifdef TC35815_USE_PACKEDBUFFER
- unsigned char fbl_curid;
- void *data_buf[RX_BUF_NUM]; /* packing */
- dma_addr_t data_buf_dma[RX_BUF_NUM];
- struct {
- struct sk_buff *skb;
- dma_addr_t skb_dma;
- } tx_skbs[TX_FD_NUM];
-#else
unsigned int fbl_count;
struct {
struct sk_buff *skb;
dma_addr_t skb_dma;
} tx_skbs[TX_FD_NUM], rx_skbs[RX_BUF_NUM];
-#endif
u32 msg_enable;
enum tc35815_chiptype chiptype;
};
@@ -482,51 +446,6 @@ static inline void *fd_bus_to_virt(struct tc35815_local *lp, dma_addr_t bus)
return (void *)((u8 *)lp->fd_buf + (bus - lp->fd_buf_dma));
}
#endif
-#ifdef TC35815_USE_PACKEDBUFFER
-static inline void *rxbuf_bus_to_virt(struct tc35815_local *lp, dma_addr_t bus)
-{
- int i;
- for (i = 0; i < RX_BUF_NUM; i++) {
- if (bus >= lp->data_buf_dma[i] &&
- bus < lp->data_buf_dma[i] + PAGE_SIZE)
- return (void *)((u8 *)lp->data_buf[i] +
- (bus - lp->data_buf_dma[i]));
- }
- return NULL;
-}
-
-#define TC35815_DMA_SYNC_ONDEMAND
-static void *alloc_rxbuf_page(struct pci_dev *hwdev, dma_addr_t *dma_handle)
-{
-#ifdef TC35815_DMA_SYNC_ONDEMAND
- void *buf;
- /* pci_map + pci_dma_sync will be more effective than
- * pci_alloc_consistent on some archs. */
- buf = (void *)__get_free_page(GFP_ATOMIC);
- if (!buf)
- return NULL;
- *dma_handle = pci_map_single(hwdev, buf, PAGE_SIZE,
- PCI_DMA_FROMDEVICE);
- if (pci_dma_mapping_error(hwdev, *dma_handle)) {
- free_page((unsigned long)buf);
- return NULL;
- }
- return buf;
-#else
- return pci_alloc_consistent(hwdev, PAGE_SIZE, dma_handle);
-#endif
-}
-
-static void free_rxbuf_page(struct pci_dev *hwdev, void *buf, dma_addr_t dma_handle)
-{
-#ifdef TC35815_DMA_SYNC_ONDEMAND
- pci_unmap_single(hwdev, dma_handle, PAGE_SIZE, PCI_DMA_FROMDEVICE);
- free_page((unsigned long)buf);
-#else
- pci_free_consistent(hwdev, PAGE_SIZE, buf, dma_handle);
-#endif
-}
-#else /* TC35815_USE_PACKEDBUFFER */
static struct sk_buff *alloc_rxbuf_skb(struct net_device *dev,
struct pci_dev *hwdev,
dma_addr_t *dma_handle)
@@ -551,7 +470,6 @@ static void free_rxbuf_skb(struct pci_dev *hwdev, struct sk_buff *skb, dma_addr_
PCI_DMA_FROMDEVICE);
dev_kfree_skb_any(skb);
}
-#endif /* TC35815_USE_PACKEDBUFFER */
/* Index to functions, as function prototypes. */
@@ -646,8 +564,6 @@ static void tc_handle_link_change(struct net_device *dev)
* TX4939 PCFG.SPEEDn bit will be changed on
* NETDEV_CHANGE event.
*/
-
-#if !defined(NO_CHECK_CARRIER) && defined(WORKAROUND_LOSTCAR)
/*
* WORKAROUND: enable LostCrS only if half duplex
* operation.
@@ -657,7 +573,6 @@ static void tc_handle_link_change(struct net_device *dev)
lp->chiptype != TC35815_TX4939)
tc_writel(tc_readl(&tr->Tx_Ctl) | Tx_EnLCarr,
&tr->Tx_Ctl);
-#endif
lp->speed = phydev->speed;
lp->duplex = phydev->duplex;
@@ -666,11 +581,9 @@ static void tc_handle_link_change(struct net_device *dev)
if (phydev->link != lp->link) {
if (phydev->link) {
-#ifdef WORKAROUND_100HALF_PROMISC
/* delayed promiscuous enabling */
if (dev->flags & IFF_PROMISC)
tc35815_set_multicast_list(dev);
-#endif
} else {
lp->speed = 0;
lp->duplex = -1;
@@ -997,25 +910,6 @@ tc35815_init_queues(struct net_device *dev)
if (!lp->fd_buf)
return -ENOMEM;
for (i = 0; i < RX_BUF_NUM; i++) {
-#ifdef TC35815_USE_PACKEDBUFFER
- lp->data_buf[i] =
- alloc_rxbuf_page(lp->pci_dev,
- &lp->data_buf_dma[i]);
- if (!lp->data_buf[i]) {
- while (--i >= 0) {
- free_rxbuf_page(lp->pci_dev,
- lp->data_buf[i],
- lp->data_buf_dma[i]);
- lp->data_buf[i] = NULL;
- }
- pci_free_consistent(lp->pci_dev,
- PAGE_SIZE * FD_PAGE_NUM,
- lp->fd_buf,
- lp->fd_buf_dma);
- lp->fd_buf = NULL;
- return -ENOMEM;
- }
-#else
lp->rx_skbs[i].skb =
alloc_rxbuf_skb(dev, lp->pci_dev,
&lp->rx_skbs[i].skb_dma);
@@ -1033,15 +927,9 @@ tc35815_init_queues(struct net_device *dev)
lp->fd_buf = NULL;
return -ENOMEM;
}
-#endif
}
printk(KERN_DEBUG "%s: FD buf %p DataBuf",
dev->name, lp->fd_buf);
-#ifdef TC35815_USE_PACKEDBUFFER
- printk(" DataBuf");
- for (i = 0; i < RX_BUF_NUM; i++)
- printk(" %p", lp->data_buf[i]);
-#endif
printk("\n");
} else {
for (i = 0; i < FD_PAGE_NUM; i++)
@@ -1074,7 +962,6 @@ tc35815_init_queues(struct net_device *dev)
lp->fbl_ptr = (struct FrFD *)fd_addr;
lp->fbl_ptr->fd.FDNext = cpu_to_le32(fd_virt_to_bus(lp, lp->fbl_ptr));
lp->fbl_ptr->fd.FDCtl = cpu_to_le32(RX_BUF_NUM | FD_CownsFD);
-#ifndef TC35815_USE_PACKEDBUFFER
/*
* move all allocated skbs to head of rx_skbs[] array.
* fbl_count mighe not be RX_BUF_NUM if alloc_rxbuf_skb() in
@@ -1092,11 +979,7 @@ tc35815_init_queues(struct net_device *dev)
lp->fbl_count++;
}
}
-#endif
for (i = 0; i < RX_BUF_NUM; i++) {
-#ifdef TC35815_USE_PACKEDBUFFER
- lp->fbl_ptr->bd[i].BuffData = cpu_to_le32(lp->data_buf_dma[i]);
-#else
if (i >= lp->fbl_count) {
lp->fbl_ptr->bd[i].BuffData = 0;
lp->fbl_ptr->bd[i].BDCtl = 0;
@@ -1104,15 +987,11 @@ tc35815_init_queues(struct net_device *dev)
}
lp->fbl_ptr->bd[i].BuffData =
cpu_to_le32(lp->rx_skbs[i].skb_dma);
-#endif
/* BDID is index of FrFD.bd[] */
lp->fbl_ptr->bd[i].BDCtl =
cpu_to_le32(BD_CownsBD | (i << BD_RxBDID_SHIFT) |
RX_BUF_SIZE);
}
-#ifdef TC35815_USE_PACKEDBUFFER
- lp->fbl_curid = 0;
-#endif
printk(KERN_DEBUG "%s: TxFD %p RxFD %p FrFD %p\n",
dev->name, lp->tfd_base, lp->rfd_base, lp->fbl_ptr);
@@ -1186,19 +1065,11 @@ tc35815_free_queues(struct net_device *dev)
lp->fbl_ptr = NULL;
for (i = 0; i < RX_BUF_NUM; i++) {
-#ifdef TC35815_USE_PACKEDBUFFER
- if (lp->data_buf[i]) {
- free_rxbuf_page(lp->pci_dev,
- lp->data_buf[i], lp->data_buf_dma[i]);
- lp->data_buf[i] = NULL;
- }
-#else
if (lp->rx_skbs[i].skb) {
free_rxbuf_skb(lp->pci_dev, lp->rx_skbs[i].skb,
lp->rx_skbs[i].skb_dma);
lp->rx_skbs[i].skb = NULL;
}
-#endif
}
if (lp->fd_buf) {
pci_free_consistent(lp->pci_dev, PAGE_SIZE * FD_PAGE_NUM,
@@ -1244,7 +1115,7 @@ dump_rxfd(struct RxFD *fd)
return bd_count;
}
-#if defined(DEBUG) || defined(TC35815_USE_PACKEDBUFFER)
+#ifdef DEBUG
static void
dump_frfd(struct FrFD *fd)
{
@@ -1261,9 +1132,7 @@ dump_frfd(struct FrFD *fd)
le32_to_cpu(fd->bd[i].BDCtl));
printk("\n");
}
-#endif
-#ifdef DEBUG
static void
panic_queues(struct net_device *dev)
{
@@ -1466,9 +1335,7 @@ static int tc35815_send_packet(struct sk_buff *skb, struct net_device *dev)
(struct tc35815_regs __iomem *)dev->base_addr;
/* Start DMA Transmitter. */
txfd->fd.FDNext |= cpu_to_le32(FD_Next_EOL);
-#ifdef GATHER_TXINT
txfd->fd.FDCtl |= cpu_to_le32(FD_FrmOpt_IntTx);
-#endif
if (netif_msg_tx_queued(lp)) {
printk("%s: starting TxFD.\n", dev->name);
dump_txfd(txfd);
@@ -1640,50 +1507,9 @@ tc35815_rx(struct net_device *dev, int limit)
struct sk_buff *skb;
unsigned char *data;
int cur_bd;
-#ifdef TC35815_USE_PACKEDBUFFER
- int offset;
-#endif
if (--limit < 0)
break;
-#ifdef TC35815_USE_PACKEDBUFFER
- BUG_ON(bd_count > 2);
- skb = dev_alloc_skb(pkt_len + NET_IP_ALIGN);
- if (skb == NULL) {
- printk(KERN_NOTICE "%s: Memory squeeze, dropping packet.\n",
- dev->name);
- dev->stats.rx_dropped++;
- break;
- }
- skb_reserve(skb, NET_IP_ALIGN);
-
- data = skb_put(skb, pkt_len);
-
- /* copy from receive buffer */
- cur_bd = 0;
- offset = 0;
- while (offset < pkt_len && cur_bd < bd_count) {
- int len = le32_to_cpu(lp->rfd_cur->bd[cur_bd].BDCtl) &
- BD_BuffLength_MASK;
- dma_addr_t dma = le32_to_cpu(lp->rfd_cur->bd[cur_bd].BuffData);
- void *rxbuf = rxbuf_bus_to_virt(lp, dma);
- if (offset + len > pkt_len)
- len = pkt_len - offset;
-#ifdef TC35815_DMA_SYNC_ONDEMAND
- pci_dma_sync_single_for_cpu(lp->pci_dev,
- dma, len,
- PCI_DMA_FROMDEVICE);
-#endif
- memcpy(data + offset, rxbuf, len);
-#ifdef TC35815_DMA_SYNC_ONDEMAND
- pci_dma_sync_single_for_device(lp->pci_dev,
- dma, len,
- PCI_DMA_FROMDEVICE);
-#endif
- offset += len;
- cur_bd++;
- }
-#else /* TC35815_USE_PACKEDBUFFER */
BUG_ON(bd_count > 1);
cur_bd = (le32_to_cpu(lp->rfd_cur->bd[0].BDCtl)
& BD_RxBDID_MASK) >> BD_RxBDID_SHIFT;
@@ -1711,7 +1537,6 @@ tc35815_rx(struct net_device *dev, int limit)
memmove(skb->data, skb->data - NET_IP_ALIGN,
pkt_len);
data = skb_put(skb, pkt_len);
-#endif /* TC35815_USE_PACKEDBUFFER */
if (netif_msg_pktdata(lp))
print_eth(data);
skb->protocol = eth_type_trans(skb, dev);
@@ -1753,19 +1578,11 @@ tc35815_rx(struct net_device *dev, int limit)
BUG_ON(id >= RX_BUF_NUM);
#endif
/* free old buffers */
-#ifdef TC35815_USE_PACKEDBUFFER
- while (lp->fbl_curid != id)
-#else
lp->fbl_count--;
while (lp->fbl_count < RX_BUF_NUM)
-#endif
{
-#ifdef TC35815_USE_PACKEDBUFFER
- unsigned char curid = lp->fbl_curid;
-#else
unsigned char curid =
(id + 1 + lp->fbl_count) % RX_BUF_NUM;
-#endif
struct BDesc *bd = &lp->fbl_ptr->bd[curid];
#ifdef DEBUG
bdctl = le32_to_cpu(bd->BDCtl);
@@ -1776,7 +1593,6 @@ tc35815_rx(struct net_device *dev, int limit)
}
#endif
/* pass BD to controller */
-#ifndef TC35815_USE_PACKEDBUFFER
if (!lp->rx_skbs[curid].skb) {
lp->rx_skbs[curid].skb =
alloc_rxbuf_skb(dev,
@@ -1786,21 +1602,11 @@ tc35815_rx(struct net_device *dev, int limit)
break; /* try on next reception */
bd->BuffData = cpu_to_le32(lp->rx_skbs[curid].skb_dma);
}
-#endif /* TC35815_USE_PACKEDBUFFER */
/* Note: BDLength was modified by chip. */
bd->BDCtl = cpu_to_le32(BD_CownsBD |
(curid << BD_RxBDID_SHIFT) |
RX_BUF_SIZE);
-#ifdef TC35815_USE_PACKEDBUFFER
- lp->fbl_curid = (curid + 1) % RX_BUF_NUM;
- if (netif_msg_rx_status(lp)) {
- printk("%s: Entering new FBD %d\n",
- dev->name, lp->fbl_curid);
- dump_frfd(lp->fbl_ptr);
- }
-#else
lp->fbl_count++;
-#endif
}
}
@@ -1872,11 +1678,7 @@ static int tc35815_poll(struct napi_struct *napi, int budget)
return received;
}
-#ifdef NO_CHECK_CARRIER
-#define TX_STA_ERR (Tx_ExColl|Tx_Under|Tx_Defer|Tx_LateColl|Tx_TxPar|Tx_SQErr)
-#else
#define TX_STA_ERR (Tx_ExColl|Tx_Under|Tx_Defer|Tx_NCarr|Tx_LateColl|Tx_TxPar|Tx_SQErr)
-#endif
static void
tc35815_check_tx_stat(struct net_device *dev, int status)
@@ -1890,16 +1692,12 @@ tc35815_check_tx_stat(struct net_device *dev, int status)
if (status & Tx_TxColl_MASK)
dev->stats.collisions += status & Tx_TxColl_MASK;
-#ifndef NO_CHECK_CARRIER
/* TX4939 does not have NCarr */
if (lp->chiptype == TC35815_TX4939)
status &= ~Tx_NCarr;
-#ifdef WORKAROUND_LOSTCAR
/* WORKAROUND: ignore LostCrS in full duplex operation */
if (!lp->link || lp->duplex == DUPLEX_FULL)
status &= ~Tx_NCarr;
-#endif
-#endif
if (!(status & TX_STA_ERR)) {
/* no error. */
@@ -1929,12 +1727,10 @@ tc35815_check_tx_stat(struct net_device *dev, int status)
dev->stats.tx_fifo_errors++;
msg = "Excessive Deferral.";
}
-#ifndef NO_CHECK_CARRIER
if (status & Tx_NCarr) {
dev->stats.tx_carrier_errors++;
msg = "Lost Carrier Sense.";
}
-#endif
if (status & Tx_LateColl) {
dev->stats.tx_aborted_errors++;
msg = "Late Collision.";
@@ -2025,9 +1821,7 @@ tc35815_txdone(struct net_device *dev)
/* start DMA Transmitter again */
txhead->fd.FDNext |= cpu_to_le32(FD_Next_EOL);
-#ifdef GATHER_TXINT
txhead->fd.FDCtl |= cpu_to_le32(FD_FrmOpt_IntTx);
-#endif
if (netif_msg_tx_queued(lp)) {
printk("%s: start TxFD on queue.\n",
dev->name);
@@ -2138,14 +1932,12 @@ tc35815_set_multicast_list(struct net_device *dev)
(struct tc35815_regs __iomem *)dev->base_addr;
if (dev->flags & IFF_PROMISC) {
-#ifdef WORKAROUND_100HALF_PROMISC
/* With some (all?) 100MHalf HUB, controller will hang
* if we enabled promiscuous mode before linkup... */
struct tc35815_local *lp = netdev_priv(dev);
if (!lp->link)
return;
-#endif
/* Enable promiscuous mode */
tc_writel(CAM_CompEn | CAM_BroadAcc | CAM_GroupAcc | CAM_StationAcc, &tr->CAM_Ctl);
} else if ((dev->flags & IFF_ALLMULTI) ||
@@ -2332,9 +2124,6 @@ static void tc35815_chip_init(struct net_device *dev)
tc_writel(DMA_BURST_SIZE | DMA_RxAlign_2, &tr->DMA_Ctl);
else
tc_writel(DMA_BURST_SIZE, &tr->DMA_Ctl);
-#ifdef TC35815_USE_PACKEDBUFFER
- tc_writel(RxFrag_EnPack | ETH_ZLEN, &tr->RxFragSize); /* Packing */
-#endif
tc_writel(0, &tr->TxPollCtr); /* Batch mode */
tc_writel(TX_THRESHOLD, &tr->TxThrsh);
tc_writel(INT_EN_CMD, &tr->Int_En);
@@ -2352,19 +2141,12 @@ static void tc35815_chip_init(struct net_device *dev)
tc_writel(RX_CTL_CMD, &tr->Rx_Ctl); /* start MAC receiver */
/* start MAC transmitter */
-#ifndef NO_CHECK_CARRIER
/* TX4939 does not have EnLCarr */
if (lp->chiptype == TC35815_TX4939)
txctl &= ~Tx_EnLCarr;
-#ifdef WORKAROUND_LOSTCAR
/* WORKAROUND: ignore LostCrS in full duplex operation */
if (!lp->phy_dev || !lp->link || lp->duplex == DUPLEX_FULL)
txctl &= ~Tx_EnLCarr;
-#endif
-#endif /* !NO_CHECK_CARRIER */
-#ifdef GATHER_TXINT
- txctl &= ~Tx_EnComp; /* disable global tx completion int. */
-#endif
tc_writel(txctl, &tr->Tx_Ctl);
}
--
1.5.6.5
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox