Re: New Qlogic qla3xxx NIC Driver v2.02.00k34 for upstream inclusion

[Jeff Garzik <jeff@garzik.org>]

Ron Mercer wrote:
> ftp://ftp.qlogic.com/outgoing/linux/network/upstream/2.02.00k34/qla3xxxp
> atch1-v2.02.00-k34.txt

Your mailer breaks this into two lines, which is a pain.

Comments on this updated version:

1) [semi-major] Infinite loop in ql_sem_spinlock(), if hardware is 
faulty or been unplugged.

2) Similarly, other rare hardware conditions could cause 
ql_sem_spinlock() to wait for a very, very long time.

3) [minor] PCI_POSTING() macro seems a bit ugly to me.  No good 
suggestions on better paths... maybe at least make it a static inline, 
to enable better type checking.

4) [minor] ql_wait_for_drvr_lock() should use ssleep() rather than msleep()

5) What is the point of using the hardware semaphore?  Is a firmware 
competing with the device driver somehow, and its activities require 
synchronization with the OS driver?

6) [major] The locking model is wrong for the API.  The very low level 
functions ql_read_common_reg() and ql_write_common_reg() take a spinlock 
inside their guts, which is quite expensive when you read the rest of 
the code:

> +       /* Clock in a zero, then do the start bit */
> +       ql_write_common_reg(qdev, &port_regs->CommonRegs.serialPortInterfaceReg,+                           ISP_NVRAM_MASK | qdev->eeprom_cmd_data |
> +                           AUBURN_EEPROM_DO_1);
> +       ql_write_common_reg(qdev, &port_regs->CommonRegs.serialPortInterfaceReg,+                           ISP_NVRAM_MASK | qdev->
> +                           eeprom_cmd_data | AUBURN_EEPROM_DO_1 |
> +                           AUBURN_EEPROM_CLK_RISE);
> +       ql_write_common_reg(qdev, &port_regs->CommonRegs.serialPortInterfaceReg,+                           ISP_NVRAM_MASK | qdev->
> +                           eeprom_cmd_data | AUBURN_EEPROM_DO_1 |
> +                           AUBURN_EEPROM_CLK_FALL);

The above quoted code just acquired and released the spinlock three 
times, an operation that could have _obviously_ been reduced to a single 
lock acquire + release.  In fact, the situation is worse than it sounds, 
because there were a bunch more ql_write_common_reg() calls in the 
function I quoted from.

Thus, this locking is _too_ low-level, and it _prevents_ more 
intelligent use of spinlocks, e.g.

	spin_lock_foo()
	write reg
	write reg
	read reg
	spin_unlock_foo()

I would venture to say that this locking model is largely useless in 
practice.  A better locking scheme is not only far more optimal, it 
would also be more _provable_ (such as with Ingo's lock validator).

7) Severe lack of comments.

8) [readability] ql_link_state_machine() suffers from overzealous 
indentation, which was caused by the functions large size.  At the very 
least, I would suggest breaking out the "/* configure the MAC */" 
sequence into a separate function.

9) [minor] "N/A" appears to be an inaccurate value for fw_version in 
ql_get_drvinfo()

10) [semi-major] More over-locking.  You take the TX spinlock for 
_every_ TX, in ql_process_mac_tx_intr() [which only processes a single 
TX completion] -> ql_free_txbuf().

11) General comment:  Have you noticed that all hot path operations save 
for ->hard_start_xmit() occur entirely inside adapter_lock ?

12) For new drivers, we see additional work and little value for 
maintained #ifdef'd NAPI and non-NAPI code paths.  Just pick the best 
one (and justify that decision with sound technical rationale).

13) The tx_lock appears to offer no value outside of the normal locking.

14) Surely there is a better way to down the adapter than masking the 
interrupts and resetting the adapter?  If this is ever used in non-MSI 
situations (common in Linux today), there is the possibility of 
screaming interrupts, in shared-interrupt situations.

15) I wonder if SA_SHIRQ needs to be set, for MSI?

16) [minor] several other places where ssleep() could be used

17) the following tests in ql3xxx_remove() should be eliminated, as they 
are impossible:

+       if (ndev == NULL)

+       if (qdev == NULL) {

18) [minor] standard practice is to print the driver name and version in 
pci_driver::probe(), on the first call to that function.  Your driver 
acts differently from others, by printing it in ql3xxx_init_module()