Messages in this thread | | | Date | Sat, 24 Jan 2004 18:57:08 -0500 | From | Jeff Garzik <> | Subject | Re: [PATCH] [2.4] forcedeth network driver |
| |
Carl-Daniel Hailfinger wrote: > Jeff Garzik wrote: > >>Manfred Spraul wrote: >> >> >>>Jeff wrote: >>> >>> >>>>* 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. > > > +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.
<shrug> Well, it's not a must-change item IMO
>>>>>+#define NV_MIIPHY_DELAY 10 >>>>>+#define NV_MIIPHY_DELAYMAX 10000 >>>> >>>> >>>> >>>>Style: 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. > > > 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 :)
>>Fair enough. You may wish to (after testing!) increase DEFAULT_MTU by 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.
> 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.
Peek at the ethernet frame header and see what it thinks the payload length is? If less than 1500...
Jeff
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |