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, ...
* Don't use typedef's ql3xx_adapter, ... etc.
* Don't define stuff in ql3_def.h that is already in other linux headers
ETH_DATA_LEN ...
* 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.
* Use netdev_priv() rather than dev->priv
* 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, ...
* 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.
* Any gigabit driver ought to be using NAPI, this isn't
* Don't do ethtool via ioctl hook use ethtool_ops.
* Do nvram via ethtool
* Needs real ethtool support for speed setting/reporting
* If you need a linked list, use list macros?
* Use new module_param() not deprecated MODULE_PARM()
* Use netif_msg_ rather than your current print_lvl / qDbgLevel
* Remove wrapper macros like QLDPRT5
* Remove HARD_ETHER_ADDRESS!
* Use existing MII code and interface
* Use C90 style initializers rather than old GNU style
* WTF is ql3xxx_driver_entry?
* ql3xxx_open is unnecessary stupid wrapper to ql_open
remove it
* Don't use a dpc thread, use a work queue instead
* qdev->stats should just be part of the netdev_priv() not malloc'd
* enable/disable interrupts should probably be inline in .h file
* ql_sem_spinlock and ql_sem_lock defined but not used? delete it
* make functions used in one file static.
* isr routine shouldn't save/restore flags
it's an isr interrupts are disabled already
* 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);
* 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.
* Different from common usage:
- Most net drivers use pci_alloc_consistent rather than
dma_alloc_coherent in network drivers
-
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
