@@ -1963,6 +1963,11 @@ static int velocity_change_mtu(struct nereturn-EINVAL;}+if(!netif_running(dev)){+dev->mtu=new_mtu;+return0;+}+if(new_mtu!=oldmtu){spin_lock_irqsave(&vptr->lock,flags);
Shouldn't this latter 'if' be removed now, btw?
No, it makes sense that if mtu is same, no action need be taken.
Actually, it would make sense to push the same check up into
the netdevice core management.
--
Stephen Hemminger [off-list ref]
@@ -1963,6 +1963,11 @@ static int velocity_change_mtu(struct nereturn-EINVAL;}+if(!netif_running(dev)){+dev->mtu=new_mtu;+return0;+}+if(new_mtu!=oldmtu){spin_lock_irqsave(&vptr->lock,flags);
Shouldn't this latter 'if' be removed now, btw?
No, it makes sense that if mtu is same, no action need be taken.
Actually, it would make sense to push the same check up into
the netdevice core management.
Sure! I was only worried velocity_open() treats dev->mtu
a bit different than velocity_change_mtu(), so eg. after:
velocity_change_mtu() // dev is down
velocity_open()
velocity_change_mtu() // dev is up
with the same mtu, vptr->rx_buf_sz could be different than after:
velocity_open()
velocity_change_mtu() // dev is up
But, probably, I miss someting.
Thanks,
Jarek P.
From: Jon Nelson <hidden> Date: 2007-11-16 02:43:04
On 11/15/07, Jarek Poplawski [off-list ref] wrote:
..
Sure! I was only worried velocity_open() treats dev->mtu
a bit different than velocity_change_mtu(), so eg. after:
velocity_change_mtu() // dev is down
velocity_open()
velocity_change_mtu() // dev is up
with the same mtu, vptr->rx_buf_sz could be different than after:
velocity_open()
velocity_change_mtu() // dev is up
But, probably, I miss someting.
There is a snag here: if I change the MTU after the device is UP
something ends up rather broken. A tcpdump shows nearly every outgoing
frame has a bad TCP checksum (and the card does not support H/W
checksumming or it is turned off as reported by ethtool).
--
Jon
From: Stephen Hemminger <hidden> Date: 2007-11-16 04:07:13
On Thu, 15 Nov 2007 20:42:55 -0600
"Jon Nelson" [off-list ref] wrote:
On 11/15/07, Jarek Poplawski [off-list ref] wrote:
..
quoted
Sure! I was only worried velocity_open() treats dev->mtu
a bit different than velocity_change_mtu(), so eg. after:
velocity_change_mtu() // dev is down
velocity_open()
velocity_change_mtu() // dev is up
with the same mtu, vptr->rx_buf_sz could be different than after:
velocity_open()
velocity_change_mtu() // dev is up
But, probably, I miss someting.
There is a snag here: if I change the MTU after the device is UP
something ends up rather broken. A tcpdump shows nearly every outgoing
frame has a bad TCP checksum (and the card does not support H/W
checksumming or it is turned off as reported by ethtool).
That is a different (and pre-existing bug).
--
Stephen Hemminger [off-list ref]
From: Stephen Hemminger <hidden> Date: 2007-11-16 04:37:29
On Thu, 15 Nov 2007 20:42:55 -0600
"Jon Nelson" [off-list ref] wrote:
On 11/15/07, Jarek Poplawski [off-list ref] wrote:
..
quoted
Sure! I was only worried velocity_open() treats dev->mtu
a bit different than velocity_change_mtu(), so eg. after:
velocity_change_mtu() // dev is down
velocity_open()
velocity_change_mtu() // dev is up
with the same mtu, vptr->rx_buf_sz could be different than after:
velocity_open()
velocity_change_mtu() // dev is up
But, probably, I miss someting.
There is a snag here: if I change the MTU after the device is UP
something ends up rather broken. A tcpdump shows nearly every outgoing
frame has a bad TCP checksum (and the card does not support H/W
checksumming or it is turned off as reported by ethtool).
Does this fix the problem.
Note: reading the code the driver has other problems (besides crappy
style). It does pci_map_single()/unmap in a way that doesn't account
correctly for the padding that was added.
Yes. The new patch adds just recomputes the rx buf size in
one place, so it can be set when device is down.
Also, the computation will always yield same result now, the old
code computed different values depending on whether it was initial
or changed MTU.
--
Stephen Hemminger [off-list ref]
Yes. The new patch adds just recomputes the rx buf size in
one place, so it can be set when device is down.
Also, the computation will always yield same result now, the old
code computed different values depending on whether it was initial
or changed MTU.
But with your patch he is getting an OOPS (he described it as
a 'segmentation fault' which is why you perhaps missed it)
so you should try to figure out why so we can fix this bug.
:-)
Thanks.
Yes. The new patch adds just recomputes the rx buf size in
one place, so it can be set when device is down.
Also, the computation will always yield same result now, the old
code computed different values depending on whether it was initial
or changed MTU.
OK. This is what I did.
Using git I grabbed a copy of Linus' tree and using the latest files
for via-velocity.[c,h], commit
99fee6d7e5748d96884667a4628118f7fc130ea0, I determined that if I
backed out change 44c10138fd4bbc4b6d6bff0873c24902f2a9da65 (PCI:
Change all drivers to use pci_device->revision) I could get it to
compile. This gets me a more recent driver.
Then I applied both of the patches you have provided me, and built and
tried that.
No sigseg, no oops on initial MTU, no sigseg or oops on subsequent MTU changes.
Performance appears to be worse after MTU changes, though. I can get
about 30MB/s with 7200 (the largest the board will support?) and about
5MB/s with 1500. If I *start out* with 1500 performance is 35-40MB/s.
However, the immediate issue appears to be solved (oopses).
If there is a better tree to pull from, I'm more than willing to keep
trying different drivers, at least for a while. I also have tg3 driver
issues to sort out.
--
Jon
OK. This is what I did.
Using git I grabbed a copy of Linus' tree and using the latest files
for via-velocity.[c,h], commit
99fee6d7e5748d96884667a4628118f7fc130ea0, I determined that if I
backed out change 44c10138fd4bbc4b6d6bff0873c24902f2a9da65 (PCI:
Change all drivers to use pci_device->revision) I could get it to
compile. This gets me a more recent driver.
Then I applied both of the patches you have provided me, and built and
tried that.
No sigseg, no oops on initial MTU, no sigseg or oops on subsequent MTU changes.
Good news! But, if I got it right your method could be tricky. These
current via-velocity files could depend on other files being current
as well. So, it's safer to use the whole new kernel (eg. 2.6.24-rc3)
or to stay with your older one. But, if you want to check the effect
of these new two patches only without any additional 'features', your
older kernel should be a better choice.
Regards,
Jarek P.
From: Jon Nelson <hidden> Date: 2007-11-28 22:02:54
Just a reminder: the following fix should (IMO) be considered for
2.6.24. The first patch does indeed fix the OOPS on MTU however
subsequent MTU changes still OOPS. The below fixes that. I've been
using both patches for over a week and they work just great. I noticed
that the first fix made 2.6.24 but this one hadn't - I just want to
try to make sure 2.6.24 doesn't make things /worse/ for via-velocity
owners instead of better.
Stephen Hemminger [off-list ref] wrote:
quoted hunk
Does this fix the problem.
Note: reading the code the driver has other problems (besides crappy
style). It does pci_map_single()/unmap in a way that doesn't account
correctly for the padding that was added.
From: Stephen Hemminger <hidden> Date: 2007-11-28 22:21:54
The VIA veloicty driver needs the following to allow changing MTU when down.
The buffer size needs to be computed when device is brought up, not when
device is initialized. This also fixes a bug where the buffer size was
computed differently on change_mtu versus initial setting.
Signed-off-by: Stephen Hemminger <redacted>
---
This is a properly formatted version of previously submitted patch.
Please apply for 2.6.24
From: Jeff Garzik <hidden> Date: 2007-12-01 21:36:23
Stephen Hemminger wrote:
The VIA veloicty driver needs the following to allow changing MTU when down.
The buffer size needs to be computed when device is brought up, not when
device is initialized. This also fixes a bug where the buffer size was
computed differently on change_mtu versus initial setting.
Signed-off-by: Stephen Hemminger <redacted>
---
This is a properly formatted version of previously submitted patch.
Please apply for 2.6.24