Re: [PATCH] [2.4] forcedeth network driver

[Jeff Garzik <jgarzik@xxxxxxxxx>]

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