* Re: [PATCH 6/8] netpoll: Allow netpoll_setup/cleanup recursion
From: Peter Zijlstra @ 2010-06-25 9:45 UTC (permalink / raw)
To: Andrew Morton
Cc: David Miller, herbert, mst, frzhang, netdev, amwang, shemminger,
mpm, paulmck, mingo
In-Reply-To: <20100625014253.698d9ff5.akpm@linux-foundation.org>
On Fri, 2010-06-25 at 01:42 -0700, Andrew Morton wrote:
> On Fri, 25 Jun 2010 10:08:56 +0200 Peter Zijlstra <peterz@infradead.org> wrote:
>
> > On Thu, 2010-06-24 at 21:42 -0700, Andrew Morton wrote:
> > > That being said, I wonder why Herbert didn't hit this in his testing.
> > > I suspect that he'd enabled lockdep, which hid the bug. I haven't
> > > worked out _why_ lockdep hides the double-mutex_unlock bug, but it's a
> > > pretty bad thing to do.
> >
> > Most weird indeed, lockdep is supposed so shout its lungs out when
> > someone wants to unlock a lock that isn't actually owned by him (and it
> > not being locked at all certainly implies you're not the owner).
> >
> > In fact, the below patch results in the below splat -- its also
> > something that's tested by the locking self-test:
>
> When I enabled lockdep, the bug actually went away. Is it possible
> that when lockdep detects this bug, it prevents mutex.count from going
> from 1 to 2?
Not lockdep itself but the DEBUG_MUTEXES code (forced by lockdep).
The difference between the normal and the debug code is that the debug
code disables all fast-path code.
The x86 fast-path code does:
LOCK incl &lock->count
jg done:
call slowpath
done:
Since 1++ is >0 it will complete without calling the slow-path, would
do:
if (__mutex_slowpath_needs_to_unlock()) /* 1 regardless of DEBUG_MUTEX */
atomic_set(&lock->count, 1);
The question I guess is, do we want double unlocks to go silently
unnoticed? In that case we need to touch the fastpath asm.
> It could be that lockdep _did_ detect (and correct!) the bug. But
> because I had no usable console output at the time, I didn't see it.
>
> I did notice that the taint output was "G W". So something warned
> about something, but I don't know what. But that was happening with
> lockdep disabled.
Hrmm,. yeah without console output lockdep isn't going to help much,
should we maybe use the speaker to read out the dmesg :-)
> It'd be interesting to add
>
> printk("%d:%d\n", __LINE__, atomic_read(&foo.count));
>
> after the mutex_unlock()s.
1352:1
^ permalink raw reply
* Re: [PATCH] s2io: add dynamic LRO disable support
From: Cong Wang @ 2010-06-25 9:09 UTC (permalink / raw)
To: Jon Mason
Cc: netdev@vger.kernel.org, nhorman@redhat.com, sgruszka@redhat.com,
herbert.xu@redhat.com, bhutchings@solarflare.com, Ramkrishna Vepa,
davem@davemloft.net
In-Reply-To: <20100625044510.GC2739@exar.com>
On 06/25/10 12:45, Jon Mason wrote:
> This patch adds dynamic LRO disable support for s2io net driver,
> enables LRO by default, increases the driver version number, and
> corrects the name of the LRO modparm.
>
Very good work, Jon! I believe this version is the best we have.
Thanks again!
^ permalink raw reply
* Re: [PATCH] smsc95xx: Add module parameter to override MAC address
From: Steve.Glendinning @ 2010-06-25 8:43 UTC (permalink / raw)
To: Sebastien Jan
Cc: Simon Horman, linux-omap@vger.kernel.org, netdev@vger.kernel.org,
davem, Ian.Saturley
In-Reply-To: <4C2452ED.8020307@ti.com>
Hi Sebastien,
> > I'm confused as to why this is desirable when the mac address
> > can already be configured after module insertion via
> > smsc95xx_netdev_ops.eth_mac_addr().
>
> For example for booting over NFS using a pre-defined MAC address, with
> a minimal setup (no initrd). Or is there another way to force a MAC
> address for this use-case?
I can't see an existing way of specifying this as a kernel parameter
in Documentation/kernel-parameters.txt. netdev= doesn't have a mac
address parameter.
During development I initially had smsc95xx driver using this logic to
select a MAC address:
1. If net->dev_addr has already been set to a valid mac address (i.e. by
an administrator before bringing the device up) then use that address.
2. If the device is already currently set to a valid mac address then
use that address. This could have been set by either the device's
EEPROM or by a previously running bootloader.
3. Generate a random mac address.
Unfortunately, this doesn't work so well as the usbnet framework sets
net->dev_addr to the USB node_id before calling our bind function, so
we usually matched at step 1 (and not with the desired outcome). So
I removed step 1 because, as Simon mentioned, it's possible to change
the mac address after the device is brought up.
I can see you have a different use case, but I don't think this specific
driver is the place for this logic. I'd rather see it added to either
the usbnet framework or (preferably) the netdev framework so *all*
ethernet drivers can do this the same way. otherwise we could end up
with slight variations of this code in every single driver!
Steve
^ permalink raw reply
* Re: [v4 Patch 1/2] s2io: add dynamic LRO disable support
From: Cong Wang @ 2010-06-25 9:01 UTC (permalink / raw)
To: Jon Mason
Cc: netdev@vger.kernel.org, nhorman@redhat.com, sgruszka@redhat.com,
herbert.xu@redhat.com, bhutchings@solarflare.com, Ramkrishna Vepa,
davem@davemloft.net
In-Reply-To: <20100625043343.GB2739@exar.com>
On 06/25/10 12:33, Jon Mason wrote:
> On Tue, Jun 22, 2010 at 01:50:07AM -0700, Amerigo Wang wrote:
>>
>> This patch adds dynamic LRO diable support for s2io net driver.
>>
>> (I don't have s2io card, so only did compiling test. Anyone who wants
>> to test this is more than welcome.)
>
> Unfortunately the patch did not quite work, mostly due to NETIF_F_LRO
> needing to be added at probe time to the device features. I was able
> to modify the patch to get it working, and will respond seperately
> with the working patch.
>
Thanks very much, Jon!
I am very pleased to see this patch gets tested. :)
^ permalink raw reply
* Re: [PATCH net-next-2.6] netfilter: allow nf_tproxy_core module to be removed
From: KOVACS Krisztian @ 2010-06-25 7:56 UTC (permalink / raw)
To: Patrick McHardy; +Cc: David Miller, fw, jpirko, netdev, Balazs Scheidler
In-Reply-To: <4C2379D9.2080608@trash.net>
Hi,
On 06/24/2010 05:29 PM, Patrick McHardy wrote:
> David Miller wrote:
>> From: Florian Westphal <fw@strlen.de>
>> Date: Wed, 23 Jun 2010 20:46:11 +0200
>>
>>> tproxy assigns skb->destructor, what prevents module unload while
>>> such skbs may
>>> still be around?
>>
>> The only reference to nf_tproxy_core.ko is for the symbol,
>> "nf_tproxy_assign_sock".
>> xt_TPROXY.c, which references this symbol, thus creates a symbol
>> dependency on this
>> module, so xt_TPROXY.o needs to unload before nf_tproxy_core.ko can
>> unload, and
>> xt_TPROXY.o has it's own manner for handling module references properly.
>
> I don't see anything waiting for skbs in flight using the tproxy
> destructor in either xt_TPROXY or nf_tproxy_core though, so I think
> Florian is correct.
Yes, I think Florian and Patrick's right. Right now there's nothing
preventing xt_TPROXY and nf_tproxy_core from being removed while there
are skbs in flight with the tproxy destructor set.
--
KOVACS Krisztian
^ permalink raw reply
* Re: [v4 Patch 1/2] s2io: add dynamic LRO disable support
From: Cong Wang @ 2010-06-25 8:59 UTC (permalink / raw)
To: Ben Hutchings
Cc: netdev, nhorman, sgruszka, herbert.xu, Ramkrishna.Vepa, davem
In-Reply-To: <1277207080.2091.2.camel@achroite.uk.solarflarecom.com>
On 06/22/10 19:44, Ben Hutchings wrote:
> On Tue, 2010-06-22 at 04:50 -0400, Amerigo Wang wrote:
>> This patch adds dynamic LRO diable support for s2io net driver.
>>
>> (I don't have s2io card, so only did compiling test. Anyone who wants
>> to test this is more than welcome.)
>>
>> This is based on Neil's initial work, and heavily modified
>> based on Ramkrishna's suggestions.
> [...]
>> +static int s2io_ethtool_set_flags(struct net_device *dev, u32 data)
>> +{
>> + struct s2io_nic *sp = netdev_priv(dev);
>> + int rc = 0;
>> + int changed = 0;
>> +
>> + if (data& ~ETH_FLAG_LRO)
>> + return -EOPNOTSUPP;
>> +
>> + if (data& ETH_FLAG_LRO) {
>> + if (lro_enable) {
>> + if (!(dev->features& NETIF_F_LRO)) {
>> + dev->features |= NETIF_F_LRO;
>> + changed = 1;
>> + }
>> + } else
>> + rc = -EOPNOTSUPP;
>
> Should lro_enable=0 really prevent enabling it later? This seems
> unusual.
>
I agree with Stanislaw on this point.
>> + } else if (dev->features& NETIF_F_LRO) {
>> + dev->features&= ~NETIF_F_LRO;
>> + changed = 1;
>> + }
>> +
>> + if (changed&& netif_running(dev)) {
>> + s2io_stop_all_tx_queue(sp);
>> + s2io_card_down(sp);
>> + sp->lro = dev->features& NETIF_F_LRO;
> [...]
>
> This means s2io_nic::lro and ring_info::lro can have the value
> NETIF_F_LRO, where previously they would only have the value 0 or 1. I
> don't know whether this could be a problem, but the safe thing to do is
> to coerce the value by writing !!(dev->features& NETIF_F_LRO).
>
Yeah, agreed.
It seems that Jon already fixed this when he updated the patch.
Thanks!
^ permalink raw reply
* Re: [PATCH 6/8] netpoll: Allow netpoll_setup/cleanup recursion
From: Ingo Molnar @ 2010-06-25 8:46 UTC (permalink / raw)
To: Andrew Morton
Cc: David Miller, herbert, mst, frzhang, netdev, amwang, shemminger,
mpm, paulmck, a.p.zijlstra, Linus Torvalds, linux-kernel
In-Reply-To: <20100624214204.a85c8ba2.akpm@linux-foundation.org>
* Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu, 24 Jun 2010 21:27:13 -0700 (PDT) David Miller <davem@davemloft.net> wrote:
>
> > From: Andrew Morton <akpm@linux-foundation.org>
> > Date: Thu, 24 Jun 2010 20:50:59 -0700
> >
> > > What happens if you want to actually *drop* a patch from net-next?
> > > Surely that happens?
> >
> > I've only respun the tree on two or three occasions and that was
> > because I made some significant error myself and screwed up the
> > GIT tree somehow.
> >
> > We've fixed much worse bugs than this one weeks after the changes
> > causing them went in, life goes on.
>
> Still sucks - this is a quite ugly drawback to how we're using git.
> I've hit bisection holes several times which held up the show.
> Sometimes you can make them go away by fiddling the .config, other
> times I've hunted down the fix and manually applied it for each
> iteration. It makes me feel all guilty each time I ask some poor sap
> to bisect a bug for us.
One solution would be, for really grave bugs that introduce a significant
window of bisection breakage, to add a special tag:
Fixes-Bug: SHA1
Which could then be parsed by a special variant of git bisect and which would
try to cherry-pick all Fixes-Bug commits into the bisection point.
But there are numerous disadvantages that:
- The bisection itself would be slower (maybe not a big issue for most
bisection sessions)
- Such cherry-picked trees wouldnt be precisely the same thing as the tree
as-it-was-there.
- Awkward combination bugs could ensue
- The cherry-pick may fail or may mis-apply things.
- If the Fixes-Bug tag is typoed or misconstrued then that might not be found
until someone tries a bisection and fails ... leading to awkward
add-the-tag-again-to-some-commit scenarios.
My gut feeling is that we really dont want to go there. As painful as
bisection breakages are, they are relatively rare (compared to regular
regressions), and the value of testing _that precise_ sha1 is a fundamental
thing - git bisect shouldnt interact and reorder fixes.
If a bug wasnt found sooner then it either didnt matter that much, or the tree
QA was bad. Neither of those factors forces a change in the development model.
Ingo
^ permalink raw reply
* Re: [PATCH 6/8] netpoll: Allow netpoll_setup/cleanup recursion
From: Andrew Morton @ 2010-06-25 8:42 UTC (permalink / raw)
To: Peter Zijlstra
Cc: David Miller, herbert, mst, frzhang, netdev, amwang, shemminger,
mpm, paulmck, mingo
In-Reply-To: <1277453336.22715.2154.camel@twins>
On Fri, 25 Jun 2010 10:08:56 +0200 Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, 2010-06-24 at 21:42 -0700, Andrew Morton wrote:
> > That being said, I wonder why Herbert didn't hit this in his testing.
> > I suspect that he'd enabled lockdep, which hid the bug. I haven't
> > worked out _why_ lockdep hides the double-mutex_unlock bug, but it's a
> > pretty bad thing to do.
>
> Most weird indeed, lockdep is supposed so shout its lungs out when
> someone wants to unlock a lock that isn't actually owned by him (and it
> not being locked at all certainly implies you're not the owner).
>
> In fact, the below patch results in the below splat -- its also
> something that's tested by the locking self-test:
When I enabled lockdep, the bug actually went away. Is it possible
that when lockdep detects this bug, it prevents mutex.count from going
from 1 to 2?
It could be that lockdep _did_ detect (and correct!) the bug. But
because I had no usable console output at the time, I didn't see it.
I did notice that the taint output was "G W". So something warned
about something, but I don't know what. But that was happening with
lockdep disabled.
> @@ -1344,6 +1346,10 @@ SYSCALL_DEFINE0(getppid)
> {
> int pid;
>
> + mutex_lock(&foo);
> + mutex_unlock(&foo);
> + mutex_unlock(&foo);
> +
> rcu_read_lock();
> pid = task_tgid_vnr(current->real_parent);
> rcu_read_unlock();
It'd be interesting to add
printk("%d:%d\n", __LINE__, atomic_read(&foo.count));
after the mutex_unlock()s.
^ permalink raw reply
* Re: [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access
From: Steffen Klassert @ 2010-06-25 8:24 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, netdev, Chase Douglas, Arne Nordmark
In-Reply-To: <1277426759.26161.179.camel@localhost>
On Fri, Jun 25, 2010 at 01:45:59AM +0100, Ben Hutchings wrote:
> >
> > The point is that we should not disable the interrupts if we don't need to
> > do so. It is not speed critical for the 3c59x driver but disabling the
> > interrupts should be avoided whenever possible. For example during device
> > probe and device open we can't race against an interrupt handler because
> > the device is not yet running.
> >
> > An example from vortex_probe1() is:
> >
> > for (i = 0; i < 6; i++)
> > window_write8(vp, dev->dev_addr[i], 2, i);
> >
> > which expands to someting like:
> >
> > for (i = 0; i < 6; i++) {
> > unsigned long flags;
> > spin_lock_irqsave(&vp->window_lock, flags);
> > window_set(vp, window);
> > iowrite8(dev->dev_addr[i], vp->ioaddr + i);
> > spin_unlock_irqrestore(&vp->window_lock, flags);
> > return ret;
> > }
> [...]
>
> I still fail to see why this matters.
>
These locks are not needed, why you want to have them arround?
It adds overhead, even if they are not in a fast-path function.
And much more important, this gives a reader of the code the
impression that these locks are needed. For somebody who tries
to understand the code, it will be probaply not that easy to
figure out which of these locks are needed and for what reason.
> The sfc driver which I look after in my day job also uses
> spin_lock_irqsave() for each CSR update, when this could be avoided in
> the initialisation path. None of the many customers using rt kernels
> has ever complained about this. It's just not an important issue.
Perhaps I'm fussy, personally I prefer to use the appropriate
lock in any case. But that's just my opinion, other people might
think different about that.
Steffen
^ permalink raw reply
* Re: [PATCH 6/8] netpoll: Allow netpoll_setup/cleanup recursion
From: Peter Zijlstra @ 2010-06-25 8:08 UTC (permalink / raw)
To: Andrew Morton
Cc: David Miller, herbert, mst, frzhang, netdev, amwang, shemminger,
mpm, paulmck, mingo
In-Reply-To: <20100624214204.a85c8ba2.akpm@linux-foundation.org>
On Thu, 2010-06-24 at 21:42 -0700, Andrew Morton wrote:
> That being said, I wonder why Herbert didn't hit this in his testing.
> I suspect that he'd enabled lockdep, which hid the bug. I haven't
> worked out _why_ lockdep hides the double-mutex_unlock bug, but it's a
> pretty bad thing to do.
Most weird indeed, lockdep is supposed so shout its lungs out when
someone wants to unlock a lock that isn't actually owned by him (and it
not being locked at all certainly implies you're not the owner).
In fact, the below patch results in the below splat -- its also
something that's tested by the locking self-test:
/*
* Double unlock:
*/
#define E() \
\
LOCK(A); \
UNLOCK(A); \
UNLOCK(A); /* fail */
---
kernel/timer.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/kernel/timer.c b/kernel/timer.c
index ee3f116..0496f71 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -1334,6 +1334,8 @@ SYSCALL_DEFINE0(getpid)
return task_tgid_vnr(current);
}
+static DEFINE_MUTEX(foo);
+
/*
* Accessing ->real_parent is not SMP-safe, it could
* change from under us. However, we can use a stale
@@ -1344,6 +1346,10 @@ SYSCALL_DEFINE0(getppid)
{
int pid;
+ mutex_lock(&foo);
+ mutex_unlock(&foo);
+ mutex_unlock(&foo);
+
rcu_read_lock();
pid = task_tgid_vnr(current->real_parent);
rcu_read_unlock();
=====================================
[ BUG: bad unlock balance detected! ]
-------------------------------------
init/1 is trying to release lock (foo) at:
[<ffffffff815bf3b6>] mutex_unlock+0xe/0x10
but there are no more locks to release!
other info that might help us debug this:
no locks held by init/1.
stack backtrace:
Pid: 1, comm: init Not tainted 2.6.35-rc3-tip-01034-g5205803-dirty #399
Call Trace:
[<ffffffff815bf3b6>] ? mutex_unlock+0xe/0x10
[<ffffffff8106d718>] print_unlock_inbalance_bug+0xd6/0xe1
[<ffffffff815bf3b6>] ? mutex_unlock+0xe/0x10
[<ffffffff8106e8c6>] lock_release+0xdb/0x196
[<ffffffff815bf32f>] __mutex_unlock_slowpath+0xc1/0x13a
[<ffffffff815bf3b6>] mutex_unlock+0xe/0x10
[<ffffffff8104f262>] sys_getppid+0x34/0xd8
[<ffffffff81002cdb>] system_call_fastpath+0x16/0x1b
^ permalink raw reply related
* Re: Question about xfrm by MARK feature
From: Gerd v. Egidy @ 2010-06-25 7:35 UTC (permalink / raw)
To: hadi; +Cc: timo.teras, kaber, herbert, netdev
In-Reply-To: <1277381094.3455.92.camel@bigi>
Hi Jamal,
thanks for your detailed answer.
> > For example I have 2 different remote networks with the same ip network
> > each and both of them have a tunnel to the same local network.
>
> It seems "Same IP network" means that two remote locations will have
> exactly same IP address?
yes
> This is hard of course - but nat may do it..
> There's also the nat zones feature that Patrick introduced a while back
> that may help you
I'm using Patricks conntrack zones. And Patrick helped me with a input chain
in the nat table. The other cases with e.g. a ip clash between local and
remote net already work.
So only the case with two remotes and same ips is missing.
> > I map their IPs to
> > something different so I can distinguish them in the local network. But
> > after the nat the xfrm code sees two tunnels with exactly the same
> > values. So this can't work.
>
> Can you look at the incoming encrypted packet headers and tell if they
> are from different remotes? If not, are different remotes coming in via
> a different network device? If yes, you can install a tc rule to mark
> them as they come in before decryption
I planned to avoid looking at the remote gateway ip (to even allow two
different remote gateways hiding natted behind the same ip) but that would be
a good fallback solution if my other ideas don't work out.
> and that mark should stay with
> them even after they get decrypted.
Didn't know that, very good.
I just contacted the strongswan maintainers about reqids and marks. Let's see
if this works out...
Kind regards,
Gerd
--
Address (better: trap) for people I really don't want to get mail from:
jonas@cactusamerica.com
^ permalink raw reply
* RE: [PATCH] vxge: fix memory leak in vxge_alloc_msix() error path {nodisc}
From: Ramkrishna Vepa @ 2010-06-25 7:21 UTC (permalink / raw)
To: Michal Schmidt; +Cc: netdev@vger.kernel.org
In-Reply-To: <20100624161344.0847a7ca@leela>
> When pci_enable_msix() returned ret<0, entries and vxge_entries were
> leaked.
> While at it, use the centralized exit idiom in the function.
>
> Not tested. It compiles OK.
>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
Reviewed and tested the patch. Thanks!
Acked-by: Ram Vepa <ram.vepa@exar.com>
^ permalink raw reply
* Re: [PATCH] smsc95xx: Add module parameter to override MAC address
From: Sebastien Jan @ 2010-06-25 6:55 UTC (permalink / raw)
To: Simon Horman
Cc: Steve Glendinning, netdev@vger.kernel.org,
linux-omap@vger.kernel.org
In-Reply-To: <20100625012520.GA12650@verge.net.au>
On 06/25/2010 03:25 AM, Simon Horman wrote:
> On Thu, Jun 24, 2010 at 10:14:14AM +0200, Sebastien Jan wrote:
>> Define a new module parameter 'macaddr' to override the MAC address
>> fetched either from eeprom, or randomly generated.
>>
>> The expected MAC address shall be in the 01:23:45:67:89:AB format.
>
> I'm confused as to why this is desirable when the mac address
> can already be configured after module insertion via
> smsc95xx_netdev_ops.eth_mac_addr().
For example for booting over NFS using a pre-defined MAC address, with
a minimal setup (no initrd). Or is there another way to force a MAC
address for this use-case?
^ permalink raw reply
* Re: [PATCH] Bluetooth: Bring back var 'i' increment
From: David Miller @ 2010-06-25 5:08 UTC (permalink / raw)
To: gustavo-THi1TnShQwVAfugRpC6u6w
Cc: padovan-Y3ZbgMPKUGA34EUeqzHoZw,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, marcel-kz+m5ild9QBg9hUCZPvPmw
In-Reply-To: <20100619002807.GD14514@vigoh>
From: "Gustavo F. Padovan" <gustavo-THi1TnShQwVAfugRpC6u6w@public.gmane.org>
Date: Fri, 18 Jun 2010 21:28:07 -0300
> * Gustavo F. Padovan <padovan-Y3ZbgMPKUGA34EUeqzHoZw@public.gmane.org> [2010-06-18 21:24:00 -0300]:
>
>> commit ff6e2163f28a1094fb5ca5950fe2b43c3cf6bc7a accidentally added a
>> regression on the bnep code. Fixing it.
>>
>> Signed-off-by: Gustavo F. Padovan <padovan-Y3ZbgMPKUGA34EUeqzHoZw@public.gmane.org>
...
> Marcel, if you want I can carry the fix on my tree for -next.
I'll apply this bug fix to net-2.6 so it doesn't just rot like it is
currently.
Thanks.
^ permalink raw reply
* Re: tcp: sending reset to the already closed socket
From: David Miller @ 2010-06-25 4:56 UTC (permalink / raw)
To: khorenko; +Cc: netdev
In-Reply-To: <4C1A2502.1030502@openvz.org>
Your email was a better commit message (lots of useful traces and
information and analysis, that will be useful to someone in the future
looking at this commit) than your patch's actual commit message which
was terse and non-descriptive. :-)
So I combined the two :-)
I've applied this to net-next-2.6, if this problem causes serious
documentable grief for a significant number of real users we can
toss it into net-2.6 or -stable later.
Thanks!
^ permalink raw reply
* Re: [PATCH 6/8] netpoll: Allow netpoll_setup/cleanup recursion
From: David Miller @ 2010-06-25 4:52 UTC (permalink / raw)
To: akpm
Cc: herbert, mst, frzhang, netdev, amwang, shemminger, mpm, paulmck,
a.p.zijlstra, mingo
In-Reply-To: <20100624214204.a85c8ba2.akpm@linux-foundation.org>
From: Andrew Morton <akpm@linux-foundation.org>
Date: Thu, 24 Jun 2010 21:42:04 -0700
> I'd imagine that netconsole would get in the way rather a lot for net
> developers, but it's really useful!
I have a fully logging stable serial or hypervisor console on every
one of my boxes, why would I even bother?
> That being said, I wonder why Herbert didn't hit this in his testing.
> I suspect that he'd enabled lockdep, which hid the bug. I haven't
> worked out _why_ lockdep hides the double-mutex_unlock bug, but it's a
> pretty bad thing to do.
Yes, definitely something to look into.
^ permalink raw reply
* [PATCH] s2io: add dynamic LRO disable support
From: Jon Mason @ 2010-06-25 4:45 UTC (permalink / raw)
To: Amerigo Wang
Cc: netdev@vger.kernel.org, nhorman@redhat.com, sgruszka@redhat.com,
herbert.xu@redhat.com, bhutchings@solarflare.com, Ramkrishna Vepa,
davem@davemloft.net
In-Reply-To: <20100622085415.5566.22523.sendpatchset@localhost.localdomain>
This patch adds dynamic LRO disable support for s2io net driver,
enables LRO by default, increases the driver version number, and
corrects the name of the LRO modparm.
This is mostly Wang's patch based on Neil's initial work, heavily
modified based on Ramkrishna's suggestions. This has been tested on
a Neterion Xframe adapter and verified via adapter LRO statistics.
Signed-off-by: Jon Mason <jon.mason@exar.com>
Signed-off-by: WANG Cong <amwang@redhat.com>
Signed-off-by: Neil Horman <nhorman@redhat.com>
Acked-by: Neil Horman <nhorman@redhat.com>
Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com>
Cc: Ramkrishna Vepa <Ramkrishna.Vepa@exar.com>
---
diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
index 668327c..a032d72 100644
--- a/drivers/net/s2io.c
+++ b/drivers/net/s2io.c
@@ -38,7 +38,7 @@
* Tx descriptors that can be associated with each corresponding FIFO.
* intr_type: This defines the type of interrupt. The values can be 0(INTA),
* 2(MSI_X). Default value is '2(MSI_X)'
- * lro_enable: Specifies whether to enable Large Receive Offload (LRO) or not.
+ * lro: Specifies whether to enable Large Receive Offload (LRO) or not.
* Possible values '1' for enable '0' for disable. Default is '0'
* lro_max_pkts: This parameter defines maximum number of packets can be
* aggregated as a single large packet
@@ -90,7 +90,7 @@
#include "s2io.h"
#include "s2io-regs.h"
-#define DRV_VERSION "2.0.26.25"
+#define DRV_VERSION "2.0.26.26"
/* S2io Driver name & version. */
static char s2io_driver_name[] = "Neterion";
@@ -496,7 +496,7 @@ S2IO_PARM_INT(rxsync_frequency, 3);
/* Interrupt type. Values can be 0(INTA), 2(MSI_X) */
S2IO_PARM_INT(intr_type, 2);
/* Large receive offload feature */
-static unsigned int lro_enable;
+static unsigned int lro_enable = 1;
module_param_named(lro, lro_enable, uint, 0);
/* Max pkts to be aggregated by LRO at one time. If not specified,
@@ -795,7 +795,6 @@ static int init_shared_mem(struct s2io_nic *nic)
ring->rx_curr_put_info.ring_len = rx_cfg->num_rxd - 1;
ring->nic = nic;
ring->ring_no = i;
- ring->lro = lro_enable;
blk_cnt = rx_cfg->num_rxd / (rxd_count[nic->rxd_mode] + 1);
/* Allocating all the Rx blocks */
@@ -6675,6 +6674,7 @@ static u32 s2io_ethtool_op_get_tso(struct net_device *dev)
{
return (dev->features & NETIF_F_TSO) != 0;
}
+
static int s2io_ethtool_op_set_tso(struct net_device *dev, u32 data)
{
if (data)
@@ -6685,6 +6685,42 @@ static int s2io_ethtool_op_set_tso(struct net_device *dev, u32 data)
return 0;
}
+static int s2io_ethtool_set_flags(struct net_device *dev, u32 data)
+{
+ struct s2io_nic *sp = netdev_priv(dev);
+ int rc = 0;
+ int changed = 0;
+
+ if (data & ~ETH_FLAG_LRO)
+ return -EOPNOTSUPP;
+
+ if (data & ETH_FLAG_LRO) {
+ if (lro_enable) {
+ if (!(dev->features & NETIF_F_LRO)) {
+ dev->features |= NETIF_F_LRO;
+ changed = 1;
+ }
+ } else
+ rc = -EINVAL;
+ } else if (dev->features & NETIF_F_LRO) {
+ dev->features &= ~NETIF_F_LRO;
+ changed = 1;
+ }
+
+ if (changed && netif_running(dev)) {
+ s2io_stop_all_tx_queue(sp);
+ s2io_card_down(sp);
+ sp->lro = !!(dev->features & NETIF_F_LRO);
+ rc = s2io_card_up(sp);
+ if (rc)
+ s2io_reset(sp);
+ else
+ s2io_start_all_tx_queue(sp);
+ }
+
+ return rc;
+}
+
static const struct ethtool_ops netdev_ethtool_ops = {
.get_settings = s2io_ethtool_gset,
.set_settings = s2io_ethtool_sset,
@@ -6701,6 +6737,8 @@ static const struct ethtool_ops netdev_ethtool_ops = {
.get_rx_csum = s2io_ethtool_get_rx_csum,
.set_rx_csum = s2io_ethtool_set_rx_csum,
.set_tx_csum = s2io_ethtool_op_set_tx_csum,
+ .set_flags = s2io_ethtool_set_flags,
+ .get_flags = ethtool_op_get_flags,
.set_sg = ethtool_op_set_sg,
.get_tso = s2io_ethtool_op_get_tso,
.set_tso = s2io_ethtool_op_set_tso,
@@ -7229,6 +7267,7 @@ static int s2io_card_up(struct s2io_nic *sp)
struct ring_info *ring = &mac_control->rings[i];
ring->mtu = dev->mtu;
+ ring->lro = sp->lro;
ret = fill_rx_buffers(sp, ring, 1);
if (ret) {
DBG_PRINT(ERR_DBG, "%s: Out of memory in Open\n",
@@ -7974,7 +8013,8 @@ s2io_init_nic(struct pci_dev *pdev, const struct pci_device_id *pre)
dev->netdev_ops = &s2io_netdev_ops;
SET_ETHTOOL_OPS(dev, &netdev_ethtool_ops);
dev->features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
-
+ if (lro_enable)
+ dev->features |= NETIF_F_LRO;
dev->features |= NETIF_F_SG | NETIF_F_IP_CSUM;
if (sp->high_dma_flag == true)
dev->features |= NETIF_F_HIGHDMA;
^ permalink raw reply related
* Re: [PATCH 6/8] netpoll: Allow netpoll_setup/cleanup recursion
From: Andrew Morton @ 2010-06-25 4:42 UTC (permalink / raw)
To: David Miller
Cc: herbert, mst, frzhang, netdev, amwang, shemminger, mpm, paulmck,
a.p.zijlstra, mingo
In-Reply-To: <20100624.212713.242141362.davem@davemloft.net>
On Thu, 24 Jun 2010 21:27:13 -0700 (PDT) David Miller <davem@davemloft.net> wrote:
> From: Andrew Morton <akpm@linux-foundation.org>
> Date: Thu, 24 Jun 2010 20:50:59 -0700
>
> > What happens if you want to actually *drop* a patch from net-next?
> > Surely that happens?
>
> I've only respun the tree on two or three occasions and that was
> because I made some significant error myself and screwed up the
> GIT tree somehow.
>
> We've fixed much worse bugs than this one weeks after the changes
> causing them went in, life goes on.
Still sucks - this is a quite ugly drawback to how we're using git.
I've hit bisection holes several times which held up the show.
Sometimes you can make them go away by fiddling the .config, other
times I've hunted down the fix and manually applied it for each
iteration. It makes me feel all guilty each time I ask some poor sap
to bisect a bug for us.
> And the fact that it took two weeks of it being in -next before
> anyone even reported it says how wide a net this particular bug
> covers :-) (hint: personally I've still never used netconsole
> even one single time, and it's been around for what, something
> like 6 years?)
I'd imagine that netconsole would get in the way rather a lot for net
developers, but it's really useful!
That being said, I wonder why Herbert didn't hit this in his testing.
I suspect that he'd enabled lockdep, which hid the bug. I haven't
worked out _why_ lockdep hides the double-mutex_unlock bug, but it's a
pretty bad thing to do.
Presumably mutex debugging _would_ have found it, but because the bug
was in netconsole code, the mutex-debugging blurt of course didn't come
out. We don't replay the log buffer when netconsole is brought up -
perhaps we should.
And that machine has a screwy USB keyboard on which I've never managed
to invoke the vt-srcoll-backwards thing, so it would have been darned
hard for me to see and mutex-debugging warnings anyway.
^ permalink raw reply
* I am Mr. Chen Guan
From: Bank Of China @ 2010-06-25 4:36 UTC (permalink / raw)
Dear Friend,
I have a business suggestion for you. let me start by introducing myself
to you fully. I am Mr. Chen Guan, Foreign Operations Manager of the Bank
of China (Hong Kong). Before the U.S and Iraqi war our client Late Ali
Hussein who was with the Iraqi forces and also a businessman made a
numbered fixed deposit for 12 calendar months, with a value of Seventeen
Million Three Hundred Thousand United State Dollars only into an account
with my branch. Upon maturity several notice was sent to him, even during
the war . Again after the war another notification was sent and still no
response came from him. We later found out that the Ali Hussein and his
family had been killed during the war in bomb blast that hit their home.
After further investigation it was also discovered that Ali Hussein did
not declare any next of kin in his official papers including the paper
work of his bank deposit. And he also confided in me the last time he was
at my office that no one except me knew of his deposit in my bank. So,
Seventeen Million Three Hundred Thousand United State
Dollars(US$17,300,000.00) is still lying in my bank and no one will ever
come forward to claim it. What bothers me most is that according to the
laws of my country at the expiration 7 years the funds will revert to the
ownership of the Hong Kong Government if nobody applies to claim the
funds. Against this backdrop, my suggestion to you is that I will like you
as a foreigner to stand as the next of kin to Ali Hussein so that you will
be able to receive his funds. Please reply to my private email for more
Details ( chen_guan777@w.cn ) Thank you for your time and understanding
Kind Regards,
Mr. Chen Guan.
^ permalink raw reply
* Re: [v4 Patch 1/2] s2io: add dynamic LRO disable support
From: Jon Mason @ 2010-06-25 4:33 UTC (permalink / raw)
To: Amerigo Wang
Cc: netdev@vger.kernel.org, nhorman@redhat.com, sgruszka@redhat.com,
herbert.xu@redhat.com, bhutchings@solarflare.com, Ramkrishna Vepa,
davem@davemloft.net
In-Reply-To: <20100622085415.5566.22523.sendpatchset@localhost.localdomain>
On Tue, Jun 22, 2010 at 01:50:07AM -0700, Amerigo Wang wrote:
>
> This patch adds dynamic LRO diable support for s2io net driver.
>
> (I don't have s2io card, so only did compiling test. Anyone who wants
> to test this is more than welcome.)
Unfortunately the patch did not quite work, mostly due to NETIF_F_LRO
needing to be added at probe time to the device features. I was able
to modify the patch to get it working, and will respond seperately
with the working patch.
Thanks,
Jon
>
> This is based on Neil's initial work, and heavily modified
> based on Ramkrishna's suggestions.
>
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Signed-off-by: Neil Horman <nhorman@redhat.com>
> Acked-by: Neil Horman <nhorman@redhat.com>
> Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com>
> Cc: Ramkrishna Vepa <Ramkrishna.Vepa@exar.com>
>
> ---
>
> diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
> index 668327c..2e6e3b3 100644
> --- a/drivers/net/s2io.c
> +++ b/drivers/net/s2io.c
> @@ -795,7 +795,6 @@ static int init_shared_mem(struct s2io_nic *nic)
> ring->rx_curr_put_info.ring_len = rx_cfg->num_rxd - 1;
> ring->nic = nic;
> ring->ring_no = i;
> - ring->lro = lro_enable;
>
> blk_cnt = rx_cfg->num_rxd / (rxd_count[nic->rxd_mode] + 1);
> /* Allocating all the Rx blocks */
> @@ -6684,6 +6683,41 @@ static int s2io_ethtool_op_set_tso(struct net_device *dev, u32 data)
>
> return 0;
> }
> +static int s2io_ethtool_set_flags(struct net_device *dev, u32 data)
> +{
> + struct s2io_nic *sp = netdev_priv(dev);
> + int rc = 0;
> + int changed = 0;
> +
> + if (data & ~ETH_FLAG_LRO)
> + return -EOPNOTSUPP;
> +
> + if (data & ETH_FLAG_LRO) {
> + if (lro_enable) {
> + if (!(dev->features & NETIF_F_LRO)) {
> + dev->features |= NETIF_F_LRO;
> + changed = 1;
> + }
> + } else
> + rc = -EOPNOTSUPP;
> + } else if (dev->features & NETIF_F_LRO) {
> + dev->features &= ~NETIF_F_LRO;
> + changed = 1;
> + }
> +
> + if (changed && netif_running(dev)) {
> + s2io_stop_all_tx_queue(sp);
> + s2io_card_down(sp);
> + sp->lro = dev->features & NETIF_F_LRO;
> + rc = s2io_card_up(sp);
> + if (rc)
> + s2io_reset(sp);
> + else
> + s2io_start_all_tx_queue(sp);
> + }
> +
> + return rc;
> +}
>
> static const struct ethtool_ops netdev_ethtool_ops = {
> .get_settings = s2io_ethtool_gset,
> @@ -6701,6 +6735,8 @@ static const struct ethtool_ops netdev_ethtool_ops = {
> .get_rx_csum = s2io_ethtool_get_rx_csum,
> .set_rx_csum = s2io_ethtool_set_rx_csum,
> .set_tx_csum = s2io_ethtool_op_set_tx_csum,
> + .set_flags = s2io_ethtool_set_flags,
> + .get_flags = ethtool_op_get_flags,
> .set_sg = ethtool_op_set_sg,
> .get_tso = s2io_ethtool_op_get_tso,
> .set_tso = s2io_ethtool_op_set_tso,
> @@ -7229,6 +7265,7 @@ static int s2io_card_up(struct s2io_nic *sp)
> struct ring_info *ring = &mac_control->rings[i];
>
> ring->mtu = dev->mtu;
> + ring->lro = sp->lro;
> ret = fill_rx_buffers(sp, ring, 1);
> if (ret) {
> DBG_PRINT(ERR_DBG, "%s: Out of memory in Open\n",
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 2/2] broadcom: Add 5241 support
From: David Miller @ 2010-06-25 4:30 UTC (permalink / raw)
To: dbaryshkov; +Cc: netdev, mcarlson, mchan
In-Reply-To: <1276765344-12675-2-git-send-email-dbaryshkov@gmail.com>
From: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Date: Thu, 17 Jun 2010 13:02:24 +0400
> This patch adds the 5241 PHY ID to the broadcom module.
>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Applied to net-next-2.6
^ permalink raw reply
* Re: [PATCH v2 1/2] broadcom: move all PHY_ID's to header
From: David Miller @ 2010-06-25 4:30 UTC (permalink / raw)
To: dbaryshkov; +Cc: netdev, mcarlson, mchan
In-Reply-To: <1276765344-12675-1-git-send-email-dbaryshkov@gmail.com>
From: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Date: Thu, 17 Jun 2010 13:02:23 +0400
> Move all PHY IDs to brcmphy.h header for completeness and unification of code.
>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Applied to net-next-2.6.
^ permalink raw reply
* Re: [patch 1/1] fix "netpoll: Allow netpoll_setup/cleanup recursion"
From: David Miller @ 2010-06-25 4:27 UTC (permalink / raw)
To: akpm; +Cc: netdev, herbert
In-Reply-To: <201006250353.o5P3rwti014499@imap1.linux-foundation.org>
From: akpm@linux-foundation.org
Date: Thu, 24 Jun 2010 20:54:01 -0700
> From: Andrew Morton <akpm@linux-foundation.org>
>
> Remove rtnl_unlock() which had no corresponding rtnl_lock().
>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
I found a time machine and applied this before you even posted
it here :-)
^ permalink raw reply
* Re: [PATCH 6/8] netpoll: Allow netpoll_setup/cleanup recursion
From: David Miller @ 2010-06-25 4:27 UTC (permalink / raw)
To: akpm
Cc: herbert, mst, frzhang, netdev, amwang, shemminger, mpm, paulmck,
a.p.zijlstra, mingo
In-Reply-To: <20100624205059.a28756b0.akpm@linux-foundation.org>
From: Andrew Morton <akpm@linux-foundation.org>
Date: Thu, 24 Jun 2010 20:50:59 -0700
> What happens if you want to actually *drop* a patch from net-next?
> Surely that happens?
I've only respun the tree on two or three occasions and that was
because I made some significant error myself and screwed up the
GIT tree somehow.
We've fixed much worse bugs than this one weeks after the changes
causing them went in, life goes on.
And the fact that it took two weeks of it being in -next before
anyone even reported it says how wide a net this particular bug
covers :-) (hint: personally I've still never used netconsole
even one single time, and it's been around for what, something
like 6 years?)
^ permalink raw reply
* [patch 1/1] fix "netpoll: Allow netpoll_setup/cleanup recursion"
From: akpm @ 2010-06-25 3:54 UTC (permalink / raw)
To: davem; +Cc: netdev, akpm, herbert
From: Andrew Morton <akpm@linux-foundation.org>
Remove rtnl_unlock() which had no corresponding rtnl_lock().
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
net/core/netpoll.c | 1 -
1 file changed, 1 deletion(-)
diff -puN net/core/netpoll.c~fix-netpoll-allow-netpoll_setup-cleanup-recursion net/core/netpoll.c
--- a/net/core/netpoll.c~fix-netpoll-allow-netpoll_setup-cleanup-recursion
+++ a/net/core/netpoll.c
@@ -748,7 +748,6 @@ int __netpoll_setup(struct netpoll *np)
/* last thing to do is link it to the net device structure */
rcu_assign_pointer(ndev->npinfo, npinfo);
- rtnl_unlock();
return 0;
_
^ permalink raw reply
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