Hi Stephen, Thanks for your feedback. We'll implement these and post the driver again. -Amit
On Monday 03 Apr 2006 10:19 pm, Stephen Hemminger wrote: > On > > > +#undef NetXen_LOOPBACK > > +#undef SINGLE_DMA_BUF > > +#define NetXen_NIC_HW_CSUM > > +#define NetXen_NIC_NAPI > > +// #undef NetXen_NIC_NAPI > > + > > +#define NetXen_NETIF_F_TSO > > Ugh. MixedCase define's and ifdef configuration message. > Remember, either keep the code or remove it. > > > +// for cmd line parameteres---------- > > +#define NetXen_PARAM(X,S) \ > > +static int X=0; \ > > +MODULE_PARM(X,"i"); \ > > +MODULE_PARM_DESC(X,S); > > More macro crap. > > > + > > + > > +MODULE_AUTHOR("NetXen"); > > Author is a human, not a company. > > > +MODULE_DESCRIPTION("NetXen Multi port (1/10) Gigabit Network Driver"); > > +MODULE_LICENSE("GPL"); > > + > > +/*static int netxen_nic_debug = NETIF_MSG_DRV | NETIF_MSG_PROBE; TODO */ > > +int netxen_nic_debug; > > "static int debug" is good better. > > > + > > +MODULE_PARM(netxen_nic_debug, "i"); > > +MODULE_PARM_DESC(netxen_nic_debug, "Debug level (0=none,...,16=all)"); > > + > > + > > +char netxen_nic_driver_name[] = "netxen"; > > can be "static const char" > > > +char netxen_nic_driver_string[] = "NetXen Network Driver version " > > + NetXen_NIC_LINUX_VERSIONID > > + "-" > > + NetXen_NIC_BUILD_NO > > + " generated " > > + NetXen_NIC_TIMESTAMP; > > + > > +#ifndef ARCH_KMALLOC_MINALIGN > > +#define ARCH_KMALLOC_MINALIGN 0 > > +#endif > > +#ifndef ARCH_KMALLOC_FLAGS > > +#define ARCH_KMALLOC_FLAGS SLAB_HWCACHE_ALIGN > > +#endif > > More macro obfuscation. > > > + > > +#define NetXen_NETDEV_WEIGHT 120 > > +#define ADAPTER_UP_MAGIC 777 > > + > > +kmem_cache_t *cmd_desc_cache = NULL; > > +kmem_cache_t *cmd_buff_cache = NULL; > > +cmdDescType0_t *cmd_desc_array[NR_CPUS]; > > +struct netxen_cmd_buffer *cmd_buff_array[NR_CPUS]; > > + > > +static int netxen_nic_probe_err = 0; > > + > > +#define RCV_DESC_RINGSIZE (sizeof(rcvDesc_t) * > > adapter->MaxRxDescCount) +#define STATUS_DESC_RINGSIZE > > (sizeof(statusDesc_t)* adapter->MaxRxDescCount) > > Don't use structure obscuring typedef's (rcvDesc_t) ... > > > +#define JUMBO_DESC_RINGSIZE (sizeof(rcvDesc_t) * > > adapter->MaxJumboRxDescCount) +#define TX_RINGSIZE (sizeof(struct > > netxen_cmd_buffer) * adapter->MaxTxDescCount) +#define RCV_BUFFSIZE > > (sizeof(struct netxen_rx_buffer) * rcv_desc->MaxRxDescCount) + > > +#define NetXen_NIC_INT_RESET 0x2004 > > + > > +/* Local functions to NetXen NIC driver */ > > +static int __devinit netxen_nic_probe(struct pci_dev *pdev, > > + const struct pci_device_id *ent); > > +static void __devexit netxen_nic_remove(struct pci_dev *pdev); > > +static void netxen_nic_hw_reset (struct netxen_adapter_s *adapter); > > +static int netxen_nic_open(struct net_device *netdev); > > +static int netxen_nic_close(struct net_device *netdev); > > +static int netxen_nic_xmit_frame(struct sk_buff *, struct net_device > > *); +static int netxen_nic_set_mac(struct net_device *netdev, void > > *p); +static int netxen_nic_change_mtu(struct net_device *netdev, int > > new_mtu); +static int netxen_nic_hw_resources(struct netxen_adapter_s > > *adapter); +static void netxen_nic_set_multi(struct net_device > > *netdev); > > +static void initialize_adapter_sw(struct netxen_adapter_s *adapter); > > +static void netxen_post_rx_buffers(struct netxen_adapter_s *adapter, > > uint32_t ctx, + uint32_t num, uint32_t > > type); +static void netxen_process_rcv(struct netxen_adapter_s > > *adapter,int ctx, + statusDesc_t *desc); > > +static void netxen_tx_timeout(struct net_device *netdev); > > +static void netxen_tx_timeout_task(struct net_device *netdev); > > +static void netxen_process_cmd_ring(unsigned long data); > > +static void netxen_nic_down(struct netxen_port *port); > > +static int netxen_nic_do_ioctl(struct netxen_adapter_s *adapter, > > void*u_data, + struct netxen_port *port); > > +static int initialize_adapter_hw(struct netxen_adapter_s *adapter); > > +static void netxen_watchdog(unsigned long); > > +static void netxen_watchdog_task(unsigned long adapter); > > +static void netxen_nic_tune_phantom(struct pci_dev *pdev, > > + struct netxen_adapter_s*); > > +static void netxen_nic_enable_int(netxen_adapter *adapter); > > +static void netxen_nic_disable_int(netxen_adapter *adapter); > > +static uint32_t netxen_process_rcv_ring(struct netxen_adapter_s *, int, > > int); +static void netxen_nic_clear_stats(struct netxen_adapter_s *); > > +static struct net_device_stats *netxen_nic_get_stats(struct net_device > > *netdev); +static void netxen_nic_irq_enable(struct netxen_adapter_s > > *adapter); +static int netxen_handle_int(netxen_adapter *, struct > > net_device *); +static int netxen_nic_ioctl(struct net_device *netdev, > > + struct ifreq *ifr, int cmd); > > +static int netxen_nic_read_proc(char *buf, char **start, off_t > > offset, + int count, int *eof, void > > *data); +static int netxen_nic_port_read_proc(char *buf, char **start, > > off_t offset, + int count, int > > *eof, void *data); +#ifdef NetXen_NIC_NAPI > > +static int netxen_nic_rx_has_work(struct netxen_adapter_s *adapter); > > +static int netxen_nic_tx_has_work(struct netxen_adapter_s *adapter); > > +static int netxen_nic_poll(struct net_device *dev, int *budget); > > +#endif > > +#ifdef CONFIG_NET_POLL_CONTROLLER > > +static void netxen_nic_poll_controller(struct net_device *netdev); > > +#endif > > +#ifdef NetXen_NIC_HW_CSUM > > +static int netxen_tx_csum(struct netxen_adapter_s *adapter, > > cmdDescType0_t *desc, + struct sk_buff *skb); > > +#endif > > +static irqreturn_t netxen_intr(int irq, void *data, struct pt_regs > > *regs); + > > +extern void set_ethtool_ops(struct net_device *netdev); > > +struct netxen_adapter_s *g_adapter; > > +spinlock_t hal_lock; > > + > > +unsigned int loopback_start = 0; > > + > > +static struct proc_dir_entry *netxen_proc_dir_entry; > > + > > +static unsigned char *tx_user_packet_data = NULL, *rx_user_packet_data; > > +static unsigned int tx_user_packet_length = 0, rx_datalen, rx_user_pos = > > 0; +static int capture_input = 0, captured = 0; > > + > > +#define DIDX_DIF(p, c) \ > > + ((p >= c) ? (p - c) : (MAX_CMD_DESCRIPTORS - p + c)) > > Why not a simple inline? > > > + > > +#define CIS_WATERMARK 0x60 > > + > > +#define PEG_LOOP 1000 /* loop to pegstuck? */ > > + > > +#define QMAJ(hdr) ((hdr >> 19) & 0xF) > > +#define QMIN(hdr) (hdr & 0x3ffff) > > +#define QSUB(hdr) ((hdr >> 18) & 0x1) > > + > > +extern void xge_loopback(int on); > > +extern long xge_link_status (void); > > + > > +void __devinit > > +netxen_check_options(netxen_adapter *adapter) > > +{ > > + adapter->MaxTxDescCount = MAX_CMD_DESCRIPTORS; > > + adapter->MaxRxDescCount = MAX_RCV_DESCRIPTORS; > > + adapter->MaxJumboRxDescCount = MAX_JUMBO_RCV_DESCRIPTORS; > > + return; > > +} > > Why have a simple function like this ... ex C++ programmer? > > > + > > +/* PCI Device ID Table */ > > +static struct pci_device_id netxen_pci_tbl[] __devinitdata = { > > + {PCI_VENDOR_ID_NetXen,PCI_DEVICE_ID_NetXen_4PORT,PCI_ANY_ID, > > + PCI_ANY_ID,0,0,0UL}, > > Use PCI_DEVICE() for this. > > > + {0,} > > +}; > > + > > +MODULE_DEVICE_TABLE(pci, netxen_pci_tbl); > > + > > +struct netxen_adapter_s *adapterlist[MAX_NUM_CARDS]; > > This is artifically limiting . Use dynamic allocation and pci_privdata > to find adapter structure. > > > + > > +/* > > +netxen_nic_probe() > > + > > +Linux system will invoke this after identifying the vendor ID and device > > Id +in the pci_tbl where this module will search for NetXen vedonr and > > device ID for +quad port adapter. > > + > > +Even though there are 4 seperate devices (functions) on the quad port > > NIC, but +the probe will be invoked for the first device (function) only. > > This will +initialize the adapter, and setup the global parameters along > > with the port's +specific structure. > > +*/ > > +static int __devinit > > +netxen_nic_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > > +{ > > + struct net_device *netdev = NULL; > > + struct netxen_adapter_s *adapter = NULL; > > + struct netxen_port *port = NULL; > > + uint8_t *mem_ptr = NULL; > > + unsigned long mem_base, mem_len; > > + int pci_using_dac, i, err; > > + int boardno = 0; > > + int ring; > > + netdev_list_t *netlist = NULL; > > + netxen_recv_context_t *recv_ctx = NULL; > > + netxen_rcv_desc_ctx_t *rcv_desc = NULL; > > + struct netxen_cmd_buffer *cmd_buf_arr = NULL; > > + struct proc_dir_entry *procfile; > > + static int netxen_cards_found = 0; > > + > > + printk("NetXen: adapter %d\n", netxen_cards_found); > > + > > + if((err = pci_enable_device(pdev))) { > > + DPRINTK(1, ERR, "Cannot enable PCI device, aborting. > > %d\n", + err); > > + netxen_nic_probe_err = err; > > + return err; > > + } > > + > > + if (!(pci_resource_flags(pdev, 0) & IORESOURCE_MEM)) { > > + DPRINTK(1, ERR, "Cannot find proper PCI device " > > + "base address, aborting. %p\n", > > pdev); + err = -ENODEV; > > + goto err_out_disable_pdev; > > + } > > + > > + if((err = pci_request_regions(pdev, netxen_nic_driver_name))) { > > + DPRINTK(1, ERR, "Cannot find proper PCI resources" > > + " aborting:%d\n", err); > > + goto err_out_disable_pdev; > > + } > > + > > + pci_set_master(pdev); > > + > > + if(!(err = pci_set_dma_mask(pdev, PCI_DMA_64BIT)) && > > + !(err = pci_set_consistent_dma_mask(pdev, DMA_64BIT_MASK))) { > > + pci_using_dac = 1; > > + } else { > > + if((err = pci_set_dma_mask(pdev, PCI_DMA_32BIT)) || > > + (err = pci_set_consistent_dma_mask(pdev, > > DMA_32BIT_MASK))) { + DPRINTK(1, ERR, "No usable > > DMA configuration," + " > > aborting:%d\n", err); + goto err_out_free_res; > > + } > > + pci_using_dac = 0; > > + } > > + DPRINTK(1, INFO, "pci_using_dac: %d\n", pci_using_dac); > > + > > + /* remap phys address */ > > + mem_base = pci_resource_start(pdev, 0); /* 0 is for BAR 0 */ > > + mem_len = pci_resource_len(pdev, 0); > > + > > + /* 128 Meg of memory */ > > + DPRINTK(1, INFO, "ioremap from %lx a size of %lx\n", mem_base, > > + mem_len); > > + mem_ptr = ioremap(mem_base, NetXen_PCI_MAPSIZE_BYTES); > > + if (mem_ptr == 0UL) { > > + DPRINTK(1, ERR, "Cannot remap adapter memory > > aborting.:%p\n", + mem_ptr); > > + err = -EIO; > > + goto err_out_free_res; > > + } > > + DPRINTK(1, ERR, "ioremaped at %p\n", mem_ptr); > > + > > +/* > > + * Allocate a adapter structure which will manage all the > > initialization + * as well as the common resources for all ports... > > + * all the ports will have pointer to this adapter as well as > > Adapter + * will have pointers of all the ports structures. > > + */ > > + > > + /* One adapter structure for all 4 ports.... */ > > + adapter = (struct netxen_adapter_s *)kmalloc(sizeof(struct > > netxen_adapter_s), + > > GFP_KERNEL); > > Cast is unnecessary here. > > > + if (adapter == NULL) { > > + DPRINTK(1, ERR, "Could not allocate adapter > > memory:%d\n", + (int)sizeof(struct > > netxen_adapter_s)); + err = -ENOMEM; > > + goto err_out_iounmap; > > + } > > + > > + if (netxen_cards_found == 0) { > > + g_adapter = adapter; > > + spin_lock_init(&hal_lock); > > + } > > + > > + /* Initialize to 0 */ > > + memset(adapter, 0 , sizeof(struct netxen_adapter_s)); > > Use kzalloc. > > > + DPRINTK(1, INFO, "malloced memory at %p\n", adapter); > > + > > + netxen_check_options(adapter); > > + > > + cmd_buf_arr = (struct netxen_cmd_buffer*) vmalloc(TX_RINGSIZE); > > Your ring must be huge to need to vmalloc? > > > + if (cmd_buf_arr == NULL) { > > + DPRINTK(1, ERR, "Could not allocate command buffer > > array," + " order :%d\n", > > get_order(TX_RINGSIZE)); + err = -ENOMEM; > > + goto err_out_free_adapter; > > + } > > + memset (cmd_buf_arr, 0, TX_RINGSIZE); > > + > > + for (i = 0; i < MAX_RCV_CTX; ++i) { > > + recv_ctx = &adapter->recv_ctx[i]; > > + for (ring = 0; ring < NUM_RCV_DESC_RINGS; ring++) { > > + rcv_desc = &recv_ctx->rcv_desc[ring]; > > + switch (RCV_DESC_TYPE(ring)) { > > + case RCV_RING_STD: > > + rcv_desc->MaxRxDescCount = > > + adapter->MaxRxDescCount; > > + rcv_desc->flags = RCV_DESC_NORMAL; > > + rcv_desc->dma_size = RX_DMA_MAP_LEN; > > + rcv_desc->skb_size = > > MAX_RX_BUFFER_LENGTH; + break; > > + > > + case RCV_RING_JUMBO: > > + rcv_desc->MaxRxDescCount = > > + > > adapter->MaxJumboRxDescCount; + > > rcv_desc->flags = RCV_DESC_JUMBO; + > > rcv_desc->dma_size = RX_JUMBO_DMA_MAP_LEN; + > > rcv_desc->skb_size = > > + > > MAX_RX_JUMBO_BUFFER_LENGTH; + break; > > + > > + default: > > + printk("%s: netxen_nic_probe: bad > > receive " + "descriptor type %d\n", > > + netxen_nic_driver_name, > > + RCV_DESC_TYPE(ring)); > > + break; > > + } > > + rcv_desc->rx_buf_arr = (struct > > netxen_rx_buffer*) + > > pci_alloc_consistent(pdev, + > > RCV_BUFFSIZE, + > > &rcv_desc->rx_buf_phys); + > > + if(rcv_desc->rx_buf_arr == NULL) { > > + DPRINTK(1, ERR, "Could not allocate rx" > > + " buffer array ring %d," > > + " size:%u\n", ring, > > + (unsigned > > int)RCV_BUFFSIZE); + err = -ENOMEM; > > + goto err_out_free_rx_buffer; > > + } > > + memset (rcv_desc->rx_buf_arr, 0, RCV_BUFFSIZE); > > + } > > + > > + } > > + > > + adapter->cmd_buf_arr = cmd_buf_arr; > > + adapter->ahw.pci_base = (unsigned long)mem_ptr; > > + adapter->ahw.pci_len = mem_len; > > + adapter->ahw.crb_base = adapter->ahw.pci_base + > > NetXen_PCI_CRBSPACE; + rwlock_init(&adapter->adapter_lock); > > Reader writer locks are the slowest of all the locking options. Why are you > using it rather than something better. > > > + spin_lock_init(&adapter->tx_lock); > > + spin_lock_init(&adapter->lock); > > + initialize_adapter_sw(adapter); /* initialize the buffers in > > adapter */ + /* > > + * Set the CRB window to invalid. If any register in window 0 is > > + * accessed it should set the window to 0 and then reset it to > > 1. + */ > > + adapter->curr_window = 255; > > + /* > > + * Adapter in our case is quad port so initialize it beofore > > + * initializing the ports > > + */ > > + initialize_adapter_hw(adapter); /* initialize the adapter */ > > + adapter->port = > > + (struct netxen_port**) kmalloc(sizeof(struct > > netxen_adapter_s)* + > > (adapter->ahw.max_ports), + > > GFP_KERNEL); > > Use kcalloc, cast not needed > > > + if (adapter->port == NULL) { > > + DPRINTK(1, ERR, "Could not allocate port-array > > memory:%d\n", + (int)sizeof(struct > > netxen_adapter_s) * + > > (adapter->ahw.max_ports)); > > + err = -ENOMEM; > > + goto err_out_free_rx_buffer; > > + } > > + init_timer(&adapter->watchdog_timer); > > + adapter->ahw.xg_linkup = 0; > > + adapter->watchdog_timer.function = &netxen_watchdog; > > + adapter->watchdog_timer.data = (unsigned long) adapter; > > + INIT_WORK(&adapter->watchdog_task, > > + (void (*)(void *))netxen_watchdog_task, adapter); > > + adapter->ahw.vendor_id = pdev->vendor; > > + adapter->ahw.device_id = pdev->device; > > + adapter->ahw.pdev = pdev; > > + adapter->procCmdBufCounter = 0; > > + pci_read_config_byte(pdev, PCI_REVISION_ID, > > &adapter->ahw.revision_id); + pci_read_config_word(pdev, > > PCI_COMMAND, &adapter->ahw.pci_cmd_word); + > > +#if defined(CONFIG_PCI_MSI) > > + adapter->flags |= NetXen_NIC_MSI_ENABLED; > > + if (pci_enable_msi(pdev)) { > > + adapter->flags &= ~NetXen_NIC_MSI_ENABLED; > > + printk(KERN_WARNING "%s: unable to allocate MSI > > interrupt" + " > > error\n",netxen_nic_driver_name); + } > > +#endif > > + netxen_nic_tune_phantom(pdev, adapter); > > + > > + /* initialize the all the ports */ > > + DPRINTK(1, INFO, "about to allocate all the %d net devs.\n", > > + adapter->ahw.max_ports); > > + for (i = 0; i < adapter->ahw.max_ports; i ++) { > > + netlist = kmalloc(sizeof(netdev_list_t), GFP_KERNEL); > > + if (netlist == NULL) { > > + goto err_out_free_dev; > > + } > > + netlist->netdev = alloc_etherdev(sizeof(struct > > netxen_port)); + if(!netlist->netdev) { > > + DPRINTK(1, ERR, " could not allocate netdev for > > port" + " %d\n",i+1); > > + kfree(netlist); > > + goto err_out_free_dev; > > + } > > + netdev = netlist->netdev; > > + > > + SET_MODULE_OWNER(netdev); > > + > > + port = netdev->priv; > > + port->netdev = netdev; > > + port->pdev = pdev; > > + port->hw.port = port; > > + port->adapter = adapter; > > + port->portnum = i; /* Gigabit port number starting from > > 0-3 */ + port->flags &= ~NETDEV_STATUS; > > + > > + netdev->open = netxen_nic_open; > > + netdev->stop = netxen_nic_close; > > + netdev->hard_start_xmit = netxen_nic_xmit_frame; > > + netdev->get_stats = netxen_nic_get_stats; > > + netdev->set_multicast_list = netxen_nic_set_multi; > > + netdev->set_mac_address = netxen_nic_set_mac; > > + netdev->change_mtu = netxen_nic_change_mtu; > > + netdev->do_ioctl = netxen_nic_ioctl; > > + netdev->tx_timeout = netxen_tx_timeout; > > + set_ethtool_ops(netdev); > > + /* FIXME: maybe > > SET_ETHTOOL_OPS(netdev,&netxen_nic_ethtool_ops);*/ > > Yes, do it right. > > > + netdev->irq = pdev->irq; > > +#ifdef NetXen_NIC_NAPI > > + netdev->poll = netxen_nic_poll; > > + netdev->weight = NetXen_NETDEV_WEIGHT; > > +#endif > > +#ifdef CONFIG_NET_POLL_CONTROLLER > > + netdev->poll_controller = netxen_nic_poll_controller; > > +#endif > > +#ifdef NetXen_NIC_HW_CSUM > > + /* ScatterGather support */ > > + netdev->features = NETIF_F_SG ; > > + netdev->features |= NETIF_F_IP_CSUM; > > +#endif > > +#ifdef NetXen_NIC_HW_VLAN > > + netdev->features |= NETIF_F_HW_VLAN_TX; > > + netdev->features |= NETIF_F_HW_VLAN_RX; > > +#endif > > +#ifdef NetXen_NETIF_F_TSO > > + netdev->features |= NETIF_F_TSO; > > +#endif > > + > > + if(pci_using_dac) > > + netdev->features |= NETIF_F_HIGHDMA; > > + DPRINTK(1, INFO, "setting mac address: %p\n", port); > > + > > + /* > > + * Allocate h/w address and set in h/w. > > + * Eventually, we will read from the flash and set the > > mac + * address. > > + * For now set the lower 3 bytes to be same as board > > number + */ > > Do it right, otherwise you risk issues with multiple systems with same > address! > > > + boardno = netxen_nic_get_board_num(adapter); > > + port->hw.mac_addr[0] = 0x00; > > + port->hw.mac_addr[1] = 0x0e; > > + port->hw.mac_addr[2] = 0x1e; > > + port->hw.mac_addr[3] = ((boardno & (0xff << 16)) >> 16) > > + i; + port->hw.mac_addr[4] = (boardno & (0xff << 8)) >> > > 8; + port->hw.mac_addr[5] = boardno & 0xff; > > + netxen_nic_macaddr_set (port, port->hw.mac_addr); > > + memcpy(netdev->dev_addr, port->hw.mac_addr, > > netdev->addr_len); + > > if(!is_valid_ether_addr(netdev->dev_addr)) { > > + DPRINTK(1, ERR, "BAD MAC :is_valid_ether_addr" > > + " aborting:%d\n", err); > > + break; > > + } > > + INIT_WORK(adapter->tx_timeout_task + i, > > + (void (*)(void > > *))netxen_tx_timeout_task, netdev); + > > netif_carrier_off(netdev); > > + netif_stop_queue(netdev); > > + > > + if((err = register_netdev(netdev))) { > > + DPRINTK(1, ERR, "register_netdev failed port > > #%d" + " aborting\n", i+1); > > + break; > > + } > > + procfile = create_proc_entry(netdev->name, S_IRUGO, > > + netxen_proc_dir_entry); > > + if (procfile) { > > + procfile->data = netdev; > > + procfile->owner = THIS_MODULE; > > + procfile->read_proc = netxen_nic_port_read_proc; > > + } > > Is the /proc entry really needed? If so use seq_file. Also netdev->name can > change so you need to track name changes... > > > Enough crap for now. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html