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

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

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

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

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

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

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

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

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

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

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

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

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

>bug: netif_carrier_xxx not unconditionally initialized in open()
>
>
Ok.

>Further, I would really prefer that you use the lib functions in
>drivers/net/mii.c instead of re-coding this stuff from scratch.
>
>
Merging is on my TODO list.

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

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

>> + id2 = mii_rw(dev, i, MII_PHYSID2, MII_READ);
>> + if (id2 < 0)
>> + continue;
>
>also check for 0xffff returned from h/w
>
>
Ok.

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

>> + if ( (i & NVREG_POWERSTATE_POWEREDUP) == 0) {
>> + writel(NVREG_POWERSTATE_POWEREDUP|i, base + NvRegPowerState);
>> + }
>
>style: useless brackets
>
>
Ok.

>> +
>> + spin_lock_irq(&np->lock);
>> + np->in_shutdown = 1;
>> + spin_unlock_irq(&np->lock);
>> + synchronize_irq(dev->irq);
>
>bug: eliminate in_shutdown, and directly test netif_running() elsewhere
>in code. Again, re-implementing something the net stack already did for
>you :)
>
>
I'll check if I can use netif_running.

>> + np->rx_ring = pci_alloc_consistent(pci_dev, sizeof(struct ring_desc) * (RX_RING + \
>> TX_RING), + &np->ring_addr);
>> + 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.

>> +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);
>> +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, before pci_free_consistent()
>
>
Ok.

>> +static int __init init_nic(void)
>> +{
>> + printk(KERN_INFO "forcedeth.c: Reverse Engineered nForce ethernet driver. Version \
>> %s.\n", FORCEDETH_VERSION); + return pci_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


-
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.047 / U:23.440 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site