Re: [PATCH] [2.4] forcedeth network driver

[Manfred Spraul]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/