On Saturday 01 April 2006 15:50, Linsys Contractor Amit S. Kale wrote:
Not a full review, just what I noticed from a quick read.
> +static int
> +netxen_loopback_test(struct net_device *netdev, int fint, void *ptr)
[... lots of self test code snipped ...]
Isn't that a bit too much self test infrastructure for a production
driver? Would be better if you put that into a separate module
and perhaps not include it.
BTW you might end up with a name space collision with the xen people
with that prefix. Better double check everything is static or use something
more unique.
Your white space probably needs some improvement. Maybe a Lindent run.
> +static int
> +netxen_nic_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
> +{
I believe custom ioctls like this are strongly discouraged. Any statistics
should go through ethtool and the rest probably doesn't belong in there.
> + netif_device_detach(netdev);
> +
> + if(netif_running(netdev)) {
> + netif_carrier_off(netdev);
> + netif_stop_queue(netdev);
> + port->state = PORT_SUSPEND;
> + netxen_nic_down(port);
> + }
I don't think resume functions are supposed to mess with
the upper layer queueing like this. They should keep
that the same or perhaps only do a netif_detach_device()
> +
> +/* pci_save_state(pdev, port->pci_state); */
> +/* pci_disable_device(pdev); */
> +
> + return 0;
> +}
> +
> +static int
> +netxen_nic_resume(struct pci_dev *pdev)
> +{
> + uint32_t ret;
> + struct net_device *netdev = pci_get_drvdata(pdev);
> + struct netxen_port *port = netdev->priv;
> + ret = pci_enable_device(pdev);
> + port->state = PORT_UP;
> + netif_device_attach(netdev);
> + return ret;
So what reinitializes the device registers? Looks broken.
> + unsigned long flags;
> + void* addr;
> + if(adapter->ahw.board_type == NetXen_NIC_XGBE) {
> + write_lock_irqsave(&adapter->adapter_lock, flags);
> + netxen_nic_pci_change_crbwindow(adapter, 0);
> + NetXen_NIC_LOCKED_READ_REG(NetXen_NIU_XGE_TX_BYTE_CNT,
> &(netxen_stats->tx_bytes));
Your identifiers are far too long and redundant.
> + switch (data.cmd) {
> + case netxen_nic_cmd_pci_read:
Again looks like a lot of debugging support that shouldn't be there.
> + case netxen_nic_cmd_pci_config_read:
Especially that since Linux already has other ways to read/write
config space.
> +static int
> +netxen_nic_port_read_proc(char *buf, char **start, off_t offset, int count,
> + int *eof, void *data)
> +{
This should be probably in ethtool
> + printk(KERN_INFO "%s \n", netxen_nic_driver_string);
In general drivers shouldn't print anything when no hardware is found.
> + netxen_proc_dir_entry = proc_mkdir(netxen_nic_driver_name, proc_net);
> + if (!netxen_proc_dir_entry) {
> + printk(KERN_WARNING "%s: Unable to create /proc/net/%s",
> + netxen_nic_driver_name,
> netxen_nic_driver_name);
> + return -ENOMEM;
> + }
> + netxen_proc_dir_entry->owner = THIS_MODULE;
> +
> + cmd_desc_cache = kmem_cache_create("cmd_desc_cache",
> + sizeof(cmdDescType0_t) * ((MAX_BUFFERS_PER_CMD/4)+3),
> + ARCH_KMALLOC_MINALIGN, ARCH_KMALLOC_FLAGS, NULL, NULL);
> + cmd_buff_cache = kmem_cache_create("cmd_buff_cache",
> + sizeof(struct netxen_cmd_buffer), ARCH_KMALLOC_MINALIGN,
> + ARCH_KMALLOC_FLAGS, NULL, NULL);
> + for (i=0; i<NR_CPUS; i++) {
> + cmd_desc_array[i] =
> + (cmdDescType0_t *) kmem_cache_alloc (cmd_desc_cache,
> + SLAB_ATOMIC);
> + cmd_buff_array[i] =
> + (struct netxen_cmd_buffer *) kmem_cache_alloc
> (cmd_buff_cache,
> + SLAB_ATOMIC);
Lots of NULL pointer checks missing.
And you shouldn't use GFP_ATOMIC here since it's unreliable and not needed since
you don't hold any locks.
>
> + if (tx_user_packet_data != NULL) {
> + kfree(tx_user_packet_data);
> + tx_user_packet_data = NULL;
> + tx_user_packet_length = 0;
> + }
> + for(i=0; i<MAX_NUM_CARDS; i++)
> + {
> + adapter = adapterlist[i];
> + if (adapter == NULL)
> + continue;
> + if (adapter->netlist != NULL && adapter->netlist->netdev !=
> NULL) {
> +/* struct net_device *netdev = netxen_netlist->netdev;
> + struct netxen_port *port = netdev->priv;
> + struct netxen_adapter_s *adapter = port->adapter;
> +*/
> + read_lock(&adapter->adapter_lock);
In general rw locks should be only used when absolutely needed because
they're much slower than normal spinlocks.
> + netxen_nic_disable_int(adapter);
> + read_unlock(&adapter->adapter_lock);
> + /*
> + * Wait for some time to allow the dma to drain, if any.
> + */
> + mdelay(5);
This is not enough to make sure the interrupt handler can't execute
on any CPU anymore I think.
> + pci_unregister_driver(&netxen_driver);
> + remove_proc_entry(netxen_proc_dir_entry->name, proc_net);
> + for (i=0; i<NR_CPUS; i++) {
Using NR_CPUS like this is bad because it can be very large and waste lots of
memory.
Use for_each_possible_cpu. And consider using per cpu data.
-Andi
-
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
