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

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

[Ron Mercer]

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

Does anyone know what would be appropriate for this ethtool command?  My
device does not use firmware.  I took the "N/A" idea from e1000 and
skge.

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

[Jeff Garzik]

Ron Mercer wrote:
>> 9) [minor] "N/A" appears to be an inaccurate value for fw_version in 
>> ql_get_drvinfo()
> 
> Does anyone know what would be appropriate for this ethtool command?  My
> device does not use firmware.  I took the "N/A" idea from e1000 and
> skge.

It does both ethernet and iSCSI entirely in silicon?

Is there an easy way to get silicon rev?

	Jeff

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

[Ron Mercer]

 

> Ron Mercer wrote:
> >> 9) [minor] "N/A" appears to be an inaccurate value for 
> fw_version in
> >> ql_get_drvinfo()
> > 
> > Does anyone know what would be appropriate for this ethtool 
> command?  
> > My device does not use firmware.  I took the "N/A" idea 
> from e1000 and 
> > skge.
> 
> It does both ethernet and iSCSI entirely in silicon?
> 
> Is there an easy way to get silicon rev?

There is firmware to support iSCSI.  It is not used by the ethernet
function.

There is no way to get the iSCSI firmware version from the ethernet
function, but there is a way to get silicon rev.

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

[Ron Mercer]

Jeff,

I will address all of your points in my next post. 

> 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?

>From the body of my previous post:
- ql_wait_for_drvr_lock is used to protect resources that are shared 
  across the network and iSCSI functions.  This protection is mostly 
  during chip initialization and resets, but also include link 
  management.


The drvr_lock is to allow the iSCSI driver, iSCSI firmware, and the
network driver exclusive access to common points in the hardware.


> 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).

I am not sure what sound technical rationale would apply here.  NAPI has
better thoughput and non-NAPI has lower latency.  However, I will pick
one and remove the other.

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

[Ron Mercer]

Jeff/Andrew,

Please find the Qlogic qla3xxx Ethernet driver posted at the URL below.
This is a complementary network driver for our ISP4XXX parts.
There is a concurrent effort underway to get the iSCSI driver (qla4xxx)
integrated upstream as well.

I have added the missing license that Francois pointed out, plus
Andrew's patch and suggestions from yesterday.

This submission is contained in a patch file that does the following:

Adds:
drivers/net/qla3xxx.c
drivers/net/qla3xxx.h
Documentation/networking/LICENSE.qla3xxx

Modifies:
MAINTAINERS
drivers/net/Makefile
drivers/net/Kconfig

Patch file qla3xxxpatch1-v2.02.00-k34.txt is at the following link:

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

Signed-off-by: Ron Mercer <ron.mercer@qlogic.com>

Some notes on the driver/hardware:

- Built and tested using kernel 2.6.17-rc4.
- The chip supports two ethernet and two iSCSI functions.
- The functions ql_sem_lock, ql_sem_spinlock, ql_sem_unlock, and
ql_wait_for_drvr_lock are used to protect resources that are shared
across the network and iSCSI functions.  This protection is mostly
during chip initialization and resets, but also include link management.
- The PHY/MII are not exported through ethtool due to the fact that the
iSCSI function will control the common link at least 50% of the time.



Regards,

Ron Mercer
Qlogic Corporation

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

[Jeff Garzik]

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()

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

[Roland Dreier]

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

Why would it?  MSI vectors are always unique, aren't they?

 - R.

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

[Jeff Garzik]

Roland Dreier wrote:
>  > 15) I wonder if SA_SHIRQ needs to be set, for MSI?
> 
> Why would it?  MSI vectors are always unique, aren't they?

Precisely what my question implies...

	Jeff