lkml.org 
[lkml]   [2004]   [Jan]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] [2.4] forcedeth network driver
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.


>>> +#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.


>>> +/* 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.


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

It's pointless in C, and so I've been stripping such casts out of all
net drivers when I find them.


>>> +/*
>>> + * 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]
>
>>> + 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.

hmmmm, is nForce ever found on non-x86 boxes? I would think that
skb_reserve might be -required- for some platforms.


>> I wonder about calling dev_kfree_skb() from dev->tx_timeout() with
>> dev->xmit_lock held...
>>
>>
> Is that bug in the networking core still not fixed?

I am not aware of a bug in this area.


>>> + /* 2) check that the packets were not sent already: */
>>> + tx_done(dev);
>>
>>
>> bug: tx_done unconditionally calls dev_kfree_skb_irq(), but here we
>> are not in an interrupt.
>>
>>
> What is the xxx_kfree_skb_xxx function that just works?

dev_kfree_skb_any


>>> + /*
>>> + * 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?
>>
>> 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... :)


>>> +/*
>>> + * 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?
>>
> 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.

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


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

Yes, hot unplug or hardware fault.


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

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




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

\
 
 \ /
  Last update: 2005-03-22 14:00    [W:0.046 / U:0.468 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site