urgent e1000 fix

Please pull from 'misc-fixes' branch of rsync://rsync.kernel.org/pub/scm/linux/kernel/git/jgarzik/netdev-2.6.git

to obtain the spinlock fix described in the attached text.

 drivers/net/e1000/e1000_main.c |    1 +
 1 files changed, 1 insertion(+)

Mitch Williams:
e1000: fix spinlock bug

@@ -2307,6 +2307,7 @@ e1000_xmit_frame(struct sk_buff *skb, st
 	tso = e1000_tso(adapter, skb);
 	if (tso < 0) {
 		dev_kfree_skb_any(skb);
+		spin_unlock_irqrestore(&adapter->tx_lock, flags);
 		return NETDEV_TX_OK;
 	}
 

hmm, I know I'm not that experianced with patch, but when I saved this to a file and did patch -p1 <file the hunk was rejected, the reject file is saying

***************
*** 2307,2312 ****

         tso = e1000_tso(adapter, skb);
         if (tso < 0) {
                 dev_kfree_skb_any(skb);
                 return NETDEV_TX_OK;
         }

--- 2307,2313 ----

         tso = e1000_tso(adapter, skb);
         if (tso < 0) {
                 dev_kfree_skb_any(skb);
+                spin_unlock_irqrestore(&adapter->tx_lock, flags);
                 return NETDEV_TX_OK;
         }

I manually put the line in and am compiling it now to test it, but is someone could take a few seconds and teach me what I did wrong it would be appriciated.

David Lang

There are two ways of constructing a software design. One way is to make it so simple that there are obviously no deficiencies. And the other way is to make it so complicated that there are no obvious deficiencies.
 -- C.A.R. Hoare

 drivers/net/e1000/e1000_main.c |    1 +
 1 files changed, 1 insertion(+)

Mitch Williams:
e1000: fix spinlock bug

@@ -2307,6 +2307,7 @@ e1000_xmit_frame(struct sk_buff *skb, st
 	tso = e1000_tso(adapter, skb);
 	if (tso < 0) {
 		dev_kfree_skb_any(skb);
+		spin_unlock_irqrestore(&adapter->tx_lock, flags);
 		return NETDEV_TX_OK;
 	}
 

It's probably the whitespace thing that Linus's git-apply gadget was complaining about.

I'm terribly surprising, though, since my patch(1) applied the diff just fine.

<shrug>

Jeff

I could easily make git-apply accept empty lines as if they had a single space on it. What I find surprising is that "patch" allows that kind of whitespace corruption by default, and silently. Usually you have to give it the "-l" flag to make it ignore whitespace differences..

Linus

patch(1) is a huge collection of heuristics like this, even without '-l', so I'm not surprised that it worked.

Does git-apply support patches with "fuzz", out of curiosity?

Jeff

This does not solve the problem that I reported last thursday where the fourth port of a e1000 quad card doesn't work under SMP (I don't know if it was supposed to, but since it was a locking fix I had hopes).

here's the symptom I am seeing

happy1-p:~# ifconfig eth11 192.168.255.1
SIOCSIFFLAGS: Invalid argument
happy1-p:~# netstat -nr
Kernel IP routing table
Destination     Gateway         Genmask         Flags   MSS Window  irtt Iface
172.20.252.0    0.0.0.0         255.255.252.0   U         0 0          0 eth1
10.201.0.0      0.0.0.0         255.255.0.0     U         0 0          0 eth0
0.0.0.0         10.201.0.2      0.0.0.0         UG        0 0          0 eth0
happy1-p:~# ifconfig eth11
eth11     Link encap:Ethernet  HWaddr 00:04:23:B4:BB:97
          inet addr:192.168.255.1  Bcast:192.168.255.255  Mask:255.255.255.0
          BROADCAST MULTICAST  MTU:1500  Metric:1
          RX packets:0 errors:0 dropped:0 overruns:0 frame:0
          TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
          collisions:0 txqueuelen:1000
          RX bytes:0 (0.0 b)  TX bytes:0 (0.0 b)
          Base address:0x60c0 Memory:fe460000-fe480000

userspace is debian woody

David Lang

There are two ways of constructing a software design. One way is to make it so simple that there are obviously no deficiencies. And the other way is to make it so complicated that there are no obvious deficiencies.
 -- C.A.R. Hoare

No, git is strictly "zero fuzz". So was my "dotest" script even before switching it to "git-apply", btw (ie I explicitly had a "--fuzz=0" there when using GNU patch).

Now, I may have to reconsider the strictness if it causes lots of problems, and it's not like I hate fuzz (or ignoring whitespace) with a passion. I'd just rather try to be as strict as possible at least initially.

Your special case of a corrupted patch where a single space turned into an empty line is actually one case I considered allowing, just because the fuzz there wouldn't be in the data itself, only in the first character that just tells what kind of line it is (new, deleted or unmodified), and just saying "empty means unmodified" doesn't really introduce any possibility of ambiguity.

So let's see what happens. Tell me if you see more of this particular kind of corruption, and I'll just make "git-apply" work around it. I don't think _one_ patch is much of an indication in any direction, but two or three might convince me to just relax the checks a bit.

To actually allow real fuzz or to allow real whitespace differences in the patch data itself is a _much_ bigger issue than this trivial patch corruption, and I'd prefer to avoid going there if at all possible.

Linus

How about automatic stripping of _trailing_ whitespace on all incoming patches? IIRC no file type (C, sh, Makefile, you name it) depends on conservation of it, thus it's 100% safe.
--
vda

One (admittedly rare) case - adding a text file that contains an embedded patch, so you have a patch that includes a patch. This is sometimes done in Documentation files when an external file has to be changed. In embedded patch, empty lines are converted to a single space, which then appears as trailing whitespace. Not sure if that is a big enough reason not to strip whitespace.

There's a much more important reason never _ever_ to mess with whitespace in patches: it by definition measn that the resulting whitespace now does not match the thing at the other end, and that _will_ mean merge problems later (ie subsequent patches will have increasingly incorrect whitespace, and now everybody has to live with whitespace not being reliable).

So no. The only reliable way to handle whitespace is to never corrupt it. Don't make excuses for broken email clients etc.

Linus

Linus .. why for kernel tree can't be used indent or other source code formater ?

If indent tool can't feet all what is neccessary or have some not ease solveable bugs or can't be adopted other now avalaible tool IMO *it is* *time* for start project with special formater for kernel source tree. Using it can cut all this kind dissusions :>

If indent can handle correcly kernel source tree it will be posiible place
in source root tree one .indent.pro file and add small modificatiom to make suit with additional "indent" target. In case using indent neccessary patch can take only few lines :>

After this all developers before generate patches will must only pass "make indent" .. nothing more.
As result can be also removed Documentation/CodingStyle file (description about passing "make indent" before generate patches can be moved to Documentation/SubmittingPatches)

kloczek

-----------------------------------------------------------
*Ludzie nie mają problemów, tylko sobie sami je stwarzają*
-----------------------------------------------------------
Tomasz Kłoczko, sys adm @zie.pg.gda.pl|*e-mail: [email protected]*

Works iff the patched code is similarly mangled first... I can hear a distant howling on LKML on the bare thought of proposing this.

You also can't assume that spaces at the end of lines make no difference for all uses people might want to put git to.

Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Sure, we do it, but then we try to make it obvious to all sides.

It's the "small and non-obvious" differences that are really poisonous. You don't see them in the soruces, yet patches don't apply.

So don't do subtle whitespace "fixups" by default. It just makes everybody unhappy down the line.

That's not to say that we don't do whitespace fixups _occasionally_. When it's ugly enough to be noticeable, I sure as hell fix up whitespace. But not by default.

Linus