[git patch] urgent e1000 fix

[git patch] urgent e1000 fix

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 174 bytes --]

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.


[-- Attachment #2: netdev-2.6.txt --]
[-- Type: text/plain, Size: 495 bytes --]


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


Mitch Williams:
  e1000: fix spinlock bug


diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -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;
 	}
 

Re: [git patch] urgent e1000 fix

[David Lang]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1528 bytes --]

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

  On Thu, 23 Jun 2005, Jeff Garzik wrote:

> Date: Thu, 23 Jun 2005 05:24:05 -0400
> From: Jeff Garzik <jgarzik@pobox.com>
> To: Andrew Morton <akpm@osdl.org>, Linus Torvalds <torvalds@osdl.org>
> Cc: Linux Kernel <linux-kernel@vger.kernel.org>,
>     Netdev List <netdev@vger.kernel.org>
> Subject: [git patch] 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.
>
>
>

-- 
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


[-- Attachment #2: Type: TEXT/PLAIN, Size: 495 bytes --]


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


Mitch Williams:
  e1000: fix spinlock bug


diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -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;
 	}
 

Re: [git patch] urgent e1000 fix

[Jeff Garzik]

David Lang wrote:
> 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

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

Re: [git patch] urgent e1000 fix

[Linus Torvalds]

On Thu, 23 Jun 2005, Jeff Garzik wrote:
> 
> 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.

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

Re: [git patch] urgent e1000 fix

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Thu, 23 Jun 2005, Jeff Garzik wrote:
> 
>>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.
> 
> 
> 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..


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

Re: [git patch] urgent e1000 fix

[Linus Torvalds]

On Thu, 23 Jun 2005, Jeff Garzik wrote:
> 
> 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?

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

Re: [git patch] urgent e1000 fix

[Denis Vlasenko]

On Friday 24 June 2005 02:33, Linus Torvalds wrote:
> 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.

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

Re: [git patch] urgent e1000 fix

[Keith Owens]

On Fri, 24 Jun 2005 09:49:05 +0300, 
Denis Vlasenko <vda@ilport.com.ua> wrote:
>On Friday 24 June 2005 02:33, Linus Torvalds wrote:
>> 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.
>
>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.

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.

Re: [git patch] urgent e1000 fix

[Linus Torvalds]

On Fri, 24 Jun 2005, Keith Owens wrote:

> On Fri, 24 Jun 2005 09:49:05 +0300, 
> Denis Vlasenko <vda@ilport.com.ua> wrote:
> >On Friday 24 June 2005 02:33, Linus Torvalds wrote:
> >> 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.
> >
> >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.
> 
> 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

Re: [git patch] urgent e1000 fix

[Tomasz Kłoczko]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2715 bytes --]

On Fri, 24 Jun 2005, Linus Torvalds wrote:

>
>
> On Fri, 24 Jun 2005, Keith Owens wrote:
>
>> On Fri, 24 Jun 2005 09:49:05 +0300,
>> Denis Vlasenko <vda@ilport.com.ua> wrote:
>>> On Friday 24 June 2005 02:33, Linus Torvalds wrote:
>>>> 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.
>>>
>>> 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.
>>
>> 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 .. 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: kloczek@rudy.mif.pg.gda.pl*

Re: [git patch] urgent e1000 fix

[Linus Torvalds]

On Fri, 24 Jun 2005, Tomasz K³oczko wrote:
> 
> Linus .. why for kernel tree can't be used indent or other source 
> code formater ?

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

Re: [git patch] urgent e1000 fix

[Horst von Brand]

Denis Vlasenko <vda@ilport.com.ua> wrote:
> On Friday 24 June 2005 02:33, Linus Torvalds wrote:
> > 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.
> 
> 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.

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

Re: [git patch] urgent e1000 fix

[David Lang]

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

On Thu, 23 Jun 2005, Jeff Garzik wrote:

> David Lang wrote:
>> 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
>
> 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
>
>
>

-- 
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