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
Carl-Daniel Hailfinger wrote:
> Marcelo,
>
> attached is the current version of forcedeth, a network driver for
> nForce{,2,3} chipsets which are fairly common today.
> So far, the only support for nForce chipsets has been a binary-only driver
> from NVidia.

> The patch is against your current bk tree.
> Please apply.

I wish you'd mailed me first... let's not be so hasty.


General comments:

* This was suggested (and ignored), but IMO needs to be done -- the
function names are -way- too generic. If this driver oops's, people
might see an oops in "open" or "close" functions, which is fairly
useless information unless correlated with other info. Please follow
style and add a driver-specific prefix to your function names.

* Interrupt handler is SCARY. You can potentially take and release the
spinlock -many times- during a single interrupt.


> +#if (LINUX_VERSION_CODE < KERNEL_VERSION(2,5,28))
> +static inline void synchronize_irq_wrapper(unsigned int irq) { synchronize_irq(); }
> +#undef synchronize_irq
> +#define synchronize_irq(irq) synchronize_irq_wrapper(irq)
> +#endif

Just introduce a diff between 2.4 and 2.6 versions.


> +/*
> + * Hardware access:
> + */
> +
> +#define DEV_NEED_LASTPACKET1 0x0001
> +#define DEV_IRQMASK_1 0x0002
> +#define DEV_IRQMASK_2 0x0004
> +#define DEV_NEED_TIMERIRQ 0x0008
> +
> +enum {
> + NvRegIrqStatus = 0x000,
> +#define NVREG_IRQSTAT_MIIEVENT 0x040
> +#define NVREG_IRQSTAT_MASK 0x1ff
> + NvRegIrqMask = 0x004,
> +#define NVREG_IRQ_RX 0x0002
> +#define NVREG_IRQ_RX_NOBUF 0x0004
> +#define NVREG_IRQ_TX_ERR 0x0008
> +#define NVREG_IRQ_TX2 0x0010
> +#define NVREG_IRQ_TIMER 0x0020
> +#define NVREG_IRQ_LINK 0x0040
> +#define NVREG_IRQ_TX1 0x0100
> +#define NVREG_IRQMASK_WANTED_1 0x005f
> +#define NVREG_IRQMASK_WANTED_2 0x0147
> +#define NVREG_IRQ_UNKNOWN (~(NVREG_IRQ_RX|NVREG_IRQ_RX_NOBUF|NVREG_IRQ_TX_ERR|NVREG_IRQ_TX2|NVREG_IRQ_TIMER|NVREG_IRQ_LINK|NVREG_IRQ_TX1))
> +
> + NvRegUnknownSetupReg6 = 0x008,
> +#define NVREG_UNKSETUP6_VAL 3
> +
> +/*
> + * NVREG_POLL_DEFAULT is the interval length of the timer source on the nic
> + * NVREG_POLL_DEFAULT=97 would result in an interval length of 1 ms
> + */
> + NvRegPollingInterval = 0x00c,
> +#define NVREG_POLL_DEFAULT 970
> + NvRegMisc1 = 0x080,
> +#define NVREG_MISC1_HD 0x02
> +#define NVREG_MISC1_FORCE 0x3b0f3c
> +
> + NvRegTransmitterControl = 0x084,
> +#define NVREG_XMITCTL_START 0x01
> + NvRegTransmitterStatus = 0x088,
> +#define NVREG_XMITSTAT_BUSY 0x01
> +
> + NvRegPacketFilterFlags = 0x8c,
> +#define NVREG_PFF_ALWAYS 0x7F0008
> +#define NVREG_PFF_PROMISC 0x80
> +#define NVREG_PFF_MYADDR 0x20
> +
> + NvRegOffloadConfig = 0x90,
> +#define NVREG_OFFLOAD_HOMEPHY 0x601
> +#define NVREG_OFFLOAD_NORMAL 0x5ee
> + NvRegReceiverControl = 0x094,
> +#define NVREG_RCVCTL_START 0x01
> + NvRegReceiverStatus = 0x98,
> +#define NVREG_RCVSTAT_BUSY 0x01
> +
> + NvRegRandomSeed = 0x9c,
> +#define NVREG_RNDSEED_MASK 0x00ff
> +#define NVREG_RNDSEED_FORCE 0x7f00
> +
> + NvRegUnknownSetupReg1 = 0xA0,
> +#define NVREG_UNKSETUP1_VAL 0x16070f
> + NvRegUnknownSetupReg2 = 0xA4,
> +#define NVREG_UNKSETUP2_VAL 0x16
> + NvRegMacAddrA = 0xA8,
> + NvRegMacAddrB = 0xAC,
> + NvRegMulticastAddrA = 0xB0,
> +#define NVREG_MCASTADDRA_FORCE 0x01
> + NvRegMulticastAddrB = 0xB4,
> + NvRegMulticastMaskA = 0xB8,
> + NvRegMulticastMaskB = 0xBC,
> +
> + NvRegTxRingPhysAddr = 0x100,
> + NvRegRxRingPhysAddr = 0x104,
> + NvRegRingSizes = 0x108,
> +#define NVREG_RINGSZ_TXSHIFT 0
> +#define NVREG_RINGSZ_RXSHIFT 16
> + NvRegUnknownTransmitterReg = 0x10c,
> + NvRegLinkSpeed = 0x110,
> +#define NVREG_LINKSPEED_FORCE 0x10000
> +#define NVREG_LINKSPEED_10 10
> +#define NVREG_LINKSPEED_100 100
> +#define NVREG_LINKSPEED_1000 1000
> + NvRegUnknownSetupReg5 = 0x130,
> +#define NVREG_UNKSETUP5_BIT31 (1<<31)
> + NvRegUnknownSetupReg3 = 0x134,
> +#define NVREG_UNKSETUP3_VAL1 0x200010
> + NvRegTxRxControl = 0x144,
> +#define NVREG_TXRXCTL_KICK 0x0001
> +#define NVREG_TXRXCTL_BIT1 0x0002
> +#define NVREG_TXRXCTL_BIT2 0x0004
> +#define NVREG_TXRXCTL_IDLE 0x0008
> +#define NVREG_TXRXCTL_RESET 0x0010
> + NvRegMIIStatus = 0x180,
> +#define NVREG_MIISTAT_ERROR 0x0001
> +#define NVREG_MIISTAT_LINKCHANGE 0x0008
> +#define NVREG_MIISTAT_MASK 0x000f
> +#define NVREG_MIISTAT_MASK2 0x000f
> + NvRegUnknownSetupReg4 = 0x184,
> +#define NVREG_UNKSETUP4_VAL 8
> +
> + NvRegAdapterControl = 0x188,
> +#define NVREG_ADAPTCTL_START 0x02
> +#define NVREG_ADAPTCTL_LINKUP 0x04
> +#define NVREG_ADAPTCTL_PHYVALID 0x4000
> +#define NVREG_ADAPTCTL_RUNNING 0x100000
> +#define NVREG_ADAPTCTL_PHYSHIFT 24
> + NvRegMIISpeed = 0x18c,
> +#define NVREG_MIISPEED_BIT8 (1<<8)
> +#define NVREG_MIIDELAY 5
> + NvRegMIIControl = 0x190,
> +#define NVREG_MIICTL_INUSE 0x10000
> +#define NVREG_MIICTL_WRITE 0x08000
> +#define NVREG_MIICTL_ADDRSHIFT 5
> + NvRegMIIData = 0x194,
> + NvRegWakeUpFlags = 0x200,
> +#define NVREG_WAKEUPFLAGS_VAL 0x7770
> +#define NVREG_WAKEUPFLAGS_BUSYSHIFT 24
> +#define NVREG_WAKEUPFLAGS_ENABLESHIFT 16
> +#define NVREG_WAKEUPFLAGS_D3SHIFT 12
> +#define NVREG_WAKEUPFLAGS_D2SHIFT 8
> +#define NVREG_WAKEUPFLAGS_D1SHIFT 4
> +#define NVREG_WAKEUPFLAGS_D0SHIFT 0
> +#define NVREG_WAKEUPFLAGS_ACCEPT_MAGPAT 0x01
> +#define NVREG_WAKEUPFLAGS_ACCEPT_WAKEUPPAT 0x02
> +#define NVREG_WAKEUPFLAGS_ACCEPT_LINKCHANGE 0x04
> +
> + NvRegPatternCRC = 0x204,
> + NvRegPatternMask = 0x208,
> + NvRegPowerCap = 0x268,
> +#define NVREG_POWERCAP_D3SUPP (1<<30)
> +#define NVREG_POWERCAP_D2SUPP (1<<26)
> +#define NVREG_POWERCAP_D1SUPP (1<<25)
> + NvRegPowerState = 0x26c,
> +#define NVREG_POWERSTATE_POWEREDUP 0x8000
> +#define NVREG_POWERSTATE_VALID 0x0100
> +#define NVREG_POWERSTATE_MASK 0x0003
> +#define NVREG_POWERSTATE_D0 0x0000
> +#define NVREG_POWERSTATE_D1 0x0001
> +#define NVREG_POWERSTATE_D2 0x0002
> +#define NVREG_POWERSTATE_D3 0x0003
> +};
> +
> +struct ring_desc {
> + u32 PacketBuffer;
> + u16 Length;
> + u16 Flags;
> +};
> +
> +#define NV_TX_LASTPACKET (1<<0)
> +#define NV_TX_RETRYERROR (1<<3)
> +#define NV_TX_LASTPACKET1 (1<<8)
> +#define NV_TX_DEFERRED (1<<10)
> +#define NV_TX_CARRIERLOST (1<<11)
> +#define NV_TX_LATECOLLISION (1<<12)
> +#define NV_TX_UNDERFLOW (1<<13)
> +#define NV_TX_ERROR (1<<14)
> +#define NV_TX_VALID (1<<15)
> +
> +#define NV_RX_DESCRIPTORVALID (1<<0)
> +#define NV_RX_MISSEDFRAME (1<<1)
> +#define NV_RX_SUBSTRACT1 (1<<3)
> +#define NV_RX_ERROR1 (1<<7)
> +#define NV_RX_ERROR2 (1<<8)
> +#define NV_RX_ERROR3 (1<<9)
> +#define NV_RX_ERROR4 (1<<10)
> +#define NV_RX_CRCERR (1<<11)
> +#define NV_RX_OVERFLOW (1<<12)
> +#define NV_RX_FRAMINGERR (1<<13)
> +#define NV_RX_ERROR (1<<14)
> +#define NV_RX_AVAIL (1<<15)
> +
> +/* Miscelaneous hardware related defines: */
> +#define NV_PCI_REGSZ 0x270
> +
> +/* various timeout delays: all in usec */
> +#define NV_TXRX_RESET_DELAY 4
> +#define NV_TXSTOP_DELAY1 10
> +#define NV_TXSTOP_DELAY1MAX 500000
> +#define NV_TXSTOP_DELAY2 100
> +#define NV_RXSTOP_DELAY1 10
> +#define NV_RXSTOP_DELAY1MAX 500000
> +#define NV_RXSTOP_DELAY2 100
> +#define NV_SETUP5_DELAY 5
> +#define NV_SETUP5_DELAYMAX 50000
> +#define NV_POWERUP_DELAY 5
> +#define NV_POWERUP_DELAYMAX 5000
> +#define NV_MIIBUSY_DELAY 50
> +#define NV_MIIPHY_DELAY 10
> +#define NV_MIIPHY_DELAYMAX 10000

Style: it's fairly silly to mix enums and constants.

(and you want to prefer enums, because they convey more type information
to the compiler and symbol information to the debugger)


> +#define NV_WAKEUPPATTERNS 5
> +#define NV_WAKEUPMASKENTRIES 4
> +
> +/* General driver defaults */
> +#define NV_WATCHDOG_TIMEO (2*HZ)

this seems too short, and might trigger on normal events?


> +#define DEFAULT_MTU 1500 /* also maximum supported, at least for now */
> +
> +#define RX_RING 128
> +#define TX_RING 16
> +/* limited to 1 packet until we understand NV_TX_LASTPACKET */
> +#define TX_LIMIT_STOP 10
> +#define TX_LIMIT_START 5
> +
> +/* rx/tx mac addr + type + vlan + align + slack*/
> +#define RX_NIC_BUFSIZE (DEFAULT_MTU + 64)
> +/* even more slack */
> +#define RX_ALLOC_BUFSIZE (DEFAULT_MTU + 128)
> +
> +#define OOM_REFILL (1+HZ/20)
> +#define POLL_WAIT (1+HZ/100)
> +
> +/*
> + * SMP locking:
> + * All hardware access under dev->priv->lock, except the performance
> + * critical parts:
> + * - rx is (pseudo-) lockless: it relies on the single-threading provided
> + * by the arch code for interrupts.
> + * - tx setup is lockless: it relies on dev->xmit_lock. Actual submission
> + * needs dev->priv->lock :-(
> + * - set_multicast_list: preparation lockless, relies on dev->xmit_lock.
> + */
> +
> +/* in dev: base, irq */
> +struct fe_priv {
> + spinlock_t lock;
> +
> + /* General data:
> + * Locking: spin_lock(&np->lock); */
> + struct net_device_stats stats;
> + int in_shutdown;
> + u32 linkspeed;
> + int duplex;
> + int phyaddr;
> +
> + /* General data: RO fields */
> + dma_addr_t ring_addr;
> + struct pci_dev *pci_dev;
> + u32 orig_mac[2];
> + u32 irqmask;
> +
> + /* rx specific fields.
> + * Locking: Within irq hander or disable_irq+spin_lock(&np->lock);
> + */
> + struct ring_desc *rx_ring;
> + unsigned int cur_rx, refill_rx;
> + struct sk_buff *rx_skbuff[RX_RING];
> + dma_addr_t rx_dma[RX_RING];
> + unsigned int rx_buf_sz;
> + struct timer_list oom_kick;
> + struct timer_list nic_poll;
> +
> + /*
> + * tx specific fields.
> + */
> + struct ring_desc *tx_ring;
> + unsigned int next_tx, nic_tx;
> + struct sk_buff *tx_skbuff[TX_RING];
> + dma_addr_t tx_dma[TX_RING];
> + u16 tx_flags;
> +};
> +
> +/*
> + * Maximum number of loops until we assume that a bit in the irq mask
> + * is stuck. Overridable with module param.
> + */
> +static int max_interrupt_work = 5;
> +
> +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.


> +static int reg_delay(struct net_device *dev, int offset, u32 mask, u32 target,
> + int delay, int delaymax, const char *msg)
> +{
> + u8 *base = get_hwbase(dev);
> +
> + pci_push(base);
> + do {
> + udelay(delay);
> + delaymax -= delay;
> + if (delaymax < 0) {
> + if (msg)
> + printk(msg);
> + return 1;
> + }
> + } while ((readl(base + offset) & mask) != target);
> + return 0;
> +}
> +
> +#define MII_READ (-1)

MII_READ is used by callers, and shouldn't be buried near the function
itself.


> +/* mii_rw: read/write a register on the PHY.
> + *
> + * Caller must guarantee serialization
> + */
> +static int mii_rw(struct net_device *dev, int addr, int miireg, int value)
> +{
> + u8 *base = get_hwbase(dev);
> + int was_running;
> + u32 reg;
> + int retval;
> +
> + writel(NVREG_MIISTAT_MASK, base + NvRegMIIStatus);
> + was_running = 0;
> + reg = readl(base + NvRegAdapterControl);
> + if (reg & NVREG_ADAPTCTL_RUNNING) {
> + was_running = 1;
> + writel(reg & ~NVREG_ADAPTCTL_RUNNING, base + NvRegAdapterControl);
> + }
> + reg = readl(base + NvRegMIIControl);
> + if (reg & NVREG_MIICTL_INUSE) {
> + writel(NVREG_MIICTL_INUSE, base + NvRegMIIControl);
> + udelay(NV_MIIBUSY_DELAY);
> + }
> +
> + reg = NVREG_MIICTL_INUSE | (addr << NVREG_MIICTL_ADDRSHIFT) | miireg;
> + if (value != MII_READ) {
> + writel(value, base + NvRegMIIData);
> + reg |= NVREG_MIICTL_WRITE;
> + }
> + writel(reg, base + NvRegMIIControl);
> +
> + if (reg_delay(dev, NvRegMIIControl, NVREG_MIICTL_INUSE, 0,
> + NV_MIIPHY_DELAY, NV_MIIPHY_DELAYMAX, NULL)) {
> + dprintk(KERN_DEBUG "%s: mii_rw of reg %d at PHY %d timed out.\n",
> + dev->name, miireg, addr);
> + retval = -1;
> + } else if (value != MII_READ) {
> + /* it was a write operation - fewer failures are detectable */
> + dprintk(KERN_DEBUG "%s: mii_rw wrote 0x%x to reg %d at PHY %d\n",
> + dev->name, value, miireg, addr);
> + retval = 0;
> + } else if (readl(base + NvRegMIIStatus) & NVREG_MIISTAT_ERROR) {
> + dprintk(KERN_DEBUG "%s: mii_rw of reg %d at PHY %d failed.\n",
> + dev->name, miireg, addr);
> + retval = -1;
> + } else {
> + /* FIXME: why is that required? */
> + udelay(50);
> + retval = readl(base + NvRegMIIData);
> + dprintk(KERN_DEBUG "%s: mii_rw read from reg %d at PHY %d: 0x%x.\n",
> + dev->name, miireg, addr, retval);
> + }
> + if (was_running) {
> + reg = readl(base + NvRegAdapterControl);
> + writel(reg | NVREG_ADAPTCTL_RUNNING, base + NvRegAdapterControl);
> + }
> + return retval;
> +}

Locking for this function and update_linkspeed() is a bit random...
sometimes it's called inside the lock, sometimes not.


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


> +/*
> + * 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)
> +{
> + struct fe_priv *np = get_nvpriv(dev);
> + unsigned int refill_rx = np->refill_rx;
> +
> + while (np->cur_rx != refill_rx) {
> + int nr = refill_rx % RX_RING;
> + struct sk_buff *skb;
> +
> + if (np->rx_skbuff[nr] == NULL) {
> +
> + skb = dev_alloc_skb(RX_ALLOC_BUFSIZE);
> + if (!skb)
> + break;
> +
> + skb->dev = dev;
> + np->rx_skbuff[nr] = skb;
> + } else {
> + skb = np->rx_skbuff[nr];
> + }
> + np->rx_dma[nr] = pci_map_single(np->pci_dev, skb->data, skb->len,
> + PCI_DMA_FROMDEVICE);
> + np->rx_ring[nr].PacketBuffer = cpu_to_le32(np->rx_dma[nr]);
> + np->rx_ring[nr].Length = cpu_to_le16(RX_NIC_BUFSIZE);
> + wmb();
> + np->rx_ring[nr].Flags = cpu_to_le16(NV_RX_AVAIL);
> + dprintk(KERN_DEBUG "%s: alloc_rx: Packet %d marked as Available\n",
> + dev->name, refill_rx);
> + refill_rx++;
> + }
> + np->refill_rx = refill_rx;
> + if (np->cur_rx - refill_rx == RX_RING)
> + return 1;
> + return 0;
> +}

skb_reserve() seems to be missing


> +static void drain_tx(struct net_device *dev)
> +{
> + struct fe_priv *np = get_nvpriv(dev);
> + int i;
> + for (i = 0; i < TX_RING; i++) {
> + np->tx_ring[i].Flags = 0;
> + if (np->tx_skbuff[i]) {
> + pci_unmap_single(np->pci_dev, np->tx_dma[i],
> + np->tx_skbuff[i]->len,
> + PCI_DMA_TODEVICE);
> + dev_kfree_skb(np->tx_skbuff[i]);
> + np->tx_skbuff[i] = NULL;
> + np->stats.tx_dropped++;
> + }
> + }
> +}

I wonder about calling dev_kfree_skb() from dev->tx_timeout() with
dev->xmit_lock held...


> +static void drain_rx(struct net_device *dev)
> +{
> + struct fe_priv *np = get_nvpriv(dev);
> + int i;
> + for (i = 0; i < RX_RING; i++) {
> + np->rx_ring[i].Flags = 0;
> + wmb();
> + if (np->rx_skbuff[i]) {
> + pci_unmap_single(np->pci_dev, np->rx_dma[i],
> + np->rx_skbuff[i]->len,
> + PCI_DMA_FROMDEVICE);
> + dev_kfree_skb(np->rx_skbuff[i]);
> + np->rx_skbuff[i] = NULL;
> + }
> + }
> +}
> +
> +static void drain_ring(struct net_device *dev)
> +{
> + drain_tx(dev);
> + drain_rx(dev);
> +}
> +
> +/*
> + * start_xmit: dev->hard_start_xmit function
> + * Called with dev->xmit_lock held.
> + */
> +static int start_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> + struct fe_priv *np = get_nvpriv(dev);
> + int nr = np->next_tx % TX_RING;
> +
> + np->tx_skbuff[nr] = skb;
> + np->tx_dma[nr] = pci_map_single(np->pci_dev, skb->data,skb->len,
> + PCI_DMA_TODEVICE);
> +
> + np->tx_ring[nr].PacketBuffer = cpu_to_le32(np->tx_dma[nr]);
> + np->tx_ring[nr].Length = cpu_to_le16(skb->len-1);
> +
> + spin_lock_irq(&np->lock);
> + wmb();
> + np->tx_ring[nr].Flags = np->tx_flags;
> + dprintk(KERN_DEBUG "%s: start_xmit: packet packet %d queued for transmission.\n",
> + dev->name, np->next_tx);
> + {
> + int j;
> + for (j=0; j<64; j++) {
> + if ((j%16) == 0)
> + dprintk("\n%03x:", j);
> + dprintk(" %02x", ((unsigned char*)skb->data)[j]);
> + }
> + dprintk("\n");
> + }

would be nice if this loop was optimized out by the compiler.


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


> + return 0;
> +}
> +
> +/*
> + * tx_done: check for completed packets, release the skbs.
> + *
> + * Caller must own np->lock.
> + */
> +static void tx_done(struct net_device *dev)
> +{
> + struct fe_priv *np = get_nvpriv(dev);
> +
> + while (np->nic_tx < np->next_tx) {
> + struct ring_desc *prd;
> + int i = np->nic_tx % TX_RING;
> +
> + prd = &np->tx_ring[i];
> +
> + dprintk(KERN_DEBUG "%s: tx_done: looking at packet %d, Flags 0x%x.\n",
> + dev->name, np->nic_tx, prd->Flags);
> + if (prd->Flags & cpu_to_le16(NV_TX_VALID))
> + break;
> + if (prd->Flags & cpu_to_le16(NV_TX_RETRYERROR|NV_TX_CARRIERLOST|NV_TX_LATECOLLISION|
> + NV_TX_UNDERFLOW|NV_TX_ERROR)) {
> + if (prd->Flags & cpu_to_le16(NV_TX_UNDERFLOW))
> + np->stats.tx_fifo_errors++;
> + if (prd->Flags & cpu_to_le16(NV_TX_CARRIERLOST))
> + np->stats.tx_carrier_errors++;
> + np->stats.tx_errors++;
> + } else {
> + np->stats.tx_packets++;
> + np->stats.tx_bytes += np->tx_skbuff[i]->len;
> + }
> + pci_unmap_single(np->pci_dev, np->tx_dma[i],
> + np->tx_skbuff[i]->len,
> + PCI_DMA_TODEVICE);
> + dev_kfree_skb_irq(np->tx_skbuff[i]);
> + np->tx_skbuff[i] = NULL;
> + np->nic_tx++;
> + }
> + if (np->next_tx - np->nic_tx < TX_LIMIT_START)
> + netif_wake_queue(dev);
> +}
> +
> +/*
> + * tx_timeout: dev->tx_timeout function
> + * Called with dev->xmit_lock held.
> + */
> +static void tx_timeout(struct net_device *dev)
> +{
> + struct fe_priv *np = get_nvpriv(dev);
> + u8 *base = get_hwbase(dev);
> +
> + dprintk(KERN_DEBUG "%s: Got tx_timeout. irq: %08x\n", dev->name,
> + readl(base + NvRegIrqStatus) & NVREG_IRQSTAT_MASK);
> +
> + spin_lock_irq(&np->lock);
> +
> + /* 1) stop tx engine */
> + stop_tx(dev);
> +
> + /* 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.


> + /* 3) if there are dead entries: clear everything */
> + if (np->next_tx != np->nic_tx) {
> + printk(KERN_DEBUG "%s: tx_timeout: dead entries!\n", dev->name);
> + drain_tx(dev);
> + np->next_tx = np->nic_tx = 0;
> + writel((u32) (np->ring_addr + RX_RING*sizeof(struct ring_desc)), base + NvRegTxRingPhysAddr);
> + netif_wake_queue(dev);
> + }
> +
> + /* 4) restart tx engine */
> + start_tx(dev);
> + spin_unlock_irq(&np->lock);
> +}
> +
> +static void rx_process(struct net_device *dev)
> +{
> + struct fe_priv *np = get_nvpriv(dev);
> +
> + for (;;) {
> + struct ring_desc *prd;
> + struct sk_buff *skb;
> + int len;
> + int i;
> + if (np->cur_rx - np->refill_rx >= RX_RING)
> + break; /* we scanned the whole ring - do not continue */
> +
> + i = np->cur_rx % RX_RING;
> + prd = &np->rx_ring[i];
> + dprintk(KERN_DEBUG "%s: rx_process: looking at packet %d, Flags 0x%x.\n",
> + dev->name, np->cur_rx, prd->Flags);
> +
> + if (prd->Flags & cpu_to_le16(NV_RX_AVAIL))
> + break; /* still owned by hardware, */
> +
> + /*
> + * 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.


> + {
> + int j;
> + dprintk(KERN_DEBUG "Dumping packet (flags 0x%x).",prd->Flags);
> + for (j=0; j<64; j++) {
> + if ((j%16) == 0)
> + dprintk("\n%03x:", j);
> + dprintk(" %02x", ((unsigned char*)np->rx_skbuff[i]->data)[j]);
> + }
> + dprintk("\n");
> + }
> + /* look at what we actually got: */
> + if (!(prd->Flags & cpu_to_le16(NV_RX_DESCRIPTORVALID)))
> + goto next_pkt;
> +
> +
> + len = le16_to_cpu(prd->Length);
> +
> + if (prd->Flags & cpu_to_le16(NV_RX_MISSEDFRAME)) {
> + np->stats.rx_missed_errors++;
> + np->stats.rx_errors++;
> + goto next_pkt;
> + }
> + if (prd->Flags & cpu_to_le16(NV_RX_ERROR1|NV_RX_ERROR2|NV_RX_ERROR3|NV_RX_ERROR4)) {
> + np->stats.rx_errors++;
> + goto next_pkt;
> + }
> + if (prd->Flags & cpu_to_le16(NV_RX_CRCERR)) {
> + np->stats.rx_crc_errors++;
> + np->stats.rx_errors++;
> + goto next_pkt;
> + }
> + if (prd->Flags & cpu_to_le16(NV_RX_OVERFLOW)) {
> + np->stats.rx_over_errors++;
> + np->stats.rx_errors++;
> + goto next_pkt;
> + }
> + if (prd->Flags & cpu_to_le16(NV_RX_ERROR)) {
> + /* framing errors are soft errors, the rest is fatal. */
> + if (prd->Flags & cpu_to_le16(NV_RX_FRAMINGERR)) {
> + if (prd->Flags & cpu_to_le16(NV_RX_SUBSTRACT1)) {
> + len--;
> + }
> + } else {
> + np->stats.rx_errors++;
> + goto next_pkt;
> + }
> + }
> + /* got a valid packet - forward it to the network core */
> + skb = np->rx_skbuff[i];
> + np->rx_skbuff[i] = NULL;
> +
> + skb_put(skb, len);
> + skb->protocol = eth_type_trans(skb, dev);
> + dprintk(KERN_DEBUG "%s: rx_process: packet %d with %d bytes, proto %d accepted.\n",
> + dev->name, np->cur_rx, len, skb->protocol);
> + netif_rx(skb);
> + dev->last_rx = jiffies;
> + np->stats.rx_packets++;
> + np->stats.rx_bytes += len;
> +next_pkt:
> + np->cur_rx++;
> + }
> +}
> +
> +/*
> + * 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?

bug #2: need a minimum bound for the MTU as well



> +static void link_irq(struct net_device *dev)
> +{
> + struct fe_priv *np = get_nvpriv(dev);
> + u8 *base = get_hwbase(dev);
> + u32 miistat;
> + int miival;
> +
> + miistat = readl(base + NvRegMIIStatus);
> + writel(NVREG_MIISTAT_MASK, base + NvRegMIIStatus);
> + printk(KERN_DEBUG "%s: link change notification, status 0x%x.\n", dev->name, miistat);
> +
> + miival = mii_rw(dev, np->phyaddr, MII_BMSR, MII_READ);
> + if (miival & BMSR_ANEGCOMPLETE) {
> + update_linkspeed(dev);
> +
> + if (netif_carrier_ok(dev)) {
> + stop_rx(dev);
> + } else {
> + netif_carrier_on(dev);
> + printk(KERN_INFO "%s: link up.\n", dev->name);
> + }
> + writel(NVREG_MISC1_FORCE | ( np->duplex ? 0 : NVREG_MISC1_HD),
> + base + NvRegMisc1);
> + start_rx(dev);
> + } else {
> + if (netif_carrier_ok(dev)) {
> + netif_carrier_off(dev);
> + printk(KERN_INFO "%s: link down.\n", dev->name);
> + stop_rx(dev);
> + }
> + writel(np->linkspeed, base + NvRegLinkSpeed);
> + pci_push(base);
> + }
> +}

bug: netif_carrier_xxx not unconditionally initialized in open()

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


> +static irqreturn_t nic_irq(int foo, void *data, struct pt_regs *regs)
> +{
> + struct net_device *dev = (struct net_device *) data;
> + struct fe_priv *np = get_nvpriv(dev);
> + u8 *base = get_hwbase(dev);
> + u32 events;
> + int i;
> +
> + dprintk(KERN_DEBUG "%s: nic_irq\n", dev->name);
> +
> + 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

> + if (events & (NVREG_IRQ_TX1|NVREG_IRQ_TX2|NVREG_IRQ_TX_ERR)) {
> + spin_lock(&np->lock);
> + tx_done(dev);
> + spin_unlock(&np->lock);
> + }
> +
> + if (events & (NVREG_IRQ_RX|NVREG_IRQ_RX_NOBUF)) {
> + rx_process(dev);
> + if (alloc_rx(dev)) {
> + spin_lock(&np->lock);
> + if (!np->in_shutdown)
> + mod_timer(&np->oom_kick, jiffies + OOM_REFILL);
> + spin_unlock(&np->lock);
> + }
> + }
> +
> + if (events & NVREG_IRQ_LINK) {
> + spin_lock(&np->lock);
> + link_irq(dev);
> + spin_unlock(&np->lock);
> + }
> + if (events & (NVREG_IRQ_TX_ERR)) {
> + dprintk(KERN_DEBUG "%s: received irq with events 0x%x. Probably TX fail.\n",
> + dev->name, events);
> + }
> + if (events & (NVREG_IRQ_UNKNOWN)) {
> + printk(KERN_DEBUG "%s: received irq with unknown events 0x%x. Please report\n",
> + dev->name, events);
> + }
> + 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;
> + }

This is completely bogus.

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.


> + }
> + dprintk(KERN_DEBUG "%s: nic_irq completed\n", dev->name);
> +
> + return IRQ_RETVAL(i);
> +}
> +
> +static void do_nic_poll(unsigned long data)
> +{
> + struct net_device *dev = (struct net_device *) data;
> + struct fe_priv *np = get_nvpriv(dev);
> + u8 *base = get_hwbase(dev);
> +
> + disable_irq(dev->irq);
> + /*
> + * reenable interrupts on the nic, we have to do this before calling
> + * nic_irq because that may decide to do otherwise
> + */
> + writel(np->irqmask, base + NvRegIrqMask);
> + pci_push(base);
> + nic_irq((int) 0, (void *) data, (struct pt_regs *) NULL);
> + enable_irq(dev->irq);
> +}

The other half of the NAPI re-implementation...


> +static int open(struct net_device *dev)
> +{
> + struct fe_priv *np = get_nvpriv(dev);
> + u8 *base = get_hwbase(dev);
> + int ret, oom, i;
> +
> + dprintk(KERN_DEBUG "forcedeth: open\n");
> +
> + /* 1) erase previous misconfiguration */
> + /* 4.1-1: stop adapter: ignored, 4.3 seems to be overkill */
> + writel(NVREG_MCASTADDRA_FORCE, base + NvRegMulticastAddrA);
> + writel(0, base + NvRegMulticastAddrB);
> + writel(0, base + NvRegMulticastMaskA);
> + writel(0, base + NvRegMulticastMaskB);
> + writel(0, base + NvRegPacketFilterFlags);
> + writel(0, base + NvRegAdapterControl);
> + writel(0, base + NvRegLinkSpeed);
> + writel(0, base + NvRegUnknownTransmitterReg);
> + txrx_reset(dev);
> + writel(0, base + NvRegUnknownSetupReg6);
> +
> + /* 2) initialize descriptor rings */
> + np->in_shutdown = 0;
> + oom = init_ring(dev);
> +
> + /* 3) set mac address */
> + {
> + u32 mac[2];
> +
> + mac[0] = (dev->dev_addr[0] << 0) + (dev->dev_addr[1] << 8) +
> + (dev->dev_addr[2] << 16) + (dev->dev_addr[3] << 24);
> + mac[1] = (dev->dev_addr[4] << 0) + (dev->dev_addr[5] << 8);
> +
> + writel(mac[0], base + NvRegMacAddrA);
> + writel(mac[1], base + NvRegMacAddrB);
> + }
> +
> + /* 4) continue setup */
> + np->linkspeed = NVREG_LINKSPEED_FORCE|NVREG_LINKSPEED_10;
> + np->duplex = 0;
> + writel(NVREG_UNKSETUP3_VAL1, base + NvRegUnknownSetupReg3);
> + writel(0, base + NvRegTxRxControl);
> + pci_push(base);
> + writel(NVREG_TXRXCTL_BIT1, base + NvRegTxRxControl);
> + reg_delay(dev, NvRegUnknownSetupReg5, NVREG_UNKSETUP5_BIT31, NVREG_UNKSETUP5_BIT31,
> + NV_SETUP5_DELAY, NV_SETUP5_DELAYMAX,
> + KERN_INFO "open: SetupReg5, Bit 31 remained off\n");
> + writel(0, base + NvRegUnknownSetupReg4);
> +
> + /* 5) Find a suitable PHY */
> + writel(NVREG_MIISPEED_BIT8|NVREG_MIIDELAY, base + NvRegMIISpeed);
> + for (i = 1; i < 32; i++) {
> + int id1, id2;
> +
> + id1 = mii_rw(dev, i, MII_PHYSID1, MII_READ);
> + if (id1 < 0)
> + continue;
> + id2 = mii_rw(dev, i, MII_PHYSID2, MII_READ);
> + if (id2 < 0)
> + continue;

also check for 0xffff returned from h/w

> + dprintk(KERN_DEBUG "%s: open: Found PHY %04x:%04x at address %d.\n",
> + dev->name, id1, id2, i);
> + np->phyaddr = i;
> +
> + update_linkspeed(dev);
> +
> + break;
> + }
> + 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


> + /* 6) continue setup */
> + writel(NVREG_MISC1_FORCE | ( np->duplex ? 0 : NVREG_MISC1_HD),
> + base + NvRegMisc1);
> + writel(readl(base + NvRegTransmitterStatus), base + NvRegTransmitterStatus);
> + writel(NVREG_PFF_ALWAYS, base + NvRegPacketFilterFlags);
> + writel(NVREG_OFFLOAD_NORMAL, base + NvRegOffloadConfig);
> +
> + writel(readl(base + NvRegReceiverStatus), base + NvRegReceiverStatus);
> + get_random_bytes(&i, sizeof(i));
> + writel(NVREG_RNDSEED_FORCE | (i&NVREG_RNDSEED_MASK), base + NvRegRandomSeed);
> + writel(NVREG_UNKSETUP1_VAL, base + NvRegUnknownSetupReg1);
> + writel(NVREG_UNKSETUP2_VAL, base + NvRegUnknownSetupReg2);
> + writel(NVREG_POLL_DEFAULT, base + NvRegPollingInterval);
> + writel(NVREG_UNKSETUP6_VAL, base + NvRegUnknownSetupReg6);
> + writel((np->phyaddr << NVREG_ADAPTCTL_PHYSHIFT)|NVREG_ADAPTCTL_PHYVALID,
> + base + NvRegAdapterControl);
> + writel(NVREG_UNKSETUP4_VAL, base + NvRegUnknownSetupReg4);
> + writel(NVREG_WAKEUPFLAGS_VAL, base + NvRegWakeUpFlags);
> +
> + /* 7) start packet processing */
> + writel((u32) np->ring_addr, base + NvRegRxRingPhysAddr);
> + writel((u32) (np->ring_addr + RX_RING*sizeof(struct ring_desc)), base + NvRegTxRingPhysAddr);
> + writel( ((RX_RING-1) << NVREG_RINGSZ_RXSHIFT) + ((TX_RING-1) << NVREG_RINGSZ_TXSHIFT),
> + base + NvRegRingSizes);
> +
> + i = readl(base + NvRegPowerState);
> + if ( (i & NVREG_POWERSTATE_POWEREDUP) == 0) {
> + writel(NVREG_POWERSTATE_POWEREDUP|i, base + NvRegPowerState);
> + }

style: useless brackets


> + pci_push(base);
> + udelay(10);
> + writel(readl(base + NvRegPowerState) | NVREG_POWERSTATE_VALID, base + NvRegPowerState);
> + writel(NVREG_ADAPTCTL_RUNNING, base + NvRegAdapterControl);
> +
> +
> + writel(0, base + NvRegIrqMask);
> + pci_push(base);
> + writel(NVREG_IRQSTAT_MASK, base + NvRegIrqStatus);
> + pci_push(base);
> + writel(NVREG_MIISTAT_MASK2, base + NvRegMIIStatus);
> + writel(NVREG_IRQSTAT_MASK, base + NvRegIrqStatus);
> + pci_push(base);
> +
> + ret = request_irq(dev->irq, &nic_irq, SA_SHIRQ, dev->name, dev);
> + if (ret)
> + goto out_drain;
> +
> + writel(np->irqmask, base + NvRegIrqMask);
> +
> + spin_lock_irq(&np->lock);
> + writel(NVREG_MCASTADDRA_FORCE, base + NvRegMulticastAddrA);
> + writel(0, base + NvRegMulticastAddrB);
> + writel(0, base + NvRegMulticastMaskA);
> + writel(0, base + NvRegMulticastMaskB);
> + writel(NVREG_PFF_ALWAYS|NVREG_PFF_MYADDR, base + NvRegPacketFilterFlags);
> + start_rx(dev);
> + start_tx(dev);
> + netif_start_queue(dev);
> + if (oom)
> + mod_timer(&np->oom_kick, jiffies + OOM_REFILL);
> + if (!(mii_rw(dev, np->phyaddr, MII_BMSR, MII_READ) & BMSR_ANEGCOMPLETE)) {
> + printk("%s: no link during initialization.\n", dev->name);
> + netif_carrier_off(dev);
> + }
> +
> + spin_unlock_irq(&np->lock);
> +
> + return 0;
> +out_drain:
> + drain_ring(dev);
> + return ret;
> +}
> +
> +static int close(struct net_device *dev)
> +{
> + struct fe_priv *np = get_nvpriv(dev);
> + u8 *base;
> +
> + 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 :)


> + del_timer_sync(&np->oom_kick);
> + del_timer_sync(&np->nic_poll);
> +
> + netif_stop_queue(dev);
> + spin_lock_irq(&np->lock);
> + stop_tx(dev);
> + stop_rx(dev);
> + base = get_hwbase(dev);
> +
> + /* disable interrupts on the nic or we will lock up */
> + writel(0, base + NvRegIrqMask);
> + pci_push(base);
> + dprintk(KERN_INFO "%s: Irqmask is zero again\n", dev->name);
> +
> + spin_unlock_irq(&np->lock);
> +
> + free_irq(dev->irq, dev);
> +
> + drain_ring(dev);
> +
> + /* FIXME: power down nic */
> +
> + return 0;
> +}
> +
> +static int __devinit probe_nic(struct pci_dev *pci_dev, const struct pci_device_id *id)
> +{
> + struct net_device *dev;
> + struct fe_priv *np;
> + unsigned long addr;
> + u8 *base;
> + int err, i;
> +
> + dev = alloc_etherdev(sizeof(struct fe_priv));
> + np = get_nvpriv(dev);
> + err = -ENOMEM;
> + if (!dev)
> + goto out;
> +
> + np->pci_dev = pci_dev;
> + spin_lock_init(&np->lock);
> + SET_MODULE_OWNER(dev);
> + SET_NETDEV_DEV(dev, &pci_dev->dev);
> +
> + init_timer(&np->oom_kick);
> + np->oom_kick.data = (unsigned long) dev;
> + np->oom_kick.function = &do_rx_refill; /* timer handler */
> + init_timer(&np->nic_poll);
> + np->nic_poll.data = (unsigned long) dev;
> + np->nic_poll.function = &do_nic_poll; /* timer handler */
> +
> + err = pci_enable_device(pci_dev);
> + if (err) {
> + printk(KERN_INFO "forcedeth: pci_enable_dev failed (%d) for device %s\n",
> + err, pci_name(pci_dev));
> + goto out_free;
> + }
> +
> + pci_set_master(pci_dev);
> +
> + err = pci_request_regions(pci_dev, dev->name);
> + if (err < 0)
> + goto out_disable;
> +
> + err = -EINVAL;
> + addr = 0;
> + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> + dprintk(KERN_DEBUG "%s: resource %d start %p len %ld flags 0x%08lx.\n",
> + pci_name(pci_dev), i, (void*)pci_resource_start(pci_dev, i),
> + pci_resource_len(pci_dev, i),
> + pci_resource_flags(pci_dev, i));
> + if (pci_resource_flags(pci_dev, i) & IORESOURCE_MEM &&
> + pci_resource_len(pci_dev, i) >= NV_PCI_REGSZ) {
> + addr = pci_resource_start(pci_dev, i);
> + break;
> + }
> + }

Do nForce NICs really put the registers on random PCI BARs???

I would rather just hardcode is the PCI BAR if at all possible.



> + if (i == DEVICE_COUNT_RESOURCE) {
> + printk(KERN_INFO "forcedeth: Couldn't find register window for device %s.\n",
> + pci_name(pci_dev));
> + goto out_relreg;
> + }
> +
> + err = -ENOMEM;
> + dev->base_addr = (unsigned long) ioremap(addr, NV_PCI_REGSZ);
> + if (!dev->base_addr)
> + goto out_disable;
> + dev->irq = pci_dev->irq;
> + 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.


> + np->tx_ring = &np->rx_ring[RX_RING];
> +
> + dev->open = open;
> + dev->stop = close;
> + dev->hard_start_xmit = start_xmit;
> + dev->get_stats = get_stats;
> + dev->change_mtu = change_mtu;
> + dev->set_multicast_list = set_multicast;
> + dev->do_ioctl = nic_ioctl;
> + dev->tx_timeout = tx_timeout;
> + dev->watchdog_timeo = NV_WATCHDOG_TIMEO;
> +
> + pci_set_drvdata(pci_dev, dev);
> +
> +
> + /* read the mac address */
> + base = get_hwbase(dev);
> + np->orig_mac[0] = readl(base + NvRegMacAddrA);
> + np->orig_mac[1] = readl(base + NvRegMacAddrB);
> +
> + dev->dev_addr[0] = (np->orig_mac[1] >> 8) & 0xff;
> + dev->dev_addr[1] = (np->orig_mac[1] >> 0) & 0xff;
> + dev->dev_addr[2] = (np->orig_mac[0] >> 24) & 0xff;
> + dev->dev_addr[3] = (np->orig_mac[0] >> 16) & 0xff;
> + dev->dev_addr[4] = (np->orig_mac[0] >> 8) & 0xff;
> + dev->dev_addr[5] = (np->orig_mac[0] >> 0) & 0xff;
> +
> + if (!is_valid_ether_addr(dev->dev_addr)) {
> + /*
> + * Bad mac address. At least one bios sets the mac address
> + * to 01:23:45:67:89:ab
> + */
> + printk(KERN_ERR "%s: Invalid Mac address detected: %02x:%02x:%02x:%02x:%02x:%02x\n",
> + pci_name(pci_dev),
> + dev->dev_addr[0], dev->dev_addr[1], dev->dev_addr[2],
> + dev->dev_addr[3], dev->dev_addr[4], dev->dev_addr[5]);
> + printk(KERN_ERR "Please complain to your hardware vendor. Switching to a random MAC.\n");
> + dev->dev_addr[0] = 0x00;
> + dev->dev_addr[1] = 0x00;
> + dev->dev_addr[2] = 0x6c;
> + get_random_bytes(&dev->dev_addr[3], 3);
> + }

I don't like this random MAC address stuff. Regardless of hardware
behavior, this breaks MAC address uniqueness.

I would much rather that ->open() failed, if a valid MAC address were
not present. Then a simple userspace program can be run by the admin
(policy!) that sets the MAC address, before bringing up the interface.


> + dprintk(KERN_DEBUG "%s: MAC Address %02x:%02x:%02x:%02x:%02x:%02x\n", pci_name(pci_dev),
> + dev->dev_addr[0], dev->dev_addr[1], dev->dev_addr[2],
> + dev->dev_addr[3], dev->dev_addr[4], dev->dev_addr[5]);
> +
> + np->tx_flags = cpu_to_le16(NV_TX_LASTPACKET|NV_TX_LASTPACKET1|NV_TX_VALID);
> + if (id->driver_data & DEV_NEED_LASTPACKET1)
> + np->tx_flags |= cpu_to_le16(NV_TX_LASTPACKET1);
> + if (id->driver_data & DEV_IRQMASK_1)
> + np->irqmask = NVREG_IRQMASK_WANTED_1;
> + if (id->driver_data & DEV_IRQMASK_2)
> + np->irqmask = NVREG_IRQMASK_WANTED_2;
> + if (id->driver_data & DEV_NEED_TIMERIRQ)
> + np->irqmask |= NVREG_IRQ_TIMER;
> +
> + err = register_netdev(dev);
> + if (err) {
> + printk(KERN_INFO "forcedeth: unable to register netdev: %d\n", err);
> + goto out_freering;
> + }
> + printk(KERN_INFO "%s: forcedeth.c: subsystem: %05x:%04x bound to %s\n",
> + dev->name, pci_dev->subsystem_vendor, pci_dev->subsystem_device,
> + pci_name(pci_dev));
> +
> + return 0;
> +
> +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()



> +static struct pci_device_id pci_tbl[] = {
> + { /* nForce Ethernet Controller */
> + .vendor = PCI_VENDOR_ID_NVIDIA,
> + .device = 0x1C3,
> + .subvendor = PCI_ANY_ID,
> + .subdevice = PCI_ANY_ID,
> + .driver_data = DEV_IRQMASK_1|DEV_NEED_TIMERIRQ,
> + },
> + { /* nForce2 Ethernet Controller */
> + .vendor = PCI_VENDOR_ID_NVIDIA,
> + .device = 0x0066,
> + .subvendor = PCI_ANY_ID,
> + .subdevice = PCI_ANY_ID,
> + .driver_data = DEV_NEED_LASTPACKET1|DEV_IRQMASK_2|DEV_NEED_TIMERIRQ,
> + },
> + { /* nForce3 Ethernet Controller */
> + .vendor = PCI_VENDOR_ID_NVIDIA,
> + .device = 0x00D6,
> + .subvendor = PCI_ANY_ID,
> + .subdevice = PCI_ANY_ID,
> + .driver_data = DEV_NEED_LASTPACKET1|DEV_IRQMASK_2|DEV_NEED_TIMERIRQ,
> + },
> + {0,},
> +};

You'll note this structure is wildly larger than most net drivers...
maintainer's preference, but I think this is way too verbose.


> +static struct pci_driver driver = {
> + .name = "forcedeth",
> + .id_table = pci_tbl,
> + .probe = probe_nic,
> + .remove = __devexit_p(remove_nic),
> +};
> +
> +
> +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.

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    [from the cache]
©2003-2011 Jasper Spaans