Stephen, Thank you for taking the time to review my driver. The feedback was great. I will endeavor to bring it up to an A+. Please see my comments below.
> -----Original Message----- > From: Stephen Hemminger [mailto:[EMAIL PROTECTED] > Sent: Friday, March 17, 2006 1:35 PM > To: Ron Mercer > Cc: netdev@vger.kernel.org; Linux Driver > Subject: Re: [RFC] new qla3xxx NIC Driver v2.02.00b01 > > Overall, if this were a class assignment I would give it a C- if I was > feeling generous. It tries to follow the linux driver style, but it > really needs more work and is incomplete at this time. > > * The most troublesome design flaw seems to be that the hardware knows > about the ip_address. Well, if you are doing some offloading, your going > to get screwed cause you don't handle address changes, and gob of other > issues related to addreses, IPV6, ... The hardware is capable of knowing the ip_address, but it is not initialized for basic MAC functionality. This driver does no offloading, so I will remove all references. The 3022 device that this driver controls does not handle IPv6, but the next device (3032) does. > > * Don't use typedef's ql3xx_adapter, ... etc. Will be removed in the next release. > > * Don't define stuff in ql3_def.h that is already in other linux headers > ETH_DATA_LEN ... Will be removed in the next release. > * You have to allocate an skb and copy the data into the buffer on > every receive in interrupt. > Boy I bet that really sucks for performance. Next release. > > * Use netdev_priv() rather than dev->priv Next release. > > * You are making more work than necessary. The driver is keeping > stuff in the adapter data structure that is available already > in net_device mtu, eth_addr, multicast list, ... Next release. > > * Also lots of rx/tx block information is redundant and can be > rederived. If you have the skb and map address that should > be sufficient. Next release. > > * Any gigabit driver ought to be using NAPI, this isn't Also mentioned in my submittal. It will be added to a future release. > > * Don't do ethtool via ioctl hook use ethtool_ops. OK > > * Do nvram via ethtool I am not sure what you mean here. The driver reads nvram to determine the register settings for the chip. Do you mean I should expose the parameters to the user for modification? > > * Needs real ethtool support for speed setting/reporting Next release. > > * If you need a linked list, use list macros? Next release. > > * Use new module_param() not deprecated MODULE_PARM() Next release. > > * Use netif_msg_ rather than your current print_lvl / qDbgLevel Next release. > > * Remove wrapper macros like QLDPRT5 Next release. > > * Remove HARD_ETHER_ADDRESS! Next release. > > * Use existing MII code and interface Subsequent release. > > * Use C90 style initializers rather than old GNU style OK > > * WTF is ql3xxx_driver_entry? It's gone. I was using it as a marker to print the top of the driver for debugging. > > * ql3xxx_open is unnecessary stupid wrapper to ql_open > remove it Will do. > > * Don't use a dpc thread, use a work queue instead Will do. > > * qdev->stats should just be part of the netdev_priv() not malloc'd Will change this. > > * enable/disable interrupts should probably be inline in .h file Will change this. > > * ql_sem_spinlock and ql_sem_lock defined but not used? delete it They are used in macros that protects registers between different PCI functions (iSCSI driver). I will clean this up. > > * make functions used in one file static. Will do. > > * isr routine shouldn't save/restore flags > it's an isr interrupts are disabled already Will remove it. > > * code is full of places where it looks like you are being paid by the LOC > if (retval) > return IRQ_HANDLED; > else > return IRQ_NONE; > > vs. > return IRA_HANDLED(retval); This is left over from a bygone era that supported 2.4 and 2.6. I will remove it. > > > * Minor > - use const char for name/string/version > - add MODULE_VERS(DRV_VERSION) > - use PCI_DEVICE() macro in pci_device_id table > - source lines > 80 characters please indent > - Splitting things up into separate files isn't helpful for > a driver this size. I will change these things. The driver file issues will take a few releases. > > * Different from common usage: > - Most net drivers use pci_alloc_consistent rather than > dma_alloc_coherent in network drivers This I wasn't sure about. I looked at a 2.6 driver porting document that indicated dma_alloc_xxx was the way to go. I did notice that most network drivers in the kernel were using pci_alloc_xxx. - 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
