[2.4] forcedeth network driver
Subsystems (2)
Quoted 2 lines · Jeff
* Interrupt handler is SCARY. You can potentially take and release the spinlock -many times- during a single interrupt.
I think that can happen only in theory: A new packet is completed while the driver processes rx packets. I all normal cases there should be one spinlock operation per tx irq, and 0 per rx irq. And error handling IMHO doesn't count: it should be rare.
Or do I overlook a common case?
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+#define NV_MIIPHY_DELAY 10 +#define NV_MIIPHY_DELAYMAX 10000Style: it's fairly silly to mix enums and constants.
Right now: enum for the nic registers, #define for the rest. If you don't like it I can change it.
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+/* General driver defaults */ +#define NV_WATCHDOG_TIMEO (2*HZ)this seems too short, and might trigger on normal events?
I think I copied it from another driver - which value would you recommend?
Quoted 10 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+static inline struct fe_priv *get_nvpriv(struct net_device *dev) +{ + return (struct fe_priv *) dev->priv; +}What's the point of this wrapper?
You don't need to cast from a void pointer, either.
I usually try to write code that compiles as cpp - is that a forbidden in kernel modules?
Quoted 2 lines
Locking for this function and
update_linkspeed()is a bit random... sometimes it's called inside the lock, sometimes not.
Should be always inside the lock - I'll check for bugs.
Quoted 12 lines
Review
Quoted 8 lines
(quoted diff) +8
-0+/* + * nic_ioctl: dev->do_ioctl function + * Called with rtnl_lock held. + */ +static int nic_ioctl(struct net_device *dev, struct ifreq *rq, int cmd) +{ + return -EOPNOTSUPP; +}These days, I am very much not interested in merging drivers that do not implement the stupid-simple
ETHTOOL_GDRVINFOioctl.
Ok.
Quoted 8 lines
Quoted 7 lines
(quoted diff) +7
-0+/* + * alloc_rx: fill rx ring entries. + * Return 1 if the allocations for the skbs failed and the + * rx engine is without Available descriptors + */ +static int alloc_rx(struct net_device *dev) +{
[snip]
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ return 0; +}
skb_reserve()seems to be missing
Do you have specs that show that all nForce versions support unaligned buffers? skb_reserve is a performance feature, I don't want to add it yet. Testing that it works is on our TODO list.
Quoted 3 lines
I wonder about calling
dev_kfree_skb()fromdev->tx_timeout()withdev->xmit_lockheld...
Is that bug in the networking core still not fixed?
Quoted 11 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+ np->next_tx++; + + dev->trans_start = jiffies; + if (np->next_tx - np->nic_tx >= TX_LIMIT_STOP) + netif_stop_queue(dev); + spin_unlock_irq(&np->lock); + writel(NVREG_TXRXCTL_KICK, get_hwbase(dev) + NvRegTxRxControl);need
pci_push()here?
Ok.
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ /* 2) check that the packets were not sent already: */ + tx_done(dev);bug:
tx_doneunconditionally callsdev_kfree_skb_irq(), but here we are not in an interrupt.
What is the xxx_kfree_skb_xxx function that just works?
Quoted 11 lines
Review
Quoted 8 lines
(quoted diff) +8
-0+ /* + * the packet is for us - immediately tear down the pci mapping, and + * prefetch the first cacheline of the packet. + */ + pci_unmap_single(np->pci_dev, np->rx_dma[i], + np->rx_skbuff[i]->len, + PCI_DMA_FROMDEVICE); + prefetch(np->rx_skbuff[i]->data);is this just guessing? or has this actually shown some value?
Quoted 3 lines
I would prefer not to put stuff like this in unless it shows a measureable CPU usage or cache miss impact.
Just guessing - it shouldn't hurt. CPU usage won't be important until nForce supports GigE. Should I remove it for now?
Quoted 5 lines
Quoted 5 lines
(quoted diff) +5
-0+/* + * change_mtu: dev->change_mtu function + * Called with dev_base_lock held for read. + */ +static int change_mtu(struct net_device *dev, int new_mtu)
Quoted 9 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+{ + if (new_mtu > DEFAULT_MTU) + return -EINVAL; + dev->mtu = new_mtu; + return 0; +}bug #1: have you tested changing the MTU while the NIC is actually running?
What should the nic do? I'll continue to allocate 1.8 kB buffers because I don't know how to reconfigure the nic hardware to reject large packets.
Quoted 3 lines
bug #2: need a minimum bound for the MTU as well
What is the minimum MTU? I remember a flamewar lkml about 200 byte MTU for noisy radio links.
Quoted 3 lines
bug:
netif_carrier_xxxnot unconditionally initialized inopen()
Ok.
Quoted 3 lines
Further, I would really prefer that you use the lib functions in
drivers/net/mii.cinstead of re-coding this stuff from scratch.
Merging is on my TODO list.
Quoted 11 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+ for (i=0; ; i++) { + events = readl(base + NvRegIrqStatus) & NVREG_IRQSTAT_MASK; + writel(NVREG_IRQSTAT_MASK, base + NvRegIrqStatus); + pci_push(base); + dprintk(KERN_DEBUG "%s: irq: %08x\n", dev->name, events); + if (!(events & np->irqmask)) + break;bug: check for
0xffffffff
What causes 0xfffffff? Hotplug? I think the irq handler could leave immediately if a reserved bit is set. I'll add that.
Quoted 4 lines
Don't reimplement NAPI yourself ;-( this is just duplicating kernel code.... in a worse fashion. At least NAPI can load balance using an idea of overall system load, not just load on a single NIC.
This is not NAPI: on some hardware, the irq bit remained stuck on, forever. Instant lock-up. The code is a workaround against that. I'd like to leave that in - we get a good bug report, instead of a system that doesn't boot.
Quoted 7 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ id2 = mii_rw(dev, i, MII_PHYSID2, MII_READ); + if (id2 < 0) + continue;also check for
0xffffreturned from h/w
Ok.
Quoted 11 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ if (i == 32) { + printk(KERN_INFO "%s: open: failing due to lack of suitable PHY.\n", + dev->name); + ret = -EINVAL; + goto out_drain; + }bug: check #0 after checking #1, before giving up
MII id 0 a valid mii address? Or is that broadcast to all?
Quoted 7 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ if ( (i & NVREG_POWERSTATE_POWEREDUP) == 0) { + writel(NVREG_POWERSTATE_POWEREDUP|i, base + NvRegPowerState); + }style: useless brackets
Ok.
Quoted 9 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ + spin_lock_irq(&np->lock); + np->in_shutdown = 1; + spin_unlock_irq(&np->lock); + synchronize_irq(dev->irq);bug: eliminate
in_shutdown, and directly testnetif_running()elsewhere in code. Again, re-implementing something the net stack already did for you :)
I'll check if I can use netif_running.
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +1
-0+ np->rx_ring = pci_alloc_consistent(pci_dev, sizeof(struct ring_desc) * (RX_RING + \
TX_RING), +&np->ring_addr);(quoted diff) +2
-0+ if (!np->rx_ring) + goto out_unmap;to avoid wasting memory, rx ring should be allocated at ->open() time, and freed at ->close() time. Otherwise this just reserves memory needlessly when the interface is down.
Ok.
Quoted 7 lines
Quoted 7 lines
(quoted diff) +7
-0+out_freering: + pci_free_consistent(np->pci_dev, sizeof(struct ring_desc) * (RX_RING + TX_RING), + np->rx_ring, np->ring_addr); +out_unmap: + iounmap(get_hwbase(dev)); +out_relreg: + pci_release_regions(pci_dev);
Quoted 10 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+out_disable: + pci_disable_device(pci_dev); +out_free: + free_netdev(dev); + pci_set_drvdata(pci_dev, NULL); +out:The order here is weird... when unwinding,
pci_set_drvdata()call should be first, beforepci_free_consistent()
Ok.
Quoted 10 lines
Quoted 5 lines
(quoted diff) +3
-0+static int __init init_nic(void) +{ + printk(KERN_INFO "forcedeth.c: Reverse Engineered nForce ethernet driver. Version \%s.\n",
FORCEDETH_VERSION); + returnpci_module_init(&driver);
+}minor bug: for built-in drivers, only print version if you actually found an interface. For modular drivers, the above is fine.
That's why you see '
printed_version' logic in various net drivers.
Ok.
--
Manfred
Quoted 7 lines · Manfred Spraul
Quoted 2 lines · Jeff
* Interrupt handler is SCARY. You can potentially take and release the spinlock -many times- during a single interrupt.
I think that can happen only in theory: A new packet is completed while the driver processes rx packets. I all normal cases there should be one spinlock operation per tx irq, and 0 per rx irq. And error handling IMHO doesn't count: it should be rare.
Or do I overlook a common case?
Any amount of load at all will lead to multiple interations of the master loop in the interrupt handler.
Quoted 8 lines
Quoted 7 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+#define NV_MIIPHY_DELAY 10 +#define NV_MIIPHY_DELAYMAX 10000Style: it's fairly silly to mix enums and constants.
Right now: enum for the nic registers, #define for the rest. If you don't like it I can change it.
enums are definitely preferred... communicates more type/symbol information to the compiler and more symbol info to the debugger.
Quoted 8 lines
Quoted 7 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+/* General driver defaults */ +#define NV_WATCHDOG_TIMEO (2*HZ)this seems too short, and might trigger on normal events?
I think I copied it from another driver - which value would you recommend?
5 seconds is the norm, but it also depends on whether your link interrupt is 100% reliable. If you don't have to synchronize the link watchdog timeout with other driver-private timers, the task is easier.
Tangent -- you may wish to check for link in ->tx_timeout(), before resetting the NIC. Again, this depends on link interrupt/timer setup as well. The basic point is that TX timeout -may- occur because link went away. One way to handle this is to ensure that link-down events are always noticed before the watchdog timer kicks. Another way to handle this is to simply check for link when ->tx_timeout() is called.
Quoted 8 lines
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+static inline struct fe_priv *get_nvpriv(struct net_device *dev) +{ + return (struct fe_priv *) dev->priv; +}What's the point of this wrapper?
Quoted 4 lines
Quoted 3 lines
You don't need to cast from a void pointer, either.
I usually try to write code that compiles as cpp - is that a forbidden in kernel modules?
It's pointless in C, and so I've been stripping such casts out of all net drivers when I find them.
Quoted 11 lines
Quoted 9 lines
Quoted 7 lines
(quoted diff) +7
-0+/* + * alloc_rx: fill rx ring entries. + * Return 1 if the allocations for the skbs failed and the + * rx engine is without Available descriptors + */ +static int alloc_rx(struct net_device *dev) +{[snip]
Quoted 8 lines
Quoted 7 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ return 0; +}
skb_reserve()seems to be missingDo you have specs that show that all nForce versions support unaligned buffers?
skb_reserveis a performance feature, I don't want to add it yet. Testing that it works is on our TODO list.
hmmmm, is nForce ever found on non-x86 boxes? I would think that skb_reserve might be -required- for some platforms.
Quoted 4 lines
Quoted 3 lines
I wonder about calling
dev_kfree_skb()fromdev->tx_timeout()withdev->xmit_lockheld...Is that bug in the networking core still not fixed?
I am not aware of a bug in this area.
Quoted 8 lines
Quoted 7 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ /* 2) check that the packets were not sent already: */ + tx_done(dev);bug:
tx_doneunconditionally callsdev_kfree_skb_irq(), but here we are not in an interrupt.What is the
xxx_kfree_skb_xxxfunction that just works?
dev_kfree_skb_any
Quoted 11 lines
Quoted 11 lines
Quoted 9 lines
(quoted diff) +2
-0+ /* + * the packet is for us - immediately tear down the pcimapping, and
(quoted diff) +6
-0+ * prefetch the first cacheline of the packet. + */ + pci_unmap_single(np->pci_dev, np->rx_dma[i], + np->rx_skbuff[i]->len, + PCI_DMA_FROMDEVICE); + prefetch(np->rx_skbuff[i]->data);
Quoted 6 lines
Quoted 5 lines
is this just guessing? or has this actually shown some value?
I would prefer not to put stuff like this in unless it shows a measureable CPU usage or cache miss impact.
Just guessing - it shouldn't hurt. CPU usage won't be important until nForce supports GigE. Should I remove it for now?
I would rather remove it. "premature optimization" and all that. Otherwise this guess will be cut-n-pasted into other drivers, I guarantee, all without any verification of the guess... :)
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
(quoted diff) +6
-0+/* + * change_mtu: dev->change_mtu function + * Called with dev_base_lock held for read. + */ +static int change_mtu(struct net_device *dev, int new_mtu) +{Quoted 9 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ if (new_mtu > DEFAULT_MTU) + return -EINVAL; + dev->mtu = new_mtu; + return 0; +}bug #1: have you tested changing the MTU while the NIC is actually running?
Quoted 1 line
What should the nic do? I'll continue to allocate 1.8 kB buffers because I don't know how to reconfigure the nic hardware to reject large packets.
Fair enough. You may wish to (after testing!) increase DEFAULT_MTU by 4 bytes, to support VLAN.
Quoted 4 lines
Quoted 3 lines
bug #2: need a minimum bound for the MTU as well
What is the minimum MTU? I remember a flamewar lkml about 200 byte MTU for noisy radio links.
Usually the ethernet standard 60 is fine.
Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
(quoted diff) +7
-0+ for (i=0; ; i++) { + events = readl(base + NvRegIrqStatus) & NVREG_IRQSTAT_MASK; + writel(NVREG_IRQSTAT_MASK, base + NvRegIrqStatus); + pci_push(base); + dprintk(KERN_DEBUG "%s: irq: %08x\n", dev->name, events); + if (!(events & np->irqmask)) + break;
Quoted 4 lines
Quoted 3 lines
bug: check for
0xffffffffWhat causes
0xfffffff? Hotplug? I think the irq handler could leave immediately if a reserved bit is set. I'll add that.
Yes, hot unplug or hardware fault.
Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
(quoted diff) +2
-0+ if (i == 32) { + printk(KERN_INFO "%s: open: failing due to lack of suitablePHY.\n",
(quoted diff) +4
-0+ dev->name); + ret = -EINVAL; + goto out_drain; + }
Quoted 5 lines
Quoted 4 lines
bug: check #0 after checking #1, before giving up
MII id 0 a valid mii address? Or is that broadcast to all?
It's usually something akin to an alias of one of the other phy id's, but if you found -no- phys at all, it wouldn't hurt to try zero.
Thanks,
Jeff
Quoted 14 lines · Jeff Garzik
Quoted 10 lines · Manfred Spraul
Quoted 3 lines · Jeff
* Interrupt handler is SCARY. You can potentially take and release the spinlock -many times- during a single interrupt.
I think that can happen only in theory: A new packet is completed while the driver processes rx packets. I all normal cases there should be one spinlock operation per tx irq, and 0 per rx irq.
And error handling IMHO doesn't count: it should be rare. Or do I overlook a common case?
Quoted 2 lines
Any amount of load at all will lead to multiple interations of the master loop in the interrupt handler.
+static int max_interrupt_work = 5;
[...]
+ for (i=0; ; i++) {
+ if (i > max_interrupt_work) {
+ spin_lock(&np->lock); + /* disable interrupts on the nic */ + writel(0, base + NvRegIrqMask); + pci_push(base);
+
+ if (!np->in_shutdown)
+ mod_timer(&np->nic_poll, jiffies + POLL_WAIT);
+ printk(KERN_DEBUG "%s: too many iterations (%d) in
nic_irq.\n", dev->name, i);
+ spin_unlock(&np->lock);
+ break;
+ }
So actually it should not happen that often since we exit the master loop after 5 iterations.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+#define NV_MIIPHY_DELAY 10 +#define NV_MIIPHY_DELAYMAX 10000Style: it's fairly silly to mix enums and constants.
Quoted 6 lines
Quoted 2 lines
Right now: enum for the nic registers, #define for the rest. If you don't like it I can change it.
enums are definitely preferred... communicates more type/symbol information to the compiler and more symbol info to the debugger.
The current order has the benefit that the values which might be written to a register (most of which are constants) are located next to the enum for the register. Anyways, I have no strong opinion about it.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+/* General driver defaults */ +#define NV_WATCHDOG_TIMEO (2*HZ)this seems too short, and might trigger on normal events?
Quoted 7 lines
Quoted 2 lines
I think I copied it from another driver - which value would you recommend?
5 seconds is the norm, but it also depends on whether your link interrupt is 100% reliable. If you don't have to synchronize the link watchdog timeout with other driver-private timers, the task is easier.
Hardware availability problem. We always have to assume something works until some user/tester complains.
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
skb_reserve()seems to be missingDo you have specs that show that all nForce versions support unaligned buffers?
skb_reserveis a performance feature, I don't want to add it yet. Testing that it works is on our TODO list.hmmmm, is nForce ever found on non-x86 boxes? I would think that
skb_reservemight be -required- for some platforms.
Since nForce is an Athlon/Athlon64 chipset, I assume it is not found on any non-x86/x86-64 boxes. That might change, however, if NVidia licenses their nic core to other vendors. So far, they have not given any evidence supporting that.
I have seen some hints that earlier nForce versions do NOT support unaligned buffers.
Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
Review
Quoted 11 lines
(quoted diff) +11
-0+/* + * change_mtu: dev->change_mtu function + * Called with dev_base_lock held for read. + */ +static int change_mtu(struct net_device *dev, int new_mtu) +{ + if (new_mtu > DEFAULT_MTU) + return -EINVAL; + dev->mtu = new_mtu; + return 0; +}bug #1: have you tested changing the MTU while the NIC is actually running?
Quoted 3 lines
Quoted 3 lines
What should the nic do? I'll continue to allocate 1.8 kB buffers because I don't know how to reconfigure the nic hardware to reject large packets.
I think the spec says something about it, but I'd have to get my hands on the hardware to test if it actually works.
Quoted 2 lines
Fair enough. You may wish to (after testing!) increase
DEFAULT_MTUby 4 bytes, to support VLAN.
Already tried. If we increase the length of the packet the nic has to send beyond 1500 bytes, we simply get an TX error/timeout. So far, I have not seen any hints that the nic might do VLAN tagging internally.
Jeff, there is another bug in some hardware revisions: For every received packet it reports 1500 bytes length regardless of the real length. We take this len as correct and hope for the best. At best, it ruins our RX statistics. At worst, it might leak kernel memory to userspace since the unused remainder of the 1500 bytes is not initialized with anything, yet passed up with netif_rx(skb). Does the networking core provide any built-in function to address this problem?
+ prd = &np->rx_ring[i];
[...]
+ len = le16_to_cpu(prd->Length);
[...]
+ skb_put(skb, len);
[...]
+ netif_rx(skb);
The above code would be correct if the hardware actually gave us the right values. I know that at least some nForce2 systems suffer from this.
Carl-Daniel
Quoted 40 lines · Carl-Daniel Hailfinger
Quoted 19 lines · Jeff Garzik
Quoted 7 lines · Manfred Spraul
Quoted 3 lines · Jeff
* Interrupt handler is SCARY. You can potentially take and release the spinlock -many times- during a single interrupt.
Quoted 5 lines
I think that can happen only in theory: A new packet is completed while the driver processes rx packets. I all normal cases there should be one spinlock operation per tx irq, and 0 per rx irq.
And error handling IMHO doesn't count: it should be rare. Or do I overlook a common case?Any amount of load at all will lead to multiple interations of the master loop in the interrupt handler.
(quoted diff) +11
-0+static int max_interrupt_work = 5; + for (i=0; ; i++) { + if (i > max_interrupt_work) { + spin_lock(&np->lock); + /* disable interrupts on the nic */ + writel(0, base + NvRegIrqMask); + pci_push(base); + + if (!np->in_shutdown) + mod_timer(&np->nic_poll, jiffies + POLL_WAIT); + printk(KERN_DEBUG "%s: too many iterations (%d) innic_irq.\n",
dev->name, i);(quoted diff) +3
-0+ spin_unlock(&np->lock); + break; + }
Quoted 2 lines
So actually it should not happen that often since we exit the master loop after 5 iterations.
<shrug> Well, it's not a must-change item IMO
Quoted 13 lines
Quoted 13 lines
Quoted 11 lines
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+#define NV_MIIPHY_DELAY 10 +#define NV_MIIPHY_DELAYMAX 10000Style: it's fairly silly to mix enums and constants.
Right now: enum for the nic registers, #define for the rest. If you don't like it I can change it.
Quoted 7 lines
Quoted 2 lines
enums are definitely preferred... communicates more type/symbol information to the compiler and more symbol info to the debugger.
The current order has the benefit that the values which might be written to a register (most of which are constants) are located next to the enum for the register.
You can do this with enums, too :)
Quoted 7 lines
Quoted 2 lines
Fair enough. You may wish to (after testing!) increase
DEFAULT_MTUby 4 bytes, to support VLAN.Already tried. If we increase the length of the packet the nic has to send beyond 1500 bytes, we simply get an TX error/timeout. So far, I have not seen any hints that the nic might do VLAN tagging internally.
The current limit sounds correct, then.
Quoted 16 lines
Jeff, there is another bug in some hardware revisions: For every received packet it reports 1500 bytes length regardless of the real length. We take this len as correct and hope for the best. At best, it ruins our RX statistics. At worst, it might leak kernel memory to userspace since the unused remainder of the 1500 bytes is not initialized with anything, yet passed up with
netif_rx(skb). Does the networking core provide any built-in function to address this problem?(quoted diff) +4
-0+ prd = &np->rx_ring[i]; + len = le16_to_cpu(prd->Length); + skb_put(skb, len); + netif_rx(skb);
Quoted 2 lines
The above code would be correct if the hardware actually gave us the right values. I know that at least some nForce2 systems suffer from this.
Peek at the ethernet frame header and see what it thinks the payload length is? If less than 1500...
Jeff
Quoted 6 lines · Jeff Garzik
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
(quoted diff) +2
-0+static int alloc_rx(struct net_device *dev) +{[snip]
Quoted 3 lines
Quoted 2 lines
(quoted diff) +2
-0+ return 0; +}
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
skb_reserve()seems to be missingDo you have specs that show that all nForce versions support unaligned buffers?
skb_reserveis a performance feature, I don't want to add it yet. Testing that it works is on our TODO list.hmmmm, is nForce ever found on non-x86 boxes? I would think that
skb_reservemight be -required- for some platforms.
AMD64 and PPC64 as far as I know. But you may consider the first one still a x86 box.
Signature · 2 lines
Vojtech Pavlik SuSE Labs, SuSE CR
Quoted 17 lines · Vojtech Pavlik
Quoted 15 lines · Jeff Garzik
Quoted 14 lines
Quoted 2 lines
Quoted 2 lines
(quoted diff) +2
-0+static int alloc_rx(struct net_device *dev) +{[snip]
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ return 0; +}
skb_reserve()seems to be missingDo you have specs that show that all nForce versions support unaligned buffers?
skb_reserveis a performance feature, I don't want to add it yet. Testing that it works is on our TODO list.
Quoted 6 lines
Quoted 2 lines
hmmmm, is nForce ever found on non-x86 boxes? I would think that
skb_reservemight be -required- for some platforms.AMD64 and PPC64 as far as I know. But you may consider the first one still a x86 box.
Hmmm. I thought only GeForce graphics were available on PPC64 and nForce mainboard chipsets (including the onboard nic) were not.
Carl-Daniel
Signature · 1 line
http://www.hailfinger.org/
Quoted 8 lines · Carl-Daniel Hailfinger
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Do you have specs that show that all nForce versions support unaligned buffers?
skb_reserveis a performance feature, I don't want to add it yet. Testing that it works is on our TODO list.hmmmm, is nForce ever found on non-x86 boxes? I would think that
skb_reservemight be -required- for some platforms.
Quoted 5 lines
Quoted 2 lines
AMD64 and PPC64 as far as I know. But you may consider the first one still a x86 box.
Hmmm. I thought only GeForce graphics were available on PPC64 and nForce mainboard chipsets (including the onboard nic) were not.
Well, my memory may be tricking me (I'm not really sure about this), but I remember there was supposed to be a PPC64 northbridge with a HT link, made exactly for the purpose of connecting an nForce southrbridge to it. But it definitely is not in production yet.
Signature · 2 lines
Vojtech Pavlik SuSE Labs, SuSE CR
Quoted 4 lines · Vojtech Pavlik
Well, my memory may be tricking me (I'm not really sure about this), but I remember there was supposed to be a PPC64 northbridge with a HT link, made exactly for the purpose of connecting an nForce southrbridge to it. But it definitely is not in production yet.
The U3 northbridge in the Apple G5 powermac has a HT link coming out of it, which connects via an AMD PCI-X tunnel chip to an Apple K2 southbridge. I have never heard of anyone using (or planning to use) an nForce southbridge on a PPC64 system though.
Regards,
Paul.
Quoted 20 lines · Jeff Garzik
Quoted 19 lines · Manfred Spraul
Quoted 17 lines · Jeff
Review
Quoted 9 lines
(quoted diff) +2
-0+ /* + * the packet is for us - immediately tear down the pcimapping, and
(quoted diff) +6
-0+ * prefetch the first cacheline of the packet. + */ + pci_unmap_single(np->pci_dev, np->rx_dma[i], + np->rx_skbuff[i]->len, + PCI_DMA_FROMDEVICE); + prefetch(np->rx_skbuff[i]->data);is this just guessing? or has this actually shown some value?
I would prefer not to put stuff like this in unless it shows a measureable CPU usage or cache miss impact.
Quoted 6 lines
Quoted 2 lines
Just guessing - it shouldn't hurt. CPU usage won't be important until nForce supports GigE. Should I remove it for now?
I would rather remove it. "premature optimization" and all that. Otherwise this guess will be cut-n-pasted into other drivers, I guarantee, all without any verification of the guess... :)
There was a thread about using prefetch() on netdev or linux-net. The idea is to have the Ethernet header prefetched by the time eth_type_trans() needs it.
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
bug #2: need a minimum bound for the MTU as well
What is the minimum MTU? I remember a flamewar lkml about 200 byte MTU for noisy radio links.
Usually the ethernet standard 60 is fine.
Minimum MTU != minimum frame length. An MTU of 0 should work fine. It is just not very useful in most cases to send just the Ethernet header but no payload.
Signature · 2 lines
Regards, Rask Ingemann Lambertsen